In D7, we have an 'update_log_queries' variable which you can set to TRUE when running updates. If you do, the database query logger is turned on, and the results of the queries are supposed to somehow be displayed at the end of the update process.

Currently, they are not displayed at all.

We may want to wait for #870918: Regression: Multiple messages returned from the same update function aren't displayed to figure out the best way to solve this, since that makes it possible to cleanly print multiple messages to the screen for the same update function (which would be needed for this to work).

CommentFileSizeAuthor
#2 query-log-870922-2.patch1.12 KBDavid_Rothstein

Comments

moshe weitzman’s picture

I thought we just ditched this feature altogether. IMO, we can ditch it for D7. Programmatic execution of update.php (like drush) can choose to enable a logger if it wants.

David_Rothstein’s picture

Status: Active » Needs review
StatusFileSize
new1.12 KB

Well, ditching it would certainly be simplest :)

And dumping it on the screen might be overwhelming, since unlike D6 it doesn't just show the queries the module author specifically chooses, but rather every single query that runs, so it might be difficult to figure out how to have update.php present this information nicely.

The attached patch is what it would look like to ditch it. I guess the question is whether there are people who want to run update.php via the core UI and still have the complete logging information available - if they exist they would rather see this optional feature fixed than ditched.

yched’s picture

IIRC there was some discussion about removing / keeping that feature in #394268: DIE update_sql() DIE!. In the end it was kept, but I don't remember for which intended output. Possibly for a special option in drush updb ?

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

A contrib module could add a logger for the update.php and its batch requests. They can figure out how to sensibly display the data.

dries’s picture

I'd be OK with that but want to wait a bit longer for feedback.

damien tournoud’s picture

I don't see what harm this does. The integration is pretty nice and clean. I don't see why and how drush would want to hack this feature in, if it's already cleanly integrated here.

sun’s picture

If moshe (as Devel maintainer) is happy with this, then I don't see a reason to hold off this patch.

sun’s picture

#2: query-log-870922-2.patch queued for re-testing.

sun’s picture

Version: 7.x-dev » 8.x-dev

Although a good clean-up, this is D8 material according to the rules (I had to learn today). It may be backported at a later point in time (though that's unlikely).

David_Rothstein’s picture

Version: 8.x-dev » 7.x-dev

Unless I am missing something, the current code in Drupal 7 does absolutely nothing here.

We should either remove the code that does nothing, or make it do something.

dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.