Problem/Motivation

In #2487588: Move CMI import/export directory "staging" to "sync", as it is confused with staging environments the config.storage.staging service was deprecated and replaced with config.storage.sync, but some contrib still refers to the old service name.

Proposed resolution

Fully deprecate the config.storage.staging service. The final removal will have to wait until Drupal 10.

Remaining tasks

Patch, test, commit.

User interface changes

None

API changes

Using the config.storage.staging service will report a deprecation warning.

Data model changes

None

Release notes snippet

CommentFileSizeAuthor
#13 interdiff.3115159.11-13.txt2.89 KBlongwave
#13 3115159-13.patch2.98 KBlongwave
#11 interdiff.3115159.5-11.txt2.58 KBlongwave
#11 3115159-11.patch2.9 KBlongwave
#5 3115159-5.patch1.97 KBdaffie
#2 3115159-2.patch1.05 KBdaffie

Comments

longwave created an issue. See original summary.

daffie’s picture

daffie commented
Status: Active » Needs review
StatusFileSize
new 3115159-2.patch1.05 KB

Made the service "config.storage.sync" the default service and the deprecated service "config.storage.staging" an alias of the first. Also added the deprecation line.

daffie’s picture

daffie commented
Version: 8.9.x-dev » 9.0.x-dev

#3114192: Remove deprecated services in core.services.yml is for 9.0. Lets do the same for this issue.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Issue tags: +Needs tests

Thanks for posting the patch.

We should add a test that confirms the deprecation message is triggered as expected.

daffie’s picture

daffie commented
Issue tags: -Needs tests
StatusFileSize
new 3115159-5.patch1.97 KB

Added a test. It fails, but I do not know how to fix it.

Status: Needs review » Needs work

The last submitted patch, 5: 3115159-5.patch, failed testing. View results

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

So I think this has uncovered a bug in our YamlFileLoader, in that aliased services cannot be deprecated - we return early as soon as we find an alias and ignore other keys. Symfony however does support deprecating aliases as per https://symfony.com/blog/new-in-symfony-4-3-deprecating-service-aliases

        if (isset($service['alias'])) {
            $public = !array_key_exists('public', $service) || (bool) $service['public'];
            $this->container->setAlias($id, new Alias($service['alias'], $public));

            return;
        }
...
        if (array_key_exists('deprecated', $service)) {
            $definition->setDeprecated(true, $service['deprecated']);
        }

#3111008: Use native Symfony YamlFileLoader should fix this for good, but for now we can duplicate the service definition instead of aliasing it? Or we can try and fix the YamlFileLoader somehow.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Related issues: +#3111008: Use native Symfony YamlFileLoader
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Hmmm... my original thought was duplicating is tricky because people who are altering services are going to be affected. But they already are because they should be using the config.storage.sync service to alter BUT that is currently an alias SO... I think that duplicating in this instance is okay.

I'm also thinking that maybe doing the deprecation in 9.x might be better because having the two services which are not aliased is not great.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

It is only 1 page of things to fix on http://grep.xnddx.ru/search?text=config.storage.staging&filename=

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs work » Needs review
StatusFileSize
new 3115159-11.patch2.9 KB
new interdiff.3115159.5-11.txt2.58 KB

The fix to YamlFileLoader is fairly straightforward, maybe this is better than duplicating the service?

However this doesn't apply to 8.9.x as Symfony\Component\DependencyInjection\Alias::setDeprecated() doesn't exist in Symfony 3.x.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented 
+++ b/core/lib/Drupal/Core/DependencyInjection/YamlFileLoader.php
@@ -149,7 +149,11 @@ private function parseDefinition($id, $service, $file)
+            $this->container->setAlias($id, $alias = new Alias($service['alias'], $public));

I think setAlias() returns the alias so you we don't need the hidden assignment.

Doing this only in Drupal 9 seems okay tbh.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
StatusFileSize
new 3115159-13.patch2.98 KB
new interdiff.3115159.11-13.txt2.89 KB

Thanks for review, addressed #12 and also improved the deprecation message format so we no longer need the comments above the service.

Status: Needs review » Needs work

The last submitted patch, 13: 3115159-13.patch, failed testing. View results

daffie’s picture

daffie commented
Status: Needs work » Reviewed & tested by the community

Now I know why I could not get the test to work.
I have been looking at the method setAlias() and yes it returns the Alias service. What is confusing is that the method uses the parameters $alias and $id in a reversed order to how they are used in YamlFileLoader class.
All the code changes look good to me.
All the remarks of @alexpott have been addressed.
For me it is RTBC.

@longwave and @alexpott: Thanks, I learned something new.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed a95e111 and pushed to 9.0.x. Thanks!

It would be nice to backport this to 8.9.x since this service is deprecated via a comment there but as SF3 doesn't support deprecating aliases that doesn't seem possible.

  • alexpott committed a95e111 on 9.0.x 
    Issue #3115159 by longwave, daffie, alexpott: Properly deprecate config....

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.