computerminds / cm_config_tools

DEPRECATED: Mirror of http://cgit.drupalcode.org/cm_config_tools
1 stars 0 forks source link

Config in config/install with third_party_settings cause a Fatal Error during site install #19

Closed jamsilver closed 7 years ago

jamsilver commented 7 years ago

Steps to reproduce

  1. Add locale module as a dependency of your install profile
  2. Add language_hierarchy module as a dependency of your install profile,
  3. Add/configure 14 languages and set up some language_hierarchy fallbacks for them,
  4. Use drush cmce (or indeed drush cex) to export all languages files (e.g. language.entity.en-hk.yml) to my_install_profile/config/install
  5. Re-install the site using drush site-install -y my_install_profile

Expected results Site install completes successfully.

Actual results Fatal error:

Drupal\cm_config_tools\Exception\UnmetDependenciesException: Configuration objects provided by <em class="placeholder">language</em> have unmet dependencies: <em            [error]
class="placeholder">language.entity.en (language_hierarchy)</em> in
/.../modules/contrib/cm_config_tools/src/Exception/UnmetDependenciesException.php:82
Stack trace:

Workaround Manually remove the language_hierarchy dependencies from all language.entity.XY.yml files. The site installs fine, and the language_hierarchy module appears to correctly pick up its third-party settings.

I've attached some example yml files which prompt the fatal error, and also the fixed version of the same files which install fine:

Caveats The problem does not arise if only a single language yml has the dependency, but does reliably occur with 14 languages exported. I do not know why this is.

Comments Commenting out the throw UnmetDependenciesException in ConfigInstaller::checkConfigurationToInstall() does fix the issue. So this may just be some over-zealous validation in cm_config_tools.

anotherjames commented 7 years ago

Oo interesting discovery: there may be a potential circular dependency between language_hierarchy and the core language module when the exported language configuration entities contain the language_hierarchy dependencies. I reckon resolve that, and this is all resolved, maybe.... testing now :-)

anotherjames commented 7 years ago

For reference ...

anotherjames commented 7 years ago

@jamsilver our export of the 'English' ('en') language entity contains 'GB' for its country code. Is this correct / is it needed? I'm guessing it's the site default language and it's country code is therefore actually irrelevant perhaps?

Reason being ... if we just remove the language.entity.en entity from our install profile, and pop the 3rd-party dependencies back in, it all installs fine. The English language entity has no fallback so the language_hierarchy setting really isn't needed, so I'm just checking the one that is added by spx_country_language_negotiator.

The only reason the exception is thrown is because of this circular dependency between language & the 3rd party dependencies, which cannot really be solved with a general purpose solution to apply to all types of config entity, since some would require the 3rd-party module to be installed first whilst others would require the original providing module to be installed first (or for some handling code in the 3rd party module to handle things better for specific types of config perhaps).

anotherjames commented 7 years ago

On further reflection, I reckon that the simplest way to handle this might actually be to just pop an additional key in the .info.yml file, something like 'delay_install' (or something else that might have more use for when in update/import land too?). That would then allow the original module's config to be installed rather than the one in our module, and then replace it only once everything else that could be a dependency has been installed. I'm reluctant to add more 'special' keys as it increases the apparent magic, but it may be the best option here if necessary. I'm also only suggesting this because we already happen to override the core ConfigInstaller which would need to respect the key. Ideally, we wouldn't need to override ConfigInstaller - we're only doing it to get around other bugs, and possibly one(s) that have already been fixed in latest core release.

Another more specific idea I had would be to remove the specific en language from the export list in the .info.yml, so people don't accidentally override the existing version that has had its dependencies stripped manually. But then someone will still want to configure it further at some point and this will all crop up again, just less often. But anyway, that's still not a more general solution.

jamsilver commented 7 years ago

Wow, thanks for the thorough investigation =). The only thought I have is regarding your comment:

"Since any third-party, or dependant, module could be expecting config to exist, and in its 'final' form, we can't really just automatically install cm_config_tools-overridden config at some 'later' point during installation."

Are we sure this is true? Modules should support their config being changed post-install as long as it's through the core APIs, I would assume.

Installing all config later feels like it has the advantage of being consistent for all types of config, and avoids multiple magical keys/sub-folders/concepts.

anotherjames commented 7 years ago

A few things:

1) yes, modules can support their config being changed ... but other (contrib) modules that are dependent on those modules might not cope so well.

2) much more importantly, sometimes config is used by dependant modules during their installation, so it's actually quite necessary sometimes to have been able to customise that config in between installing different modules. So I don't think it's sensible to change the installation process to avoid changing/installing all config until 'later'. I think opting specific config into 'later' installation avoids changing too much of the core installation workflow, and provides a general solution for when it's needed.

But it does sound for this particular project's scenario, it may just be easiest to take the English language entity out of the profile altogether, and if any customisations to it are needed, then we do that in an install task / hook.

anotherjames commented 7 years ago

Right, the issue has been resolved on SPX as of https://github.com/computerminds/SPX/commit/ece44f and https://github.com/computerminds/SPX/commit/df107d35f16e6c648e3b6a101851b163cda2f558.

So, with that, I think we can leave the rest of this as a 'nice-to-have' at best, as the outstanding item discussed on this ticket was implementing a 'delay_install'-style key, but I don't think that's really worth spending time on unless it's actually needed rather than actually quite possible with other (more 'normal') workarounds.