8000 [2.6] Marked the ResolveParameterPlaceHoldersPassTest as legacy by saro0h · Pull Request #13941 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[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

Merged
merged 1 commit into from
Mar 17, 2015

Conversation

saro0h
Copy link
Contributor
@saro0h saro0h commented Mar 16, 2015
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.

@@ -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()));
Copy link
Member

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What should I do?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@saro0h wait for #13924 to be merged and rebase

Copy link
Member

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.

@saro0h saro0h changed the title Marked the ResolveParameterPlaceHoldersPassTest as legacy [2.6] Marked the ResolveParameterPlaceHoldersPassTest as legacy Mar 17, 2015
@saro0h saro0h force-pushed the fix-legacy branch 2 times, most recently from 623f2f8 to 11e8632 Compare March 17, 2015 10:08
@saro0h
Copy link
Contributor Author
saro0h commented Mar 17, 2015

ping @fabpot @nicolas-grekas

class ProjectServiceContainer extends ContainerBuilder
{
/**
* Be careful here: this class is included in Tests\Dumper\GraphvizDumperTest::testDumpWithFrozenCustomClassContainer
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/class/file

Copy link
Member

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 :)

Copy link
Member

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)

@saro0h
Copy link
Contributor Author
saro0h commented Mar 17, 2015

Thanks guys :) Changes have been made.

@fabpot
Copy link
Member
fabpot commented Mar 17, 2015

Thank you @saro0h.

@fabpot fabpot merged commit 67ca1f4 into symfony:2.6 Mar 17, 2015
fabpot added a commit that referenced this pull request Mar 17, 2015
… 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants
0