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

4.2.2 release changed the way tagged service are injected #29836

Closed
lyrixx opened this Issue Jan 10, 2019 · 9 comments

Comments

Projects
None yet
7 participants
@lyrixx
Copy link
Member

lyrixx commented Jan 10, 2019

Symfony version(s) affected: 4.2.2

Description
The framework changed the way tagged service are injected

How to reproduce
We are using API Platform, and it uses the following code:

        <service id="api_platform.subresource_data_provider" class="ApiPlatform\Core\DataProvider\ChainSubresourceDataProvider">
            <argument type="tagged" tag="api_platform.subresource_data_provider" />
        </service>

https://github.com/api-platform/core/blob/c20b86cfa6def8f1921dd7e02280d283fcd80e8a/src/Bridge/Symfony/Bundle/Resources/config/data_provider.xml#L25-L27

On 4.2.1 when I dump the injected service I have:

array:6 [▼
  0 => AggregatedLogDataProvider {#854 ▶}
  # ....
  5 => CollectionDataProvider {#1117 ▶}
]
array:6 [▼
  0 => CollectionDataProvider {#6297 ▶}
  1 => AggregatedLogDataProvider {#1071 ▶}
  # ...
]

Note: CollectionDataProvider is the default Data Provider from API Platform, and AggregatedLogDataProvider, #... are my Data Provider

Possible Solution

Patch all third party relying on this feature or fix Symfony.
Note: I tried to change the priority of the default Data Provider in APIP, and it fixes my issue.

@lyrixx lyrixx added the Bug label Jan 10, 2019

@lyrixx lyrixx removed the Bug label Jan 10, 2019

@stof

This comment has been minimized.

Copy link
Member

stof commented Jan 10, 2019

any chance to bisect which PR broke this ?

@lyrixx

This comment has been minimized.

Copy link
Member

lyrixx commented Jan 10, 2019

This is a bit hard to bisect because we merge older branch.
git bisect tells me it 7a7165e
but obviously it's not that :(
I will try on a 3.4 project but I don't have such project.

If someone has this, I used the following script:

<?php

require __DIR__.'/vendor/autoload.php';

exec('rm -rf '.__DIR__.'/var/cache');

try {
    $k = new AppKernel('dev', true);
    $k->boot();
    $c = $k->getContainer();
} catch (\Exception $e) {
    exit(255);
}

$dataProviders = $c->get('api_platform.collection_data_provider');
if (iterator_to_array($dataProviders->dataProviders)[0] instanceof AppBundle\Api\DataProvider\AggregatedLogDataProvider) {
    exit(0);
}

exit(1);

and in vendor/symfony/symfony folder:

git bisect start && git bisect good v4.2.1 && git bisect bad v4.2.2 && git bisect run php ../../../test.php
@stof

This comment has been minimized.

Copy link
Member

stof commented Jan 10, 2019

well, is there a way to force bisect to identify the merge commit merging the issue ? All our changes happen through merges (from PR, or from older branches). That would restrict the list of impacted changes (if it is a branch merge, we would then have to check only the few PRs merged in by this particular merge)

@stof

This comment has been minimized.

Copy link
Member

stof commented Jan 10, 2019

just found https://github.com/dealertrack/merge-bisect which might help here.

@lyrixx

This comment has been minimized.

Copy link
Member

lyrixx commented Jan 10, 2019

I took a fresh 3.4 website-skeleton and added APIP on it.
I created a stupid DataProvider and run git bisect on it.
It could be e07ad2b // cc @nicolas-grekas
If I took v3.4.21 and revert this commit, the test is green

@stof

This comment has been minimized.

Copy link
Member

stof commented Jan 10, 2019

Yeah, setDefinition now removes the old definition entirely before adding the new one. So replacing a definition (when resolving ChildDefinition or decoration for instance) will now change the order of services (the service will move to the end).

@lyrixx

This comment has been minimized.

Copy link
Member

lyrixx commented Jan 11, 2019

Is it considered as a BC Break ? IMHO it should be because it breaks so many applications

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Jan 11, 2019

Yes, let's revert that PR, it doesn't correctly fix the issue it was supposed to anyway. PR welcome.

@fabpot fabpot closed this Jan 13, 2019

fabpot added a commit that referenced this issue Jan 13, 2019

bug #29853 Revert "bug #29597 [DI] fix reporting bindings on override…
…n services as unused" (mmarynich)

This PR was merged into the 3.4 branch.

Discussion
----------

Revert "bug #29597 [DI] fix reporting bindings on overriden services as unused"

This reverts commit 44e9a91, reversing
changes made to 91b28ff.

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

4.2.2 release changed the way tagged service are injected

As asked by @nicolas-grekas #29836 (comment)

Commits
-------

b3e17d2 Revert "bug #29597 [DI] fix reporting bindings on overriden services as unused (nicolas-grekas)"
@bastnic

This comment has been minimized.

Copy link

bastnic commented Jan 13, 2019

Thanks @fabpot

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment