Problem/Motivation

In #3561087: Add equivalent updates hook to prevent downgrades from 10.6 to 11.2, discovered an issue with the equivalent updates that installing a site on the earlier version (e.g. 10.4) does not register the equivalent update for the later version (11.1), because equivalent updates are registered in update hooks, which do not run on site install.

So a user who installs a site on 10.4 will not see a warning messag when running database updates after going to 11.0.

This issue to address that gap by possibly registering equivalent updates in the system hook_install()implementation.

Steps to reproduce

Proposed resolution

Add a MarkFutureUpdateEquivalent attribute that can be applied to hook_update_N() functions. The attribute has two arguments, futureUpdateNumber and futureVersionString. During module install and when database updates are run, update functions are reflected to check for the attribute, and if the function has one, then the future equivalent version is registered.

Remaining tasks

User interface changes

Introduced terminology

API changes

See Proposed resolution.

Data model changes

Release notes snippet

CommentFileSizeAuthor
#16 3561302-nr-bot_inpa8q08.txt667 bytesneeds-review-queue-bot

Issue fork drupal-3561302

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

godotislate created an issue. See original summary.

xjm’s picture

xjm
she/her
Primary language English
commented
Parent issue: #3561087: Add equivalent updates hook to prevent downgrades from 10.6 to 11.2 » #3449808: [12.x] [meta] Requirements for tagging 12.0.0-beta1
dww’s picture

dww
we/he/they
commented
Related issues: +#3561087: Add equivalent updates hook to prevent downgrades from 10.6 to 11.2
godotislate’s picture

godotislate
he/him
commented

Quick thought is that we can do this with attributes.

For example, if we were doing this for 10.6/11.3:
10.6

/**
  * Equivalent to 11300.
  */
#[MarkFutureUpdateEquivalent(future_update_number: 11300, future_version_string: '11.3.0')]
function system_10600(): void {}

11.3

/**
  * Equivalent to 10600.
  */
#[CheckEquivalentUpdate]
function system_11300(): void {}

Then when running database updates, check the update function for attributes. If it has the MarkFutureUpdateEquivalent attribute, then register the equivalent update. If it has the CheckEquivalentUpdate attribute, then check for a registered equivalent update.

During module install, in this part:

      // Set the schema version to the number of the last update provided by
      // the module, or the minimum core schema version.
      $version = \Drupal::CORE_MINIMUM_SCHEMA_VERSION;
      $versions = $this->updateRegistry->getAvailableUpdates($module);
      if ($versions) {
        $version = max(max($versions), $version);
      }

Can loop through the available update functions, use reflection to look for the MarkFutureUpdateEquivalent attribute and register the equivalent update.

catch’s picture

catch
he/him
Primary language English
commented

I think #4 would work.

In slack @xjm brought up hard-coding (or semi-hardcoding, depending on how clever we can get with predicting future branch numbers) a list of equivalent branches, and then doing the registration directly in update.php and site install based on the current version. I'm not sure if we should explore that here or in a second issue.

godotislate opened merge request !14036

godotislate’s picture

godotislate
he/him
commented
Status: Active » Needs review

So #4 was actually pretty easy: MR 14036.

I didn't add the #[CheckEquivalentUpdate] attribute, since I think the future version update doesn't need to run on module install. I don't think it's needed for no-op, either, because this already exists in update_do_one() (and the corresponding Drush command code):

  $equivalent_update = \Drupal::service('update.update_hook_registry')->getEquivalentUpdate($module, $number);
  if ($equivalent_update instanceof EquivalentUpdate) {
    $ret['results']['query'] = $equivalent_update->toSkipMessage();
    $ret['results']['success'] = TRUE;
    $context['sandbox']['#finished'] = TRUE;
  }

Drush will need to be updated to support the new attribute if we go this way.

