Remove internals from autowiring #530
Remove internals from autowiring #530
Conversation
055fde3
to
d753cee
Compare
@@ -103,10 +103,10 @@ public function load(array $configs, ContainerBuilder $container) | |||
if (class_exists(SecurityExpressionLanguage::class)) { | |||
$container->setAlias('sensio_framework_extra.security.expression_language', new Alias($config['security']['expression_language'], false)); | |||
} else { | |||
$container->removeDefinition('Sensio\Bundle\FrameworkExtraBundle\Security\ExpressionLanguage'); | |||
$container->removeDefinition('framework_extra_bundle.expression_language'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the configuration prefix is "sensio_framework_extra", should use the same?
Yes, I agree with you
2017-11-19 18:26 GMT+01:00 Nicolas Grekas <notifications@github.com>:
… ***@***.**** commented on this pull request.
------------------------------
In DependencyInjection/SensioFrameworkExtraExtension.php
<#530 (comment)>
:
> @@ -103,10 +103,10 @@ public function load(array $configs, ContainerBuilder $container)
if (class_exists(SecurityExpressionLanguage::class)) {
$container->setAlias('sensio_framework_extra.security.expression_language', new Alias($config['security']['expression_language'], false));
} else {
- $container->removeDefinition('Sensio\Bundle\FrameworkExtraBundle\Security\ExpressionLanguage');
+ $container->removeDefinition('framework_extra_bundle.expression_language');
the configuration prefix is "sensio_framework_extra", should use the same?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#530 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADSq8rDfWJ0aY13awWYkxPmzmn6sBWLhks5s4GS8gaJpZM4Qi9Ke>
.
|
This PR was merged into the 4.0-dev branch. Discussion ---------- [HttpKernel] add type-hint for the requestType | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | see sensiolabs/SensioFrameworkExtraBundle#530 | License | MIT #SymfonyConHackday2017 Commits ------- 62d933d [HttpKernel] add type-hint for the requestType
d753cee
to
daffc9c
Compare
I just found #488, which this PR basically reverts. We should revert to the same names that we used before, don't you think? |
Yeah I agree, I'm going to do that |
daffc9c
to
4e0b3ae
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a BC break of course, but none of these services were meant to be used for autowiring.
So 👍
Reusing the old name has an advantage: it will avoid breaking bundles needing to deal with these services, if they kept support for the previous version of the bundle (I'm looking at FOSRestBundle here). |
Thank you @Simperfit. |
This PR was merged into the 5.1.x-dev branch. Discussion ---------- Remove internals from autowiring The bug does not seems to be in 3.0. #SymfonyConHackday2017 Commits ------- 4e0b3ae Remove internals from autowiring
<tag name="request.param_converter" converter="datetime" /> | ||
</service> | ||
|
||
<service id="Symfony\Component\ExpressionLanguage\ExpressionLanguage" class="Symfony\Component\ExpressionLanguage\ExpressionLanguage" public="false" /> | ||
<service id="framework_extra_bundle.expression_language" class="Symfony\Component\ExpressionLanguage\ExpressionLanguage" public="false" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't it be sensio_framework_extra.converter.doctrine.orm.expression_language
to match what is used in the framework_extra_bundle.doctrine_param_converter
service ?
<argument type="service" id="sensio_framework_extra.security.expression_language" on-invalid="null" /> | ||
<service id="sensio_framework_extra.security.listener" class="Sensio\Bundle\FrameworkExtraBundle\EventListener\SecurityListener" public="false"> | ||
<argument type="service" id="framework_extra_bundle.argument_name_convertor" /> | ||
<argument type="service" id="sensio_framework_extra.security.expression_language.default" on-invalid="null" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's wrong. It must used the configured service (which will be an alias)
|
||
<service id="Sensio\Bundle\FrameworkExtraBundle\Request\ParamConverter\DoctrineParamConverter" class="Sensio\Bundle\FrameworkExtraBundle\Request\ParamConverter\DoctrineParamConverter" public="false"> | ||
<service id="framework_extra_bundle.doctrine_param_converter" class="Sensio\Bundle\FrameworkExtraBundle\Request\ParamConverter\DoctrineParamConverter" public="false"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lots of services are missing the sensio_
part in the prefix, so they don't respect the best practices, and are not reverted to the old names
@fabpot this PR contains a bunch of mistakes when reverting to old ids. It would be great to fix them |
I will add followup pr with the a bug fix for the mistake.
Ill do that today.
2017-11-30 12:27 GMT+01:00 Christophe Coevoet <notifications@github.com>:
… @fabpot <https://github.com/fabpot> this PR contains a bunch of mistakes
when reverting to old ids
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#530 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADSq8kS6p1aK__WXoD43J1-yi-Uihie3ks5s7pEegaJpZM4Qi9Ke>
.
|
Yea, I approved too quickly - I thought it was a clean revert of the earlier PR :/ |
Fixed in #534 |
…rryan) This PR was merged into the 5.1.x-dev branch. Discussion ---------- Fixing #530 by manually looking at #488 and reverting Hi guys! #530 was basically meant to revert #488, but it was not done perfectly. I approved it too quickly - my apologies for that. This fixes #532 I looked at each line in #488 and manually (and carefully) changed the code to use the old service id's everywhere. I believe this should fix the issues! Cheers! Commits ------- cb0851b Fixing #530 by manually looking at #488 and reverting
The bug does not seems to be in 3.0.
#SymfonyConHackday2017