Currently, the executed SQL statements are not displayed in the Upgrade Wizard. There is a patch, but questions came up.
Problem
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.uc in StartModuleUpdate : serialiized array) … and flash message will look broken
Problem C: Naming of parameter $databaseQueries in performUpdate is too specific
Possible solutions
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 notchange 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.
Impact
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)
Pro
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.
Con
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.
I don’t understand what solution 1 should do, can you please clarify this?
Solution 2 suggest copy pasting something (the query?).
Can you explain the usecase of copying a query that just ran through?
Solution 3
getSQL should never, ever replace the placeholders.
The understand the resoning read up on what prepared statements are.
In order to achieve what you want to achieve, the view would need to replace them manually; I guess that is what you mean by solution 3a.
Solution 3b is no solution, it just makes things worse.
Solution 4 seems somewhat viable, but opens up a new set of meta code that needs to be built by developers.
I’d like to take the discussion back on step.
WHY do we want to show the SQL statements?
What’s the benefit to the user?
I have the same question as Mattes here: what exactly is the use case we want to fix?
I mean, if it’s purely informational, I’d say just remove the checkbox. If it was meant to provide debugging information in case something went wrong, I’d say it’s the wrong place (as the query will probably fail and hopefully in case of an error an error message should be displayed).
So, I’m a bit at a loss what good that checkbox should do and whether it makes sense to have it at all…
The idea was changing the checkbox to “Verbose output” and allow more than sql statements to be returned because a AbstractUpdate descendant can not only update the database. But this solves nothing just opens the parameter usage.
That’s a good point at all and brings up the question what’s the value of such a feature. Showing sql statements after they are executed is IMO very questionable. Good informations after failure would be the most important.
So removing the checkbox would probably the best option.
My initial reaction was: There was some misunderstanding, It looks like not everyone read my initial post properly and the answers seem to assume that I am incredibly stupid, suggest copy-pasting, don’t know what prepared statements are etc. Frankly, I was a little pissed off there.
BUT the suggestions are good and they are better then my initial idea. I agree that removing the checkbox is a better idea.
Solution 1 is just the renaming of a parameter (and necessary for Solution 4)
I never suggested copy-pasting anything.I suggested leaving the initial queries as is ($queryBuilder->set()->execute). What we are talking about here is just debugging output for the user. This was done in previous TYPO3 versions.
I know what prepared statements are. They are used for performance reason and to protect against SQL injection. In my suggestion, the prepared statement is still used to execute the query (even though there is no user input and it doesn’t even make sense in this case …). But, I explicitly suggested to not change that. What might be a problem is dumping the SQL content. That has also been done in previous versions. That must be handled in each Update Wizard carefully to not expose sensitive data, of course.
I now agree in the point that getSQL should not be changed.
It would of course be easier to remove the checkbox. It might have been a better idea in the first place. Does the user really need to know what is happening under hood? If everything works, probably not. Personally I like to know what is going on but you don’t get that for other actions either. If someone really wants to check the DB changes themselves, there are other tools for this.
Does the user really need to know what is happening under hood? If everything works, probably not. Personally I like to know what is going on but you don’t get that for other actions either. If someone really wants to check the DB changes themselves, there are other tools for this.
We are not talking about average users (like visitors or editors) here. We are talking about admins with at least some developers skills. Those people like to have detailed output, such that they have a starting point to investigate errors. It is not only about the problematic installation, as you will likely never see the verbose output, but rather to be table to compare it to a clean state.
Without the verbose output of queries, one need to dig into the code, piece together the puzzle and then start trying to understand where things go wrong, whereas one could have simply run the query from a clean installation on the failing DB and have a look.
What other tools for DB changes are you talking about? The only thing that comes to my mind here is a query log, that is likely going to slow down the whole DB.
I would prefer that the checkbox becomes a general verbose output option, depending on the upgrade wizard. Queries should be printed without values. The values should be shown separately. At least that is what I do when I am debugging queries.
I personally really hate it if tools do not provide a verbose output option, start failing and you are none the wiser why. Most notable Microsoft products that output the very distinctive error message “There was an error”, even on a shell admin utility. No way to trace the error. No way to find out what a successful run does and what might have possibly gone wrong.
I fully agree on the “when there is an error I want verbose output part” but not in the form of that checkbox - I prefer sensible error handling here - if an error happens, I want verbose information about it - ideally database queries executed, possibly a stacktrace or depending on the update wizard the exact step it was performing when failing. I don’t see a point for that if all went well.
I’d therefor suggest:
Remove the checkbox
Take a close look at the error handling and define per type of upgrade wizard which kind of information it should output in case of error (which is btw. half-way done by the $customMessage part that gets displayed in case of errors)
In development context // depending on the configured error levels and handling the exception / error with stacktrace should be shown (which might not be the case at the moment since the rewrite to ajax requests - you’d need to look at the response of the ajax call).
(in case of 2: I’d go with displaying the prepared statements and optionally display the substituted parameters next to it - most of the time it’s not the substitution but the statement syntax causing the error, but the parameters may prove useful)
I like log files too.
but not the apache or mysql log files as these would be scattered.
What about a complete log of all the actions of the upgrade process?
with clear information what step results in what action and not only on the screen but in one file.
If the upgrade went wrong (I always would test with a copy or at least had an initial dump) I could use the log to verify each step and see where the culprit is.
I’d like to have this SQL queries complete (with data not as prep statement) and not only if the error happens.
Some transformation may be correct from the view of SQL but not correct in the view of the database.
For example a drop table abc; won’t throw an error, as it is correct but maybe it isn’t expected to happen.
Error handling is most easily done with log files. Just use them, no checkbox needed.
In my experience detailed log files tend to be hard to follow and it is easy to miss the important information. That is more an option for automated upgrades.
The SQL queries are shown AFTER they are executed, so no difference if you read the log or the statements. And in general you don’t want to see the raw statements as an average user. The executed statements are usually no surprise, otherwise the upgrade wizard is buggy or its description.
