Currently, the executed SQL statements are not displayed in the Upgrade Wizard. There is a patch, but questions came up.
Problem A: Placeholder from preparedStatement (e.g.
SET uc = :dcValue2 WHERE) are returned from getSQL(), not actual values, currently with the patch these would be displayed in the flash messages, not the actual values.
Problem B: Long and unreadable SQL queries where it’s hard to see what was changed (e.g. UPDATE on
be_users.ucin StartModuleUpdate : serialiized array) … and flash message will look broken
- Problem C: Naming of parameter $databaseQueries in performUpdate is too specific
- Solution 1: change $performUpdates parameter naming in performUpdate to $results (as Simon Gilli suggested in slack). This is more generic and you might want to display other results instead of SQL statements.
- Solution 2: Do not change the original query (to remove quoting etc.) because people will just copy-paste this, we don’t want that and the code is not easily maintainable. The query part should be as you usually would do it, IMO.
- Solution 3a: Use QueryBuilder::getParameters instead
- OR Solution 3b: It would be cool if QueryBuilder::getSQL() did all the work and already returned an SQL statement with the replaced placeholders. This is also very useful for debugging (not sure how complex this would be)
Solution 4: Do not output SQL statement in case of danger of it being too noisy (e.g.
be_users.uc) but provide some pseudo-code for it (“descriptive solution”), e.g. “Update be_users: change field uc to include startModule[‘help_AboutAbout’] instead of startModule[‘help_aboutmodules’] (uid=1)”. This can easily be done in the foreach loop.
Clarification: If you output SQL query verbatim or use “descriptive solution” would depend on what is most useful in user-interface and might be done differently in each update Wizard.
- Output of SQL statements in UpdateWizards may change slightly
- performUpdate functions need to be changed (they do anyway!)
- for 3b only: getSQL() needs to be changed (but then, not all performUpdate functions)
- original SQL queries don’t need to be changed
- more user-friendly
- 3b: we get better getSQL() for debugging
- 3b: less code duplication if we do it directly in getSQL.
- 3b: If the getSQL() is changed to replace the placeholders, probably not an easy job …, the rest is trivial in comparison and the work needs to be done anyway.
Remarks and notes
Topic Initiator: Sybille Peters
Topic Mentor: Benni Mack