Closed (fixed)
Project:
Drupal core
Version:
11.x-dev
Component:
base system
Priority:
Critical
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
9 Apr 2024 at 22:57 UTC
Updated:
4 May 2024 at 00:34 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #3
quietone commentedThe 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
Comment #4
quietone commentedI left some local testing files in the MR, with those removed the failing tests are
Comment #5
quietone commentedNeed to investigate core/modules/systems/tests/fixtures/update/drupal-8.*.
Comment #6
quietone commentedComment #7
catchAdding this to #3416928: [Meta] Remove updates added up until 10.3.0 from Drupal 11
Comment #8
catchComment #9
catchIt 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.
Comment #10
quietone commentedThere 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)?
Comment #11
catchLeft 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.
Comment #12
needs-review-queue-bot commentedThe 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.
Comment #13
catchI 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.
Comment #14
quietone commentedI've rebased and updated the MR. Not sure what needs to be done for ViewsConfigUpdater and I am too tired to think about it.
Comment #15
catchI think we can do ViewsConfigUpdater in a follow-up per #13. I'll try to review the MR again soon.
Comment #16
quietone commentedOK, I have pulled out any thing related to ViewsConfigUpdater. At least, I hope I have.
Comment #17
quietone commentedFor now, I have just added a note to the remaining tasks about ViewConfigUpdater om #3295574: [meta] Remove deprecated classes, methods, procedural functions and code paths outside of deprecated modules on the Drupal 11 branch
Comment #18
catchPushed 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.
Comment #19
quietone commentedThe 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.
Comment #21
catchOK 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.
Comment #22
quietone commentedComment #23
catchI reviewed again and didn't find anything.
Added a change record and release note.
Comment #24
alexpottNote 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.
Comment #25
andypostBlocker is in
Comment #26
catchThe 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.
Comment #27
andypostLooking 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...
Comment #28
smustgrave commentedAll green!
Comment #29
catch@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).
Comment #30
catchOpened #3442162: Remove redundant hook_ENTITY_TYPE_presave() and ViewsConfigUpdater methods.
Comment #32
catchDid 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.
Comment #35
quietone commented