Problem/Motivation
The API for improving configuration management was added in #3047812: Add a Config Transformation event dispatching during config import and export to the experimental module so that it could be removed if there was no use of the API in core. Now there is (currently still part of the config_environment module) so we can move the API to where it should be.
Proposed resolution
Add a config transfomer API that allows event subscribers to edit the configuration just before it is imported or exported, knowing which it is and in which context the action is performed.
We have one use of the api so far in core #3077504: Add config_exclude functionality to core and it will move out of config_environment as well on its own in #3079029: Move module config exclusion from config_environment to core.services.
Since that feature is stable and does not need to be behind a experimental module.
But we no longer need it in the potentially removable experimental module.
Remaining tasks
Move code to core namespace.
User interface changes
None, the API is transparent for users if there are no systems using it.
API changes
None but we can use the api without class aliases and without the config_environment being installed.
Data model changes
None
Release notes snippet
Configuration storage transformation API is added for interacting with the configuration synchronisation process. See the corresponding change notice.
Original Motivation
The configuration management system in Drupal 8 is great and allows configuration to be synchronized between installations of the same site in different environments. This is done by exporting the configuration to an intermediary storage (files in git or tarball) transferring the intermediary storage and then importing it on the second installation. The ConfigImporter then takes the storage and checks for differences and updates the database to reflect the new configuration, creating or dropping tables etc.
However, the current workflow presumes that the configuration will be exactly the same in the different environments. If they are not, developers have to edit the configuration in the intermediate storage before it is imported again. The state of the art solution in contrib is Config Filter. It works great but has an important drawback as an API for both the DX and API architecture: It allows modifying the sync storage as it is manipulated but it has no concept of the intent. For example config filter plugins do not know if the sync storage is used for importing the configuration from it or exporting the configuration to it. A developer has to know which methods are called in which order of both the import and export process to correctly interact with it.
The concept of altering the configuration and importing it again is also used outside of the configuration synchronization process in contrib with Config Distro. The idea there is that the configuration importer can import the active configuration again after it has been modified by distro filters. This allows configuration to be updated with updated distribution configuration through the same ConfigImporter API but is not limited to distributions as such. Configuration can either be changed manually by calling methods on the configuration entities or it can be changed through importing all of the configuration through the ConfigImporter. The alter allows developers to change "arrays" and assures that all the appropriate side effects happen as they do when configuration is synchronized.
Comment | File | Size | Author |
---|---|---|---|
#64 | 2991683-64.patch | 39.83 KB | bircher |
#61 | interdiff-2991683-59-61.txt | 3.13 KB | bircher |
#61 | 2991683-61.patch | 37.89 KB | bircher |
#59 | interdiff-2991683-56-59.txt | 1.57 KB | bircher |
#59 | 2991683-59.patch | 37.19 KB | bircher |
Comments
Comment #2
bircherAttached is a first draft for this.
If you want to test it enable the
config_transformer_test
module.Export the configuration with
$ drush ev "\Drupal::service('config.exporter')->export();"
Import the configuration via the UI.
The test module adds a event subscriber that always creates a config change on both import and export to demonstrate how it works and does not need you to change some configuration to see the effect.
There are currently no automated tests yet as I would like to get feedback and maybe refine the API before testing it in more detail.
Comment #3
bircherSome additional comments:
I only added the export and import transformations because these are conceptually the easiest to understand.
I am open to add also a generic transformation.
And we might need that for the tarballs. For example before creating the tarball we do
$configTransformer->transformExport('config.storage.zip');
then save that to the zip. and when uploading a zip do:$configTransformer->transform(from_zip(), 'config.storage.zip', 'config.storage.sync')
before writing it to the sync folder.This would have to be done for config_split et al. to work with zips for example which currently is just hopeless but it could also be done in a followup issue.
I also created two different events for importing and exporting even though they have the same content because I wasn't sure if we would need more things that are specific for the different actions. And I took the opportunity to put the documentation there and allow for type hinting the different events. But again this is up for debate.
Comment #4
heddnJust a fly-by review. Should this read export?
Leaving at NR for other feedback.
Comment #5
nedjoI've worked extensively in contrib with the plugin type provided by Config Filter. It does the trick, but I have run up against limitations.
Conceptually, modelling a config alter API through events and event subscribers does indeed seem like a big improvement. The proposed event approach looks both a lot simpler than the current Config Filter plugin API and also more flexible. Nice!
Adding #2960941: Provide an API for persistent configuration alters, including for extension-provided config as a related issue. I'm thinking here that ideally a configuration transformation API would also apply at extension install time, allowing just-installed extensions to transform config in the active storage (config which may have been provided by previously-installed extensions). For that type of transformation, at least, there are existing instances in core that could be upgraded to a new API, such as various config alters that currently are done programmatically in standard_install().
Some of this method closely parallels
ConfigManager::createSnapshot()
. Could some of it be abstracted out into a trait or utility class?Typo in the table name. Ditto for
'config_transfomer_export'
.It looks like we're using this as a way to have a storage used just while we're doing the transform? If so, in a similar situation in contrib, I used a custom config storage class,
InMemoryStorage
. Not saying that's necessarily better, just noting it as an option. I'm not immediately seeing how we ensure that these storages are emptied prior to a subsequent use. Did you consider adding a service for each of these storages?Comment #6
nedjoOh, I think I see, we empty it out on the next run in
::copyConfig()
. Hmm, that works, but it leaves vestigial config stored after use. Feels preferable to delete it when we're done.Comment #7
bircherHi
Thanks for the positive feedback.
Yes the reason for passing on the name of the storage as context is precisely so that it could be used in other places too.
Passing
config.storage.extentsion_install
for example as the name and then just do an import transformation before, instead of straight using theFileStorage
, could be precisely the api for #2960941: Provide an API for persistent configuration alters, including for extension-provided config. Maybe we would even need more context there though.The MemoryStorage was also my first ideas as it would clean itself by definition. But I don't know how well that scales to big sites and badge processes. Which type of storage it is is not part of the API but an implementation detail, actually it could probably be one and the same database table as such, but then you could not kick off both operations at the same time (you shouldn't anyway as the
ConfigImporter
may rebuild the container and do all sorts of things, but the transformer API still runs in a safe environment).We should not make services out of the storages, they are replacing the other storage in the implementation where it is used. As such one should always call the transformer when dealing with it. What we could do is make a factory service that creates the storage for the transformer. Then we would not tie it to the database directly.
And yes the
copyConfig
method is also used in tests. Whether it is a trait or a static function somewhere is not really something I have a strong opinion about. The test trait doesn't concern itself with collections at all, the one from theConfigManager
doesn't handle non-default collections and the one here doesn't properly clean the collections. Less repetition and reusing one that does the job gets my vote.And yea.. there are a couple of spelling mistakes that I didn't catch in the plane/train when working on it. One we agree on the architecture and approach, we can clean up the patch and add lots of tests. The patch from #2 is meant as a proof of concept with a working test example so that one can give it a spin.
Comment #8
tstoecklerI think this makes a lot of sense, but I think it would be a good idea to introduce this regardless of this issue. This would already be useful to Drush right now and I guess to Drupal Console, as well. Maybe we can try to utilize utilizing it from the export UI by creating an
ArchiveStorage
or something like that.Related to the above point I think we are missing a
config.exporter.export
event that gets invoked from the exporter directly - to matchconfig.importer.import
.Playing around with the export a fair bit recently in the context of Config Overlay I had some issues with this approach, which is the same one currently used by Drush. What I did instead for testing Config Overlay is using the
StorageComparer
to compute the required delete/create/update operations and perform those directly. See here for an example. Note that Config Overlay is a) somewhat of a specific use-case and b) still explicitly alpha, so the fact that this fixed an issue I was having is not super significant in its own right, but thinking about it for a bit I actually find using the storage comparer a bit nicer. When you export with Drush, for example, it actually tells you what it's going to do and it computes that list using the storage comparer, but then it doesn't use it for the actual operations.Not sure if I'm missing something, but couldn't this be collapsed into a
StorageTransformEvent
Edit: Ahh I see you answered this in #3. Sure, not a big deal either way...
Comment #9
bircherThanks for the feedback
RE: 1 & 2) Yes both are good ideas, I didn't focus on the exporter as such because I only added it to make the patch a workable example. I think it is much easier to understand when you see the whole picture. I guess the static method could also have been on the ConfigManager then it would even be more clear.
RE: 3) Well yes, but the method is not about exporting as such, it is about making sure that a second storage is a copy of the first. So it makes sense to delete all first. Yet this is exactly one of the shortcomings of the config_filter api, and the reason you had to resort to the storage comparer and the reason in Config Split the conditionally split config is never deleted from the sync storage. It shouldn't matter how you get the two storages to be the same but with config_filter it does. The new API gives you more context and decouples you as the API consumer from the specific import or export process.
With the new API the event happens before anything gets deleted or altered. You get passed a storage that you can change to your will depending on what the storage will be used for. So Config Overlay can go read all the extension provided config and add it if it is not already there for the import event and it can remove all the config which is the same as the ones from the extensions on exporting. As such you will not need to know what files are in the sync directory and if we did this for the ArchiveStorage you mentioned in (1) then Config Overlay would also work with zips. (All you would have to do is apply your changes for both
config.storage.sync
andconfig.storage.zip
).RE: 4) Yea I can also live with just one event class, as long as different events are fired.
Comment #10
tstoecklerThanks for the feedback. You're explanation for #3 makes sense in that it shouldn't be a problem with the proposed API. However, I still think it's strange that we are using the storage comparer to preview a list of changes and then not actually use the storage comparer to perform those changes. But I guess that can be discussed independently of this issue...
Opened #2992763: Add a configuration exporter for the exporter issue.
Comment #11
bircherSo I uploaded a couple of patches.
The biggest one ( 2991683-11-all.patch) is all of the things together and is what you want to get for a full picture.
The 2991683-11-transformer-only.patch is only the things needed for the new API. This is the things you want to look at and review.
2991683-11-transformer-import.patch adds the transfomer API to the import mechanism so that it actually works.
2991683-11-transformer-config_dev.patch Adds a new module that uses the API and is very similar to a basic config_split.
It is probably what #2844681: Allow exported configuration to be environment-specific and #2830300: Allow development modules to opt out of config-export want. It is still in very rough shape and will need much more polishing. This should not be reviewed here but later on one mentioned issues. We yet have to define what exactly we want this to do. Right now it allows you to specify modules that are then removed when switching to the prod environment and added back again when switching to dev.
The interdiffs are highliting the additions so that they can be consumed individually.
The complete patch also adds the exporter form before and also uses the transformer API in the module config intsaller which should provide a starting point for #2960941: Provide an API for persistent configuration alters, including for extension-provided config.
I can explain more details if needed.
Comment #13
nedjoYou've been busy!
I've given 2991683-11-transformer-only.patch a quick first pass. Overall, it looks straightforward. It closely mirrors existing config events.
A few comments and suggestions:
Should be "Gets" rather than "Get".
Nice cleanup.
Because we don't have this public method in the interface and it's too late to add it for this major version, we may need to move this to a new (utility?) class.
It seems awkward to bail at this point. Can we ensure the method is not called if the directory doesn't exist?
What scenario can lead to a non-existent directory? Is this a bug fix, and if so, do we need to do it here?
Incomplete sentence.
I don't immediately follow this comment. What is "the original storage"?
For parallelism with other existing isSomething() methods, this and the other new is methods could be "Checks whether the..." or "Whether the..."
Missing blank line above.
Should have a first line of "Adds a context to the event." or similar.
"Gets"
The existence of the generic ConfigEvents::STORAGE_TRANSFORM implies there may be additional use cases, so we should cover that here.
Because we have this custom public method, we should implement a new interface that extends StorageInterface.
Comment #14
alexpottNo real review yet - some coding standards tidy-ups to make less noise for in whilst reviewing and test fixes to make less noise here.
Also addressed the following points from @nedjo's review - 1, 5, 7, 8, 9, 10.
Comment #15
alexpottSome thoughts. Pasting on before a meeting. Only a start on a review.
If we need this we need a new interface. Do we need it or can we always pass in the raw storage?
I think transformation of config during extension install should be a secondary consideration. And one dealt with in a a follow-up. The are numerous complexities here - one of the top of my head is that during a config import we use the storage from the ConfigImporter which will already be transformed so we'll be transforming twice. Also the $type and $name are bing used for install from config/install but not the other locations which feels inconsistent.
I think we need to somehow say this is not config import. And maybe prevent from copying to the active storage.
For other storages in Drupal this is called MemoryStorage see \Drupal\Core\KeyValueStore\MemoryStorage for example or MemoryBackend for caches.
Comment #17
mpotter CreditAttribution: mpotter at Phase2 commentedJust getting start reviewing.
Is this really related to ConfigTransform or is this a more generic core improvement that should be done regardless?
As we discussed on the call, we really shouldn't overload the "context" terminology as it will be confusing. Looks like this is just "extraData" that can be used for whatever purpose the caller wants.
As an example of the confusion, are these "Contexts" the additional data for the Event above, or are these "contexts" something else?
It's possible I'm not understanding the purpose of this (till I look at the other patches), but feels like there should be a better way to handle this rather than having to re-implement all the $this->storage->* methods.
Haven't looked much as the full patch yet, but on initial glance my worry would be around "config_dev". If we are going to put an implementation of ConfigTransform into core, I'd like to see it deal with more than just a single "dev" environment and be more like "config_environment" that could be used for dev, stage, prod, etc. Also, "config_dev" made me think it should be a dev-only module and not something added to composer.json.
Since it's going to be messy to review these different patches, maybe split "config_dev" into a new contrib module that requires the configTransform patch. Then we can also discuss separately bringing that new module into core once the basic API is vetted.
For the ConfigInstaller changes, still thinking about those, but probably minor quip to pass the ConfigTransformer at the end of the argument list instead of in the middle.
Comment #18
alexpottRe #17.1 opened #3001979: \Drupal\Core\Config\FileStorage::getAllCollectionNames() should work when the directory does not exist
Comment #19
bircherOk so I removed all the non-essential bits from the patch. Ie instead of transferring information via the storage we just use the drupal state.
I also made config_dev a bit smaller to only have it be a list of modules, one can always manually add a a dev module as an dependency on your view etc.. also there are required modules that can never be dev modules, we should also exclude those.
I added config_dev here because to vet the transformer API we need something that makes use of the API to be in core. config_dev needs to be enabled if you want to be able to switch between the dev mode and the non-dev mode, It is also what keeps the dev-only configuration out of the active config while the site is in dev-mode as the way it works is by having all the config in the sync directory and then filtering it out when importing the config. In a way it is the opposite strategy as config_split has, but since this is core we still need to be able to work with a zip download and upload and as such we need to differentiate differently.
Of course the name of the module probably needs to change and be a better name. I called it config_dev because I wanted it to be something like composers "require --dev". It is actually more like a config_nodev. Ie you set it up while it is not active and select the modules to turn off when it becomes active, then you activate it (which uninstalls all the dev modules and their dependent config) then when you import config from the sync storage it keeps them uninstalled (but when exporting the config it lets them be exported as installed). When you deactivate the prod mode it installs all the dev modules and their configuration. When you import configuration while it is in prod mode it will update install or uninstall modules and their configuration depending on the updated config_dev.settings.
We can of course make it treat a more complex use case with different environments, but I am afraid that this just adds complexity which then delays everything. Also instead of this (experimental) module one can use a contrib solution such as config_split (which can be re-written to use the new API rather easily) for multiple environments.
In a way config_dev from this patch is what #2830300: Allow development modules to opt out of config-export wants to achieve.
One important aspect here is that we re-use the internal ConfigImportForm and the diff routes. It would be great if those could be re-written in a more extensible way which would also help config_distro. But we can maybe do that also in parallel.
Comment #20
alexpottI wonder if we can have this as a utility class rather than another bit of API in ConfigManager - otherwise we have to consider this API and add it to the interface. I think it might be better to have StorageCopier class.
I think this might be able work with array_intersect_key() ie.
return array_intersect_key($this->config[$this->collection], array_flip($names));
This feels wrong. I think we should return the same object but with collection set differently. We don't need a whole new object here.
Comment #21
mpp CreditAttribution: mpp commentedDoes this imply that both config_filter & config_split will still remain needed in order to achieve the use case of environment specific config or will they merely be a UI on top of the new API?
I believe the aim of this issue is to improve DX (c.f. Dries' blogpost) so it would make sense if these modules were no longer needed. On the other hand, writing custom code to override environment specific config does not seem like a DX improvement?
Perhaps someone can clarify where we want to head with this issue?
Comment #22
bircherYes I needed to update the proposed solution based on #3014384: Meeting minutes 14 November 2018
Config_filter will be needed for modules which have not been updated to the new API, it will not conflict with the new API and can co-exist. I expect it to go away though because the new API is both easier to use and allows to do more.
Config_split will probably be ported over to the new API in a 2.x branch, but its use will also decrease as the new module added here takes care of most of its use case and one can probably easily upgrade the splits into environments. (I could imagine adding a drush command to config_split to that effect)
But there might be use cases for config_split not covered in the new module, so I expect it to stick around for those.
Comment #23
bircherOk attached is the patch with the API part without the config_dev module that I yet have to re-implement as config_environment (other name suggestions welcome)
I also addressed the comments from #20:
1) any review % is appreciated.
2) Good point, I like that a lot, tests are still missing for this one though but in my opinion this could be a standalone issue too.
3) yes good point.
4) actually it does need to be
new
here, and in fact I added a test for it which covers this. In my opinion this could also be added individually beforehand. I have seen this class now in a couple of places with slightly different implementations.Comment #24
Upchuk CreditAttribution: Upchuk as a volunteer commentedThe memory storage is being handled in a separate issue here: #3015886: Add an in-memory config storage so added the reference.
Comment #25
bircherAdding also #3016429: Add a config storage copy utility trait and fix ConfigManager::createSnapshot and #2992763: Add a configuration exporter as related issues.
Comment #26
mpotter CreditAttribution: mpotter at Phase2 commentedAdding #3028179: Config Environment module (original issue) as related issue. The config_dev work in #14 and #19 can be moved over to that issue for continued work.
Comment #27
bircherrerolled the patch now that #3015886: Add an in-memory config storage is committed, I also updated it with the updated patch from #3016429: Add a config storage copy utility trait and fix ConfigManager::createSnapshot.
This patch can be manually tested and reviewed independently (just install the config_transformer_test module from this patch and follow instructions from #2) But the real reason for re-rolling it is to use it as a base for #3028179: Config Environment module (original issue)
Comment #28
bircherI forgot to add the exporter if you want to play with it.
Comment #29
mpotter CreditAttribution: mpotter at Phase2 commentedConfirming that the patch in #27 still works when testing via the config_exporter in #28 and the config_transformer_test module. Also to clarify that the patch in #28 is applied after #27.
Comment #30
Tim Bozeman CreditAttribution: Tim Bozeman at CyberSolution for CyberSolution commentedomg. Best alter ever.
Comment #31
Tim Bozeman CreditAttribution: Tim Bozeman at CyberSolution for CyberSolution commentedYou can't get much better performance than memory storage. I share the concerns for larger sites though.
@bircher I really liked the storage factory service you were talking about in #7. I checked the memory foot print of my current site with this script and it's only 35MB, but I can imagine that bigger sites or sites with a bunch of webforms would get pretty resource intensive. We might not want to hard wire MemoryStorage in.
Comment #32
joebotA couple of minor issues in the patch for #27:
This description could use some clarity, its a bit confusing as written.
the line - `@see \Drupal\Core\Config\StorageTransformer::transformImport` - should reference the transformExport method
Comment #33
mpotter CreditAttribution: mpotter at Phase2 commentedAs discussed in the CMI 2 Slack chat, moving this API into the config_environment experimental module for now #3028179: Config Environment module (original issue). API can be moved back into core when ready for 8.8. Marking this issue as Postponed until that time.
Comment #34
larowlanfeels like we could have an $event->getEventName()?
is doing this in a constructor a good idea?
Comment #35
bircher1) This will be completely removed altogether and simplified significantly by making the transformer an explicit sync transformer as discussed with mpotter in a call related to this.
Other places that would like to do the same trick (ie config_distro) can easily copy the code as it is just two lines and the same would be necessary to properly call the method on the transformer service. This means we can get rid of the names and destinations entirely and focus on the sync operation that core does. Handling orthogonal operations such as config_distro does will have an easier code to look at for their inspiration.
2) No it is certainly not a good idea. This was just a one line change to avoid unnecessary noise but allows the system to work and be played with.
Thanks for the review of this still much in progress issue.
Comment #36
mpotter CreditAttribution: mpotter at Phase2 commented@larowlan the code from (1) was removed in the latest patch in #3028179: Config Environment module (original issue) (#33). Please post additional review comments there. Once we have an Alpha experimental module we will split out the API again and bring it back here.
Comment #38
bircherComment #39
alexpottWe need to spread the issue summary over to #3047812: Add a Config Transformation event dispatching during config import and export
Comment #40
alexpottComment #41
bircherNo interdiff since the previous patches are out dated.
We keep @internal on all the classes but we know that the feature added in #3077504: Add config_exclude functionality to core will be moved to core too and so we don't need to be able to remove the api any more. @internal will be removed when config_environment will have its beta stability and thus we will have two use cases for the API.
Comment #42
bircherfixing coding style issue and updating issue summary
Comment #43
alexpottThe problem is that the
\Drupal\Core\Config\ExportStorageManager::onConfigChange()
is firing in the installer prior to the state being created. Thinking about this I'm not sure that this event needs to be active during a Drupal install so I've disabled it using \Drupal\Core\Installer\NormalInstallerServiceProvider.Not reviewed the patch yet.
Comment #44
bircherMany thanks for finding this issue.
I think the fix you made is ok. But we could simplify it by removing state from this service, make it not be an event subscriber, remove also the StorageRebuildNeededEvent and just fire the transformation event every time. The reason for which we didn't do that in the first place was to avoid triggering the transformation when it is not necessary and in addition make sure that one does not conflate the event being triggered and the configuration being exported.
But I can see that being an event subscriber also has some consequences and complexity we may not need after all. And since we have the StorageRebuildNeededEvent one could force the rebuild all the time anyway. I liked the "caching" but I won't insist on it.
Comment #45
bircherQuoting @alexpott from slack:
Great! Here is the issue: #3083065: Remove EventSubscriberInterface from ExportStorageManager and remove StorageRebuildNeededEvent
Comment #46
Krzysztof Domański1. #3083065: Remove EventSubscriberInterface from ExportStorageManager and remove StorageRebuildNeededEvent was commited.
2. Changes from #43 are no longer necessary so I reverted 42-43-interdiff.txt.
Comment #47
Krzysztof Domański-- ignore wrong patch --
Comment #48
Krzysztof DomańskiComment #49
ricardoamaro CreditAttribution: ricardoamaro as a volunteer and at Acquia commentedSeems to be working now. I've tried testing this briefly using simplytest.me but it seems to be having problems today.
Putting this into needs work so that we can test launching a site instance with it.
Comment #50
ricardoamaro CreditAttribution: ricardoamaro as a volunteer and at Acquia commentedComment #51
ricardoamaro CreditAttribution: ricardoamaro as a volunteer and at Acquia commentedI've applied the patch:
[13:16] ricardo@ricardo-t440s:~/DrupalCore/drupal 8.8.x(1011+/569-,1)⇉*
$ git apply 2991683-47.patch
installed the site and got this error:
Comment #52
alexpott@ricardoamaro I've tried to reproduce the error you are experiencing. I did this:
=> No error
I also applied the patch to an existing site. I visited admin/config/development/configuration before clearing the caches. This resulted in an error about a missing service - not the same error you got. But this went away after clearing the cache.
@ricardoamaro can you provide precise instructions on how to reproduce the error you are seeing?
Comment #53
Krzysztof Domański@ricardoamaro #47 is wrong. Try #48.
Comment #54
ricardoamaro CreditAttribution: ricardoamaro as a volunteer and at Acquia commentedYou are right. applying 48
Comment #55
ricardoamaro CreditAttribution: ricardoamaro as a volunteer and at Acquia commentedAlso page is working now.
https://www.drupal.org/files/issues/2019-09-26/2991683-48.patch looks good!
Comment #56
bircherremoving the
@internal
form\Drupal\Core\Config\ImportStorageTransformer
in response to #3048873: Milestone: Move Config Environment module to beta - #4As the change notice says:
Drush and co are expected to use:
$storage = \Drupal::service('config.import_transformer')->transform($storage);
This is a very minor change to the code so I am leaving it RTBC
Comment #57
larowlanTagging for RM review
Comment #58
xjm@internal
is insufficient (and misleading and confusing). It leads to people basically just ignoring@internal
.@internal
should only be used for code that is actually designed as internal, and it should also always be accompanied by a docblock explanation of why the thing is internal (i.e. is it protected, a plugin implementation, part of an experimental module, not intended for reuse because$reasons
, etc.).@internal
also doesn't get you out of providing BC, because our policy is now to provide BC even for internal code (except in uncommon cases where providing BC is useless or actually dangerous, etc., at release manager discretion).For all these reasons (based on reading the IS, not the patch or comments), the approach needs to be reconsidered. There are two options:
\Drupal\Core
. That way, when the module is eventually marked stable, the code can be added to the core namespace, the experimental module's old classes/methods/etc. can simply be deprecated and wrap the new core version, and there shouldn't be any disruption. There might be some complications related to configuration discovery, haven't thought it through, but that's the alternative that's been used in the past.Comment #59
bircherWe are clearly in the case 1 from #58.
So I removed @internal from all classes and added final to the import and export bits so that they can not be extended to make sure the API surface is clearly the use of the classes and not the extending of the classes. We can always remove
final
if there is a good use case at any moment without breaking any api.I also updated the IS.
So per #58 we need framework manager review.
Comment #60
alexpottAs a framework manager I'm +1 about the new API going in to core because it allows us to do #3079029: Move module config exclusion from config_environment to core.services which is one of the key missing features for configuration management.
The new core API classes being added are:
We are also adding two services:
config.storage.export.manager
andconfig.import_transformer
. As far as I can seeconfig.storage.export.manager
could bepublic: false
as the only accesses to it via a $container->get() is in tests.config.import_transformer
is accessed in controllers so we can't make thatpublic: false
.So for the ones marked
final
we need to document why. And did I miss any of the new API - what do you think @bircher?On adding new things as final - I think this is a good plan because it restricts on API extension point and we can always remove
final
it as use-cases are made.Another thing we need to do is to improve the release note section - I'm sure that
really sells this. I guess #3079029: Move module config exclusion from config_environment to core.services is bigger news.
Comment #61
bircherI added some more documentation on the classes.
#60 is correct and lists all the new things.
config.import_transformer
is the new interaction point that mirrorsconfig.storage.export
from #3036193: Add ExportStorage to allow config export in third party tools.The new api tries to follow the lessons learned outlined by Wim's session at Drupalcon Seattle. So rather than having an interface for a single purpose class that would allow developers to swap out the class, the extension point is the event system itself. If the processes we add here need to be side-stepped then one can simply give the
ConfigImporter
an non-transformed storage or export directly fromconfig.storage.active
.That's it for my lunch break, if someone has a better version for the text then I am happy too.
As for the release notes, yea the news is that the API is available so we need to probably just use one #3047812: Add a Config Transformation event dispatching during config import and export or the one here, or a mix if they both happen in the same release.
Comment #63
bircherSo while reviewing this to be a stable API @alexpott found a bug: #3085604: Protect against concurrent config transformations
The patch there includes the suggestions and change requests form #58 and #60.
I am postponing this now instead of fixing the broken test from #61 because it will have been fixed with the bug we discovered.
Comment #64
bircherre-rolled patch
Comment #65
ricardoamaro CreditAttribution: ricardoamaro as a volunteer and at Acquia commentedManual test and review done.
The pages seem also working fine.
Comment #66
ricardoamaro CreditAttribution: ricardoamaro as a volunteer and at Acquia commentedComment #68
catchOK we're in a good state for this going to stable, and the classes need to be in the core namespace for that to happen. Patch looks fine and has good manual testing, so committed 1076c7a and pushed to 8.8.x. Thanks!
Comment #69
bircherUpdate Release notes snippet and change notice removing the experimental module warning.
Comment #70
xjmThe release note here doesn't describe the disruption from this issue. Can it be updated to explain the disruption? Docs here have some recommendations and examples for the release notes: https://www.drupal.org/issue-summaries#release-notes
Comment #71
xjmCome to think of it, this also maybe actually isn't disruptive, since this was only alpha code before and not in any tagged release. I think this is another issue that should possibly be tagged as a highlight instead?
Comment #72
alexpottYep I agree this is not disruptive. It's a highlight. All the authors of the large contributed module config ecosystem have a new API to play with - think config_split, config_filter, config_make_me_a_sandwich.