Problem/Motivation
This issue adds a Config transformation API to the config_environment module. It does by dispatching an event on import and export.
Proposed resolution
Add events that are dispatched for import and export.
The API will initially be developed as part of the config_environment module. When ready for Beta release, this event dispatching API piece will be moved into the core Config namespace since the events should be dispatched regardless of the specific config_environment use case. This will allow contrib modules such as Config Split to use the new event API.
NOTE: Currently this module defines the core ConfigEvents class and uses namespace aliases. Code is marked with @todo to indicate which pieces will be moved into Core API when config_environment becomes Beta. See #2991683: Move configuration transformation API in \Drupal\Core\Config namespace.
There is a config_filter patch #3050529: Create 2.x branch using the new core api that can be used with this api, and an accompanying drush PR. With that config_split, config_ignore etc can be used with the new api even on drupal 8.6 when one copies the whole new experimental module.
Remaining tasks
- Write patch
- Write good docs final event class to really describe how this works.
User interface changes
None
API changes
- Add new Config Transform import/export events that allow subscribers to modify config.
- Trigger Config Transform export event in ConfigExportStorage.
- Trigger Config Transform Import event in ConfigImport.
Data model changes
N/a
Release notes snippet
n/a -- moved to release note snippet from #2991683: Move configuration transformation API in \Drupal\Core\Config namespace since both are now in 8.8.0
Comment | File | Size | Author |
---|---|---|---|
#34 | 3047812-34.patch | 38.58 KB | alexpott |
#34 | 25-34-interdiff.txt | 6.63 KB | alexpott |
#25 | interdiff-3047812-22-25.txt | 1001 bytes | bircher |
#25 | 3047812-25.patch | 37.97 KB | bircher |
#22 | interdiff-3047812-18-22.txt | 3.03 KB | bircher |
Comments
Comment #2
bircherAdding the event dispatching.
Comment #3
bircherComment #5
bircherAdding hook_help from #3047804: Add scaffolding for config_environment experimental module
Comment #7
alexpottRetitling for clarity.
Comment #8
mpotter CreditAttribution: mpotter at Phase2 commentedComment #9
mpotter CreditAttribution: mpotter at Phase2 commentedComment #10
bircherUploading new patch with tests.
Comment #11
bircherComment #12
bircherre-roll since #3047804: Add scaffolding for config_environment experimental module got committed
Comment #13
ricardoamaro CreditAttribution: ricardoamaro as a volunteer and at Acquia commentedI see Easter holidays were productive :)
I would like to do a manual test on this config transformation event.
Comment #14
ricardoamaro CreditAttribution: ricardoamaro as a volunteer and at Acquia commentedI manually tested this patch today.
1 - added to settings.php:
$config['system.logging']['error_level'] = 'verbose';
$settings['extension_discovery_scan_tests'] = TRUE;
2 - Enabled:
- devel
- config_environment
- config_transformer_test
3 - Ran tests
4 - Exported and imported the resulting tar.gz file via admin/config/development/configuration and got positive results:
Patch is reviewed and tested!
Comment #15
Krzysztof DomańskiImproved coding standards, docs and tests.
Comment #16
alexpottI think we need far more documentation for the developer about what is happening here. This is the core of the new API we're adding. I think we need some examples of the things you can do and why.
Comment #17
Krzysztof DomańskiThe 'config.transform.import' still should be better described, because is similar to 'config.importer.import' from
core/lib/Drupal/Core/Config/ConfigEvents.php
.Comment #18
bircherAttached a version with more information and explanation.
Comment #19
bircherFor the brave that want to test this on their complex sites with config split etc.
You can use the config_filter patch from #3050529: Create 2.x branch using the new core api and the drush PR.
Comment #20
borisson_I only have small nitpicks/questions. This looks really solid.
I think the Needs tests tag should be removed, because it has testcoverage.
This is really cool and it should make it easier to get this to beta/stable. +1
Shouldn't this be 80 cols?
Oh, that is a pity. Shouldn't that be a different event?
remove below could lead to trouble if/when something new is added to this file before we do this. So it should probably mention wich use should be removed?
Comment #21
larowlanThis looks good, the dance around keeping this untangled from core is really difficult.
lets also put @internal on these to make it super-clear that there is no API here
same here
Comment #22
bircherNew patch addressing the comments form #20 and #21.
RE #20:
1. Thanks, credits to @alexpott
2. Right, I rephrased it a bit, I am never sure when it is allowed due to namespaces.
3. No, it should be the same event whether the config is really imported or just prepared to show the difference of what is about to be imported. If they were different events one would have to subscribe to both in order for the import diff not to lie. We have the
'config.importer.import'
event to react to config having been imported and new the'config.transform.import'
event to influence the config to be imported. If one wants to interact with the import more directly the'config.importer.validate'
event is for that. I think that should be enough.4. I removed the comment, in fact it is probably covered with moving the files as then the namespace matches and is redundant, it applies also to all other classes in the
Drupal\config_environment\Core\Config
namespace of course.RE #21:
Yes I would have been fine also just adding this directly to core. But of course I understand that we don't want to add API without consuming it and this is the compromise to make it easy to pull all that code in one swoop by deleting the experimental module.
1. & 2. yes I added the internal annotation, we can always remove it later when needed.
Comment #23
borisson_This now looks really good. I don't see anything in the patch here that confuses me or that could be improved.
Comment #24
larowlanThis was discussed in the core committers meeting #3061345: [Core Committers] Meeting notes 2019/06/06 and we decided to formalize the class alias approach in documentation - please see the policy issue #3061347: [Policy, no patch] Document use of class aliases by Experimental modules where the API's final destination will be a core namespace, lets get the policy/documentation updated and then get this in
Comment #25
bircherThe documentation is now in place, so this can go back to RTBC.
Comment #26
bircherAdded the link to the drush PR and config_filter patch that uses this API.
Comment #27
larowlanThis is ready to go, can we get a change record describing the new APIs - when done, please ping me so it doesn't sit at RTBC again.
Comment #28
bircherI also added the release note snippet to the issue summary.
Comment #29
bircheradding link to the change notice to the release snippet.
Comment #30
borisson_This now has a change record and release note. Back to RTBC.
Comment #31
larowlanadding review credits including mpotter for issue summary updates
Comment #32
larowlanComment #33
larowlanI went to commit this but there are quite a few coding standards issues, well not issues per-se, but violations from our current rule-set 😭
Most of them relate to our todos
I think we're going to need to update phpcs.xml to prevent these violations - as we don't want to change the code.
Comment #34
alexpottAs we're going to remove all these @todos I think we can use
// @codingStandardsIgnoreStart
and// @codingStandardsIgnoreEnd
to get around this.As this is only adding comments and adding things that will be removed setting back to rtbc.
Comment #35
alexpott@larowlan wrote about #34
Also #34 was my first patch so I think it's fine for me to commit this.
Committed 3ae3e52 and pushed to 8.8.x. Thanks!
Comment #37
alexpottI think we should only publish the CR once it is part of core. Otherwise the CR is very very confusing.
Comment #38
bircherI guess @andypost didn't see #37. I saw it pop up in my rss reader. I just edited it to put the warning about experimental modules be at the top.
Comment #40
bircher