🇩🇪Germany @tstoeckler

Really wavering back and forth on this one a bit...

Tried what @bircher mentioned in the core issue in #5: To add the config overlay config transformer via settings.php. That's not super intuitive, though you need this whole things in your services.yml:

services:
  # See config_overlay.services.yml
  config_overlay.extension_storage_factory:
    class: Drupal\config_overlay\Config\ExtensionStorageFactory
    arguments: ['@extension.list.profile']
  config_overlay.config_subscriber.transform_early:
    class: Drupal\config_overlay\EventSubscriber\ConfigTransformEarlySubscriber
    arguments:
      - '@extension.list.profile'
      - '%install_profile%'
      - '@config_overlay.extension_storage_factory'
      - '@config.storage'
    tags:
      - { name: event_subscriber }

And then, because Config Overlay is not in the autoloader by default, the following in your project's composer.json:

    "autoload": {
        "classmap": [
            "web/modules/contrib/config_overlay/src/Config/ExtensionOptionalStorage.php",
            "web/modules/contrib/config_overlay/src/Config/ExtensionStorageFactory.php",
            "web/modules/contrib/config_overlay/src/Config/ReadOnlyUnionStorage.php",
            "web/modules/contrib/config_overlay/src/EventSubscriber/ConfigTransformEarlySubscriber.php",
            "web/modules/contrib/config_overlay/src/EventSubscriber/ConfigTransformSubscriberBase.php"
        ]
    },

Not sure if there's a way we can tidy that up a bit, but not sure I want to actually recommend people doing so, as it seems like a lot of work...

Not sure about #5, because as far as I know all modules will still be installed first even in an install-from-config. But didn't verify that.

However, I agree it does kind of make sense to have a dependency on user module, even if there is no concrete functional bug without it. I was about to write we should fix the core issue I opened to get that dependency for free, but after looking at that one a bit I realized that in core all config dependencies, including those on entity type providers, are handled by the respective action plugins and that the type config property is actually unused entirely. So I think what we should do here is have UpdateUsernameAction add the config dependency on user module.

Added a commit to the merge request which does so by extending EntityActionBase. (For full disclosure, I was feeling bold and used the Web IDE for this, but since there is test coverage for the action (👏👏👏!!!), I hope that's all right.)

Was in the process of creating a merge request for this, but now repurposing this instead (without a merge reuqest for now). I had thought that the type of action configuration entities is in fact configurable in some way. For example, that the entity:publish_action plugin ID is used regardless of the entity type and the entity type is determined by the type set in the action configuration. That's not actually the case, however. Instead, there is a plugin deriver that derives plugin definitions for all applicable entity types so that the actual plugin ID being used is, for example, entity:publish_action:media. The type is then just copied over from that plugin definition's type property. This is also reflected in how the (now contrib) UI does things.

And all those entity-type-specific, derived plugin definitions the config dependencies already end up being set up correctly, because the (derived) plugins add their entity type provider as a dependency and that then gets picked up by the action entity due to the use of a plugin collection.

So there really is no bug here unless we explicitly want to support setting an entity type ID as an action type for an action that does not extend EntityActionBase. Instead, the question is why store the type at all, if it's just a "denormalized" version of the plugin's definition's type.

The only usage I could find in core is in views_views_data() where Views decides whether or not to add a $entity_type . '_bulk_form' views field to an entity type depending on whether there are any actions for that entity type. But that check could easily be changed to check the action plugin's definition instead.

So I propose deprecating the type configuration property of actions.

Re-titling and updating the issue summary, accordingly.

Hit this again, so shameless bump after all this time, would be nice to get this in (even though the minor priority is definitely still deserved). Thanks!

Note that I opened 📌 Action config entities should have a config dependency on their (entity) type provider Active in core because I do think that in principle the dependency on user is correct because that is the entity type that the action is for. Note that that could in theory be done via an enforced config dependency, but I'm not sure if it's really worth it because user module cannot be uninstalled anyway, so it's quite a theoretical case.

tstoeckler → created an issue.

tstoeckler → created an issue.

Hmm... yeah I think all of that stuff can be deprecated in favor of pointing people to 🐛 Dispatch config transformation event during site install from configuration Active if they encounter any installation issues.

Tentatively renaming the issue...

Ha, noticed that we cannot use ModuleHandlerInterface::loadInclude() as Config Overlay is not installed at that moment (inherently), so we need to manually require the file. Updated the issue summary...

tstoeckler → created an issue.

tstoeckler → created an issue.

Wow, that's awesome, thanks a lot!

tstoeckler → created an issue.

Thanks for taking a look @alexpott!

Basically the tl:dr; of the below is: would love to hear more of the thoughts behind your comment ;-)

