Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Actions module provides configuration action.settings.recursion_limit
which is not used by core (except tests and migrations)
Proposed resolution
Remove it and fix tests with some other useful config
Remaining tasks
- Remove config
- Fix tests
- Decide about migrations
- Add update hooks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#47 | 3022401-actions-config-47.patch | 17.3 KB | andypost |
#47 | interdiff.txt | 553 bytes | andypost |
#46 | interdiff.txt | 3.02 KB | andypost |
Comments
Comment #2
andypostFirst attempt
Comment #4
mikelutzWe can't just delete migrations. They are technically plugin definitions, so they need to be deprecated.
If the actions_max_stack variable truly has no counterpart in D8 and the migration is useless, then you can to set it to use a null source and destination, remove it from the Drupal 6 and Drupal 7 groups so it isn't run, mark it deprecated and throw a trigger error somewhere, probably the migration constructor.
Comment #5
andypostAdded update hook and trying to fix test
@mikelutz thank for pointer - I will try to dig how to skip migration or point it to null storage
Comment #7
andypostHere's rework of patch
- #4 set to null plugins and reverted tests, @mikelutz I have no idea why that fails locally
- replaced action with automated cron module in tests
Comment #9
andypostComment #10
YurkinPark CreditAttribution: YurkinPark at Skilld commentedMigrate checks all migrations for source plugins existing. It happens in \Drupal\migrate\Plugin\MigrationPluginManager::getDiscovery . Seems like \Drupal\Tests\action\Kernel\Migrate\d6\MigrateActionConfigsTest can not be passed with such changes.
Comment #11
mikelutzI'm going to need to dig into this a bit more and see what they best thing to do it. We've never deprecated a whole migration without a replacement before that I'm aware of, so we need a way to render it inert while still allowing it to be instantiated and ran. I thought we could do it with th enull plugins, but since they aren't stub migrations, the requirements system is rejecting them. We may have to do something hacky like using an embedded data source with no rows (which might not even work)
Comment #12
andypostMaybe we can keep migrations in-place just point destination to some no-op (null does not work for some reason)
But what it really need is anounce that this migration does nothing and change record
Comment #13
andypostFixed source and used to add
nowhere
plugin for destination becausenull
hasrequirements_met = false
in its annotationComment #15
andypostAnother attempt - fix config destination to skip import if name is null
Probably it needs test to expect that behavior
Comment #16
andypostFix test
Comment #17
andypost@mikelutz skip on empty works with empty source!
Comment #20
andypostFix last test,
Comment #21
quietone CreditAttribution: quietone as a volunteer commentedSince this is deprecating a migration adding some migrate tags.
Comment #22
andypostI bet no tests needed
Comment #24
mikelutzThe migration handling here looks to be correct for now, and has my sign-off as a migration sub-system maintainer. Migrations are plugins, they cannot simply be removed or it is considered a BC break because someone might be relying on their plugin id. The plugin implementation can change without a bc break provided the id and signature remain. This migration serves no purpose as the config it imports is never used. Removing its functionality is not a BC break because the plugin implementation still exists with all methods.
This patch renders the migration useless without removing it. A normal plugin would be deprecated and marked for removal, but we have recently realised that we do not currently have a mechanism to deprecate a migration yml and mark it for removal. A follow up to this will be in #3039240: Create a way to declare a plugin as deprecated. When we decide on and implement a process for deprecation of migration ymls, we will include this migration as the first one to be deprecated.
The remainder of the patch and test changes look correct to me, but I will leave for someone with more domain knowledge around this particular config to RTBC.
Comment #25
naveenvalechaLooks pretty neat. We need the Update test to verify that the configuration deleted after the update hook.
How this is related to the scope of this issue? Shouldn't this be a separate issue?
Comment #26
andypost@naveenvalecha that change is required to fix test that using subject
Comment #27
andypostAfter #2863986: Allow updating modules with new service dependencies
Comment #28
andypoststill needs upgrade tests and probably other fixes for migrations
Comment #30
vacho CreditAttribution: vacho at Skilld commentedSolving testing problems.
Comment #31
andypostComment #32
andypostI guess it does not need upgrade tests because this config is unused and migrations has coverage
Comment #35
alexpottThis config was made obsolete by #1846172: Replace the actions API
I'm not sure we've removed a whole config file before. I wonder about the BC implications of this and whether a module's configuration forms part of its API. Other modules could be doing something like
\Drupal::config('action.settings')->get('recursion_limit')
for what its worth. https://www.drupal.org/core/d8-bc-policy does not mention config.Comment #37
alexpottDiscussed with @catch. This issue needs a change record - https://www.drupal.org/node/add/changenotice?field_project=3060&field_is...
@catch felt that config is along the lines of schema or data structure. Meaning he think's we can remove it with a change record. There's bound to be cases where that's harder because people actually check the value directly, but as a general rule.
Comment #38
andypostFiled CR https://www.drupal.org/node/3083486
Comment #39
andypostPatch still applies cleanly
Comment #41
thallesThe tests run to me!
Comment #42
catchThis should be a hook_post_update_NAME() since it's just removing configuration not changing schema at all.
Comment #43
andypostComment #44
catchOne more thing - we should probably have a test to ensure the hook_post_update_NAME() works, there is very similar test coverage in #2893795: Remove serialization.module BC layers for one example.
Assuming this comes back green it looks ready to go for me otherwise.
Comment #45
andypostyes, totally missed it
that's not enough because cobers only migrations
Comment #46
andypostHere's a test
Comment #47
andypostFix CS
Comment #48
longwaveUpdated the change record a little bit. Patch looks good to me, so RTBC.
Comment #51
catchCommitted 54e0602 and pushed to 9.0.x. Backported to 8.9.x. Thanks!