Because of the uncaught exception the config rewrite process is interrupted by "Uncaught PHP Exception" message that ends up in Fatal Error. This prevents the modules either from being installed or updated.
| Comment | File | Size | Author |
|---|---|---|---|
| #14 | 2986272-catch-non-existing-config-exception-2.patch | 2.12 KB | iampuma |
| #2 | 2986272-catch-non-existing-config-exception.patch | 1.79 KB | a.dmitriiev |
Comments
Comment #2
a.dmitriiev commentedAttaching the patch with proper exception handling. Some references:
Comment #3
a.dmitriiev commentedWith this patch the non existing configuration will be skipped but others will be processed. The message will be logged in watchdog.
Comment #4
jepster_Sorry, I must decline this patch. If a module tries to replace any "not existing" configuration, it must inform the user and immediately abort the module installation. Because the replacement of not existing configuration would definitely lead to errors.
Comment #5
a.dmitriiev commentedI think you misunderstood the purpose of the patch. Patch still doesn't allow the replacement of "not existing" configuration. The patch only handles the exception in proper way. It is not a good idea to throw and not catch the exception. And the idea of the patch is to help avoiding of "not existing" configurations rewrite, and not prevent the module from installing.
Let's examine the example:
1. In config/rewrite folder of my module I have some changes to some views, e.g. admin/conten and admin/media: adding 1 more column with some additional information to the overview tables. And also the module brings a lot more other plugins and templates and what ever.
2. Module media is not enabled in my installation.
3. Behaviour with patch: the module will be installed, the changes to admin/content view will be applied, the changes to admin/media will be skipped and logged to watchdog with a proper exception handling. This will happen with any other config that doesn't exist in current installation. But for the sites with media module enabled there would be changes to both views applied.
4. Behaviout without patch: the module is not installed, no changes, no plugins, nothing.
Comment #6
a.dmitriiev commentedComment #7
jepster_No worries: I do understand what you are trying to do Artem.
I am proposing you two alternatives:
1., Use an install hook for this and use the optional folder to install config in dependence of existing config.
2., I won't allow the config replace module to install modules and log errors which you will ignore. As an alternative you could implement a property which will be optionally set during the module install. This property would be propagated to config replace, so that it would install only config if the original config is existing and log useful info logs, which would not be scary. Like "Bypassing config xyz, because original config is not existing". The current behavior helps a lot in preventing wrong config.
One more thing: If you extend the config replace module, you "must" provide an automated test. Unit tests or kernel base tests are welcome. It seems that you do not like to write tests, but still: it is a must.
Comment #8
a.dmitriiev commented1. If you check my use case from previous comment it can't be solved with proposed alternative, because it is rewrite of the config and not optional or the one that needs to be installed.
2. I don't understand your point. So in my use case described in the previous comment you suggest forcing to install missing dependecies or just not having them at all, not adding rewrite at all? Then this module is useless, sorry. Also the proper handling of the exception still prevents the wrong configs to be installed, I don't get why you don't want to just wrap the code in try - catch structure (the only thing that patch is doing, there is no other change proposed)??
3. I don't see the need in writing the test for including try-catch in the code for proper exception handling.
Comment #9
jepster_You wrote that you want to skip config. Therefor you could set an option for this in your install hook. Also you could implement a method call for installing your config, if you foresee a "skipping".
I do not want to pollute my logs with errors which will be ignored.
Testing exception handling is a basic method in unit testing.
If this module is useless for you: do not use it.
Comment #10
jepster_It is not a must to always catch exceptions. An exception is good for showing "why" something is failing. Therefor the NonexistentInitialConfigException is being implemented.
Comment #11
a.dmitriiev commentedPerhaps I will not use it. Thank you.
I don't really see any reason why this module replaced the config_rewrite module that was doing absolutely the same.
Comment #12
jepster_The project description describes the reason:
There has been lots of broken configs in deGov because configs from the "rewrite" folder where propagated into the database, without having the original config. Configuration replace does NOT install any modules that want to replace not existing original configuration.
Something like this could be implemented in your module install hook (second parameter is new):
That would skip the replacement of configuration, without having original configuration.
A more granular config management by separating config into modules or using the optional config folder could also solve the issue.
I have proposed you a date for a google hangout session to talk about this. It's up to you to attend.
Comment #13
jepster_Comment #14
iampuma1. I agree with patch provided here by @artem. At least it gracefully will log the error instead of breaking the website.
2. Due to config translations we need to provide an option so creating configuration files that do not exist yet works. As all configuration in deGov should be in English and it should also provide the German translation configuration files. See patch.
3. I am also not sure why it was chosen here to create a complete copy of the module config_rewrite instead of using a patch on that module? Was some attempt done to reach the maintainer or to ask for co-maintenance of the module?
Comment #15
iampumaScratch the above patch of mine. I was confused for a while, because I was too focused with the rewrite folder. Of course in case new configuration needs to be written it would be in the config/install folder.