Anyway, I wasn't sure if I could just add arguments to UpdateHookRegistry::markFutureUpdateEquivalent(), so I followed the instructions per https://www.drupal.org/about/core/policies/core-change-policies/how-to-d.... I haven't created the follow up yet in case this isn't necessary.

godotislate’s picture

godotislate
he/him
commented
Issue summary: View changes
godotislate’s picture

godotislate
he/him
commented
Issue tags: +Needs Review Queue Initiative

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.

smustgrave’s picture

smustgrave commented

Should MarkFutureUpdateEquivalent get a CR?

nicxvan’s picture

nicxvan commented

I don't think so

godotislate’s picture

godotislate
he/him
commented

Added a CR for how to use the new attribute: https://www.drupal.org/node/3572245. Also updated the placeholder URL parameter change notice in the MR to the actual CR URL.

nicxvan’s picture

nicxvan commented

Thank you for creating that, for some reason I misunderstood that the request was about the first equivalent update, not the actual api and attribute.

Makes sense to have a CR for this.

smustgrave’s picture

smustgrave commented

CR reads well to me, well detailed.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3561302-nr-bot_inpa8q08.txt667 bytes

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

godotislate’s picture

godotislate
he/him
commented
Status: Needs work » Needs review

Re-ran PHPCS and PHPStan jobs only and they passed, so setting back to NR despite what the bot said.

smustgrave’s picture

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

Revisiting this one (sorry it fell off) but think it would be good to maybe land this in 11.4? With 12 approaching at the end of the year this may be useful for that. Going to mark it.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

I think we need to do a bit more here.

We need to update module.api.php

 * Future updates can be marked as equivalent by adding the following code to an
 * update.
 * @code
 * function my_module_update_10400() {
 *   \Drupal::service('update.update_hook_registry')->markFutureUpdateEquivalent(11101, '11.1.1');
 *
 *   // The rest of the update function.
 * }
 * @endcode

I think we should deprecate calling markFutureUpdateEquivalent without a module and number and tell people to use the attribute.

And therefore we should make markFutureUpdateEquivalent @internal because there is no reason to call it. In fact it's wrong to not use the attribute.

godotislate’s picture

godotislate
he/him
commented
Status: Needs work » Needs review

Updated MR per feedback.

godotislate’s picture

godotislate
he/him
commented
Status: Needs review » Needs work
godotislate’s picture

godotislate
he/him
commented
Status: Needs work » Needs review

Addressed MR comments.

godotislate’s picture

godotislate
he/him
commented

Update the CR.

nicxvan’s picture

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

Looks great now!

Read through the CR and the changes again.

quietone made their first commit to this issue’s fork.

quietone’s picture

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

I read the comments, the change record and the MR. I didn't do a code review.

I made significant changes to the change record and to comments in the MR. They will need to be reviewed, so changing status. For the change record it should be concise and limit to the change itself and what action sites may need to take. The level of detail to supply can be different for different types of changes, but in all cases, a good example can explain more than lots of text. And, I think it is fair to expect people who want to know more will follow the link to the issue or in some way read the code changes.

I updated credit.

godotislate’s picture

godotislate
he/him
commented

Thanks, @quietone. I took another pass at the CR and applied your MR suggestion. I also answered your question about the @todo.

godotislate’s picture

godotislate
he/him
commented

Created the follow up for Drupal 13 to make the parameters required: #3617733: [pp-1] Make $module and $ran_update_number parameters for UpdateHookRegistry::markFutureUpdateEquivalent required Drupal 13
Updated the MR to reference the follow up URL.

quietone’s picture

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

I tweaked comments. I also rebased. In Slack @godotislate confirmed by changes were OK, so I am restoring the RTBC on this issue.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 1ab223a55d3 to main and 941d0ac7d07 to 11.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • alexpott committed 941d0ac7 on 11.x 
    task: #3561302 Register equivalent updates on site install

By:...

  • alexpott committed 1ab223a5 on main 
    task: #3561302 Register equivalent updates on site install

By:...

Status: Fixed » Closed (fixed)

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