-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[2.6] Marked the ResolveParameterPlaceHoldersPassTest as legacy #13941
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
Conversation
@@ -38,6 +38,7 @@ public function process(ContainerBuilder $container) | |||
$definition->setFile($parameterBag->resolveValue($definition->getFile())); | |||
$definition->setArguments($parameterBag->resolveValue($definition->getArguments())); | |||
$definition->setFactoryClass($parameterBag->resolveValue($definition->getFactoryClass())); | |||
$definition->setFactory($parameterBag->resolveValue($definition->getFactory())); |
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.
Of you did it this way, parameters would also be responded in factory method names (see #13924 for another approach).
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.
What should I do?
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.
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.
#13924 has been merged now.
623f2f8
to
11e8632
Compare
ping @fabpot @nicolas-grekas |
class ProjectServiceContainer extends ContainerBuilder | ||
{ | ||
/** | ||
* Be careful here: this class is included in Tests\Dumper\GraphvizDumperTest::testDumpWithFrozenCustomClassContainer |
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.
s/class/file
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.
No need for the "Be careful here" prefix :)
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.
see also #13947 :) (though I think it's easier to merge this and close the other PR)
Thanks guys :) Changes have been made. |
Thank you @saro0h. |
… legacy (saro0h) This PR was merged into the 2.6 branch. Discussion ---------- [2.6] Marked the ResolveParameterPlaceHoldersPassTest as legacy | Q | A | ------------- | --- | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | ~ | License | MIT | Doc PR | ~ Splitted the ResolveParameterPlaceHoldersPassTest to obtain a legacy test to avoid deprecation rise. Commits ------- 67ca1f4 Marked the ResolveParameterPlaceHoldersPassTest as legacy
Splitted the ResolveParameterPlaceHoldersPassTest to obtain a legacy test to avoid deprecation rise.