Remove Drupal 8 updates up to and including 88**

Only 6 failures, you missed only a few spots! Impressive for a patch this size :)

#2942096: [policy, no patch] Remove old update hooks prior to each major version (Drupal 10 and later) hasn't had much discussion and I think we can make progress here while it's being discussed (or not discussed).

The current plan on that issue is that we'd remove all updates up to and including 88**, but anything committed from 8.8.0 onwards (i.e. with an 89** update number) would need to say in Drupal 9.0.x, so that sites can update from 8.8.0 or later straight to 9.0.x

This would be straightforward except that it means every update test added to 8.9.x onwards will need to start from an 8.8.0 or higher database dump, but theoretically it should be fine - i.e. a test that passes on 9.x with an 8.8.0 database dump will also pass on 8.9.x

It also means that fixes to the update paths removed here will need to land against 8.9.x only, but that also should be OK.

This is a massive patch so I did not attempt a proper review yet, however:

diff --git a/core/modules/aggregator/aggregator.install b/core/modules/aggregator/aggregator.install
index 8722892093..cfd836b747 100644
--- a/core/modules/aggregator/aggregator.install
+++ b/core/modules/aggregator/aggregator.install
@@ -21,32 +21,3 @@ function aggregator_requirements($phase) {
   }
   return $requirements;
 }
-
-/**
- * The simple presence of this update function clears cached field definitions.
- */
-function aggregator_update_8001() {
-  // Feed ID base field is now required.
-}
-
-/**
- * Make the 'Source feed' field for aggregator items required.
- */
-function aggregator_update_8200() {
-  // aggregator_update_8001() did not update the last installed field storage
-  // definition for the aggregator item's 'Source feed' field.
-  $definition_update_manager = \Drupal::entityDefinitionUpdateManager();
-  $field_definition = $definition_update_manager->getFieldStorageDefinition('fid', 'aggregator_item');
-  $field_definition->setRequired(TRUE);
-  $definition_update_manager->updateFieldStorageDefinition($field_definition);
-}
-
-/**
- * Add a default value for the 'Refresh' field for aggregator feed entities.
- */
-function aggregator_update_8501() {
-  $definition_update_manager = \Drupal::entityDefinitionUpdateManager();
-  $field_definition = $definition_update_manager->getFieldStorageDefinition('refresh', 'aggregator_feed');
-  $field_definition->setDefaultValue(3600);
-  $definition_update_manager->updateFieldStorageDefinition($field_definition);
-}

All removals like this should be accompanied by a hook_update_last_removed() https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension...

I'm also bumping this to critical since it needs to happen before we release a 9.0.0 beta.

Thanks, @jibran, for connecting this to #3087991: Drupal\Tests\system\Functional\Update\UpdatePathTestBaseFilledTest::testUpdateHookN Failed asserting that 8000 matches expected 8001.!

It's great to see this green. Removing Drupal 7 updates from Drupal 8 when it opened was one of the things that made it almost impossible to fix the 6-7 upgrade path (due to inability to backport patches). While things have changed a lot since then, trying to think through how this could affect things.

Currently, the patch removes all Drupal 8 updates, post updates, and test coverage, and implements hook_update_last_removed() for the hook_update_N(). This means that sites will need to update to Drupal 8.8.0 or higher before they can update to Drupal 9, which is what we want.

What this means in terms of future patches that will fix or add upgrade paths:

- Existing updates and tests in Drupal 8 can be modified as 8.8/8.9-only patches since the code is not in Drupal 9 at all, but it's all still in 8.8/8.9. This is completely fine.

- New updates in Drupal 8 or 9 will need to be tested against an 8.8.0 database (from prior to the update being added), this is more complex.

What the second point means, is that as soon as we commit this patch, we won't be able to commit any new updates to either Drupal 8 or Drupal 9 unless we have new boilerplate or explicit upgrade path test coverage.

Up until recently, this would have meant adding an 8.8.0 database dump to work from, but with #3082230: [meta] Convert some tests to Build tests we might be able to do everything as build tests instead. But this needs some discussion as to whether we should block this issue on some boilerplate test coverage and what it's going to look like if so.

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. I'm agnostic on whether we should do that here or in a follow-up, since it seems like nice-to-have test coverage really.

Adding 8.8.0-alpha1 dumps and boilerplate coverage seems like it would be easier in a spin-off than adding to a 4mb patch?

I've been wondering if it would be possible to split the patch up for review - say one for .install files, one for .post_update.php, and one for test changes?

So the .install patch - just removes updates, implements hook_update_last_removed() for each module, and in one or two cases removes some supporting code for the updates. All looks good.

.post_update.php - this removes the entire post update files for all modules that have them. Seems fine but I realised we've never added support hook_post_update_last_removed() - it would be theoretically possible for a module to ship only with post updates and not be able to require them to have run before its updated. Don't think it impacts this issue at all but do we need a follow-up?

Tests only - mostly removing wholesale tests for specific updates, only logic change is to rename some module schema numbering from 8001 to 9001 since we can't update from 8001 any more. Also looks good.

Also for the existing sites updating from 8.8 or 8.9 I think we have to implement an update hook to clear update.post_update_registry because we don't want to execute D8 post-update hooks in D9 and let people reuse the same name again.

I think that would break the following example:

