Add container_rebuild_required key to localgov_media to resolve Drupal 11.2 config dependencies

[tonypaulbarker]

container_rebuild_required defaults to false in 11.2. This causes some config dependency issues localgovdrupal/localgov_paragraphs#247
and is a similar treatment to localgovdrupal/localgov_topics#35

https://www.drupal.org/project/drupal/issues/3529670

Alex said "try adding container_rebuild_required: true to modules/localgov_media/localgov_media.info.yml - this should fix this."

How to test

Currently failing tests related to media config should pass for Drupal 11.2
Should not cause regressions for previous versions
The project should install for Drupal 11.2

[millnut]

@stephen-cox I'm slowly making progress on this, just a quick question around the below to understand historic context;

The reason it currently works this way is much of the config in the media module is copied from the standard profile and would cause errors if the module was enabled with this profile rather than the LocalGov profile.

I'm thinking of putting a requirement on localgov_media that it can only be installed on the localgov profile along with some other changes. Can you see any issues with that, given the reason why they are optional and then installing after is due errors when attempting on the "standard" profile?

Or do we use config_rewrite on config that standard profile may already have? It would be good to have a list of what config has been copied from standard.

With recipes, we won't have to worry about this, but would something like that work for now?

[tonypaulbarker]

Files appearing in localgov_media 2.14.10 and Drupal 10.5.1. Note I did see an anomaly in my diff where the document media type didn't show up for some reason, so this needs checking.

Standard profile: /web/core/profiles/standard/config/optional/
LocalGov Media: /web/modules/contrib/localgov_core/modules/localgov_media/config/optional/

type

media.type.document.yml
media.type.image.yml
media.type.remote_video.yml

field storage

field.storage.media.field_media_document.yml
field.storage.media.field_media_image.yml
field.storage.media.field_media_oembed_video.yml

field instance

field.field.media.document.field_media_document.yml
field.field.media.image.field_media_image.yml
field.field.media.remote_video.field_media_oembed_video.yml

form display

core.entity_form_display.media.document.default.yml
core.entity_form_display.media.image.default.yml
core.entity_form_display.media.remote_video.default.yml

view display

core.entity_view_display.media.document.default.yml
core.entity_view_display.media.image.default.yml
core.entity_view_display.media.remote_video.default.yml

[tonypaulbarker]

@millnut @stephen-cox

I came to a similar conclusion. See also the standard profile test. We might find that some other things have similar requirements and will fail further along. Even if we were to move the shared config to the profile itself (since one or the other profile would be expected) it doesn't mean that the configuration would be useful.

I think that whilst nobody needs to install the standard profile then done right It might be a useful test for keeping things granular and we should probably alter standard config that we need instead of replacing it in the long run.

I think that recipes is ultimately the way forward but we are working on some wider changes localgovdrupal/localgov#866 so I would prefer not to change too much just now if we can avoid it.

[millnut]

I agree @tonypaulbarker, it would be better if we check for existing config and alter vs replace.

For Drupal 11.2.x, the only way I could get this to work/install was;

1. Adding missing dependencies to the localgov_media info file. I discovered 4 when looking in more detail.
   • drupal:path
   • entity_usage:entity_usage
   • paragraphs:paragraphs_library
   • localgov_core:localgov_core
2. Adding a hook_requirements() check to not install localgov_media if the install profile is not localgov
   • I'm not sure if this is the best way, as I don't have enough historical context as to whether we want to enforce such a requirement
3. Removing the installOptionalConfig() code out of hook_modules_installed()
4. Moving the config from optional to install

Once all of those steps are made I could install via the UI and tests pass without unmet dependency errors.

[stephen-cox]

One problem I can see is we may want to use some modules with the minimal profile (for example). This still means that some config items (like directory views and paragraph types) depend on config in media that hasn't necessarily been installed.

One option is to only allow localgov_media to be installed with the localgov profile but also copy the config that a module depends on in media into the optional directory for that module and ensure all the dependencies are met to ensure that it gets installed.

[tonypaulbarker]

@stephen-cox that option doesn't sound ideal, it's an overhead with config out of place and would need to be removed later. I think I would prefer to see what mileage we can get from converting the media module to recipes and improving tests although I was hoping we could do that as part of upcoming improvements.

[tonypaulbarker]

More debugging and findings against the home page paragraphs test:

Moving the config to the optional directory of module where tests are failing didn't help.
Neither did explicitly setting the dependency on localgov_media
Neither did setting weights
I don't think the container rebuild has any effect in the test scenario
I think the config validation in the test fails before installing anything even if the requirements would be met because the validation doesn't appear to find the optional config.

If we just move the one piece of config (image media type) that this test depends on to 'install' instead of 'optional' in the media module the test passes.

I speculate that:

• a behat testing the same scenario would probably pass
• refactoring to recipes would give the validation at test time what it expects to find because the orchestration has had exposure to recipes
• Drupal should do a better job of validating optional config for test scenarios

[millnut]

I'm hoping to take a look at this again when time allows, but just wanted to flag that it's not just tests that are affected. This error also prevents installing the LGD profile via the web install ui and drush install as this handling change of dependencies for 11.2.x is at the module installation layer.

[tonypaulbarker]

@millnut back in June I did manage to install through the UI when the key was present. I will try again.

[tonypaulbarker]

With the container_rebuild_required key in place in localgov_media, I am able to install the profile via the UI with Drupal 11.2 in DDEV and enable the demo content, this prompted me to open the MR. However tests fail.

[tonypaulbarker]

This may be of some interest although I didn't pinpoint how it might have been activated for our cases https://www.drupal.org/node/3362879

"The above made it sound like invalid configuration would cause tests to fail. This is true only in Drupal core. For contrib/custom extensions (themes, modules, profiles), invalid configuration causes only deprecation notices, not test failures. In Drupal 11, these will become actual test failures."

field_group might be causing an issue with the home page paragraphs test.

[tonypaulbarker]

Tested real installs in depth with the key and found that some things fail silently. Some uninstalled modules cannot later be installed because some config already exists.

Feels like our options are to rewrite some things or go for @millnut 's workaround (localgov_media will only install with localgov profile) and then rewrite some things.

I agree @tonypaulbarker, it would be better if we check for existing config and alter vs replace.

For Drupal 11.2.x, the only way I could get this to work/install was;

1. Adding missing dependencies to the localgov_media info file. I discovered 4 when looking in more detail.
   • drupal:path
   • entity_usage:entity_usage
   • paragraphs:paragraphs_library
   • localgov_core:localgov_core
2. Adding a hook_requirements() check to not install localgov_media if the install profile is not localgov
   • I'm not sure if this is the best way, as I don't have enough historical context as to whether we want to enforce such a requirement
3. Removing the installOptionalConfig() code out of hook_modules_installed()
4. Moving the config from optional to install

Once all of those steps are made I could install via the UI and tests pass without unmet dependency errors.