Remove post updates added prior to 8.8.0

Potentially handling an individual case in #3110393: Remove @deprecated views code datetime_range.module, where some deprecated code is coupled to a post_update.

Tagging as beta requirement per @xjm in #3108254-17: [META] Drupal 8-9 upgrade path clean-up.

+++ b/core/modules/action/action.post_update.php
@@ -5,22 +5,13 @@
+    'action_post_update_move_plugins' => '9.0.0',

Not quite as elegant, but technically shouldn't these all be 9.0.0-beta1 instead? Or are we going to assume that if someone gets into the error state and is told "you must upgrade to the last release before 9.0.0", that everyone will assume "last official release, not including alphas/betas/rcs, etc"?

If we want to use 9.0.0 for these, maybe we want a clearer message about it in #3066801: Add hook_removed_post_updates() to avoid any possible confusion / headaches?

#3066801: Add hook_removed_post_updates() is in 9.0.x so this can roll on.

field.post_update.php needs updating. I think all the post updates in that file can be remvoed.

I think we could consider removing layout_builder_post_update_override_entity_form_controller() because it's pointless on a major version upgrade - even though it has been added since.

RE #31.2 Good point - we could file an issue to remove it altogether - it's not been in a release. But plenty of people have applied the patch that adds it in prod - so let's leave it well alone.

Excluding the ones explicitly mentioned in the IS, here are the remaining post_updates left after the patch:

// non-test
action_post_update_move_plugins
system_post_update_entity_revision_metadata_bc_cleanup
system_post_update_extra_fields_form_display
text_post_update_add_required_summary_flag_form_display

// test-only
module_test_post_update_test
update_test_failing_post_update_first
update_test_postupdate_post_update_first
update_test_postupdate_post_update_second
update_test_postupdate_post_update_test_batch
update_test_postupdate_post_update_test0
update_test_postupdate_post_update_test1
workspace_update_test_post_update_check_active_workspace

I think some of them should be added to the IS, but at least the action one looks removable.

I don't want to hold this issue up on that, but #3119027: Use filled dump for RestSettingsDeletionUpdateTest would be a cleaner fix for RestSettingsDeletionUpdateTest, we wouldn't need any changes in that test anymore in this issue, I can also reroll that once this is in.

Fix coding standards according to https://www.drupal.org/pift-ci-job/1608391

Thanks @jungle for helping with so many of the beta blockers. We have a set of coding standards rules that have to pass for patches to be committed, defined in phpcs.xml.dist. If those rule don't pass on a changed file, the patch can't be committed. However, it's a smaller ruleset than the full Drupal standard, because it only includes rules that core complies with globally.

However, coding standards cleanups to unchanged lines not in the passing ruleset are out of scope unless the issue is specifically about coding standards fixes. (See https://www.drupal.org/core/scope#creep for more information.) I'm able to commit this The reason is that it adds more lines for the reviewer to read in an already large patch. It also can cause conflicts with other patches.

And... apparently the test results are using a different ruleset than that, because I'm able to commit #38 locally with no error, which means the coding standards fixes are out of scope. Sorry about that ! Whitespace fixes are usually innocuous enough, but this change should be reverted from the patch (even though it's fixing a valid coding standards bug):

+++ b/core/modules/system/system.post_update.php
@@ -96,7 +97,7 @@ function system_post_update_entity_revision_metadata_bc_cleanup() {
-  foreach ($last_installed_definitions as $entity_type_id => $entity_type) {
+  foreach ($last_installed_definitions as $entity_type) {

What you could do is file a followup issue to check all of core for failures of this particular rule, which we'd then follow with an issue to add that to the enabled core ruleset. (This can be a big project!) Reference here: https://www.drupal.org/core/scope#coding-standards

Thanks!

Thanks, @xjm for your detailed explanation. Understood and learnt it again :), I am reverting it next. @alexpott did share that link to me once on another issue I worked on.

Started from #38 again, skip #42 please.

@jungle to run the current core coding standards the easiest way is do $ composer run phpcs -- ./core -ps from the command line. Note you can change the ./core to target only the paths you've changed (so it's a bit quicker).

I think that was should leave workspaces_post_update_remove_association_schema_data() in? I was added in #3098427: Manipulating the revision metadata keys before running the BC layer breaks the BC layer so this update was added after 8.8.0. It was first released in 8.8.3 and the hook_update_removed is 8803 - which was present in 8.8.2. So I *think* (but not 100% sure) that workspaces_post_update_remove_association_schema_data() has to stay. As we allow updating from 8.8.2 workspaces to 9.0.0.

Thanks, @alexpott for your tips. Bring back workspaces_post_update_remove_association_schema_data ()

NW for #49.

Adding workspaces_post_update_remove_association_schema_data to the IS along with the others.

The only oddity remaining is that drupal_migrate_post_update_clear_migrate_field_plugin_cache is removed outright, with no corresponding entry in a hook_removed_post_updates implementation.
I see it's because it was supposed to be migrate_drupal, not drupal_migrate.

In that case, I can confirm that 100% of the removed updates are represented in a removed hook, and that no other additions are made.
Thanks!

Reviewing the thing.

Alright! I read the whole patch, including looking at the removed implementations, and also read side-by-side to compare the removed code with the added hook implementations. I also verified that the removed helper functions in LB were indeed not used outside the post-update.

One thing did not make sense on a unprepped read of the patch:

+++ /dev/null
@@ -1,15 +0,0 @@
-function drupal_migrate_post_update_clear_migrate_field_plugin_cache() {
-  // Empty post-update hook.

There's no hook_removed_post_updates() for this. Going to look in the issue for why...

Okay, that's explained in #22:

migrate_drupal added a post update that was namespaced incorrectly so could never run. It was only to clear a cache so we can just remove it entirely as if it never existed.

Kind of like a test to see if the reviewer is paying attention. ;)

Going to do one more read-through of added lines only, and then apply it locally and start checking post-updates.

So the only thing locally that I can't account for from the IS is:
workspace_update_test_post_update_check_active_workspace

It's mentioned in #35 but there's no response from @catch about that particular one, nor is it mentioned in #49 as being related to the infamous Workspaces 8.8.3 fix. It's a test implementation, for a test that is testing....🥁.... (This is me reading the code and blaming....) #3102059: Make tests of the update system use UpdatePathTestTrait instead of UpdatePathTestBase ? 🤔

Reading the diff from that commit, the same test used to be in....
core/modules/workspaces/tests/src/Functional/Update/WorkspacesUpdateTest.php

And the bit there dates to #3093879: Ensure that there's no active workspace while running database updates. Which doesn't seem to be the fated post-8.8.0 issue; it was committed in November before 8.8.0.

@Berdir confirmed that the test that uses workspace_update_test_post_update_check_active_workspace() is meant as a permanent regression test (versus being related to any particular update that needs to be tested).

Onwards to credits...

Committed and pushed to 9.0.x. Yay!

We should mention this in the beta1 release notes (since it's a change from the alphas) and then it's just rolled in with the general note about update hooks in the 9.0.0 notes. I attached the change record from the hook_update_N() removals to this issue as well.

Added a release note to the IS.

I've been told this is the issue status you Drupalists use for committed patches. ;)