1. Site is on 8.9, which has mymodule_post_update_foo() (contrib or custom)
2. Site updates to 9.0.x, the update clears update.post_update_registry, does not update mymodule which is both 8.x and 9.x compatible.
3. Now the site is going to run mymodule_post_update_foo() again because it's not in the registry.

edit: A new 9.0.x site would have the post update in the registry when it's installed, because the post update would be picked up by the install process (and the update to clear the registry wouldn't run on a new site).

I've opened #3089900: Drupal 8.8/8.9/9.0 update test coverage to discuss the update testing infrastructure we'll need for 8.8->9.0

@jibran I think we should not clear the post update registry at all at least in this patch. But also we should open a spin-off issue to discuss more.

@jibran I think we need to do this in #3089900: Drupal 8.8/8.9/9.0 update test coverage and keep this issue for just the removal of the old updates and coverage.

Just committed #3089900: Drupal 8.8/8.9/9.0 update test coverage, so we're unblocked here (at least for now!).

Also just committed #3093879: Ensure that there's no active workspace while running database updates since it was RTBC and not actually adding a new update, but the patch here will need to be re-rolled to preserve that upgrade path test into Drupal 9.

Looks like there are still update functions in a few real modules? Searching for "_update_8", I find update functions in locale, syslog, migrate and user.module.

Also, copying my own comments from slack:

@jibran looked at the failing workspace test. The problem is that the core/modules/workspaces/tests/fixtures/update/drupal-8.6.0-workspaces_installed.php wasn't updated for the last installed schema version, we should also rename it to 8.8.0 I suppose then.

the error happens because $update then doesn't contain 'start' but only a warning, so this also seems like a core bug that it results in a notice, the next page would then show that error

however, this is going to get pretty nasty
workspaces_update_8801() does install all those base fields that are now missing in the dump
it seems to work if you just copy the content of the update function into the test method, but I suppose we need to convert it to raw db queries in the dump file?

Views also needs resave, faced in #3097752: Remove views.module BC layers

1. Post updates: per suggestion of @alexpott in #3097661: No hook_update_last_removed() equivalent for post updates, should we add update functions that clear the post update storage per module? Maybe we can add these update functions in the referenced issue as a follow-up? will be easier to review then and we might also need API additions for that. Also, looks like locale.post_update.php and responsive_image.post_update.php haven't been removed yet?

