Problem/Motivation

In #3087644: Remove Drupal 8 updates up to and including 88**, we noticed that using that hook_update_last_removed() is not actually that strictly enforced (it requires to have an update function), doesn't actually show what's going on and just displays a warning message, together with a success message:

Proposed resolution

Add something to system_requirements() that checks for that explicitly, for all modules implementing such a hook. Display an error with clear info and don't allow users to continue in that case.

Remaining tasks

User interface changes

(copied from #40)

Before:

you only get a warning that doesn't prevent you from doing the update and that warning only shows up on a later step when the updates are listed:

After:

Update.php stops on the requirements page and displays an error that doesn't allow you to continue:

Contrib:

API changes

Data model changes

Release notes snippet

Drupal now shows a more user-friendly warning when a site tries to upgrade to a new version without having run required intermediate database updates first.

If you see 'Unsupported schema version' when attempting to run updates, you should:

  1. Back up your site and codebase.
  2. Locate an older version of the contributed module which contains the updates you missed.
  3. Downgrade the module to that version in your code base, and attempt to run updates again.

When this error is shown, it will now also prevent any updates from proceeding, so may make a persistent issue on an existing site more obvious.

Ideally, you should always attempt updates of contributed modules and core on a backup of your site (such as a local development environment) prior to running them on production.

CommentFileSizeAuthor
#123 last-removed-3098475-98-8.9.patch8.16 KBdww
#117 last-removed-3098475-98-8.9-test-only.patch4.66 KBGábor Hojtsy
#113 last-removed-3098475-98-8.9.patch8.16 KBGábor Hojtsy
#98 last-removed-3098475-98-8.9-interdiff.txt1.45 KBBerdir
#98 last-removed-3098475-98-interdiff.txt2.17 KBBerdir
#98 last-removed-3098475-98-8.9.patch8.16 KBBerdir
#98 last-removed-3098475-98.patch5.12 KBBerdir
#96 last-removed-3098475-96.patch3.4 KBBerdir
#96 last-removed-3098475-96-8.9-interdiff.txt3.23 KBBerdir
#96 last-removed-3098475-96-8.9.patch8.15 KBBerdir
#86 last-removed-3098475-86-8.9-interdiff.txt2.37 KBBerdir
#86 last-removed-3098475-86-8.9.patch7.89 KBBerdir
#81 3098475-81-interdiff.txt6.31 KBBerdir
#81 3098475-81.patch10.63 KBBerdir
#79 3098475-79-interdiff.txt1.38 KBBerdir
#79 3098475-79.patch11.43 KBBerdir
#76 3098475-interdiff-75-77.txt636 bytescatch
#76 3098475-77.patch11.43 KBcatch
#75 3098475-68-75-interdiff.txt1.38 KBcatch
#75 3098475-75.patch10.81 KBcatch
#70 contrib-only.png112.64 KBcatch
#70 core-and-contrib.png175.64 KBcatch
#70 core.png171.05 KBcatch
#68 core-message.png71.05 KBcatch
#68 contrib-message.png60.77 KBcatch
#68 3098475-interdiff-67-68.txt2.34 KBcatch
#68 3098475-68.patch10.8 KBcatch
#67 3098475-interdiff-9-38-66.txt2.6 KBcatch
#67 3098475-66.patch10.79 KBcatch
#64 last-removed-3098475-64-interdiff.txt1.33 KBTravisCarden
#64 last-removed-3098475-64-8.9.patch7.88 KBTravisCarden
#40 update_after.png133.73 KBBerdir
#40 update_before.png54.56 KBBerdir
#38 last-removed-3098475-38-8.9-interdiff.txt1.98 KBBerdir
#38 last-removed-3098475-38-8.9.patch7.88 KBBerdir
#38 last-removed-3098475-38-interdiff.txt1.83 KBBerdir
#38 last-removed-3098475-38.patch10.66 KBBerdir
#36 last-removed-3098475-36-interdiff.txt950 bytesBerdir
#36 last-removed-3098475-36.patch10.33 KBBerdir
#35 last-removed-3098475-35-interdiff.txt1.06 KBBerdir
#35 last-removed-3098475-35.patch10.3 KBBerdir
#34 last-removed-3098475-34-D8.patch7.77 KBBerdir
#34 last-removed-3098475-34-interdiff.txt2.72 KBBerdir
#34 last-removed-3098475-34.patch10.3 KBBerdir
#23 last-removed-3098475-23-interdiff.txt7.59 KBBerdir
#23 last-removed-3098475-23.patch8.57 KBBerdir
#17 Screenshot from 2020-01-24 10-49-07.png19.34 KBcatch
#16 last-removed-3098475-16-interdiff.txt3.33 KBBerdir
#16 last-removed-3098475-16.patch6.67 KBBerdir
#4 last-removed-3098475-4.patch5.69 KBBerdir
#11 last-removed-3098475-11.patch5.51 KBBerdir
#4 schema_version.png24.01 KBBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir created an issue. See original summary.

Berdir’s picture

catch’s picture

Priority: Normal » Critical

This should probably be critical since it's the only thing preventing sites from trying to update from too-old Drupal 8 versions.

Berdir’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
24.01 KB
5.69 KB

Going to come up with a decent message might be a bit tricky, as we only know schema versions and not which module version that correlat es to, but it's definitely better than HEAD already:

Patch against 9.0.x until #3106395: Move tests that test the update system to UpdateSystem namespace is backported.

I had to add a static cache reset of the installed versions to \Drupal\Tests\UpdatePathTestTrait::runUpdates() as manually setting a version polluted that and it assumed that the information hasn't been loaded yet.

andypost’s picture

Title: Add more strict checking of update_last_removed() and better explanation » Add more strict checking of hook_update_last_removed()() and better explanation
andypost’s picture

Title: Add more strict checking of hook_update_last_removed()() and better explanation » Add more strict checking of hook_update_last_removed() and better explanation

The hook docs could be improved

Status: Needs review » Needs work

The last submitted patch, 4: last-removed-3098475-4.patch, failed testing. View results

catch’s picture

The message in #4 looks good, although it would be better if it said 'Drupal' rather than 'system' there probably, but we can link off to a documentation page with more info.

Berdir’s picture

Yes, I thought we could special case system there and show a more specific message about updating to Drupal 8.8/8.9 first. I did fake it of course with the 9000, but once #3087644: Remove Drupal 8 updates up to and including 88** is in, it won't be just system but also 20 others. So I'm thinking that we check system first, if that is too old, we don't show any others and have a useful message, if we have other modules (more likely to be just one or few contrib modules), then we show a more generic message.

catch’s picture

I see a couple of options:

Option 1. Special case system, don't check any other modules if we find a problem, show a specific message that core is out of date. I think this is berdir's suggestion in #9.

One problem with this is that if you have say system and pathauto both needing updates, you could end up sorting out your core version, then trying again only to see a message for pathauto.

To try to avoid that:

Option 2. Special case everything with package: Core, show a single message for all of those. Show all other modules separately in their own entry.

The only problem with #2 is that nothing stops a contrib module from having 'Core' as its package, but they shouldn't really do that so might be fine?

Berdir’s picture

Status: Needs work » Needs review
FileSize
5.51 KB
+++ b/core/modules/system/system.install
@@ -2495,3 +2515,10 @@ function system_update_8805() {
+/**
+ * Implements hook_update_last_removed().
+ */
+function system_update_last_removed() {
+  return 9000;
+}

Well, d'oh, it would help if I'd remove my hack for testing this manually.

> One problem with this is that if you have say system and pathauto both needing updates, you could end up sorting out your core version, then trying again only to see a message for pathauto.

Yeah, that could happen, not quite sure what I think about it. Will need to see if checking the package makes the patch more complicated, probably would have to collect all problems first anyway and in with the package option group them by $problems[$package][$module] or so and then build.

catch’s picture

Going to come up with a decent message might be a bit tricky, as we only know schema versions and not which module version that correlat es to

I think given we're going to hard-code something for core, we can add even more hard-coding for the core error message too.

We know that we're only mass-removing updates in 9.0.x, so if the current version is 9* and system schema is 8* we can explicitly say something like this:

Drupal 9 can only be updated to from a version of Drupal 8.8.0 or higher. If you are trying to upgrade from an older version, please update to the latest release of Drupal 8 first.

xjm’s picture

I think this is a beta blocker (maybe a should have).

xjm’s picture

Issue tags: +Needs usability review
xjm’s picture

FWIW the message in #12 seems like a good start.

Berdir’s picture

#12: Yes exactly, that was my idea, the message looks great to me as well. I kept the title for now but improved it with the readable module name not the machine name.

Once we have the last update hooks in place I can also easily test that specific integration for system.module. We could in theory also commit this and then update the tests in the other issue, but that will be harder to review.

catch’s picture

Here's what that looks like.

I think we should change the requirements title from 'Unsupported schema version: system', to 'The version of Drupal you are trying to update from is too old' but getting very close.

We also need a link to a page like this for more explanation, but that page does not exist yet afaik: https://www.drupal.org/docs/8/update

+1 to getting the update removal patch in first.

Berdir’s picture

Thanks for the review, I'll finish this up when the other issue is in.

xjm’s picture

Test coverage-wise, we will eventually want something like what @catch said on the other issue:

So thinking more, one test we'll probably want is an 8.7.0 database, attempt an upgrade to 9.0.x and see the hook_update_last_removed() error.

And probably a test with the 8.8 fixtures that simply assert the same message isn't there.

catch’s picture

I think we can just reset a load of core module schema versions prior to hitting update.php rather than using a real 8.7 fixture, but should definitely have that coverage yeah.

Berdir’s picture

> And probably a test with the 8.8 fixtures that simply assert the same message isn't there.

Well, the requirements page never shows if there are no errors, so it is implicitly tested by the fact that all other tests can still update. You can see how badly that #4 failed because I forgot to remove test system_update_last_removed() that I added for testing.

And yes, my plan was to fake the installed schema version to 87something just like I'm already doing for the test module, but what I can't fake is the hook_update_last_removed(), so I need that to be committed (pretty please :))

xjm’s picture

In addition to resolving this issue with the UI for update.php, we also need to test what happens when people try to update with Drush. =/

Berdir’s picture

Yay, the blocker is in. Here's an updated patch with a few bugs fixed and extend test coverage for the core message, also included the new title.

This can be backported to Drupal 8, but I'll need to remove the special case for system and corresponding test coverage, if that's OK I'll wait until this is in or at least RTBC in case we need to make more changes to the wording or so.

I also removed the system_update_9000() that is now not needed anymore.

> In addition to resolving this issue with the UI for update.php, we also need to test what happens when people try to update with Drush. =/

True, I think drush does not actually check update requirements, but that can only be fixed by drush and does by far not only affect this issue.

Berdir’s picture

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

Setting the version to 9.0.x for now.

Berdir’s picture

DamienMcKenna’s picture

Tagging as a requirement for Drupal 9.0-beta1.

larowlan’s picture

This looks good, but looks like we'll need an 8.9 specific patch?

Given we're only adding this to 8.9 and 8.8...don't we also need to add this to 8.7 for it to be effective? i.e. if this only gets into the next point release of 8.8, doesn't that defeat the purpose?

  1. +++ b/core/modules/system/system.install
    @@ -1076,6 +1076,51 @@ function system_requirements($phase) {
    +      if ($last_removed && $last_removed > drupal_get_installed_schema_version($module)) {
    ...
    +          'installed_version' => drupal_get_installed_schema_version($module),
    

    should we put this in a local variable to save looking it up twice?

  2. +++ b/core/modules/system/system.install
    @@ -1076,6 +1076,51 @@ function system_requirements($phase) {
    +        'description' => t('Drupal 9 can only be updated to from a version of Drupal 8.8.0 or higher. If you are trying to upgrade from an older version, please update to the latest release of Drupal 8 first. '),
    

    should we be hard-coding these version numbers? is this something that should evolve for D9-D10 too, or is that getting ahead of ourselves

Berdir’s picture

Thanks for the review.

As discussed in slack, this is something that is triggered after they updated the code to D9, so that's fine. Per #23, I did plan to provide a D8 patch once this is RTBC, to avoid extra rerolls as the 8.x patch will be quite different.

#27.1: I thought about that, but drupal_get_installed_schema_version() has a static cache.
#27.2: Hm, maybe just put the version numbers in variables so the message doesn't need to be translated but keep them hardcoded, we don't know for sure yet which exactly will be the supported min-version for updating and so on. Also noticed an extra space there.

catch’s picture

Yes the 9.x commit is a critical blocker for 9.x. The 8.9.x backport is a proper bug fix, but will only help site owners who update contributed modules that have removed hook_update_N() - core won't remove any hook_update_N() in 8.9.x so it doesn't affect 8.x core as such.

Putting the version numbers in variables for that message seems like a good plan to save translation changes later.

alexpott’s picture

Status: Needs review » Needs work

This is replacing code that currently doesn't work very well in update.inc.

      // \Drupal::moduleHandler()->invoke() returns NULL for non-existing hooks,
      // so if no updates are removed, it will == 0.
      $last_removed = \Drupal::moduleHandler()->invoke($module, 'update_last_removed');
      if ($schema_version < $last_removed) {
        $ret[$module]['warning'] = '<em>' . $module . '</em> module cannot be updated. Its schema version is ' . $schema_version . '. Updates up to and including ' . $last_removed . ' have been removed in this release. In order to update <em>' . $module . '</em> module, you will first <a href="https://www.drupal.org/upgrade">need to upgrade</a> to the last version in which these updates were available.';
        continue;
      }

I think we need to remove that hunk because as it stands it doesn't work and with this change is superfluous. I've not reviewed the change but I agree with the idea of moving this check to hook_requirements. I think the idea of the current code is to allow the user to perform some of the available updates and continue soldiering on - but I think that that solution is from the world were you can disable a module and have it magically come back to life when it's being updated.

xjm’s picture

Status: Needs work » Needs review
Issue tags: -Drupal 9.0.0-beta1 requirement
Related issues: +#3100110: Convert update_calculate_project_update_status() into a class

Will this conflict wit #3100110: Convert update_calculate_project_update_status() into a class? If so we should probably land that one first.

xjm’s picture

Status: Needs review » Needs work
Issue tags: +Drupal 9.0.0-beta1 requirement

Didn't mean to change status/tags.

catch’s picture

@xjm #31 no that's entirely within update module (from a quick look), where as this is entirely within database update system.

Berdir’s picture

Adding placeholders and removing the part in update.inc. Thought about that but wasn't sure.

Btw, I did look into drush update requirements a bit and at least drush9 does check them, but also allows to skip over errors and it's fairly common to run it with -y, then it just skips over it. Might need to be improved a bit, but it's also tricky, I noticed it because google_tag has a bogus check where it has an error for something that it fixes in its own update function later on.

Also adding a patch for D8, without the system.module special case and corresponding test coverage.

Berdir’s picture

And now without the typo. The D8 patch is still good as it doesn't have this part.

Berdir’s picture

And now also without coding standard errors. That does affect the D8 patch but can possibly be fixed on commit if there's nothing else.

alexpott’s picture

  1. This is looking pretty good. The message and way we fail is much more helpful than current behaviour.
  2. +++ b/core/modules/system/system.install
    @@ -1044,6 +1044,55 @@ function system_requirements($phase) {
    +        'description' => t('Drupal @current_major can only be updated to from a version of Drupal @required_min_version or higher. If you are trying to upgrade from an older version, please update to the latest release of Drupal @previous_major first.', [
    

    I was pondering if this should be tested on not. Not sure. At least the title is tested so we have test coverage that the requirement is being triggered.

  3. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdatePathLastRemovedTest.php
    @@ -0,0 +1,91 @@
    +    $this->updateUrl = $GLOBALS['base_url'] . '/update.php';
    

    Let's not use the global. Let's do Url::fromRoute('system.db_update');

  4. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdatePathLastRemovedTest.php
    @@ -0,0 +1,91 @@
    +    // Set the expected schema version for the node and test module, updates are
    +    // successful now.
    +    drupal_set_installed_schema_version('update_test_last_removed', 8002);
    +    drupal_set_installed_schema_version('node', 8700);
    +
    +    $this->runUpdates();
    +  }
    

    Let's assert that update_test_last_removed's schema is 8003 after running the updates. It feels nice to have a positive assertion after calling $this->runUpdates() - yes that does checking that updates a successful but still.

Berdir’s picture

Thanks for the review.

#37.2 Added an assert on the final string, doesn't hurt especially now that we have these placeholders.
#37.3 Fixed. Seems like we have a few identical setUp() methods, could almost put that in the trait too..
#37.4: Added another assert.

Also updated the D8 patch where applicable, I realized this only works in 8.9 as we didn't backport that update test trait?

xjm’s picture

Issue tags: +Needs screenshots

We need some "after" screenshots here and a UX review once the patch is ready-ish.

Berdir’s picture

Issue tags: -Needs screenshots
FileSize
54.56 KB
133.73 KB

I think this is ready now, it has been reviewed by @catch, @alexpott and @larowlan already and the last review was only nitpicks.

Who is doing UX reviews these days? I understand that's important, but this is also a very obvious bug and the current state is an mess. There are also no new UI elements here, just new text.

The new screens are based on an actual D8.7 installation that I updated, so it's a little bit different than the screenshot in the issue summary in the before screenshot.

Before:

you only get a warning that doesn't prevent you from doing the update and that warning only shows up on a later step when the updates are listed:

After:

Update.php stops on the requirements page and displays an error that doesn't allow you to continue:

Arguably we could consider to remove the existing "Minimum schema version" check at the bottom of the screenshot that is done by update_system_schema_requirements(), not only is that now redundant, it's also literally impossible to get that far as anything with a schema version of <8000 is a D7 site with a completely different schema version storage that D8/9 can not access (the system table). But we already have #3106712: Decide what to do with Drupal::CORE_MINIMUM_SCHEMA_VERSION and surrounding logic for that.

catch’s picture

For me the patch and the screenshot both look good.

Agree with leaving the minimum schema version stuff to #3106712: Decide what to do with Drupal::CORE_MINIMUM_SCHEMA_VERSION and surrounding logic.

catch’s picture

There is one thing to talk about here for the 8.9.x backport.

Currently, if a contrib module has implemented hook_update_last_removed() and you're trying to skip an update, update.php lets you run the other updates anyway. You could then theoretically downgrade the contrib module and run its updates again later. (Note I am not 100% sure this is possible in practice).

With this change, update.php just won't let you run updates at all any more - you have to fix the situation before you can proceed.

This is 100% necessary for core to stop people from completely destroying their databases. I think it's a very good change to make for contrib modules too, since it will either discover incorrect hook_update_last_removed() implementations or prevent data issues with missing contrib updates too.

However if you have an existing 8.x site which already has a wrongly-updated contrib module, you will previously have been able to keep updating core (with this warning every time), but now you wouldn't. For me that's a really good reason to avoid backporting this to 8.8.x in case someone got caught out when the next security release comes out, I think it's a good bugfix for 8.9.x but wanted to document what could potentially happen.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Marking this RTBC from a code standpoint. We can discuss the 8.9.x backport more once it's in 9.0.x

Would be good to get an additional UX opinion on the text here since we really want people who run into this to be able to figure out what to do next.

benjifisher’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs issue summary update, +DrupalCampNJ2020

I am setting the status back to NW: not for anything in the code, but for the issue summary (IS). Before a committer looks at this issue, the summary should include some screenshots, probably under the "User interface changes" section. That should also be done before the usability review, and the usability review should happen before RTBC.

I am reading through the comments now. When I am done, I will update the IS and do a usability review. If someone who has already worked on this issue updates the IS before I get to it, that might be better.

benjifisher’s picture

I tested running drush updatedb with and without the patch. I am using Lando locally, and d is an alias for lando drush. My drush version is 10.2.0.

Upgrade from Drupal 8.9.x (Umami profile) to 9.0.x without the patch:

$ d updatedb
 -------- ----------- --------------- ----------------------------------------------------------- 
  Module   Update ID   Type            Description                                                
 -------- ----------- --------------- ----------------------------------------------------------- 
  system   9000        hook_update_n   Ensures that Drupal is updated from a supported version.   
 -------- ----------- --------------- ----------------------------------------------------------- 


 Do you wish to run the specified pending updates? (yes/no) [yes]:
 > 

>  [notice] Update started: system_update_9000
>  [notice] Update completed: system_update_9000
 [success] Finished performing updates.

Upgrade from Drupal 8.9.x (Umami profile) to 9.0.x with the patch:

$ d updatedb
 [success] No pending updates.

Upgrade from Drupal 8.7.x (Umami profile) to 9.0.x without the patch:

$ d updatedb
 [warning] The following module is missing from the file system: demo_umami_tour bootstrap.inc:192
 -------------------- ------------------------------------------- ------------- ----------------------------------------------------------------------------- 
  Module               Update ID                                   Type          Description                                                                  
 -------------------- ------------------------------------------- ------------- ----------------------------------------------------------------------------- 
  content_moderation   views_field_plugin_id                       post-update   Update the moderation state views field plugin ID.                           
  media                add_status_extra_filter                     post-update   Add a status extra filter to the media view default display.                 
  path                 create_language_content_settings            post-update   Create the language content settings configuration object for path aliases.  
  search               block_page                                  post-update   Configures default search page for instantiated blocks.                      
  system               entity_reference_autocomplete_match_limit   post-update   Populate the new 'match_limit' setting for the ER autocomplete widget.       
  system               extra_fields_form_display                   post-update   Update all entity form displays that contain extra fields.                   
  system               layout_plugin_schema_change                 post-update   Clear the schema cache.                                                      
  taxonomy             configure_status_field_widget               post-update   Add status with settings to all form displays for taxonomy entities.         
  text                 add_required_summary_flag                   post-update   Update text_with_summary fields to add summary required flags.               
  text                 add_required_summary_flag_form_display      post-update   Update text_with_summary widgets to add summary required flags.              
  views                limit_operator_defaults                     post-update   Define default values for limit operators settings in all filters.           
  views                remove_core_key                             post-update   Remove core key from views configuration.                                    
 -------------------- ------------------------------------------- ------------- ----------------------------------------------------------------------------- 


 Do you wish to run the specified pending updates? (yes/no) [yes]:
 > 

 [warning] The following module is missing from the file system: demo_umami_tour bootstrap.inc:192
>  [notice] Update started: system_update_9000
>  [notice] Update completed: system_update_9000
>  [notice] Update started: content_moderation_post_update_views_field_plugin_id
>  [notice] Update completed: content_moderation_post_update_views_field_plugin_id
>  [notice] Update started: media_post_update_add_status_extra_filter
>  [notice] The 'Published status or admin user' filter was added to the Media view.
>  [notice] Update completed: media_post_update_add_status_extra_filter
>  [notice] Update started: path_post_update_create_language_content_settings
>  [error]  The "path_alias" entity type does not exist. 
>  [error]  Update failed: path_post_update_create_language_content_settings 
 [error]  Update aborted by: path_post_update_create_language_content_settings 
 [error]  Finished performing updates. 

Upgrade from Drupal 8.7.x (Umami profile) to 9.0.x with the patch (responding "yes" to each prompt):

$ d updatedb
 [warning] The following module is missing from the file system: demo_umami_tour bootstrap.inc:192
 [error]  Drupal 9 can only be updated to from a version of Drupal 8.8.0 or higher. If you are trying to upgrade from an older version, please update to the latest release of Drupal 8 first. 
 [error]  The installed version of the <em class="placeholder">Interface Translation</em> module is too old to update. Update to an earlier version first (last removed version: 8800, installed version: 8500). 

 Requirements check reports errors. Do you wish to continue? (yes/no) [yes]:
 > 

 -------------------- ------------------------------------------- ------------- ----------------------------------------------------------------------------- 
  Module               Update ID                                   Type          Description                                                                  
 -------------------- ------------------------------------------- ------------- ----------------------------------------------------------------------------- 
  content_moderation   views_field_plugin_id                       post-update   Update the moderation state views field plugin ID.                           
  media                add_status_extra_filter                     post-update   Add a status extra filter to the media view default display.                 
  path                 create_language_content_settings            post-update   Create the language content settings configuration object for path aliases.  
  search               block_page                                  post-update   Configures default search page for instantiated blocks.                      
  system               entity_reference_autocomplete_match_limit   post-update   Populate the new 'match_limit' setting for the ER autocomplete widget.       
  system               extra_fields_form_display                   post-update   Update all entity form displays that contain extra fields.                   
  system               layout_plugin_schema_change                 post-update   Clear the schema cache.                                                      
  taxonomy             configure_status_field_widget               post-update   Add status with settings to all form displays for taxonomy entities.         
  text                 add_required_summary_flag                   post-update   Update text_with_summary fields to add summary required flags.               
  text                 add_required_summary_flag_form_display      post-update   Update text_with_summary widgets to add summary required flags.              
  views                limit_operator_defaults                     post-update   Define default values for limit operators settings in all filters.           
  views                remove_core_key                             post-update   Remove core key from views configuration.                                    
 -------------------- ------------------------------------------- ------------- ----------------------------------------------------------------------------- 

 Do you wish to run the specified pending updates? (yes/no) [yes]:
 > 

 [warning] The following module is missing from the file system: demo_umami_tour bootstrap.inc:192
>  [notice] Update started: content_moderation_post_update_views_field_plugin_id
>  [notice] Update completed: content_moderation_post_update_views_field_plugin_id
>  [notice] Update started: media_post_update_add_status_extra_filter
>  [notice] The 'Published status or admin user' filter was added to the Media view.
>  [notice] Update completed: media_post_update_add_status_extra_filter
>  [notice] Update started: path_post_update_create_language_content_settings
>  [error]  The "path_alias" entity type does not exist. 
>  [error]  Update failed: path_post_update_create_language_content_settings 
 [error]  Update aborted by: path_post_update_create_language_content_settings 
 [error]  Finished performing updates. 
benjifisher’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

A little warning for anyone trying to reproduce the tests manually: Drupal 9.0.x requires PHP 7.3, and Drupal 8.7.7 does not work with PHP 7.3, so switching back and forth is challenging. At least, I think that is what caused my problems. I did not have a particularly good reason for choosing 8.7.7, so maybe a later patch release would not have this problem.

I also tested upgrading through the UI, and got pretty much the same results as in #40, so I will add those screenshots to the IS. Since I was testing with the Umami profile, I got an additional error message when testing with this patch:

Unsupported schema version: Interface Translation
The installed version of the Interface Translation module is too old to update. Update to an earlier version first (last removed version: 8800, installed version: 8500).

Berdir’s picture

> and Drupal 8.7.7 does not work with PHP 7.3

8.7 should work fine with PHP 7.3?

Testing with drush isn't that meaningful. You're not supposed to be able to skip this error, that can not and will not work, but that's not something that we can do anything about here.

Yes, I realized that the package does not cover all core modules, I'd need a different check for that. Either I'm dropping that group again and then I'll just _only_ show system if that's in the list, even if there's a contrib module in there as well or find another way to group them just by core/not core.

Also, that module-does-not-exist warning is another thing that we're dealing with in another issue and too should block the update.

Also, in regards to 8.8, backporting this to 8.8 would be annoying anyway, as we didn't backport all these update path test changes and it would require quite a different test.

benjifisher’s picture

Issue summary: View changes
Issue tags: -Needs usability review +Needs screenshots, +Needs issue summary update

Usability review

The text in the screenshot (IS and #40) meets the primary requirements: it explains what is wrong, and it tells the reader how to fix it:

Drupal 9 can only be updated to from a version of Drupal 8.8.0 or higher. If you are trying to upgrade from an older version, please update to the latest release of Drupal 8 first.

I have a few quibbles with the wording:

  • The usual advice is to avoid saying "please" in this sort of message. The reader will not be doing us a favor, and the word does not add any real information.
  • The word "release" is used as a synonym for "version". IMO it is more helpful to be consistent than to avoid using the same word multiple times.
  • If we are going to use "update" in the title text, then let's use that in the description text, too, instead of "upgrade".
  • The "to from" in the first sentence is awkward.
  • I think "Drupal version 8.8.0" is clearer than "a version of Drupal 8.8.0".

I suggest something like the following:

Updating to Drupal 9 is only supported from Drupal version 8.8.0 or higher. If you are trying to update from an older version, first update to the latest version of Drupal 8.

Is there any reason to recommend "the latest release"? If we recommend upgrading to a specific version, such as 8.8.0, then

  • We can also give a link to the release page for that version.
  • We can test the heck out of the upgrade path from 8.8.0 to 9.0.0.
  • Giving the reader less choice may be an improvement.

I guess one advantage of recommending the latest release is that it should be more secure, and there is a chance that someone will get stuck on the intermediate version, even though their goal is to get to D9.

I will remove the tag for usabillity review, but keep the status at NW and add tags for new screenshots and an IS update, since we will need to update the IS with new screenshots (or maybe just one) once the wording has been updated. I will also add those steps to the IS.

benjifisher’s picture

8.7 should work fine with PHP 7.3?

In that case, I do not know why I had so much trouble.

Testing with drush isn't that meaningful.

These issue comments are a form of documentation. There has already been some discussion of "drush updatedb" in earlier comments, so someone having problems with that process might find this page with a search engine. It might help that person if we record the expected results here.

I am not asking for any changes beyond the rewording.

benjifisher’s picture

Issue summary: View changes

The screenshots only show the messages from trying to update (upgrade) Drupal core. Peeking at the code, I see

+          'title' => t('Unsupported schema version: @module', ['@module' => $data['info']->info['name']]),
+          'description' => t('The installed version of the %module module is too old to update. Update to an earlier version first (last removed version: @last_removed_version, installed version: @installed_version).', [

Maybe "intermediate" would be better than "earlier". Can we also get a screenshot of that message, and add it to the IS? Is there a "before" version for comparison?

IMO we should end the last sentence with "first." and admit that the part in parentheses is neither a complete sentence nor part of one. I do not care strongly, so leave it as is if you disagree.

Berdir’s picture

Yes, the second message is shown for other modules, you had it too in your drush output. I just meant that "ignoring" that error with drush is too easy, so what you did shouldn't be done and is IMHO slightly misleading, your site will be very broken if you try that. Sorry if that sounded defensive, was just trying to point that out.

> Is there any reason to recommend "the latest release"? If we recommend upgrading to a specific version, such as 8.8.0, then

I think that's what we support vs. what we recommend. IMHO, the best course of action is to update to the latest D8 version including updating all contrib modules first, and only then update to D9. See also #2942096-28: [policy, no patch] Remove old update hooks prior to each major version (Drupal 10 and later) (comment 28 specifically but the whole discussion there is relevant).

Especially for sites that had errors or problems updating to 8.7/8.8, we don't want themto update to 8.8.0 but the latest version that might have some fixes for the open critical upgrade path issues that we have.

We could for example link to https://www.drupal.org/project/drupal/releases?api_version%5B%5D=7234, which should then always show the most recent D8 release on top? Assuming the URL structure/ID isn't going to change in the future, but worst case is they can filter themself.

As a non-native speaker, not sure if I understand all your feedback on the message, would be helpful if you could provide a complete sentence of your recommendation, then I can update the patch. Will also check with @catch about the non-core-package-but-core-modules problem.

catch’s picture

@benjifisher

Is there any reason to recommend "the latest release"? If we recommend upgrading to a specific version, such as 8.8.0

Security is one reason and you also mentioned that. People might do that update on production.

The other reason is that we have outstanding critical upgrade path bugs that will be backported to 8.8.x patch releases. In some cases a site won't be able to update to 8.8.0 but will be able to update to say 8.8.4.

Updating to Drupal 9 is only supported from Drupal version 8.8.0 or higher. If you are trying to update from an older version, first update to the latest version of Drupal 8.

This wording change seems good to me.

@berdir

Also, in regards to 8.8, backporting this to 8.8 would be annoying anyway

I think we definitely shouldn't backport this to 8.8.x for the reasons given in #3098475-42: Add more strict checking of hook_update_last_removed() and better explanation.

Yes, I realized that the package does not cover all core modules, I'd need a different check for that. Either I'm dropping that group again and then I'll just _only_ show system if that's in the list, even if there's a contrib module in there as well or find another way to group them just by core/not core.

Let's show just system for now, but then open a follow-up to try find a better way to group into core/not-core. It would be nice to get the core/not-core split in, but it's more important not to show messages for individual core modules.

benjifisher’s picture

Issue summary: View changes

From #51:

I just meant that "ignoring" that error with drush is too easy, so what you did shouldn't be done ...

I agree.

You convinced me that "latest version" is the right recommendation.

We could for example link to ...

Adding a link is a great idea. Either "latest version" or "latest version of Drupal 8" could be used as the link text. This would solve the problem of having too much choice; I was trying to solve that by suggesting a fixed version, like 8.8.0. A simpler link would be https://www.drupal.org/8/download, which redirects to the release notes for the latest version.

Aside: this redirect was added not so long ago ... some time in 2019, maybe. The main menu of d.o, Try Drupal > Download, leads to https://www.drupal.org/download, and the link I suggested is there with the link text "read release notes". (I asked on Slack whether we can count on this redirect after the release of D9.)

I updated the "Remaining tasks" in the IS. I hope that is clearer.

I do not have an opinion on core/non-core packages, nor whether to backport to 8.8. It looks as though you are reaching agreement on those points.

catch’s picture

Linking to https://www.drupal.org/8/download is a good idea.

benjifisher’s picture

I asked on Slack whether we can count on this redirect after the release of D9.

I am glad I asked. I am adding #3111136: Update /8/download path to be accurate as a related issue.

catch’s picture

It looks like there isn't a URL we can reliably link to from core at the moment, however Gabor just updated: https://www.drupal.org/docs/9/how-to-prepare-your-drupal-7-or-8-site-for...

So... I think we should link to there, and then we can link from that page to the latest release link when it's available too.

benjifisher’s picture

Right. I think the resolution of #3111136: Update /8/download path to be accurate is now clear, and we should not use /8/download here.

benjifisher’s picture

Issue summary: View changes

If the link goes to https://www.drupal.org/project/drupal/releases?api_version%5B%5D=7234, then we should use "latest version of Drupal 8" or just "Drupal 8" as the link text.

If the link goes to https://www.drupal.org/docs/9/how-to-prepare-your-drupal-7-or-8-site-for..., then we should use "Updating to Drupal 9" as the link text.

I updated the IS again, suggesting those links.

From #40:

Who is doing UX reviews these days?

Sorry, I never replied to that. In theory, an issue will get some attention if it is tagged "Needs usability review" and the status is NR. In practice, it helps to post a message to the #ux channel in Drupal Slack. I think that @catch did that, which is how I noticed this issue.

xjm’s picture

We can also bring this issue to next week's UX meeting, which is the best way to get review from the team and discuss suggestions in real time.

FWIW, I think

Updating to Drupal 9 is only supported from Drupal version 8.8.0 or higher. If you are trying to update from an older version, first update to the latest version of Drupal 8.

is a good suggestion, so let's start with that.

In terms of links, linking the upgrade docs is a great idea and we should add that link. Linking the d.o list of releases is not a good idea; instead, the words "update to the latest version of Drupal 8" could link to the update status report.

dww’s picture

Re:

instead, the words "update to the latest version of Drupal 8" could link to the update status report.

Except Update module is optional, many sites don't run it, and that'd be a dead link for those sites... :/

Linking to https://www.drupal.org/project/drupal/releases?api_version%5B%5D=7234 is definitely gross, and we're trying to get rid of "api_version" everywhere on d.o for semver contrib releases.

Linking to https://www.drupal.org/8/download seems like the safest, best course of action IMHO.

Cheers,
-Derek

catch’s picture

We can't link to the update status report, because we're dealing with a non-upgradeable Drupal 8 database running on a Drupal 9 codebase - it might well fatal. Also the site might be in maintenance mode with a logged out user etc.

drupal.org/8/download is going to change according to issues linked above, can't link to it. The only place to link to 'latest' I can think of would be https://drupal.org/project/drupal - which might actually be fine? If not I think we should stick with the link to the d.o documentation and add more/better links in a follow-up if/when we have them.

xjm’s picture

xjm’s picture

As in:

Updating to Drupal 9 is only supported from Drupal version 8.8.0 or higher. If you are trying to update from an older version, first update to the latest version of Drupal 8. (<a href=":url">Drupal 9 upgrade guide</a>)

Where :url is the link in #62.

Let's create a prototype implementing that for UX review?

WRT the drush testing, I think what's in scope is to file issues in the Drush queue if Drush bypasses it.

TravisCarden’s picture

Updated patch for @xjm's feedback.

Berdir’s picture

Status: Needs review » Needs work

I lost track a bit, but I think we also need to adjust the other message a bit, see #50 and following comments. And tests might need an update for the string change too?

xjm’s picture

@Berdir I +1ed the message be used as a starting point in #59 and no one pushed back on anything but the link target. That's the message @TravisCarden used. If there's a suggestion for an improvement let's make a specific proposal.

It does look like we need a test fix too here though.

catch’s picture

Status: Needs work » Needs review
FileSize
10.79 KB
2.6 KB

#64 looks like a re-roll of the 8.9.x patch, but we need to change the 9.0.x patch instead.

catch’s picture

Missed the suggestion in the issue summary to change the contrib wording from 'earlier version' to 'intermediate version' - intermediate is probably better.

catch’s picture

Issue summary: View changes
Issue tags: -Needs screenshots
catch’s picture

Issue summary: View changes
FileSize
171.05 KB
175.64 KB
112.64 KB

Redoing the screenshots.

Note I hacked workspaces around to make it a 'contrib' module so pretend it has another name.

Also including when a core and contrib module are found together.

catch’s picture

Issue summary: View changes
catch’s picture

Also note the workspaces message is the generic contributed module one. For #3108416: Remove workspace_update_8803() we would customize so that if we hit this condition, people see the core message (or other custom text tbd, but maybe the core message is actually best).

edit: realised after typing that we'll need a message that looks almost exactly the same as the core message, but with 8.8.2 instead of 8.8.0 as the minimum version.

The last submitted patch, 67: 3098475-66.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 68: 3098475-68.patch, failed testing. View results

catch’s picture

Fixing the oversight with node module string.

Haven't debugged the rest settings update test yet.

catch’s picture

Should fix the REST update test.

The last submitted patch, 75: 3098475-75.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 76: 3098475-77.patch, failed testing. View results

Berdir’s picture

Status: Needs work » Needs review
FileSize
11.43 KB
1.38 KB

Fixed that last test fail, just needs an update for the new wording.

That said, @catch pointed out in slack that the current logic groups by package to avoid showing a a whole bunch of messages instead of just the special one for core. This does have a small problem though, because core has a bunch of modules that are not in the Core package but e.g. Multilingual. So if e.g. language.module would have a relevant last-updated hook and your site is old enough, you'd see system and a separate message for language module. That was also already discussed a bit in #46/#47.

There are a few in there with high last update removed numbers, e.g. locale has 8800, file has 8700. So multilingual 8.7 sites would see messages for that, and pretty much all 8.6 and older sites for file.

Options:
* Ignore it, the main goal here is to not flood you with a dozne errors, 2-3 messages might be OK? System should also be the first in the list.
* Switch to checking for modules in core/..., that means we can no longer test the behavior of non-core modules as the test module is in core too. Maybe with some tricks like putting a module in the sites folder, but not sure if that's worth the trouble?
* Drop the grouping, if system is in there, only show that. Worst case scenario: Someone has system and some contrib module that would show a message too. They go back to 8.9 or so, then they get the message for the contrib module and need to downgrade that as well.

Thoughts?

alexpott’s picture

I like

Drop the grouping, if system is in there, only show that. Worst case scenario: Someone has system and some contrib module that would show a message too. They go back to 8.9 or so, then they get the message for the contrib module and need to downgrade that as well.

It's less complex and means that people get the foundations (i.e. core) in order first. If they have a contrib module in the same situation they'll get that warning next and then deal with it.

Berdir’s picture

Ok, here's a patch for that. The test now has one case less where a core module would show a message, it's just either the specific one for core or the generic message for the test module.

catch’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Removed the 'core and contrib' screenshot from the issue summary, since we no longer show both messages at once.

I think the short-circuit on System is the least-unfriendly thing we can do here.

I only applied minor string changes and some test debugging here so I think I'm allowed to RTBC.

alexpott’s picture

Version: 9.0.x-dev » 8.9.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed 5d4c083 and pushed to 9.0.x. Thanks!

I think we should backport this to 8.9.x too.

  • alexpott committed 5d4c083 on 9.0.x
    Issue #3098475 by Berdir, catch, TravisCarden, xjm, benjifisher,...
catch’s picture

Status: Patch (to be ported) » Needs work

#3098475-39: Add more strict checking of hook_update_last_removed() and better explanation has an 8.9 patch, although it will need updating for the latest changes to the 9.0.x patch too. Moving to 'needs work'.

Berdir’s picture

Updated last-removed-3098475-38-8.9.patch with intermediate instead of earlier. that should be all?

catch’s picture

Status: Needs review » Reviewed & tested by the community

Yes I think that's it.

benjifisher’s picture

Since #3087644: Remove Drupal 8 updates up to and including 88** was committed to 9.0.x but not 8.9.x, why do we need this issue to be back-ported? Is it to keep the promise that 9.0 will be just like 8.9 except for deprecated code and updated dependencies?

catch’s picture

@benjifisher it fixes the handling for contrib modules that may have implemented hook_update_last_removed(), so it's worth backporting as a bugfix rather than purely to keep things in sync.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

A couple blocking points:

  1. What happened to the special message for core? It's in screenshots in the IS, but does not appear anywhere in the current patch. From a UX perspective, the custom core message was very useful.

  2. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdatePathLastRemovedTest.php
    @@ -0,0 +1,72 @@
    +    // Access the update page with too old versions for the
    +    // test module.
    

    Wrapping early and also not really grammatical. I'd say:

    Access the update page with a schema version that is too old for the test module.

  3. +++ b/core/tests/Drupal/Tests/UpdatePathTestTrait.php
    @@ -59,6 +59,7 @@ protected function runUpdates($update_url = NULL) {
    +      drupal_get_installed_schema_version(NULL, TRUE);
    

    The purpose of this line is presumably to rebuild the static cache in the test trait. Seems like this should have an inline comment.

    Also, this is an expensive operation, so justifying it would be a good idea. Is there a reason the entire trait now requires it, vs. just the one test?

Miscellaneous trivial docs nitpicks:

  1. +++ b/core/modules/system/tests/modules/update_test_last_removed/update_test_last_removed.info.yml
    @@ -0,0 +1,5 @@
    +name: 'Update test with update_last_removed implementation'
    

    Nit missing parens for hook_update_last_removed().

  2. +++ b/core/modules/system/tests/modules/update_test_last_removed/update_test_last_removed.install
    @@ -0,0 +1,20 @@
    + * Install, update and uninstall functions for the update_test_invalid_hook module.
    

    Nit: missing comma after "update".

  3. +++ b/core/modules/system/tests/modules/update_test_last_removed/update_test_last_removed.install
    @@ -0,0 +1,20 @@
    + * Implements hook_update_last_removed()
    

    Nit: missing period.

  4. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdatePathLastRemovedTest.php
    @@ -0,0 +1,72 @@
    +   * Test that a module with a too old schema version can not be updated.
    

    (Tests that...)

Thanks!

xjm’s picture

Issue tags: +Needs usability review

If we're eliminating the user-friendly message for core from the patch, this is going to need another round of usability review, so tagging accordingly.

Berdir’s picture

@xjm:

1. This is the Drupal 8 backport, the core specific message is only in Drupal 9, it doesn't make sense for Drupal 8 as there are no removed updates in Drupal 8. The Drupal 9 patch was already committed. Some of the nitpicks apply there as well, we can apply that there too.

3. It's in the trait because there's no other way how we could intercept it just at that moment, everything happens in the trait. This is a trait-only thing and it's really not that expensive, it's just a single query that we have to do again, plus merging the result with the module list. It's just a static cache because it is called a lot while running updates and there's no reason to run the query again, except in this case.

Berdir’s picture

Issue tags: -Needs usability review
webchick’s picture

We reviewed this on today's UX meeting (video link forthcoming), based on the issue summary (which I'm worried now after reading the most recent comments was not the right thing to be reviewing..?)

+1 to the core wording in the "after" screenshot. That's extremely helpful, and very clear what the next step is.

The contrib wording in the "after" screenshot is comparably "meh," but my understanding from @benjifisher is that's about the best we can do based on what info the hook provides, until/unless #3117789: Allow hook_update_last_removed() to provide version information gets done.

The only additional recommendation we had was to link the module name to the project page, as the module maintainer could theoretically put "known issues" types of information there to recommend specific next steps for people on older versions. (We briefly discussed linking to release notes instead, but can't guarantee that the release notes for 2.2.23 will repeat everything that was in 2.2.17 (or whatever), which was the thing that actually removed the update.)

xjm’s picture

As discussed in #3066801: Add hook_removed_post_updates(), linking the project page isn't feasible ATM. @catch filed #3118128: Add 'homepage' to .info.yml which might allow it to be addressed in the future, but there are potential concerns with that as well.

Berdir’s picture

Updated the nitpicks. Didn't now the grammar thing with update, and, but I changed it to just Update functions, it was over 80 lines and we won't ever add install or uninstall functions there anyway ;)

Also attaching a patch for those nitpicks for 9.0.

Status: Needs review » Needs work

The last submitted patch, 96: last-removed-3098475-96.patch, failed testing. View results

Berdir’s picture

Duh, forgot to update the test assertion for the changed module name. So, D8.9 and 9.0 patches and interdiffs.

benjifisher’s picture

In #48, I added the "Needs issue summary update" tag and added a few items to the "Remaining tasks" list. Since then, the tasks have been removed, but the issue tag is still there. If the issue summary is up to date (current screenshots and so on) then please remove the tag.

catch’s picture

Yes we're all up-to-date here, just minor clean-ups in 9.0/8.9 and the 8.8 backport.

Berdir’s picture

> Yes we're all up-to-date here, just minor clean-ups in 9.0/8.9 and the 8.8 backport.

Correction: clean-ups in 9.0 and backport to 8.9, was not yet committed there.

8.8 backport is currently not easily possible as the update path test changes that we rely on here haven't been backported ((the new trait for example)

catch’s picture

Status: Needs review » Reviewed & tested by the community

Oh good point, I am starting to get confused here.

Both patches in #98 look great to me, so moving to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed fd11c5a and pushed to 9.0.x. Thanks!
Committed 8a1fbab and pushed to 8.9.x. Thanks!

  • alexpott committed fd11c5a on 9.0.x
    Issue #3098475 by Berdir, catch, TravisCarden, xjm, benjifisher,...

  • alexpott committed 8a1fbab on 8.9.x
    Issue #3098475 by Berdir, catch, TravisCarden, xjm, benjifisher,...
xjm’s picture

Version: 8.9.x-dev » 8.8.x-dev
Status: Fixed » Patch (to be ported)

But we still want to discuss an 8.8 backport, right?

catch’s picture

So for this one, it is not fixing any concrete 8.8.x upgrade path bugs - it was really just critical for 9.0.0 since we're removing core updates. However it would also be an improvement if we backported it for sites that update contrib modules that have should get the error message. Overall feels very optional to me for 8.8.0. Less of a priority than hook_removed_post_updates() since no support for that existed in 8.8 at all, whereas there's already an error message in 8.8 for this, just a not entirely reliable or clear one.

Berdir’s picture

> Overall feels very optional to me for 8.8.0.

Same :) For 8.x, it's at best a major bugfix, it's a mess but it has been like that since basically forever, so doens't seem like an urgent need to backport, unlike #3066801: Add hook_removed_post_updates() and #2917600: update_fix_compatibility() puts sites into unrecoverable state. The first of those overlaps a bit with this issue but backporting without this has already started.

It's also not trivial to backport due to the test refactorings

There's also an argument to be made similar to the issues above that someone could currently have a module where they ignored the warning message as it didn't prevent them from updating so far, things weren't too broken and now this would prevent them from running update.php on a security update.

catch’s picture

One reason to backport this would be to keep the overall system_requirements() changes consistent when we backport #3066801: Add hook_removed_post_updates(), but still feels optional depending on how that issue goes - there is not a hard dependency only a very loose one.

xjm’s picture

From a data integrity perspective, it's not as critical as actual upgrade path criticals, but it's a huge WTF from a user experience point of view. It is also a priority from the multiple branch compatibility work.

xjm’s picture

There's also an argument to be made similar to the issues above that someone could currently have a module where they ignored the warning message as it didn't prevent them from updating so far, things weren't too broken and now this would prevent them from running update.php on a security update.

I missed this implication; do we have a release note about this scenario?

catch’s picture

Issue summary: View changes
Issue tags: -Needs release note

Added a release notes snippet.

Gábor Hojtsy’s picture

Status: Patch (to be ported) » Needs review
FileSize
8.16 KB

The 8.9 patch in #98 applies cleanly to 8.8. As per the above it would fail due to lack of some dependent issues backported, but let's see if that is still the case. Uploading same patch from #98.

dww’s picture

  1. +++ b/core/modules/system/tests/modules/update_test_last_removed/update_test_last_removed.install
    @@ -0,0 +1,20 @@
    + * Update functions for the update_test_invalid_hook module.
    

    Whoops. This is wrong here, and in the patch already committed to 9.0.x and 8.9.x. Should we fix this as part of the backport only, and open a follow-up to fix where already committed? Or upload a trivial patch here to fix to 9.0.x + 8.9.x? Or leave it wrong for the backport, and fix this in all 3 branches in a follow-up? I basically never get it right when I try to decide what's in scope where, so before I do anything, I'll defer to committers.

  2. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdatePathLastRemovedTest.php
    @@ -0,0 +1,72 @@
    +namespace Drupal\Tests\system\Functional\UpdateSystem;
    

    Heh, this shouldn't have worked, since #3106395: Move tests that test the update system to UpdateSystem namespace wasn't backported to 8.8.x. But it looks like #3066801: Add hook_removed_post_updates() was backported already into this namespace/directory, which is why the patch applies cleanly and works in 8.8.x. Not sure if we want to do this. Maybe we should move this into the 'Update' namespace for 8.8.x? Perhaps open a follow-up for #3066801 in 8.8.x to move this test back to where all the other system (DB) update tests live in 8.8.x? Or just leave it weird in 8.8.x?

  3. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdatePathLastRemovedTest.php
    @@ -0,0 +1,72 @@
    + * Modules can define their last removed update function.
    

    True statement. ;) Is that what we want this class comment to say?

  4. +++ b/core/modules/system/tests/src/Functional/UpdateSystem/UpdatePathLastRemovedTest.php
    @@ -0,0 +1,72 @@
    +    // Set the expected schema version, updates are successful now.
    +    drupal_set_installed_schema_version('update_test_last_removed', 8002);
    

    I'm shocked this bit of procedural code still exists and isn't yet deprecated. ;) Oh, I found #2124069: Convert schema.inc to the update.update_hook_registry service (UpdateHookRegistry). In progress, I see.

Maybe NW for 1 and 2, but perhaps just #NeedsFollowup? Not sure about 3. Leaving status alone.

Thanks,
-Derek

Gábor Hojtsy’s picture

@dww: we should definitely not make the API / namespaces different in Drupal 8.8. If something was wrong in the original patch that would IMHO need another issue.

xjm’s picture

Re: #114.2, it's fine so long as the tests run. Can we verify that they did, e.g. with a failing patch?

We can file a followup for the docs nits in #114.1 and .3 and commit it to all three branches after.

Gábor Hojtsy’s picture

Here is a test only version of #113 AKA #98.

Status: Needs review » Needs work

The last submitted patch, 117: last-removed-3098475-98-8.9-test-only.patch, failed testing. View results

Gábor Hojtsy’s picture

Status: Needs work » Needs review

That was supposed to fail. And it seems like it failed with the right thing. So looks good to go in?

tedbow’s picture

Status: Needs review » Reviewed & tested by the community

I confirm that the patch in #98 and #113 are identical.

#116

Can we verify that they did, e.g. with a failing patch?

#117 failed on the expected test but also in the console output of #113 https://dispatcher.drupalci.org/job/drupal_patches/38640/console You can
14:28:49 Drupal\Tests\system\Functional\UpdateSystem\UpdatePathLastRe 1 passes
It truncates the test name but there is nothing else that would match this.

RTBC!

dww’s picture

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 117: last-removed-3098475-98-8.9-test-only.patch, failed testing. View results

dww’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
8.16 KB

Bot's going to continue to be confused with #117 as the latest patch in here. Re-re-uploading #98.

Re: #115: I only meant the location and namespace of the new test class. The 8.8.x branch is now a bit weird:

core/modules/system/tests/src/Functional/Update has 51 files
core/modules/system/tests/src/Functional/UpdateSystem has 1

With this patch, we add a 2nd lonely test to UpdateSystem. It all works, it's just weird. /shrug

Anyway, back to RTBC...

xjm’s picture

@catch and I discussed this issue and (like #3104015: Replace ZendFramework/* dependencies with their Laminas equivalents) it is still being considered for backport to 8.8, but might be better backported immediately after 8.8.5, so that sites can get the upgrade path criticals and update module regression fixes following their 8.8.4 security update, without additional potential disruptions to contend with. So leaving RTBC against 8.8.x for now.

xjm’s picture

For the release note here we should probably clarify the specific impact on site owners as per #111 (when their site might break with this change, and what they should do instead). Something like:

Drupal now shows a more user-friendly warning when a site tries to upgrade to a new version without having run required intermediate database updates first, and will prevent the update from continuing until the situation has been corrected.

Previously, site owners might not have been aware an update had failed, so site owners might now see the message for the first time even though the site was already in a broken state with updates that did not run. If there is an error that there is an "Unsupported schema version" for a given module when you try to update a site, you will need to locate an earlier version of the module containing the updates that still need to run, and downgrade to that version in the codebase to run the intermediate updates. Be sure to back up your site codebase and database before attempting this.

catch’s picture

Tried to shorten this, didn't do that really but broke it up a bit:

Drupal now shows a more user-friendly warning when a site tries to upgrade to a new version without having run required intermediate database updates first.

If you see 'Unsupported schema version' when attempting to run updates, you should:

1. Back up your site and codebase
2. Locate an older version of the contributed module which contains the updates you missed.
3. Downgrade the module to that version in your code base, and attempt to run updates again.

When this error is shown, it will now also prevent any updates from proceeding, so may make a persistent issue on an existing site more obvious.

Ideally, you should always attempt updates of contributed modules and core on a backup of your site (such as a local development environmnent) prior to running them on production.

xjm’s picture

#126 LGTM; incorporating that.

  • catch committed 1e23061 on 9.1.x
    Issue #3121827 by dww: Documentation follow-up fixes for...

  • catch committed 69e7946 on 9.0.x
    Issue #3121827 by dww: Documentation follow-up fixes for...

  • catch committed 0b63e95 on 8.9.x
    Issue #3121827 by dww: Documentation follow-up fixes for...
catch’s picture

That's the git hooks getting confused, #3098475-123: Add more strict checking of hook_update_last_removed() and better explanation is still RTBC and not in 8.8.x - just for anyone else who sees those commit comments and wonders what happened.

xjm’s picture

Issue summary: View changes

Updated the release note with what we used in 8.9.0-beta1.

hchonov’s picture

Category: Task » Bug report

It looks like this is not just a critical task, but a critical bug fix.

#1894286: System updates are executed without priority is simply re-prioritizing updates so that the system updates are executed before every others.

The re-rolls in #1894286-37: System updates are executed without priority failed for 8.8.x, but passed for 8.9. I've found out that the fails on 8.8.x were caused because of #3063912: Move UpdatePathTestBase::runUpdates() to a trait and were fixed on 8.9.x by the current issue.

This makes this issue a critical bug fix as proven by the test fails from the other issue now it is possible that under circumstances not all updates will be executed. I still haven't look into the exact reasons and what change caused/solved the problem.

The first release with #3063912: Move UpdatePathTestBase::runUpdates() to a trait was 8.8.3 meaning we have this problem since then in a release.

  • catch committed fd7c289 on 8.8.x
    Issue #3098475 by Berdir, catch, Gábor Hojtsy, TravisCarden, dww, xjm,...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed fd7c289 and pushed to 8.8.x. Thanks!

  • alexpott committed 4835db3 on 8.8.x authored by catch
    Issue #3121827 by dww: Documentation follow-up fixes for...

Status: Fixed » Closed (fixed)

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

xjm’s picture

Issue tags: +8.8.6 release notes
xjm’s picture

Issue tags: -8.8.6 release notes +8.8.7 release notes
cilefen’s picture

I wonder if "installed version" is a bit vague to site owners.