Problem/Motivation

See #3463225: [meta] Prevent 'downgrades' from maintenance minors to older minor releases for the next major version for background.

If we add a hook_update_N() to system module in 11.1, and backport this to 10.4 using the equivalent updates API, then a site that attempts to 10.4.0 to 11.0.0 will get a requirements error that they're trying to update to a release that is too old, and they should update to a newer release instead (e.g. 11.1).

We don't currently have such an update, so the idea here is to add an empty/no-op update purely to prevent unintentional downgrades like the above from happening.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#12 3463226-nr-bot.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3463226

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:

Comments

catch created an issue. See original summary.

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

quietone’s picture

Status: Active » Needs review
smustgrave’s picture

Status: Needs review » Needs work

So ran the test locally without the change to system.install and the test still passes. Shouldn't it fail?

quietone’s picture

Status: Needs work » Needs review

No, it should not fail, it is independent of the system.install. The test is relying on the downgrade_test module to provide the hook and that module is only installed in the test after proving the requirement failure. The test is a functional test of the concept. There is no similar test using the actual system_update_11101 because I don't see a way to prevent the update process from finding and executing that hook.

xjm’s picture

Status: Needs review » Needs work

I left some comments on the test. It could use some clarification since update tests are dark magic.

I think we need also need the MR that adds the equivalent system update to review this properly, no?

quietone’s picture

Status: Needs work » Needs review
xjm’s picture

Status: Needs review » Needs work

Still NW for a second MR for 10.4, I think.

quietone’s picture

Status: Needs work » Needs review

Made some minor improvements and the 10.4.x MR as well.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.

quietone’s picture

Status: Needs work » Needs review
catch’s picture

Two minor nits on the MR but otherwise RTBC for me. Would be good to get this in before the 10.4 beta, although I also think it could go in during the beta if necessary.

dww’s picture

Assigned: Unassigned » dww
Status: Needs review » Needs work

Working on @catch's concerns for the 10.4 MR

dww’s picture

Assigned: dww » Unassigned
Status: Needs work » Needs review

Fixed the nits in the 10.4.x MR, then cherry-picked those commits into the 11.x MR. Also did some more cleanup in the 11.x MR. I think this is ready for re-review.

dww’s picture

FYI, I opened #3487637: Move all system_update_N() methods next to each other to cleanup a weirdness once this issue is committed to 11.x and 11.1.x.

quietone’s picture

I converted the version numbers to be specific for 10.4 and 11.

dww’s picture

Saving credits. So far, everyone has meaningfully contributed here.

godotislate’s picture

Status: Needs review » Reviewed & tested by the community

Both MRs lgtm.

  • catch committed b043b88e on 10.4.x
    Issue #3463226 by quietone, dww, smustgrave, catch, xjm: Use the new...

  • catch committed 79f428b0 on 10.5.x
    Issue #3463226 by quietone, dww, smustgrave, catch, xjm: Use the new...

  • catch committed 8d156b1c on 11.1.x
    Issue #3463226 by quietone, dww, smustgrave, catch, xjm: Use the new...

  • catch committed e4df25b6 on 11.x
    Issue #3463226 by quietone, dww, smustgrave, catch, xjm: Use the new...
catch’s picture

Version: 11.x-dev » 10.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed the two MRs to 11.x/11.1.x and 10.5.x/10.4.x respectively. Thanks!

We'll need to decide if we do a similar thing for 10.5.x->11.1.x too.

Status: Fixed » Closed (fixed)

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

damienmckenna’s picture

Should the 10.4, 11.0 and 11.1 release notes be updated to more clearly state this requirement in the upgrade process? While people will run into the issue if they try upgrading from 10.4 to 11.0, the issue is obscured and many are not aware of it.

xjm’s picture

Amending attribution.