Problem/Motivation

Like the title says removes all the pre-10.3 update hooks

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Sites must update to Drupal 10.3.0 or higher, prior to updating to Drupal 11. <a href="https://www.drupal.org/node/3442097">See the change record for more details</a>

CommentFileSizeAuthor
#12 3439769-nr-bot.txt90 bytesneeds-review-queue-bot

Issue fork drupal-3439769

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Comments

quietone created an issue. See original summary.

quietone’s picture

The test failures are in the following, all of which I think need to switch to using the new dumps. The new dumps are over in #3414563: Add new 10.3.x database dump fixtures, without modules deprecated for removal in 11.x

  • core/modules/datetime_range/tests/src/Functional/DateRangeFormatterSettingsUpdateTest.php
  • core/modules/editor/tests/src/Functional/Update/EditorSanitizeImageUploadSettingsUpdateTest.php
  • core/modules/layout_builder/tests/src/Functional/Update/LayoutBuilderSettingsUpdateTest.php
  • core/modules/system/tests/src/Functional/Update/CronLoggingUpdateTest.php
  • core/modules/system/tests/src/Functional/UpdateSystem/UpdatePathTestBaseFilledTest.php
  • core/modules/system/tests/src/Kernel/Scripts/DbImportCommandTest.php
  • core/modules/taxonomy/tests/src/Functional/Update/NullDescriptionTest.php
  • core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBaseTest.php
quietone’s picture

I left some local testing files in the MR, with those removed the failing tests are

  • core/modules/datetime_range/tests/src/Functional/DateRangeFormatterSettingsUpdateTest.php
  • core/modules/editor/tests/src/Functional/Update/EditorSanitizeImageUploadSettingsUpdateTest.php
  • core/modules/layout_builder/tests/src/Functional/Update/LayoutBuilderSettingsUpdateTest.php
  • core/modules/system/tests/src/Functional/Update/CronLoggingUpdateTest.php
  • core/modules/system/tests/src/Functional/UpdateSystem/UpdatePathTestBaseFilledTest.php
  • core/modules/taxonomy/tests/src/Functional/Update/NullDescriptionTest.php
  • core/tests/Drupal/FunctionalTests/Update/UpdatePathTestBaseTest.php
quietone’s picture

Need to investigate core/modules/systems/tests/fixtures/update/drupal-8.*.

quietone’s picture

Assigned: Unassigned » quietone
catch’s picture

catch’s picture

catch’s picture

Status: Active » Needs work

It looks like the UpdatePathTestBase failures are because we now have no updates to run - we should add an new, empty, post update to system.post_update.php to make those pass. We can then remove that stub post update when we have a real update in 11.x to run. It is kludgy, but we still want to test that updates will actually run when there's something to run.

quietone’s picture

Title: Remove all the pre-10.3.x updates hooks » Remove all the pre-10.3.0 updates hooks
Status: Needs work » Needs review

There is now only one failing test, UpdatePathTestBaseTest.php. And that does pass with the new fixture and some schema number changes to UpdatePathTestBase.php. This can be seen in MR7403 from #3414563: Add new 10.3.x database dump fixtures, without modules deprecated for removal in 11.x.

So, does this do the removals correctly (first time I have done this task)?

catch’s picture

Left some notes on the MR. I didn't review the whole thing. Everything I did review looked correct, the only thing is I think we can also remove a lot of hook_ENTITY_presave() implementations which are supporting config entity post updates - any config entity update is supposed to have its logic in presave so that it also runs when config is installed from modules (this is not implemented 100% because we make mistakes, but probably about 90%) - and those can all go when the updates go.

needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

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

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

catch’s picture

I looked at this again.

I think we should actually open a follow-up for the ViewsConfigUpdater/presave removals.

1. It'll reduce the scope here and make it more reviewable.

2. As soon as we commit this, new 11.x update hooks can use the new database dumps, so less merge conflicts in either direction.

3. As soon as we commit this, we can go ahead with module removals.