Really tricky issue and great sleuthing on working about what is good on. The issue summary is impressive.

Thanks! That really means a lot.

The MR feels very much like a sticking plaster. I'm not sure you'd expect different results from Entity::getTypedData() depending on when it is called.

Not sure what you mean by this. One symptom of this bug is exactly that Entity::getTypedData() may return different results depending on what happened before. In particular, in the example in the summary $term->getTypedData() would incorrectly return a typed data instance of entity:taxonomy_term and then if for some reason only the typed data cache was cleared it would then correctly return an instance of entity:taxonomy_term:tags. Adding this to the issue summary in fact, because I think it's helpful context, thanks for bringing that up! So, yeah, the MR doesn't feel great, 100% agreed, but maybe you can elaborate what you mean by this specifically.

Do you think that it is worth deprecating the recursive call into \Drupal\Core\Entity\EntityTypeBundleInfo::getAllBundleInfo() and in a future version of Drupal throwing a exception when this happens.

Again, not really sure what you mean by this, unfortunately. Will take some stabs at a guess, but maybe you can elaborate here, as well:

1. Are you suggesting to not have bundle-specific data types at all? Not sure how we would properly validate bundle-specific validation constraints if we were to do that. But that would very cleanly solve this issue by actually properly getting rid of the circular dependency. (To be clear, not necessarily implying you actually suggested this, just trying to explore all possible options here.)
2. Are suggesting to not allow fetching the typed data information when loading a (bundle) entity? In theory I guess this would be the less invasive half of the circle to break, as the use-cases described in the issue summary (while valid) seem less important than having bundle-specific data types at all (i.e. 1.) Not really sure how to achieve this, though, as this would mean calling $typedDataManager->getDefinitions() without a primed bundle cache would yield a (deprecation notice, but eventually an) exception?
3. Having written the above and looked at the issue again, I had an idea of a maybe less terrible way to solve this issue at hand, so just listing it here to make it easier to refer to down below: Avoid loading entities at all for the building the bundle information. We really only need the IDs and labels of the entities, so we could fetch those directly from the storage (either via an entity query directly or via a new dedicated method on the storage). The only problem would be entity types that override label() and do something fancy there, but maybe it's fair to deprecate doing that for bundle entities (because presumably we don't want to just wholesale deprecate overriding that method)? While throwing the deprecation notice / exception would then require something like the somewhat ugly method_exists($entityType->getClass(), 'label') it would at least be a way forward that does not depend on the internal state of the typed data manager.

Awesome, thanks for the endorsement, will try to cook something up. Yeah makes sense with the test coverage 👍

Ahh that's a neat idea, hadn't thought of that 👍 Thanks!

No worries about the issues, all good.

Closed as duplicate of 🐛 Dispatch config transformation event during site install from configuration Active

I recently opened 📌 Config storage transformers do not get applied when installing Drupal from configuration Needs work for this, because I hit this with Config Overlay, as well. I only had the change in the "revert" step there, which seems to be sufficient at least according to Config Overlay's test coverage ;-). I left out the change in the initial import because as far as I can tell, there are no modules installed at that point anyway, so I felt like there's really no benefit to it. And ExcludedModulesEventSubscriber (which is the only transformer in code) will not have any effect on the initial install either, as far as I can tell. Just curious about the reasoning there, not at all opposed. In fact, if it doesn't break anything, it does seem more "correct" to do it in both cases. I guess if we do add another config subscriber in core it's nice to have that already affect the initial import and not just the "revert". In any case, will close my issue as duplicate.

That's a fair point. 👍 That would probably be helpful for people evaluating the module.

Not sure when I will get to it, but will try to find some time.