+++ b/core/lib/Drupal/Core/Command/DbDumpCommand.php
@@ -392,7 +392,7 @@ protected function getTemplate() {
  *
- * This file was generated by the Drupal 8.0 db-tools.php script.
+ * This file was generated by the Drupal 9.0 db-tools.php script.
  */

+++ b/core/lib/Drupal/Core/Command/DbToolsApplication.php
@@ -13,7 +13,7 @@ class DbToolsApplication extends Application {
   public function __construct() {
-    parent::__construct('Database Tools', '8.0.x');
+    parent::__construct('Database Tools', \Drupal::VERSION);
   }

2. Should we make both of them use \Drupal::VERSION?

3. In hook_post_update_NAME(), we have some left-over references to removed update and post update functions. Not quite sure what to do with that, maybe just remove that block? I don't think we need generic code examples of what to do in a post update?

4. I think core/modules/system/tests/modules/entity_test_update/update/entity_rev_pub_updates_8400.inc is dead code that's not loaded anymore, as it was dynamically loaded based on the state variable. Lets remove that file and also core/modules/system/tests/modules/entity_test_update/entity_test_update.install? _entity_test_update_create_test_entities() is also unused and references no-longer-existing database dumps. It's either that or we removed too much and need to keep that test coverage.

5. \Drupal\system\Tests\Update\UpdatePathTestBase::$databaseDumpFiles still references the old database dump file in the documentation, this is the old simpletest base test class, so maybe just drop that? \Drupal\FunctionalTests\Update\UpdatePathTestBase::$databaseDumpFiles has the same reference though, so at least that we need to fix.

6. core/modules/workspaces/tests/fixtures/update/drupal-8.8.0-workspaces_installed.php also has references to the old filename.

Two more:

entity_test_schema_converter_post_update_make_revisionable(), we do have to keep it, it's a post update and applicable at any point, so that's fine. But, confusingly, it's wrapped inside a "@addtogroup updates-8.4.x", maybe just drop that?

UPDATE: Actually, I think we can remove the entity_test_schema_converter module as it's testing a deprecated class and doesn't seem to be called anymore.

A reference to system_post_update_fix_enforced_dependencies() in \Drupal\Tests\system\Functional\Update\UpdatePathTestBaseFilledTest(), should we just remove the reference or also the assert? I think that would be fine to remove.

Oh, we kinda crossposted, I snuck in an update into #53, didn't think you'd update it just while I was doing that.

I think the entity_test_schema_converter module isn't used by any tests now and could be removed completely? It's there to test that updater class, I'm removing that in #3069696: Remove BC layers from the entity system.

I'm not sure. It seems a bit strange to leave the module entity_test_schema_converter, because I'm pretty sure that it is not being called anymore and is dead code. In my current entity patch, I'm not removing the post update yet but I'm removing the class it calls, so it would definitely fatal, but it doesn't. There is one fail about it, but that's the kernel test that calls it directly.

So I'd suggest to remove the whole entity_test_schema_converter module completely in this issue, .info.yml and post_update.php and in my issue, I'll remove SqlContentEntityStorageSchemaConverter (which is still tested a bit in the still-failing kernel test).

As suggested by me in #52 and also by @catch in #36, we can deal with cleaning the post update registry in #3097661: No hook_update_last_removed() equivalent for post updates.

I did review the changed files extensively above and also the interdiffs based on my review. I also checked all the removed files, looks fine. Almost all are either post_update.php, Update tests or update fixtures, with a few special cases.

I did however find an interesting problem that I'm not sure is a blocker for this issue. I did some manual testing of this by installing on 8.7.x, switching the 9.0.x, applying this patch and then on update.php/selection I simply got nothing, but I expected lots of errors due to wrong schema versions. Looking at update_get_update_list(), I can see that it only checks update_last_removed for a given module *if* there are actually update functions for it. Since there are no update functions yet, it fails.

So I added this:

/**
 * Ensures that Drupal is updated from a supported version.
 */
function system_update_9000() {
  // See update_get_update_list(), there needs to be at least one update
  // function to check for the last removed schema version.
}

But somehow that also only works sometimes, I didn't investigate exactly why. On some requests, the following messages show up, on others nothing happens:

So even if it works, that's pretty confusing, not sure why that is only a warning and not an error.

So, what I'd suggest is that we add an explicit check to system_requirements() $phase == 'update' that explicitly checks \Drupal::moduleHandler()->invoke($module, 'update_last_removed') against drupal_get_installed_schema_version('system'), and if it's higher, then we display an error and show an actually useful message.

That does work just fine for example when you try to update to D9 from D8 and didn't update the config_sync_directory yet, you get a requirements error then.

Created #3098475: Add more strict checking of hook_update_last_removed() and better explanation

So, technically, the policy/plan issue for this hasn't been officially closed yet: #2942096: [policy, no patch] Remove old update hooks prior to each major version (Drupal 10 and later), but it's also the parent of this now, and we can IMHO use it then to update drupal 9 update documentation and so on.

This issue currently has two change records, one already published as it is shared with the already committed issue to add 8.8.0 database dumps. I'd propose to delete the other one and possibly create one for site builders/administrators in the parent issue.

See #52/#53 and #62 for extensive reviews and tests that I've done, I think this is ready. It's not just removing a massive amount of tests and code but also unblocks several BC-removal issues (views, entity) that basically prove that we have to do this to be able to remove our deprecated code in 9.x.

Updated the issue summary on #2942096: [policy, no patch] Remove old update hooks prior to each major version (Drupal 10 and later) and RTBCed it since I think we actually have consensus on what to do in 8/9 regarding update removal now. Thanks for opening the follow-up.

I went through this patch, found some unused use statements that could be removed, and checked for instances of _update_8 (like @Berdir in #46) and most of remaining results seem to be related to BC code that will be removed at some point, except this comment in \Drupal\Core\Theme\ThemeInitialization::getActiveThemeByName:

// Themes that depend on Stable will be fixed by system_update_8014().

Since the surrounding area of this code doesn't seem to have any "@todo Remove BC layer" comment, should we handle it in here? Maybe the comment can just be deleted?

The binary files were missing from #66, rerolled here.

As well as the comment noted in #66 there are a bunch of comments in update,inc and module.api.php that we should update to use _update_9xxx examples now, that can probably be a separate issue though.

This patch is mostly looking good. The one thing that surprises me is that I thought we were going to leave post updates to #3097661: No hook_update_last_removed() equivalent for post updates. I think removing them makes a fresh install and an updated D8 site different in important ways. Yes leaving them in means they are "untested" but I think that is okay but these are not changed and if they we'd be demanding a test.

Readded *.post_update.php except the one from a deleted test module with:

git diff --name-only origin/9.0.x | grep post_update.php$ | grep -v tests | xargs git reset HEAD
git checkout -- core

I think we're good to go here. No post updates have been removed other than a test one that's part of the entity_test_schema_converter module.
All the .install files with updates in have a hook_update_last_removed().

+1 as a framework manager. We've got a plan for post updates and all the follow-ups in place.

I was concerned that if we took the post updates out of this patch the test wouldn't pass so didn't bring it up. But of course we're removing the test coverage here entirely so they'll just be in and untested, so yes good. Given that it makes sense to do the removal in #3097661: No hook_update_last_removed() equivalent for post updates. Obviously we need to be very careful not to remove post 8.8.0 post_updates from now on, now that it's out, since those should stay in Drupal 9.

I'm happy with this patch and the overall plan we have so far, but I know xjm hasn't had a chance to catch up with the past couple of weeks of developments yet, so want to give her a chance to do that before committing this. This is the patch that properly diverges 8.x and 9.x in a serious way.

Arggh... just realised that this is getting rid of \Drupal\Tests\system\Functional\Update\UpdatePathWithBrokenRoutingTest and UpdatePathWithBrokenRoutingFilledTest - it shouldn't be. Those aren't tests of an update hook but a test that the update path is resilient to broken data. I suggest we postpone this on #3063912: Move UpdatePathTestBase::runUpdates() to a trait where we can fix one of the tests to not use UpdatePathTestBase and fix the other one to use the latest dumps.

Also we need to go through the rest of the update functions being deleted here to ensure they are all hook_update_N or a post update related test.

So this needs to be gone through with a fine tooth comb. For starters these should not be deleted:

• core/modules/system/tests/src/Functional/Update/BrokenCacheUpdateTest.php
• core/modules/system/tests/src/Functional/Update/RebuildScriptTest.php
• core/modules/system/tests/src/Functional/Update/UpdateCacheTest.php
• core/modules/system/tests/src/Functional/Update/UpdatePostUpdateTest.php

• core/modules/system/tests/src/Functional/Update/UpdatePathNewDependencyTest.php
• core/modules/system/tests/src/Functional/Update/UpdatePostUpdateFailingTest.php
• core/modules/system/tests/src/Functional/Update/UpdateSchemaTest.php
• core/modules/system/tests/src/Functional/Update/UpdateScriptTest.php
• core/modules/system/tests/src/Functional/Update/UpdatePathTestJavaScriptTest.php
• core/modules/system/tests/src/Functional/Update/InvalidUpdateHookTest.php
• core/modules/system/tests/src/Functional/Update/EntityUpdateToPublishableTest.php
• core/modules/system/tests/src/Functional/Update/EntityUpdateInitialTest.php
• everything in core/modules/system/tests/src/Functional/Entity/Update needs consideration...

We need to consider what to do with core/modules/system/tests/src/Functional/Update/UpdatesWith7xTest.php and whether we need an 8x equivalent.

Do we need to break up this enormous patch and split off at least the simpler cases where modules' updates are self-contained and can be removed individually? That would make the remaining patch a bit easier to review.

@longwave well I think the ideal case would be to identify all the non-legacy updates and convert as many as possible to use #3063912: Move UpdatePathTestBase::runUpdates() to a trait. The tricky thing are non-legacy updates using the filled db. Ideally we'd have way of not marking these @legacy. And then this issue becomes about removing the legacy updates.

The tricky thing are non-legacy updates using the filled db. Ideally we'd have way of not marking these @legacy. And then this issue becomes about removing the legacy updates.

If we switch them to using the 8.8.x database dump, then they ought to be non-legacy for now.

They'll start to trigger deprecation errors as we get to 9.1+. That means either updating them to use a 9.1+ database dump (endless task, but would make the Drupal 10 version of this issue easier to work on), or trying to switch some/all over to https://www.drupal.org/project/drupal/issues/3031379 if that's viable.

core/modules/system/tests/src/Functional/Update/UpdatesWith7xTest.php

On this test, I can't really think of what an 8.x-equivalent test would look like - we're going to (and have to) allow 8* tests to run on Drupal 9.

Also we have this issue open to fix issues with hook_update_last_removed() which will likely add more test coverage in general.
#3098475: Add more strict checking of hook_update_last_removed() and better explanation.

Been trying to think of ways to split this patch up into different chunks to commit in sequence but it's not easy - in the wrong order we get into circular dependencies between patches. Mayyybe have a suggestion though:

core/modules/system/tests/src/Functional/Update/BrokenCacheUpdateTest.php
core/modules/system/tests/src/Functional/Update/RebuildScriptTest.php
core/modules/system/tests/src/Functional/Update/UpdateCacheTest.php
core/modules/system/tests/src/Functional/Update/UpdatePostUpdateTest.php
core/modules/system/tests/src/Functional/Update/UpdatePathNewDependencyTest.php
core/modules/system/tests/src/Functional/Update/UpdatePostUpdateFailingTest.php
core/modules/system/tests/src/Functional/Update/UpdateSchemaTest.php
core/modules/system/tests/src/Functional/Update/UpdateScriptTest.php
core/modules/system/tests/src/Functional/Update/UpdatePathTestJavaScriptTest.php
core/modules/system/tests/src/Functional/Update/InvalidUpdateHookTest.php
core/modules/system/tests/src/Functional/Update/EntityUpdateToPublishableTest.php
core/modules/system/tests/src/Functional/Update/EntityUpdateInitialTest.php
everything in core/modules/system/tests/src/Functional/Entity/Update needs consideration...

The common denominator of all these is they're in system module, so what about this:

1. Fix the database dump for these tests in system module

2. Remove update tests from all core modules except system module.

3. Remove update tests from system module (obviously cross-referencing against the above list).

4. Remove hook_update_N() across the board

5. Remove hook_post_update_NAME() across the board (already has its own issue).

I've been through all the tests in core/modules/system/tests/src/Functional/Update and identified what I think we need to do with each.

We should keep all of these and fix them to work with the latest database dump (this should cover #79.1):

• BrokenCacheUpdateTest
• DependencyHookInvocationTest
• DependencyMissingTest
• DependencyOrderingTest
• EntityUpdateToPublishableTest
• RebuildScriptTest
• UpdateCacheTest
• UpdatePathNewDependencyTest
• UpdatePathTestBaseFilledTest
• UpdatePathTestJavaScriptTest
• UpdatePathWithBrokenRoutingFilledTest
• UpdatePathWithBrokenRoutingTest
• UpdatePostUpdateFailingTest
• UpdatePostUpdateTest
• UpdateSchemaTest
• UpdateScriptTest

We can remove UpdatePathRC1TestBaseFilledTest and UpdatePathRC1TestBaseTest which test various updates from Drupal 8.0.0-rc1, plus all of these update hook tests, which covers #79.3:

• AdminThemeUpdateTest tests system_update_8802
• AutomatedCronUpdateWithAutomatedCronTest tests system_update_8013
• AutomatedCronUpdateWithoutAutomatedCronTest tests system_update_8013
• ConfigOverridesUpdateTest tests system_update_8200
• EntityUpdateAddRevisionDefaultTest tests system_update_8501
• EntityUpdateAddRevisionTranslationAffectedTest tests system_update_8402 and system_update_8702
• FieldSchemaDataUninstallUpdateTest tests system_update_8007 and system_update_8008
• FilterHtmlUpdateTest tests system_update_8009
• InstallProfileSystemInstall8300Test tests system_update_8300
• LocalActionsAndTasksConvertedIntoBlocksUpdateTest tests system_update_8005
• MenuTreeSerializationTitleFilledTest tests system_update_8001
• MenuTreeSerializationTitleTest tests system_update_8001
• PageTitleConvertedIntoBlockUpdateTest tests system_update_8010
• PathAliasToEntityUpdateTest tests system_update_8803 and system_update_8804
• RemoveResponseGzipFromSystemPerformanceConfigurationTest tests system_update_8401
• RouterIndexOptimizationFilledTest tests system_update_8002
• RouterIndexOptimizationTest tests system_update_8002
• SevenSecondaryLocalTasksConvertedIntoBlockUpdateTest tests system_update_8011
• SiteBrandingConvertedIntoBlockUpdateTest tests system_update_8006
• StableBaseThemeUpdateTest tests system_update_8012
• WarmCacheUpdateFrom8dot6Test tests system_update_8601

and all of these post update tests:

• EntityReferenceAutocompleteWidgetMatchLimitUpdateTest tests system_post_update_entity_reference_autocomplete_match_limit
• MenuBlockPostUpdateTest tests system_post_update_add_expand_all_items_key_in_system_menu_block
• MoveActionsToCoreTest tests action_post_update_move_plugins
• NoDependenciesUpdateTest tests a bug in system_post_update_add_expand_all_items_key_in_system_menu_block
• RecalculatedDependencyTest tests system_post_update_recalculate_configuration_entity_dependencies
• UpdateActionsWithEntityPluginsTest tests system_post_update_change_action_plugins and system_post_update_change_delete_action_plugins
• UpdateEntityDisplayTest tests system_post_update_add_region_to_entity_displays and system_post_update_extra_fields

I am not sure what to do with these:

• EntityUpdateInitialTest from #2925550: Fields with a manually defined initial value in the schema have an Entity schema definition mismatch after updating to Drupal 8.4
• InvalidUpdateHookTest tests update hook 8000 - does this need updating or just removing?
• UpdatesWith7xTest tests update hook 7xxx - does this need updating or just removing?

As for core/modules/system/tests/src/Functional/Entity/Update, I think they can all be removed except UpdateApiEntityDefinitionUpdateTest.php.

(edited to reflect that PageTitleConvertedIntoBlockUpdateTest tests system_update_8010 and not 8009)

@longwave++ nice one.

I think we should err on the side of keeping. So let's keep

• EntityUpdateInitialTest
• InvalidUpdateHookTest
• UpdatesWith7xTest

It'd be great to move them to leverage #3063912: Move UpdatePathTestBase::runUpdates() to a trait so they are not extending UpdatePathTestBase and are not legacy tests. We can always decide to get rid of them later.

I agree with your assessment of the tests in core/modules/system/tests/src/Functional/Entity/Update - the rest are testing deprecated code that will be removed.

@longwave++ again - really nice work.

One more question with those system tests - do they need to be in system module or could they move to core/tests? Or if they need to stay in system, can we put them in a dedicated namespace that is more obviously 'do not remove'?

So I've extended #3063912: Move UpdatePathTestBase::runUpdates() to a trait to deal with quite a few of the tests listed in #80. The ones I've not dealt with are:

• UpdatePathWithBrokenRoutingFilledTest
• EntityUpdateToPublishableTest
• EntityUpdateInitialTest

They are using the filled updates so need to handled separately. Will open a new issue that blocks this issue for this.

A number of tests listed in #80 are not extending UpdatePathTestBase or annotated with @group legacy. This issue should definitely not be removing any of them. Tests in this category are:

• InvalidUpdateHookTest
• UpdatesWith7xTest
• UpdateApiEntityDefinitionUpdateTest

@catch I think we could go the other way around and add a new LegacyUpdate folder for tests that we want to remove in future major updates.

Created #3102059: Make tests of the update system use UpdatePathTestTrait instead of UpdatePathTestBase

@alexpott not sure about the naming but I think that could work (don't think we'd want to add brand new tests for a newly added hook_update_N() to a folder called LegacyUpdate).

Created #3104974: Remove Drupal 8 updates from system.module and #3104977: Remove Drupal 8 update tests except system.module as per #79 to try and split this up a bit.

I wonder if another method of chipping away at this is to remove updates and tests prior to 8.8.0 first, as there is a good chunk of those too.

Another update we can't remove is WorkspacesUpdateTest::testActiveWorkspaceDuringUpdate(). Needs the same treatment as in #3102059: Make tests of the update system use UpdatePathTestTrait instead of UpdatePathTestBase.

Suggestion to make this an easier process in Drupal 10:

Either in #3102059: Make tests of the update system use UpdatePathTestTrait instead of UpdatePathTestBase or in a quick follow-up patch, put tests of the update system in an additional UpdateSystem namespace.

Put new tests for actual updates, in an additional Updates namespace (i.e. ones added from now onwards).

That way in Drupal 10 we only need to rm -rf the Updates folder and we'll know we're not removing important coverage. Moving the updates system tests before doing other removals here will also make the removals patch easier to review.

@catch let's do it in a follow-up so the code changes in #3102059: Make tests of the update system use UpdatePathTestTrait instead of UpdatePathTestBase aren't mixed in with a tonne of moves. We could even do it in a follow-up to this issue. As a kind a final check we've got everything correct.

Reverted all update path + tests removals from #2893804: Remove rest.module BC layers in #2893804-77: Remove rest.module BC layers.

In doing so, I noticed two file that were not yet being deleted here:

1. core/modules/views/tests/modules/views_test_config/test_views/views.view.rest_export_with_authorization.yml
2. core/modules/views/tests/modules/views_test_config/test_views/views.view.rest_export_with_authorization_correction.yml

But first, rebasing #69, because it doesn't apply anymore.

And now fixing what I described in #91.

@Wim: This is basically a meta issue atm, see #86 and also #3102059: Make tests of the update system use UpdatePathTestTrait instead of UpdatePathTestBase, so that change probably belongs in #3104977: Remove Drupal 8 update tests except system.module?

UGHHHHHHHHH. Then let's make that clear 🤦‍♂️

#3102059: Make tests of the update system use UpdatePathTestTrait instead of UpdatePathTestBase is now in.

IMO the next step is to move the tests in that issue to a sub-namespace, i.e. system\Functional\Update\UpdateSystem, so that it's very easy to review the mega-patch here to see that they haven't been removed.

Then after that, we could rebuild the mega-patch here (we now have to be careful not to remove updates added since 8.8.0-rc1 was released).

Created #3106395: Move tests that test the update system to UpdateSystem namespace

And based on #96, we're be back on trying to get this one in as one patch I think, which makes sense, as we could only split up removing the tests first.

#92 did fail, as pointed out by #3104977: Remove Drupal 8 update tests except system.module there is still the kernel test that tests rest_views_presave() which is not yet removed here, so lets keep that for now. if the rest issue lands first without removing that, we can still do it here but probably easier to try and get this in first.

> (we now have to be careful not to remove updates added since 8.8.0-rc1 was released).

I believe we have no such update yet, git lg 8.8.0-rc1..8.8.x core/modules/*.install doesn't find anything and it does if you compare against older tags/versions.

I did again look leftover references to update functions, there are some in post updates of course but that's expected, we'll clean that up sepately. There is \Drupal\Core\DrupalKernel::getInstallProfile which has a separate issue that is blocked on this. There's \Drupal\Core\Theme\ThemeInitialization::getActiveThemeByName() which references system_update_8014() but seems better to clean up in a separate issue too. Leaving a note in #3104307: Remove BC layers in various Drupal\Core components.

Did find drupal-8.rest-hal_update_8301.php which we can also remove (several references to that function are probably better left for hal.module cleanup. left a note in the hal issue and another in the rest issue.

I did reroll this on top of #3106395: Move tests that test the update system to UpdateSystem namespace but verified that there's no mention of UpdateSystem in there and the patch also applies against 9.0.x

Fixed those fails, the workspace one was obvious, but no longer touching/removing UpdatePathNewDependencyTest shows that we were incorrectly removing new_dependency_test.install, restored that.

I have been through this patch in some detail and this looks almost perfect. The following remaining fixtures look to be unused:

• core/modules/layout_builder/tests/fixtures/update/layout-builder-tempstore.php
• core/modules/rest/tests/fixtures/update/drupal-8.rest-rest_post_update_resource_granularity.php
• core/modules/system/tests/fixtures/update/drupal-8.language-enabled.php

Also, do we need to consider upping \Drupal::CORE_MINIMUM_SCHEMA_VERSION now? update_system_schema_requirements() mentions "Updating directly from a schema version prior to 8000 is not supported" but this patch moves this higher?

I won't have time to update the patch until tonight, so feel free to remove them, if nobody beats me to it I'll do it then.

I think we can't change the constant, as contrib/custom modules could still add 800* updates, and have to as they could be installed/updated on both D8 and D9.

We'll also have to figure out and document how to do D8/D9 update functions for core modules, as we'll need to add them with 9xxx numbers to the 9.x branches as soon as we have at least one 9xxx update in the respective module. But then at the same time ensure that they won't run twice when that a site later on updates to D9. Fun times ahead :)

>> (we now have to be careful not to remove updates added since 8.8.0-rc1 was released).

>I believe we have no such update yet, git lg 8.8.0-rc1..8.8.x core/modules/*.install doesn't find anything and it does if you compare against older tags/versions.

What about workspaces_update_8803()? In the patch this is removed but workspaces_update_last_removed() returns 8802.

You're right, I missed that, I guess my local 8.8.x branch was outdated:

$ git lg 8.8.0-rc1..origin/8.8.x core/modules/*.install 
* fb3b95bb92 - Issue #3099986 by alexpott, amateescu: Move part of workspaces_post_update_move_association_data() to a hook_update_N (4 weeks ago) <catch>
* f61a2d58eb - Issue #3092595 by imclean: Deprecated temp file path message missing closing code element (7 weeks ago) <catch>

So yes, we need to restore that. And that means we ned to keep a workspace test around, with the huge 8.8.0 fixture that @jibran did.

(crosspost)

git lg 8.8.0-rc1..8.8.x core/modules/*.install

This would miss updates added to 8.9.x but not to 8.8.x. This is harder to check for against the tag due to our use of cherry-pick, so I ran the following which ought to pick everything up:

git log --since 22/11/2019 8.9.x core/modules/*.install
git log --since 22/11/2019 8.9.x core/modules/*.post_update.php
git log --since 22/11/2019 9.0.x core/modules/*.install
git log --since 22/11/2019 9.0.x core/modules/*.post_update.php

Things that have been added:

system_post_update_extra_fields_form_display() from #3092714: Config entity updater misbehaves when updating multiple entity types.

workspaces_update_8803 from #3099986: Move part of workspaces_post_update_move_association_data() to a hook_update_N.

Both of these are bugfixes for previous updates that have already been added, but sites will have to update to 8.8.1 (or 8.8.2+ if there's a security release first) in order to run them, prior to updating to 9.0.x. Or we need to leave them in.

I think for workspaces we could implement hook_update_last_removed() and set it to workspaces_update_8803 - because until that's run you haven't properly updated workspaces module to 8.8.x (and it's not marked stable yet so it's reasonable to expect sites to have to go to later patch releases to get a smooth update beyond them).

For system I think we should probably leave that post update in though?

Patch is currently setting hook_update_last_removed() incorrectly for workspaces:

-
-/**
- * Remove the Workspace Association entity storage if necessary.
- */
-function workspaces_update_8803() {
-  $entity_definition_update_manager = \Drupal::entityDefinitionUpdateManager();
-  $entity_type = $entity_definition_update_manager->getEntityType('workspace_association');
-
-  // We can't migrate the workspace association data if the entity type is not
-  // using its default storage.
-  // @see workspaces_post_update_move_association_data()
-  if ($entity_type && $entity_type->getHandlerClasses()['storage'] === 'Drupal\workspaces\WorkspaceAssociationStorage') {
-    \Drupal::state()->set('workspaces_update_8803.tables', [
-      'base_table' => $entity_type->getBaseTable(),
-      'revision_table' => $entity_type->getRevisionTable(),
-    ]);
-    $entity_type->setStorageClass(ContentEntityNullStorage::class);
-    $entity_definition_update_manager->uninstallEntityType($entity_type);
-  }
+function workspaces_update_last_removed() {
+  return 8802;
 }

The patch doesn't include any removal of post updates, is that because of #3097661: No hook_update_last_removed() equivalent for post updates, or to do it in two stages, or an omission?

> The patch doesn't include any removal of post updates, is that because of #3097661: No hook_update_last_removed() equivalent for post updates, or to do it in two stages, or an omission?

Yes, we decided earlier that we keep the post updates here and remove them in the other issue. See #68-72. So we don't need to deal with the system post update function here as we aren't removing it yet anyway.

Updated patch that only changes workspaces_update_last_removed() to include workspaces_update_8803().

+function workspaces_update_last_removed() {
+  return 8803;
 }

Opened #3106666: Remove post updates added prior to 8.8.0.

And removed the 3 extra fixtures identified in #101.

Also, do we need to consider upping \Drupal::CORE_MINIMUM_SCHEMA_VERSION now? update_system_schema_requirements() mentions "Updating directly from a schema version prior to 8000 is not supported" but this patch moves this higher?

iirc we added that code because there is literally no way to update from Drupal 6/7 to 8 using update.php and you have to use migrate. Checked with git log -S and it was added here #2168011: Remove all 7.x to 8.x update hooks and disallow updates from the previous major version.

Drupal::CORE_MINIMUM_SCHEMA_VERSION is telling you not to use update.php to update from older versions (i.e. Drupal 6 or 7) because it's completely unsupported with update.php

hook_update_last_removed() is telling you that you've skipped over a version that has updates required in order to continue updating (i.e. trying to go from 8.0.0 directly to 9.0.0). (Although we also need to resolve #3098475: Add more strict checking of hook_update_last_removed() and better explanation).

So we probably need to keep it exactly the same. If you have a schema version below 8000 we need to direct you to migrate, if you're missing something from hook_update_last_removed() we need to tell you to go back to 8.8/8.9 and run the interim updates first.

Drupal::CORE_MINIMUM_SCHEMA_VERSION is telling you not to use update.php to update from older versions (i.e. Drupal 6 or 7) because it's completely unsupported with update.php

FWIW, that's a very theoretical scenario at this point. When we added this then D7 and D8 still had the same system table, but that is long gone, and there's no way that Drupal 8 would be able to bootstrap update.php far enough on a D7 database to even get to the point of figuring this out.

I suppose it doesn't hurt to leave it as it is, but I feel like we could also just remove that and let contrib and custom modules in the future use lower numbers. For core, I guess we'll keep the current version schema, but the only thing that matters once this is committed is the last removed update function of the system module.

That's a good point, opened #3106712: Decide what to do with Drupal::CORE_MINIMUM_SCHEMA_VERSION and surrounding logic.

OK at 3mb this is nearly unreviewable but with the spin-off issues I think we've ensured it's not removing vital test coverage which is the most important thing. I also tried to review where I thought things might go wrong (like hook_update_last_removed() logic).

I noticed it's not removing views_view_presave() or db_log_view_presave() (referenced in test coverage) but that could happen either as part of post update removal or in a separate follow-up.

The trickiest thing here is the workspaces update removal, meaning that you have to be on 8.8.1 or higher to update workspaces. For me I think this is preferable to trying to preserve that 8.7 to 8.8 update + test coverage throughout Drupal 9 - it's an experimental module and we didn't add a new update in 8.8.1, but fixed an existing one. However would like a second opinion from xjm as the other release manager on that. We could special case workspaces in #3098475: Add more strict checking of hook_update_last_removed() and better explanation to explicitly point this out for any sites that run into it.

In practice, we want all sites to update to the very latest patch releases of 8.8 or 8.9 before they go to 9.x, not just 8.8.0/8.8.1, because we're still fixing critical upgrade path bugs from older versions and this will in some cases mean adding new post updates (as we did for one config updater bug where only half the update was actually running).

Marking this RTBC but leaving the needs release manager review tag. Extra eyes on the patch still welcome of course since it is not really a one-sitting patch.

I think the views presave functions are covered in #3097752: Remove views.module BC layers and #3097108: Remove dblog.module BC layers,

I discussed the CR in question - https://www.drupal.org/node/3098322 - @jibran said

The classes in change record were extending the old base class instead of btb

UpdatePathTestBase is not old and still exists and should be used for hook_update_N and post update testing - ie. 99% of contrib / custom update path testing. The complication that some of the issues that have blocked this commit has been that we also have tests of the update system using the database dumps. Where possible these tests have been converted to extending BrowserTestBase and using the UpdatePathTestTrait and are not using a dump - this makes them much easier to maintain and to know that we don't need to remove them when prepping the next major version. I'm not sure this info is useful for the CR because patches that add such tests are rare for core development too.

Let's add some README's to the codebase to clarify #116 and ensure that core/modules/system/tests/src/Functional/Update still exists after removing all the tests.

+1 to the README addition.

Fixing a typo.

Thought about workspaces some more, just documenting thoughts here:

The idea of making 8.8 the cut-off point is so that sites have to update to a supported release of core before they update to Drupal 9, giving us a much more limited set of variables when bugs are reported.

Making 8.8 the cut-off point means that assuming we release 8.9.0 and 9.0.0 on the same day, we won't force people on 8.8.latest to update to 8.9 first unnecessarily.

We have critical bugs in the upgrade path that we are expecting to fix in 8.8.x, so people wanting to update from previous versions always need to update to the most recent patch release to have those fixes applied.

There are only 8,500 sites on 8.8.0 compared to 73,000 sites on 8.8.1 already. Down from 30,000 sites on 8.8.0 a few weeks ago. Given 8.8.1 was a security release this should keep dropping over the next months and we don't know how many of those sites have workspaces installed.

Given all that I think we should go ahead and remove the workspaces update from Drupal 9 - we should prioritise having fewer upgrade variables when we can given the number of upgrade path issues to fix.

Removing unrelated change. Will open another issue to make and test this change.

I've rechecked all of the new hook_update_last_removed() to ensure the numbers are correct and they do appear to be.

I reviewed this by applying the patch locally and then doing

git status -s | grep "^M " | cut -c 4- | xargs git diff --cached

from the command line.

Created #3107472: DbDumpCommand should not hardcode the version string to address #121

As per earlier comments `\Drupal\system\Tests\Update\UpdatePathTestBase` still references the old files, but will be removed too as is legacy - do we have an issue for that? if so can it be linked in related?

Updated the change record to make it reference 9.0.0, as judging by the version field, that's the lowest version this can be committed to (happy to be wrong on that).

Committed 136f055 and pushed to 9.0.x. Thanks!

Removing release manager review on basis of #120 (please correct me if that was not intended catch)

Reverted this after jibran pointed out some questions that catch had and discussing with xjm
Adding back to needs release manager tag

Committed 2e53ac1 and pushed to 9.0.x. Thanks!

@Berdir re: #102: see #3010334: Document how contrib hook_update_N() should be numbered now that modules can be compatible with multiple major branches and versioned semantically

One more option with workspaces_update_8803(). Berdir suggested (I think in slack, and I think it was Berdir) leaving the update and post update in 9.x but with no test coverage (since 8.8/8.9 has the test coverage).

A slightly different version of this is to leave the update and post update in 9.x with no test coverage for now, then have a follow-up to either re-add the test with an updated database dump, or remove the update and increment hook_update_last_removed() - one of the two would be a beta blocker. That lets us defer that decision a little bit, while unblocking everything that depends on this issue.

A complication is that we haven't actually released 8.8.2 yet, and with packaging broken 8.8.2 might not come out for a little while longer, and we can't tell workspaces users to update to 8.8.2 until it actually exists.

It wasn't my idea, but I like it. Here's a reroll and restoring that update function. I was concerned that keeping this update means we also need to keep test coverage, and that currently is a pain. But it should be fairly easy with #3095333: Extend filled database dump with new stable modules and content for them. And we already agreed here on removing the test coverage for post updates and will remove once we have an API to clean that up. So I think that's OK too. As @catch said, either adding test coverage back on top of that issue or removing it will be a critical follow-up/part of the critical meta.

I personally still think that requiring 8.8.2 would be perfectly fine if you use the *experimental* module workspaces, as @catch said in the other issue, we should IMHO definitely recommend updating to the latest patch release of 8.8/8.9 and updating all contrib modules as far as possible anyway before jumping to 9.0. But I don't care that much.

Also rerolled, we changed some lines in two classes that are being removed here.

Added an explicit critical follow-up for workspaces: #3108416: Remove workspace_update_8803().

#134 interdiff looks great.

PHP Parse error:  syntax error, unexpected end of file in /var/www/html/core/modules/workspaces/workspaces.install on line 151

One character fix :)

The interdiff + interdiff look great :)