Problem/Motivation

In #394268: DIE update_sql() DIE! a big cleanup of the update system was done which made it so update functions return a simple string (indicating a success message) rather than a complicated array like before.

However, some problems were introduced:

  1. We lost the ability for a single update function to return more than one message. There is already now code in core that tries to get around that via an ugly implode() - as shown (and fixed) in the attached patch. See also discussion in #774316: Remind users about new Drupal 8 features after migration from D7 which really could use that functionality.
  2. Similarly, an update function that gets called more than once (via a batch) has its initial return values ignored; only the last one is used.
  3. There is tons of code in update.php still designed to process the old method, and the result is that when update.php is finished we print a "list" of messages for each update function (using <li> tags) but the list can never have more than one element.

This patch cleans things up and restores the ability for an update function to return multiple messages either on a single pass or accumulated over multiple passes in a batch process.

Note: Drush probably requires some D7 changes as a result of this; I don't think that can be helped.

Steps to reproduce

Proposed resolution

TBA

Remaining tasks

Reroll patch
Add test, see #1 and #26

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#10 870918-10.patch7.85 KBstar-szr
#10 interdiff.txt1.71 KBstar-szr
#8 870918-8.patch7.58 KBstar-szr
#6 update-multiple-messages-870918-6.patch7.46 KBDavid_Rothstein
update-multiple-messages.patch7.65 KBDavid_Rothstein

Issue fork drupal-870918

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

David_Rothstein’s picture

David_Rothstein commented

Here is some code I used while testing out this patch. We could possibly turn it into an actual test, if we want to try writing one:

function system_update_7075(&$sandbox) {
  $steps = &drupal_static(__FUNCTION, 1);
  if ($steps == 1) {
    $sandbox['#finished'] = 0.25;
    $message = 'FIRST RUN: message';
  }
  elseif ($steps == 2) {
    $sandbox['#finished'] = 0.5;
    $message = array(
      'SECOND RUN: message 1',
      'SECOND RUN: message 2',
    );
  }
  elseif ($steps == 3) {
    $sandbox['#finished'] = 0.75;
    $message = 'THIRD RUN: message';
  }
  else {
    $sandbox['#finished'] = 1;
    $message = 'This should not display unless the exception thrown below is commented out.';
    throw new DrupalUpdateException('Update failed');
  }
  $steps++;
  return $message;
}
David_Rothstein’s picture

David_Rothstein commented

Related issue: #870922: Database query logs are never printed when update.php is run

Since database query results, which are stored in $ret['queries'], are already broken, this patch doesn't attempt to deal with that part of the $ret array at all. I think it is better to fix this first and then do a followup in the other issue.

jacine’s picture

jacine
she/her
Location New York
commented

You rock! Subscribing for now.

yched’s picture

yched commented

This looks like a great cleanup.

I'm a little worried at what exactly the array_merge_recusrive() will do.
If I read the patch correctly, $ret is now an array with two entries, 'messages', 'queries'.
'messages' is an array of flat strings, so array_merge_recusrive() should be OK there.
I'm not sure about 'queries', though. If each logged query is itself an array, we don't want deep merging.

One detail about the test update func in #1 : you'd rather use a variable_set() / variable_get() mechanism rather than a static for $steps. A static will break if batch API interrupts processing after step N, and step N+1 is done in a subsequent HTTP request.

David_Rothstein’s picture

David_Rothstein commented

I think 'queries' is a numerically-indexed array, so even though each element is itself an array, the merging won't go any deeper than that. I'm not 100% positive, though, and array_merge_recursive() often trips me up :)

It might at least be worth a code comment there. However, #870922: Database query logs are never printed when update.php is run may remove this anyway.

David_Rothstein’s picture

David_Rothstein commented
Version: 7.x-dev » 8.x-dev
Component: update system » database update system
StatusFileSize
new update-multiple-messages-870918-6.patch7.46 KB

Rerolled the patch for Drupal 8.

Since #870922: Database query logs are never printed when update.php is run has been committed, I was able to avoid using array_merge_recursive() and simplify this patch a couple other ways also.

yched’s picture

yched commented

Still is a very nice cleanup. Code looks good.

One minor nitpick : the $ret array in update_do_one() ends up as $results in update_results_page(). $ret being a sad remnant from the old hook_update_N() return format, could we simply rename it $result all the way ?

star-szr’s picture

star-szr
he/him
Primary language English
commented
StatusFileSize
new 870918-8.patch7.58 KB

Rerolled against D8 HEAD. No code changes from #6.

yched’s picture

yched commented

Thanks for the reroll, @Cottser.
As mentioned in #7, please just rename the $ret variable in update_do_one() to $results. Then this is RTBC.

star-szr’s picture

star-szr
he/him
Primary language English
commented
StatusFileSize
new interdiff.txt1.71 KB
new 870918-10.patch7.85 KB

@yched - I've made that revision, here's an updated patch and interdiff.

yched’s picture

yched commented
Status: Needs review » Reviewed & tested by the community

Thanks !

star-szr’s picture

star-szr
he/him
Primary language English
commented

Should this have tests?

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Yes, I think it should.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.4.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.5.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

Bug reports should be targeted against the 8.6.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.7.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

quietone commented
Issue summary: View changes
Issue tags: +Bug Smash Initiative

I am wondering if this was fixed in #2250119: Run updates in a full environment. Looks like \Drupal\system\Controller\DbUpdateController::results handles multiple messages from a module.

Can someone confirm?

darvanen’s picture

darvanen
he/him
Primary language English
Location Sydney, Australia
commented

I used a slightly modified version of the test update in #1:

function system_update_9401(&$sandbox) {
  $steps = &drupal_static('bogus_test_function', 1);
  if ($steps == 1) {
    $sandbox['#finished'] = 0.25;
    $message = 'FIRST RUN: message';
  }
  elseif ($steps == 2) {
    $sandbox['#finished'] = 0.5;
    $message = array(
      'SECOND RUN: message 1',
      'SECOND RUN: message 2',
    );
  }
  elseif ($steps == 3) {
    $sandbox['#finished'] = 0.75;
    $message = 'THIRD RUN: message';
  }
  else {
    $sandbox['#finished'] = 1;
    $message = 'This should not display unless the exception thrown below is commented out.';
//    throw new DrupalUpdateException('Update failed');
  }
  $steps++;
  return $message;
}

And the output was (drumroll please):

>  [notice] Update started: system_update_9401
>  [notice] FIRST RUN: message
>  [warning] Array to string conversion UpdateDBCommands.php:270
>  [notice] Array
>  [notice] THIRD RUN: message
>  [notice] This should not display unless the exception thrown below is commented out.
>  [notice] Update completed: system_update_9401

Looks like this is still an issue.

quietone’s picture

quietone commented
Issue summary: View changes
Issue tags: +Needs reroll

@darvanen, thank you!

Did a wee update to the Issue Summary.

bhanu951’s picture

bhanu951 commented
Version: 9.4.x-dev » 10.1.x-dev

Bhanu951 opened merge request !2971

bhanu951’s picture

bhanu951 commented
Status: Needs work » Needs review
Issue tags: -Needs reroll

Rerolled the Patch 870918-10.patch for 10.1.x branch. Pls review.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Needs review » Needs work

Thanks for switching to a merge request.

However, this patch is fully ten years old - it predates Drupal 8! -- and looks like it's full of out-of-scope changes. We probably need to restart this from scratch.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented

Hiding patches in favor of the merge request.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.