4. None of the presave hooks or ViewsConfigUpdater methods are part of the public API, more like dead code, so fine to do as 'deprecated code removal' issues without blocking other things.

Looks like this needs a rebase though. Can try to do a line-by-line review after that.

quietone’s picture

I've rebased and updated the MR. Not sure what needs to be done for ViewsConfigUpdater and I am too tired to think about it.

catch’s picture

I think we can do ViewsConfigUpdater in a follow-up per #13. I'll try to review the MR again soon.

quietone’s picture

OK, I have pulled out any thing related to ViewsConfigUpdater. At least, I hope I have.

quietone’s picture

catch’s picture

Pushed one commit here - removes a dblog post update from the list of removed post updates, because it was removed from the 10.3.x and 11.x branches and wasn't run in the database dump from the other issue - so it's as though that update never existed now.

Also restored one system post update which was added after the dump was created. We'll either have to create new database dumps to remove that, or leave it in core. I don't want to block the previous issue here on that because it's blocking so many thing and new updates could be added at any time.

Going to add an extra branch here with the dumps from the other issue to try to figure out the remaining test failures.

quietone’s picture

The test failures in this issue are all in UpdatePathTestBase tests. These are the ones that are fixed in the other issue, #3414563: Add new 10.3.x database dump fixtures, without modules deprecated for removal in 11.x.
I don't see any others.

catch’s picture

OK the draft MR confirms that https://git.drupalcode.org/project/drupal/-/merge_requests/7400/diffs?co... gives us a green test run once #3414563: Add new 10.3.x database dump fixtures, without modules deprecated for removal in 11.x is committed. So I think we are good here and just need the other issue to land, then a rebase here once it has.

I had some trouble running update tests (and even manually visiting update.php/selection) locally, but that turns out to be an issue on my local, not caused by anything here or in HEAD afaict.

quietone’s picture

Status: Needs work » Needs review
catch’s picture

Title: Remove all the pre-10.3.0 updates hooks » [PP-1] Remove all the pre-10.3.0 updates hooks
Issue summary: View changes

I reviewed again and didn't find anything.

Added a change record and release note.

alexpott’s picture

Note that the 10.3.0 dumps have been created before 10.3.0 is released therefore it is possible that more updates will be added so either we will need to update dumps or not remove these updates.

andypost’s picture

Title: [PP-1] Remove all the pre-10.3.0 updates hooks » Remove all the pre-10.3.0 updates hooks

Blocker is in

catch’s picture

The changes to deprecated modules aren't really necessary here - the contrib modules already exist, and probably won't want to remove these updates just yet so that it's easy for 10.2 sites to switch to the contrib versions. However because we're going to git rm the modules anyway, I'm not sure it matters - would be more work to go through and undo the changes again.

andypost’s picture

Looking at action and tracker removal issues - there's 15 tests to update (replace 9.4 with 10.3 dumps)

For example https://git.drupalcode.org/project/drupal/-/merge_requests/7358/diffs?co...

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

All green!

catch’s picture

@andypost the removal issues will just need to remove the modules in 11.x, nothing to do in core for those (hopefully). The contrib modules may want to update to using the new database dumps (which every other contrib module with updates tests will also want to do for 11.x compatibility).

catch’s picture

  • catch committed 360a9c58 on 11.x
    Issue #3439769 by quietone, catch: Remove all the pre-10.3.0 updates...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Did one more line-by-line review and found one issue - forum was specifying a couple of help post updates in forum_removed_post_update() - these were also in help_removed_post_updates(). Sure this was copypasta, wouldn't cause any issues except confusion.

I only made very minor changes fixing merge issues/cross commits with HEAD as well as the above, so I think I'm still fine to commit this one.

One more follow-up created - we actually need to remove the old fixtures from core/modules/system/tests/update too #3442172: Remove the 9.4 database dump fixtures from 11.x.

quietone’s picture

Assigned: quietone » Unassigned

Status: Fixed » Closed (fixed)

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