Fair enough! Thanks for the pointer, haven't looked at Config Devel in ages, that does seem like a real useful companion module for Config Overlay. 👍 Will try it out, but seems to be exactly what I was thinking about from the look of it.

Will add a note to the project page about that.

Leaving open for that, for now, and also because I do think it would be nice to have some sort of "automatic" mode without having to specify all configuration explicitly. Especially for project-specific installation profiles, that list is pretty huge and does change somewhat regularly. But nevertheless, very useful hint and maybe such a feature would even be something to consider in Config Devel itself.

I would be willing to try to implement this, but wanted to check first, whether this is something that's considered desirable since it may serve somewhat of a niche use-case. On the other hand this does get rid of a couple if-else constructs in favor of a (what I would consider "proper") plugin system, but while I think that's a good trade-off in its own right, it's not my decision to make. So would love some thoughts on this, thanks!

tstoeckler → created an issue.

Oops, got the screenshots mixed up.

tstoeckler → created an issue.

Glad that you got it to work, nice.

Sounds like a good plan regarding the issues.

tstoeckler → created an issue.

The fix would be to change DraggableListBuilderTrait to set the result of buildRow() as #data rather than child elements.

Completely disagree. If we do want to change anything here, it should be the other way around. In fact, originally when '#type' => 'table' was introduced the plan was to convert all tables over to using child elements instead of '#rows'. That never happened, but the fact that most tables in core use '#rows' is simply a remnant of them previously using '#theme' => 'table' instead of '#type' => 'table' before the latter existed, not that using child elements is somehow a problem.

The reason that using child elements is in fact preferable is that as child elements they actually proper participate in form building, and can, in particular, have '#process' callbacks. Using '#rows' bypasses the form handling and just goes through the render system. On the other hand, there is nothing that using '#rows' brings over using child elements.

In particular,

for example, there's a #style attribute on the entire row. That totally fails to work when they're used as render elements.

is completely untrue, as far as I can tell. In fact DraggableListBuilderTrait itself sets the draggable class on the entire row. I don't know what you mean by a "#style attribute" but you can certainly set '#attributes' => ['style' => $style] on the row element, if you want to do that for whatever reason.

Re #41: Interesting, that didn't come up here because that would be fixed as soon as you add config schema to your entity type. But I guess it wouldn't hurt to add a (float) type-cast to getWeight(). I can open a follow-up for that, if people think that makes sense.

I guess this can be done now, so rebased the MR.

Re #35: fair enough, makes sense. Added a draft change notice: https://www.drupal.org/node/3425054 → . Feel free to publish if you think it's alright. (And then ideally set the issue back to fixed)

Re #37: Wow, never thought of that possibility, but yeah, totally makes sense to not blow up in that case, doesn't really cost much to add that fallback: Opened 🐛 [regression] Add a fallback in DraggableListBuilder::getWeight() to support entities with a NULL weight Needs review Sorry for the trouble and thanks for reporting it.

tstoeckler → created an issue.

Oops forgot to link the docs in the issue summary.

tstoeckler → created an issue.

Sorry for never seeing this issue. Release an initial alpha now.

Not sure if I'll have the time to properly drive this to stable in the near future, but at least it's usable via Composer now.

Thanks for the patch! Didn't review it, but can verify it works. Hit a bug with Diff module with this, though, that it fails to diff non-URL URIs that are created with this patch.

Created a little diff plugin for that, attaching that in case this helps anyone. Once this lands, will open an issue for that over in Diff module. To be placed in src/Plugin/diff/Field for you custom module (removing the _.txt extension) and then just replace the module name in the namespace.

Pretty sure this broke even worse with Drupal 10.2.x because with https://www.drupal.org/node/3386675 → the default value form can now be built (and, thus, WorkflowsFieldItem::getSettableOptions() can be called) without the field storage even being saved.

Thus, whether or not the workflow has a state is now not the only failure condition, also there may be no workflow selected and also the entity may not have the respective field yet.

Will create a merge request to fix that. Hope it's OK to use that issue for this slightly expanded scope.

Noticed two things, that are related, that may warrant separate issues:

1. The if ($value) check will fail for a state with the ID '0' which is not ideal
2. I had initially thought that if the value cannot be determined from the entity, that we should fall back to the initial state that the workflow has configured. That would need a check, as well, however, because the initial state may not be configured initially (no pun inteded) when creating a workflow. Since that's currently not done, though, either if the value is explcitly not set for an entity, that would be a separate issue, if we want to change that.

Note that I found this in Config Overlay → , because we actually have tests for installing from configuration that broke when we ported to the storage transformer system there. That also verifies that the simple change (will create draft MR in a minute) is sufficient, as we actually apply a core patch in the CI there and that makes the respective tests pass.

Will need an actual test here, though, I presume.

tstoeckler → created an issue.

tstoeckler → created an issue.

Alrighty, seems all tidy now.

Thanks again! Will release new bugfix versions for this (and the other one) soon, but marking this one fixed for now.

Thanks for the follow-up. Meant to reply with an updated patch "in hand", but that never materialized, sorry about that...

Sounds good regarding the getAccount()//setAccount() methods. 👍

And yes, makes sense regarding the test, as well, I was a bit in a mental rabbit hole when I wrote that thinking about the use-case in our project of having a custom AccountInterface implementation. Just setting a UserSession in a test is absolutely the sensible thing to do. 👍

Not sure when I will get around to updating the patch, but wanted to reply at least, for now.

Filled in the proposed resolution and added a draft change notice for this, so back to needs review.

Merged the branch now, since the test passed. Note that I also included a fix for hook_module_preinstall() but that is somewhat theoretical. If a module deleted it's own shipped configuration in its hook_install() then that would have previously not been recorded but should be now. Not including a test for that, though, as that seems a bit far-fetched.

Opening an MR now for the backport to 8.x-1.x and then will release new bugfix versions.

Thanks again!

Opened 🐛 Config of uninstalled modules gets recorded in config_overlay.deleted Active for the other issue. Will backport this one here to 1.x over there, since it's closely related.

Thus, marking this one as fixed.

tstoeckler → created an issue.

So what happens is that when the module is uninstalled, config_overlay.deleted gets correctly deleted, but then Config Overlay's config subscriber detects that as a configuration deletion and adds config_overlay.deleted to the list of deleted configuration in config_overlay.deleted, in effect recreating the configuration.

This can be fixed, by simply ignoring this particular case in the config subscriber. Will push a branch with a fix and a test.

I noticed, though, that there is a similar problem in that Config Overlay adds the configuration names of other modules that are deleted to config_overlay.deleted as well. Will open an issue for that.

Yeah, that sounds fair enough. Added a note to the project page now, so closing this. Feel free to re-open if you think something else should be done or if you have suggestions on better wording for the note on the project page.

Just for the record, I also tested that the message only shows up with Config Overlay 2.x (and Config Ignore 2.x). If you downgrade to Config Overlay 1.x it also disappears (although, of course, it would be better to upgrade Config Ignore instead).

Thanks again!

Added 📌 Test with Config Split 1.x and Config Ignore 2.x Active for the test coverage issue.

Note that for Config Ignore there is a weird message that shows up in the UI at least, see ✨ Misleading message on "admin/config/development/configuration" about configurations handled via config_overlay Active .

I did see some issues with the old Config Split on a project, but didn't investigate yet, so possibly the resolution here will be just to add a Composer conflict with the older versions, but not sure yet.

tstoeckler → created an issue.

Interesting, thanks for raising this. I rarely use the configuration UI, so I had never seen this.

The message only shows up when using version 8.x-2.x of Config Ignore. It was removed in 8.x-3.x without much explanation (see #3099674: Create 3.x version using the new Drupal 8.8 API → ), presumably because it did not work when porting to the core API.

So basically the easiest way to solve this, I suppose, is to upgrade Config Ignore.

Also related is #3182999-9: Automated tests failing due to config_filter dependency → (in particular comment #9, which the link links to), where the code that generates that message was updated to use the core transformer API. I don't understand all of the context, but reverting that relevant part of the code, i.e:

diff --git a/config_ignore.module b/config_ignore.module
index 96e1efd..c12713c 100644
--- a/config_ignore.module
+++ b/config_ignore.module
@@ -16,7 +16,7 @@ function config_ignore_form_config_admin_import_form_alter(&$form, FormStateInte
 
   // Load Services that we need.
   $stock_storage_sync = \Drupal::service('config_filter.storage_factory')->getSyncWithoutExcluded(['config_ignore']);
-  $active_storage_sync = \Drupal::service('config.import_transformer')->transform(\Drupal::service('config.storage.sync'));
+  $active_storage_sync = \Drupal::service('config.storage.sync');
   $storage = \Drupal::service('config.storage');
 
   // Create two StorageComparer objects, one with the filter enabled and one

actually makes the message disappear for me. That's where I stopped my investigation, though.

So not really sure how to proceed here.

I don't think there's anything Config Overlay can do to influence the result of this message, as far as I can tell that code in Config Ignore to generate that message is just broken as soon as there is any storage transformer that removes configuration. But it seems that @bircher did that change intentionally, so I don't know how realistic it would be to get that code changed. In particular as that's for an old version of Config Ignore that's not even shown on the project page anymore.

So I guess we could just add a note to Config Overlay's project page about this issue and tell people to upgrade Config Ignore to get rid of this warning. Maybe we could also add an explicit Composer conflict for this version, so people are forced to be running with the new Config Ignore version. On the one hand that seems a little heavy-handed just to avoid an incorrect warning, on the other hand the integration test coverage is already only running with the newer version, so we are at least not testing with the older one.

Not sure, do you have thoughts about this?

tstoeckler → created an issue.

Awesome, thanks!

Yeah, I totally agree, just want to avoid getting into arguments with people...

But then RTBC++, I guess ;-)

@Wim Leers summoned me to take another look at this. Really looks great, have no objections. I did review all the changes since my last patch and they are all fine, but since I did author a good chunk of what is still in the current patch, not going to RTBC as people do sometimes get very sensitive with that. Total endorsement of this landing, though, would really love to see this in for all kinds of reasons! Thanks everyone for pushing this along and AFAICT very close to the finish line.

Thanks for opening this issue!

To be perfectly honest, I was pretty confident that this was something with your particular setup, but I went ahead and tried to reproduce this and turns out it is absolutely reproducible. I am quite surprised, I guess I never re-installed the module before.

I guess somehow we mess with the config incorrectly so that when uninstalling the module, the config_overlay.deleted config is not properly deleted. Really strange, but definitely a critical bug. Will try to find some time to investigate this further.

For now, just attaching a file with my bash output for reproducing this. You can see that after uninstalling the module that the config_overlay.deleted.yml file gets exported and then the re-install fails.

Just leaving a note here, that I don't think this is correct. In fact those tests pass on Drupal 10. And the tests that are patched here explicitly install a profile, so they don't need to set $defaultTheme in the test. Leaving open, though, to investigate at some point, just wanted to note this in case anyone is wondering about this...

Here we go. This is not extensively tested, but wanted to get the ball rolling.

Not sending for a test run, because tests seem to be broken still, as far as I can tell.

tstoeckler → created an issue.

Wow, sorry for causing trouble due to the core issue! Opened 🐛 [regression] Entity::toUrl() without argument is broken for entity types with a URI callback Needs review now to rectify this, so hopefully at least no-one else will be hit by this.

I was wondering how this could have broken BC, as I thought I/we had been careful about that, but I completely missed that Entity::toUrl() does in fact handle entities with a uri_callback and that those entities do not necessarily define any link templates.

Sorry about that! Opened 🐛 [regression] Entity::toUrl() without argument is broken for entity types with a URI callback Needs review as a (hopefully) quick follow-up to revert the unintended API change.

tstoeckler → created an issue.

Wow, just saw this. Sorry for introducing this! I can only speculate as to what lead to me use that indirection, but absolutely agreed that it's much more readable now, thanks! And this will (hopefully) teach me to more carefully consider the consequences of array_flip() in the future. In any case, great to see this fixed, thanks again!

I encountered this as well, in the context of 💬 Complete JSON:API's handling of field (including computed & empty fields) cache metadata Needs work .

I went with a slightly different approach than in #7 by checking this in the field formatter rather than the entity view display. It shouldn't make much of a difference practically, but there may be cases were a formatter is used without a view display (at least theoretically).

Also added some test coverage.

tstoeckler → made their first commit to this issue’s fork.

tstoeckler → created an issue.

D34dMan → credited tstoeckler → .

Looks good to me, thanks!

Thanks for taking a look.

Fine with deprecating the methods instead, sure. Just one question: If someone actually sets an AccountInterface that is not a UserInterface then getUser() would not return a UserInterface with the current code. Not sure how would like that handled, I would see the following options:

1. That's fine, just document that that can happen, since getUser() is deprecated then, all is good (my preference)
2. Actually track a separate $user and $account object (which would be very confusing IMO)
3. In that case return the account from getAccount() but return NULL from getUser() (also confusing IMO, albeit less so)
4. Try to load a user from the account and return NULL otherwise (not much better than the previous one in my book)

Since there are currently no typehints we are pretty much free to do whatever without risking any PHP breakage, but, yeah, would be good to know what the API should be in your opinion with this change.

In terms of test coverage: An actually meaningful test would only be possible with a custom AccountInterface implementation, which this patch enables, but does not exist in core itself. Is that what you had in mind, or would you be happy with unit(-y) test coverage, as well?

Also Re #14:

1. I couldn't think of any legitimate reason why someone would extend the class either.
2. The 11.x branch (somewhat confusingly) is not yet opened for 11.x development, but always targets the next 10.x release (i.e. 10.3.x right now). I already opened 📌 Remove deprecated ContextProvidersPass Active which can be tackled once 11.x development actually starts.

I was quite confused by #13, but then re-read the issue and this is what happened:

1. In #5 @smustgrave requested a CR after which I wrote one and updated the link in the code.
2. In #8 @longwave considered the CR unnecessary, thus deleted it, but committed the patch with the link
3. In #13 @quietone noticed the broken link and added a (draf) CR

So maybe you three need to hop on a call and discuss what does or does not warrant a CR ;-)
When there's consensus on whether or not one is needed, the link can either be dropped or updated. I don't feel strongly either way (as hinted at in #6 I could certainly live without one, but I don't really care).

Added the proposed resolution (vageuly copied from #7) to the issue summary. There is no functional change here, but for the actual bugfix, so I don't think this needs a change notice.

🐛 Statically cached derivative definitions cannot be cleared in any way RTBC landed, so this is now genuinely ready for review.

Awesome thanks. Published the change notice now. (I put 10.3.0 as the version there, hope that was correct.)

Awesome, thanks a lot! Removed the branch now. Much appreciated 👍

Note that the core issue has been committed now. 🎉️

Still makes sense for the patch/MR to be committed as is, as the new trait wil only be in 10.3 but that should hopefully alleviate any concerns of the long-term maintenance burden.

The missing piece of the puzzle is you need to edit the workflow and enable it for some content types.

💡🎰 Yes, that is exactly what was missing at least in my understanding of the issue. Thanks, I will wander the earth in a slightly less confused state now thanks to you. 😉

Re #48, right, sorry, my explanation made no sense, with the dependency the view would not have been installed then. Not sure what ist going in then...

Hmm... maybe you are running into the issue I described over at #3060633: Check that views do not get created with broken handlers → which is the ordering of the import. Not sure where you copied the workflow config to, but if you copied it into the Minimal profile it will be installed after the view, so at that point the view will already have been created with the broken handlers.

I think the dependency should be on the editorial workflow config (workflows.workflow.editorial) not the workflow module itself. That should make sure there are no broken handlers.

Hmm... I disagree with that. For a module whose primary purpose is the effect it has for site-building but it does provide certain API entrypoints (classes, services, etc.), it's really very vague what is a feature and what is a bug fix. And thus the distinction between what should be 2.0.1 and 2.1.0 is also very vague. Either way, I don't intend to maintain any distinct branches within 2.x both to reduce the amount of maintenance required, but also because I don't really see any practical benefit in that. The 2.0.* release series is effectively unmaintained now. If a security issue were to arise, I would cherry-pick the fix and release a 2.0.1, but other than that nothing will happen there. And in fact I found an issue recently that may lead me to release a 2.2.0 soon anyway.

So I would really be somewhat disappointed if my (in my opinion arbitrary) decision to release 2.1.0 instead 2.0.1 lead to this being "won't fixed" and I would really appreciate me not having the risk of stumbling over the obsolete 2.0.x branch in the future, but at the end of the day it's up to you, of course, and it's not really a big issue either way, just a minor annoyance. So, again, would be grateful if this were to be resolved, but if the above doesn't convince you feel free to mark this won't fix, no hard feelings.

Just a note that I had missed the last commit accidentally when creating 2.0.0. Created 2.1.0 now to rectify that.

Ahh, darnit! Talk about the different branches causing confusion... I had meant to push the commit to both branches to make it clear that they are the same (and that one can be deleted), but I guess I messed that up. Thanks for being so diligent with this and sorry for the trouble! Also really helpful to have that network graph in Gitlab, I didn't know about that 👍.

Pushed the commit to 2.x now as I should have (and thought I did) in the first place.

(Just FTR, I also created a 2.1.0 release now with that commit, because the commit missing from 2.x also meant I had missed it when I created 2.0.0. In hindsight I guess that could/should have been 2.0.1 instead of 2.1.0, but what's done is done and it shouldn't be a big issue either way, I guess. But just wanted to mention it to make clear that that was intentional.)

Thanks for the quick and detailed response.

A dev release on its own is not something we encourage showing on project pages.

I understand the reasoning and the goal was not to encourage users to actually install the dev release, but as far as I can tell showing dev releases on the project page is the only way to get the CI pipeline badge to show up. That was why I enabled that both as a marker of quality for people evaluating the module, but also for me to quickly see if something is wrong when I am on the project page. If there is some other way to get that to show up, I have no issue hiding the dev release from the project page.

2.x is a bit nicer since issues and merge requests can continue to target 2.x for the whole major version. You don’t need to update everything through 2.0.x, 2.1.x, 2.2.x, etc.

Exactly! That's why I initially went with 2.x.

Once there is a tagged 2.0.0-alpha1 release, or higher, 2.x-dev would have shown alongside it on the project page.

Ahhh, this part was the missing puzzle piece for me, I did not know that. So I (unknowingly) jumped the gun, when I could have just waited for a release and everything would have been fine and dandy. Oh well, now I know. Thanks!

So given that I would like to propose to remove the 2.0.x release and branch instead. Updating the issue summary accordingly.

BTW I created a 2.0.0 release now, so once the 2.0.x release is gone the 2.x should hopefully appear there (including the CI badge).

Alrighty implemented that now. Will mark this fixed, finally, and now on to tag a 2.0.0 stable version.

Alrighty, finished this yesterday. Was a bit of a back and forth and also a bit unfortunate because I had just blindly assumed that Config Splits also reads the information about the splits itself from the transforming storage and not the active storage and that might have made things a bit more complicated in some cases but in the end a lot more elegant. I am also fairly strongly convinced that it's wrong that Config Split reads the active storage for that during the transformation, but presumably there are reasons for that and in any case that's likely not going to change anytime soon.

So basically everything mostly worked already since the transformer rewrite. Now changing the subscriber priority so that Config Overlay runs first fixes the issue that config splits are not detected on initial import. "Nested splits" (see https://git.drupalcode.org/project/config_overlay#nested-splits ) don't work out of the box on the initial import, but since Config Split reads the active storage (see above) that's easy to fix for people themselves so we don't need to work around that.

I was a bit disappointed that I thought there was no way to get the Config Split directories to not have "unnecessary" configuration (from the perspective of Config Overlay) until I looked at the "stackable" feature. So to support that now we actually run both before and after Config Split on both import and export. So while that's a slightly "brute-force" approach it's also not too inelegant because Config Overlay's operations are inherently idempotent.

So all in all, glad to have this fixed finally.

Having written this just now, I just realized it would be even nicer to dynamically add the "second round" of transform subscribers dynamically only if Config Split is enabled by putting them in a second service. Marking needs work again for that. That will alleviate the biggest chunk of inelegance which is that currently everything is run twice unnecessarily even if you do not have Config Split installed.

Thanks in advance and apologies for the troubles!

tstoeckler → created an issue.