Add new 10.3.x database dump fixtures, without modules deprecated for removal in 11.x

Created on 14 January 2024, 11 months ago
Updated 21 May 2024, 7 months ago

Problem/Motivation

One of the last steps for removing a module from core, is removing it from the update filled database dumps and any associated test coverage.

This is time consuming and error prone because you have to go 'back in time' to Drupal 9.4.4, load the database dump, uninstall the module, go forwards to 11.x, then dump the database again. It's also prone to merge conflicts between multiple different module removals, and also removal of update hooks themselves in preparation for Drupal 11.0.0.

I think we should try to batch the module removals this time, so that we do multiple at once - it will probably be about the same amount of effort to do multiple modules in one issue as doing one, and it'll mean less merge conflicts.

Steps to reproduce

Proposed resolution

Remaining tasks

The tricky thing will be determining timing of when we want to do this, and exactly which modules to do together.

User interface changes

API changes

Data model changes

Release notes snippet

πŸ“Œ Task
Status

Fixed

Version

10.3 ✨

Component
BaseΒ  β†’

Last updated about 2 hours ago

Created by

πŸ‡¬πŸ‡§United Kingdom catch

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @catch
  • πŸ‡¬πŸ‡§United Kingdom catch

    We could potentially bundle πŸ“Œ [policy] Remove update hooks added until 10.3 from Drupal 11 RTBC in here too.

  • πŸ‡¬πŸ‡§United Kingdom catch

    Updated issue link for update removals: πŸ“Œ [11.x] [PP-x] Remove updates added up until 10.3.0 from Drupal 11 Active .

    Increasingly I think it's worth trying to combine these, so a single issue which removes deprecated modules from the database dumps, and also updates the database dumps to 10.3.0, this would mean only having to update the database fixtures once for Drupal 11, instead of multiple times as we did in Drupal 9.

    The only problem is deciding what a 10.3.0 database looks like before it's released, which is going to be when we're not planning to commit any more updates to 10.3, but would commit them to 10.4 instead or a cut-off point where we say any updates added after that point will be left in 11.0.0 too.

  • πŸ‡³πŸ‡ΏNew Zealand quietone

    Changing parent to the issue tracking removal of all the extensions.

  • πŸ‡³πŸ‡±Netherlands spokje

    Just trying to grasp what we're trying to do in here: The IS talks about the 9.4.4 fixture, #3 πŸ“Œ Remove deprecated modules from update testing database dumps etc. Active tlaks about a 10.3.0 fixture.

    Are we going to keep the 9.4.4 fixture around _and_ add a 10.3.0 one?
    I would imagine we can loose the 9.4.4 one, since we don't support jumping from 9.x to 11.x?

  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    @Spokje: AFAICT the issue summary is just describing the current, painful situation. I do not think the proposal is to keep a D9 DB dump.

  • πŸ‡³πŸ‡±Netherlands spokje

    Thanks @Wim Leers, I suspect as much, just wanted to make it clear (and yes, I also felt the pain of fixture-fiddling)

  • πŸ‡¬πŸ‡§United Kingdom catch

    In 11.0.0 we would drop the 9.4.4 fixture, this requires having a 10.3.0 fixture (in reality it's going to probably have to be a 10.3.0 alpha fixture due to lack of time travel).

    We could add the 10.3.0 fixture to 10.3.0 itself too though, since it will help with 11.x->10.x update path backports - they'd be able to use the same fixture for any dedicated test coverage.

  • πŸ‡«πŸ‡·France andypost

    Does it mean that 10.3 should ship both 9.4 and 103.alpha fixtures?

  • πŸ‡¬πŸ‡§United Kingdom catch

    @andypost that's what we did with 9.5/10 and it's useful yes - it means any new updates in both 11.x and 10.x can be tested against the 10.3.0-alpha fixture, so makes backports easier.

  • πŸ‡³πŸ‡±Netherlands spokje

    So we know there's pain when changing fixtures, so we combining the changes so there's less pain.

    Now what if a core module doesn't have the remove issue committed on time for 10.3.0?

    We then have to go through the pain again when changing the fixture painfully made in 10.3.0 in 10.4.0?
    Could we move the removal to 12.0.0, so there's less pain?

    (Not sure about if there's already specific rules/documentation on this, just curious and pretty much done with the pain after the gazillion changes we did on fixtures during D9 => D10.)

  • πŸ‡¬πŸ‡§United Kingdom catch

    Now what if a core module doesn't have its removal issue committed on time for the release of 10.3.0?

    We then have to go through the pain again when changing the fixture painfully made in 10.3.0 in 10.4.0?
    Could we move the removal to 12.0.0, so there's less pain?

    I don't think we would deprecate any modules in 10.4.x for removal in 11.x, even if 11.0.0 gets pushed to December. So essentially anything deprecated in 10.3.x has to be removed in 11.x, but anything not deprecated in 10.3.x will probably have to be deprecated in 11.1.x or later for removal in 12.x yeah. There's not really any point deprecating a module in 10.4 for removal in 12.x because it doesn't reduce the amount of time that we have to support it in core for, so can't really see that happening either.

    I think we only have πŸ“Œ [meta] Tasks to deprecate Statistics Active and πŸ“Œ Deprecate the Statistics module Postponed left from 🌱 Deprecate dependencies, libraries, modules, and themes that will be removed from Drupal 11 core Active (field_layout isn't going to happen this time, the javascript ones aren't modules and also aren't happening), so once those are done, that's a full slate of modules, and we then need to figure out the cut-off point for upgrade path removals and start working on this.

  • πŸ‡«πŸ‡·France andypost

    I came to conclusion to replace module with obsolete stub info.yml file working on πŸ“Œ Remove Action UI module Postponed
    Removal of the stubs (help_topics and sdc ATM) could use separate issue after fuxtures update

    Moreover in case of action module we have no hook to disable it to allow end-user to consider so it's one line removal from extension.list so could be done here to unblock removal

  • πŸ‡¬πŸ‡§United Kingdom catch

    Statistics isn't quite deprecated yet, but I think it's close enough that we can remove it from the database dumps in parallel to the deprecation process now.

    That leaves whether we're going to be committing any new and complicated upgrade paths to 10.3.x, if there's nothing about to be committed, I think we should probably go ahead here.

    There are several steps, hence delaying this until it hopefully only has to be done once:

    For each of 'base' and 'filled'

    1. Load the database dump fixtures into the Drupal versions they were created from.
    2. Uninstall all the to-be-removed core modules on that version.
    3. Switch to the 10.3.x branch
    4. Run updates.
    5. Dump the database fixtures to new 10.3.x database dumps.
    6. Create MRs for 11.x and 10.3.x to add the new dumps.

    Once we have an MR with the new dumps, we can open an 11.x-only issue to remove all the pre-10.3.x updates hooks, and implement hook_update_last_removed()/hook_removed_post_updates(), and switch existing coverage to the new dumps (and remove coverage for all the updates we're removing). This will need to be stacked on top of the fixture additions until they're committed. Or it's possible to combine the whole lot in one issue but makes it a lot to review.

  • Assigned to quietone
  • πŸ‡³πŸ‡ΏNew Zealand quietone

    I have avoided making these dumps in the past and I would like to do them this time. I've got a basic script working to do the task. I just need to test the resulting database and have it run on all the source fixtures. So I am assigning this to myself.

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Thank you @quietone!

  • πŸ‡³πŸ‡ΏNew Zealand quietone

    I made a separate issue for the removal of hooks etc, πŸ“Œ Remove all the pre-10.3.0 updates hooks Fixed . I found it easier to keep that separate from the changes here to actually use the new dumps. There is a comment in that issue about some files that can probably be removed.

    So, I merged that issue and updated tests and comments. As we know that will result in a large MR, and it is 124 files + 143980 βˆ’ 8136.

  • πŸ‡³πŸ‡ΏNew Zealand quietone

    One thing to mention is that when making the new dumps I found what I believe is an error in the 'filled' one. In that database node/1 is an Article content type and part of a book. However, the book configuration only allows content type of book to be in book. I resolved that by retaining node/1 and deleting the row from the book database table.

  • πŸ‡³πŸ‡ΏNew Zealand quietone
  • πŸ‡¬πŸ‡§United Kingdom catch

    The issue split is good, we can commit the new dumps here first, then continue in the update removal issue, then after that module removals.

    Would you mind posting your script to this issue for posterity?

  • Status changed to Needs review 8 months ago
  • πŸ‡¬πŸ‡§United Kingdom catch
  • Pipeline finished with Failed
    8 months ago
    #142673
  • Status changed to Needs work 8 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Testing 7403 and there was a merge conflict around

    deleted by us:   core/modules/views/tests/src/Kernel/ViewsConfigUpdaterTest.php
    

    So resolved that. But still appears to have test failures now, so moving to NW for that as I don't want to step on anyone's toes.

    Applying 7403 to πŸ“Œ Remove Tour module Postponed to see if that works though for a removal though

  • Pipeline finished with Failed
    8 months ago
    Total: 988s
    #143026
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    But the tests that are failing should they be deleted? Since the db dump has correct array keys

  • Status changed to Needs review 8 months ago
  • πŸ‡¬πŸ‡§United Kingdom catch

    7404 is the MR to review here, 7403 is combined with πŸ“Œ Remove all the pre-10.3.0 updates hooks Fixed .

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    @catch can you rerun the javascript tests on 7404? I can't because of gitlab roles.

  • Pipeline finished with Failed
    8 months ago
    Total: 48351s
    #142363
  • Merge request !7426DB dumps β†’ (Closed) created by smustgrave
  • Pipeline finished with Success
    8 months ago
    Total: 1103s
    #143097
  • Status changed to RTBC 8 months ago
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Okay so opened separate branch of just the DB dumps for 11.x 7426

  • Status changed to Needs work 8 months ago
  • πŸ‡³πŸ‡ΏNew Zealand quietone

    The review missed that the steps in #14 are not complete. Also, in #22 the catch asked that the script I used was shared here.

  • πŸ‡³πŸ‡ΏNew Zealand quietone

    On reviewing everything I found that I missed removing some updates over in πŸ“Œ Remove all the pre-10.3.0 updates hooks Fixed . Also, I forgot to change the script to not use drush, which currently doesn't install on 11.x. So, the script pauses and I use the UI to uninstall modules and run updates. With all that fixed these two issues are working as expected for 11.x

    MR 7426 - This has the new dumps, changes needed for UpdatePathTestBaseTest to work with the new dumps, and the changes from 3439769 which does all the work to remove hooks.

  • πŸ‡«πŸ‡·France andypost

    Curious how it affected locale's tables, for tracker removal there's 2 strings

  • πŸ‡³πŸ‡ΏNew Zealand quietone

    @andypost, I don't know. How do I find out? Is this different from other modules that have translations? What are the strings?

  • πŸ‡³πŸ‡ΏNew Zealand quietone

    Hmm, I have not seen any work like that on the locale tables in the previous extension removal issues. AFAIK, the uninstall process does not alter the locale tables. I checked with a fresh install of Umami and that is true. I installed Umami, then install tracker and comment. After the table size for locale_location and locale_source increased. I then uninstalled those modules and the table size remained the same.

    Maybe I am misunderstanding, I should be asleep instead of at the keyboard.

  • πŸ‡¬πŸ‡§United Kingdom catch

    I think locale strings stay in the database after install, and also that's fine - the database is supposed to mimic what happens when you update real sites over time in the hope that this will pick up problems. e.g. if we ever added 'does this string still exist anywhere' auditing to locale module, it would have to deal with old strings. But also for real sites when they re-enable statistics, their translations will still be there.

  • πŸ‡«πŸ‡·France andypost

    Is there a reason to ship extra unused strings? it will slow down every upgrade test

  • Pipeline finished with Failed
    8 months ago
    Total: 956s
    #150127
  • Status changed to Needs review 8 months ago
  • πŸ‡³πŸ‡ΏNew Zealand quietone

    Tests are passing. This adds dumps for 10.3.0 and updates the two tests that use \Drupal\FunctionalTests\Update\UpdatePathTestBase.

  • πŸ‡³πŸ‡ΏNew Zealand quietone

    And this is the script I've been developing. It requires ddev and relies on my setup and commands.

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    @quietone would it be useful to commit the script too? Like you did for the sub tree split

  • πŸ‡³πŸ‡ΏNew Zealand quietone

    It requires ddev and relies on my setup and commands.

    No, I would not commit it because it will only work on my setup.

  • πŸ‡ΊπŸ‡ΈUnited States thejimbirch Cape Cod, Massachusetts

    I can verify the following modules are removed from tests.

    Actions
    Actions UI
    Book
    Forum
    Tour
    Tracker

    I see that the new database is added, and updated in code. Someone will still need to validate the references to the modules are removed from there also.

  • Status changed to RTBC 8 months ago
  • πŸ‡¬πŸ‡§United Kingdom catch

    πŸ“Œ Remove all the pre-10.3.0 updates hooks Fixed is actively using the new database dumps. The only other thing that could go wrong is if tests start failing when we remove deprecated modules, but I think we will only find out if there's a problem when we do that. Going to go ahead and mark this RTBC.

  • πŸ‡¬πŸ‡§United Kingdom catch

    Actually there is a problem. I thought gitlab was being clever showing the uncompressed gzipped file, but it's actually not gzipped. Pushed to the 11.x MR actually gzipping the files, the dump script doesn't do this.

  • πŸ‡¬πŸ‡§United Kingdom catch

    catch β†’ changed the visibility of the branch 3439769-with-new-dumps to hidden.

  • Status changed to Needs review 8 months ago
  • πŸ‡¬πŸ‡§United Kingdom catch

    catch β†’ changed the visibility of the branch 3414563-remove-deprecated-modules to hidden.

  • πŸ‡¬πŸ‡§United Kingdom catch

    catch β†’ changed the visibility of the branch 3414563-11.x-just-db-dumps to hidden.

  • πŸ‡¬πŸ‡§United Kingdom catch

    catch β†’ changed the visibility of the branch 3414563-remove-deprecated-modules to active.

  • Pipeline finished with Success
    8 months ago
    Total: 989s
    #151032
  • Pipeline finished with Success
    8 months ago
    Total: 1021s
    #151038
  • Status changed to RTBC 8 months ago
  • πŸ‡¬πŸ‡§United Kingdom catch

    Tested this against πŸ“Œ Remove all the pre-10.3.0 updates hooks Fixed and got to a clean test run on there. Since all I did was rename the files to remove the .gz extension, then gzip them, back to RTBC.

  • πŸ‡¬πŸ‡§United Kingdom catch

    Slightly more descriptive title.

  • πŸ‡¬πŸ‡§United Kingdom alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    I recreated the dumps locally and diff the output (fun) and did not see anything too surprising. There are some new updates for 10.3.x that change things but all that means is that either we need to recreate them or we need to leave the updates in.

    Committed 9fdd5ef and pushed to 11.x. Thanks!
    Committed 6e44daa and pushed to 10.3.x. Thanks!

    I did not use the 10.3.x branch because the dumps there seem to be flawed and contain reference to an update on 10.3.x that has since been removed. I just manually backported the dumps (and only the dumps) to 10.3.x.

    I think we should create a follow-up to ensure we have a test on 10.3.x using the new dumps just to make sure everything is working as expected.

    • alexpott β†’ committed 6e44daa2 on 10.3.x
      Issue #3414563 by quietone, catch, smustgrave, andypost, Spokje: Add new...
  • Status changed to Fixed 8 months ago
    • alexpott β†’ committed 9fdd5efd on 11.x
      Issue #3414563 by quietone, catch, smustgrave, andypost, Spokje: Add new...
  • Issue was unassigned.
  • πŸ‡³πŸ‡ΏNew Zealand quietone

    Needs followup for last paragraph in #51

  • πŸ‡«πŸ‡·France andypost

    it need one more follow-up for missed statistics settings

    ->values(array(
      'collection' => '',
      'name' => 'statistics.settings',
      'data' => 'a:2:{s:19:"count_content_views";i:0;s:15:"display_max_age";i:3600;}',
    ))
  • πŸ‡«πŸ‡·France fgm Paris, France

    There are actually lots more remaining elements to remove, beyond statistics: there are still remainders from aggregator, book, color, forum, and statistics. Most of them are config data and translations.

  • πŸ‡«πŸ‡·France andypost

    Translations are supposed to stay, to represent sites that been upgraded from earlier days, the only reason to clean-up is to speed-up of upgrade tests

  • πŸ‡³πŸ‡ΏNew Zealand quietone

    Yes, there is 'left over' data in the dumps. However, as catch explained in #36, these dumps are "supposed to mimic what happens when you update real sites over time in the hope that this will pick up problems." They are not created from a fresh install with data added. They are created by first installing the previous dumps on 9.4. The Issue Summary has more details about the process. The point is that these dumps are to reflect what really happens when an extension is uninstalled, not what we think should happen. So, the fact that data seems to be left behind is not something to be changed here. This is working as intended.

    If you think the uninstall process should be removing more date then that should be discussed in a separate issue.

  • Status changed to Needs work 7 months ago
  • πŸ‡«πŸ‡·France andypost

    At least both #56 and #58 needs work for follow-up

    regarding #58 - maybe statistics was not uninstalled properly? otherwise it's not clear why it's config is not removed

  • πŸ‡³πŸ‡ΏNew Zealand quietone

    #58, #62. I discussed this again with catch and we both agree the dumps are correct. All the modules were uninstalled correctly. The 'statistics_settings', and another settings for the removed extensions are not in 'config', they are in 'config_snapshot'. That make sense so one can do a diff on configuration settings. Or, should a new config_snapshot should be made when an module is uninstalled. That would be a discussion for a separate issue.

  • Status changed to Fixed 7 months ago
  • πŸ‡¬πŸ‡§United Kingdom catch

    Opened πŸ› config_snapshot can contain uninstalled and removed modules Active for the config_snapshot issue, going to move this back to fixed.

  • πŸ‡³πŸ‡ΏNew Zealand quietone

    @catch. thanks for adding the followup.

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

Production build 0.71.5 2024