- Issue created by @kim.pepper
- Merge request !12555[#3533291] Remove unused file_file_video_presave() β (Open) created by kim.pepper
- π¬π§United Kingdom catch
This isn't dead code as such, the upgrade path in that issue wasn't implemented correctly.
When we're updating configuration, we need a post update that updates configuration in the site database, but also a presave hook that updates config that is shipped with modules/install profiles/recipes and triggers deprecations.
See for example views_post_update_table_css_class()
And
/** * Implements hook_ENTITY_TYPE_presave(). */ #[Hook('view_presave')] public function viewPresave(ViewEntityInterface $view): void { /** @var \Drupal\views\ViewsConfigUpdater $config_updater */ $config_updater = \Drupal::classResolver(ViewsConfigUpdater::class); $config_updater->updateAll($view); }
π Add generic interface + base class for upgrade paths that require config changes Active would make it harder to get this wrong but for now we should fix this specific update to follow that pattern.
- π¬π§United Kingdom catch
Another possibility here is to ensure that the config is always valid with or without this config key. This is the current approach taken in π Make menu trail behaviour in SystemMenuBlock optional Active (see the config schema in the MR) which avoids having any update path at all for existing config - that issue isn't in yet but it might be viable for changes like this that are adding something brand new that defaults to off anyway.
- π¦πΊAustralia kim.pepper πββοΈπ¦πΊSydney, Australia
So it's just adding `requiredKey: false` under the playsinline block in the schema?
- πΊπΈUnited States smustgrave
So what's a recommended way to test this one?
- πΊπΈUnited States smustgrave
Going to go on a limb from looking at π Make menu trail behaviour in SystemMenuBlock optional Active that the requiredKey is enough.
- π¬π§United Kingdom catch
Discussed this briefly with @alexpott.
In π Make menu trail behaviour in SystemMenuBlock optional Active the configuration form unsets the config key if it's FALSE (so it doesn't appear in the configuration) - I'm not entirely sure about that but whichever issue lands first is setting a precedent and we should probably decide on a pattern.
Alex also pointed out that after this change, the original post update that was added also isn't necessary (because existing config will be valid), so we could consider emptying that update out.
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.