10000 Rename !tagged to !tagged_iterator · Issue #31289 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

Rename !tagged to !tagged_iterator #31289

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
nicolas-grekas opened this issue Apr 27, 2019 · 9 comments
Closed

Rename !tagged to !tagged_iterator #31289

nicolas-grekas opened this issue Apr 27, 2019 · 9 comments
Labels
DependencyInjection RFC RFC = Request For Comments (proposals about features that you want to be discussed)

Comments

@nicolas-grekas
Copy link
Member

In #30348 we introduced !tagged_locator, that complements the previously existing !tagged.
I feel like it would be more clear to rename !tagged to !tagged_iterator.
WDYT? Anyone willing to give it a try? That'd mean triggering a deprecation when !tagged is used.

@nicolas-grekas nicolas-grekas added the RFC RFC = Request For Comments (proposals about features that you want to be discussed) label Apr 27, 2019
@nicolas-grekas nicolas-grekas changed the title Rename !tagged to !tagge_iterator Rename !tagged to !tagged_iterator Apr 27, 2019
@chalasr
Copy link
Member
chalasr commented Apr 27, 2019

makes sense

@jschaedl
Copy link
Contributor

I'd like to work on this.

@emodric
Copy link
Contributor
emodric commented Apr 29, 2019

If I'm missing something, please excuse me, but won't this complicate definition of services for no real benefit, if one would want to support multiple versions of Symfony and at the same time fix deprecations? In my opinion, it is kinda similar to what happened to deprecating single colon notation for route controllers, which was later reversed, due to complicating support for multiple Symfony versions.

@nicolas-grekas
Copy link
Member Author
nicolas-grekas commented Apr 29, 2019

@emodric that's a valid point to consider. Thinking about it:

  • this point applies to bundles; bundles can replace the syntax by a compiler pass as they do already when supporting 2.x. This is the difference key with the colon notation, that had no alternatives.
  • the clear benefit is DX, by mean of consistency and easier understanding/discoverability.

With this analysis in mind, I'm on the side the objection shouldn't prevent the improvement from happening.

Makes sense?

@emodric
Copy link
Contributor
emodric commented Apr 29, 2019

Maybe. But the whole point of using !tagged for me was to get rid of compiler passes. You now have to introduce them again, which defeats the purpose.

But okay, it's true that in this case, there's a migration path.

nicolas-grekas added a commit that referenced this issue Jun 9, 2019
…or (jschaedl)

This PR was squashed before being merged into the 4.4 branch (closes #31321).

Discussion
----------

[DI] deprecates tag !tagged in favor of !tagged_iterator

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | yes
| Tests pass?   | yes
| Fixed tickets | #31289
| License       | MIT
| Doc PR        | tbd.

### Todo

- [x] fix tests

Commits
-------

ab8fb18 [DI] deprecates tag !tagged in favor of !tagged_iterator
nicolas-grekas added a commit that referenced this issue Jun 9, 2019
…terator (jschaedl)

This PR was merged into the 5.0-dev branch.

Discussion
----------

[DI] remove deprecated tag !tagged in favor of !tagged_iterator

| Q             | A
| ------------- | ---
| Branch?       | master <!-- see below -->
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| BC breaks?    | yes     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets | related ticket #31289   <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#tbd <!-- required for new features -->

This PR removes tag `tagged` which was deprecated in #31321

Commits
-------

9978184 [DI] removed tagged
@emodric
Copy link
Contributor
emodric commented Jun 14, 2019

@nicolas-grekas Can you give a few pointers on how to replace !tagged with !tagged_iterator with a compiler pass? In my tests, short of using a different set of config files, it is not possible to replace the syntax with a compiler pass because the argument is already converted to the instance of TaggedIteratorArgument.

@nicolas-grekas
Copy link
Member Author

the argument is already converted to the instance of TaggedIteratorArgument

This is the part that should be dropped on your side: don't use the syntax, use a regular argument instead and you'd do in SF2

@emodric
Copy link
Contributor
emodric commented Jun 14, 2019

I can't do that :/ The code in question is not a standalone app, but a product that can be installed on multiple apps (eZ Platform, Sylius) with multiple versions of Symfony (currently 3.4 and 4.3), but in 6 months it will be 3.4 (lts), 4.4 (lts) and 5.0.

This is the reason for my original comments. There's no way to programatically get rid of the deprecation and still support multiple Symfony versions (especially LTS ones) and I'd rather not reintroduce compiler passes just to injected tagged services which I already happily removed before when I dropped support for Symfony 2.8.

@emodric
Copy link
Contributor
emodric commented Jun 14, 2019

For those facing a similar issue, I found a non-intrusive way (meaning, it is only ran once, on container compile) by overriding YamlFileLo 8E1F ader and using it in my DI extension instead of the original one:

<?php

declare(strict_types=1);

namespace MyApp;

use Symfony\Component\DependencyInjection\Loader\YamlFileLoader as BaseYamlFileLoader;
use Symfony\Component\HttpKernel\Kernel;
use Symfony\Component\Yaml\Tag\TaggedValue;

final class YamlFileLoader extends BaseYamlFileLoader
{
    protected function loadFile($file): array
    {
        $content = parent::loadFile($file);

        if (Kernel::VERSION_ID < 40400 || !array_key_exists('services', $content)) {
            return $content;
        }

        foreach ($content['services'] as $serviceId => $service) {
            foreach ($service['arguments'] ?? [] as $key => $argument) {
                if ($argument instanceof TaggedValue && $argument->getTag() === 'tagged') {
                    $content['services'][$serviceId]['arguments'][$key] = new TaggedValue('tagged_iterator', $argument->getValue());
                }
            }
        }

        return $content;
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DependencyInjection RFC RFC = Request For Comments (proposals about features that you want to be discussed)
Projects
None yet
Development

No branches or pull requests

5 participants
0