Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

[3.0] Remove *.class parameters in bundles #12822

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
wants to merge 8 commits into from

Conversation

mayeco
Copy link
Contributor

@mayeco mayeco commented Dec 3, 2014

Q A
Bug fix? yes
New feature? no
BC breaks? yes
Deprecations? no
Tests pass? no
Fixed tickets #11881
License MIT
Doc PR none

Remove the *.class parameters in all bundle settings.

see: #11881

@mayeco
Copy link
Contributor Author

mayeco commented Dec 3, 2014

@stof can you please check this. (typos and posible mistakes)

@fabpot
Copy link
Member

fabpot commented Dec 3, 2014

@mayeco Can you do the same for all bundles?

</parameters>

<services>
<service id="validator" class="%validator.class%" factory-service="validator.builder" factory-method="getValidator" />
<service id="validator" class="Symfony\Component\Validator\ValidatorInterface" factory-service="validator.builder" factory-method="getValidator" />
Copy link
Member

Choose a reason for hiding this comment

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

this is wrong currently, because the validator.class parameter was replaced by the DI extension. You need to update the extension to set the service class instead now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @stof, when you say "is wrong" I did something wrong? This PR is only for change *.class parameter to the actual class in the service configuration.

Copy link
Member

Choose a reason for hiding this comment

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

Currently, the validator.class parameter is modified in the container extension. You will have to change that to modify the service definition's class directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, I now change to the new class definition, confirm is ok please

Copy link
Member

Choose a reason for hiding this comment

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

@mayeco you haven't changed the code in the DI extension to change the service class.

Even though the old API will be removed from 3.0, we should keep the code working in the meantime

@mayeco
Copy link
Contributor Author

mayeco commented Dec 3, 2014

@fabpot yes

@mayeco
Copy link
Contributor Author

mayeco commented Dec 3, 2014

Done @fabpot.

@mayeco mayeco changed the title [3.0] [WIP] Remove *.class parameters in bundles [3.0] Remove *.class parameters in bundles Dec 3, 2014
@mayeco
Copy link
Contributor Author

mayeco commented Dec 3, 2014

@fabpot What should I do with this tests?

$this->assertSame('Symfony\Component\Validator\Validator\ValidatorInterface', $container->getParameter('validator.class'));

@stof
Copy link
Member

stof commented Dec 3, 2014

@mayeco assert the service class instead

@mayeco
Copy link
Contributor Author

mayeco commented Dec 3, 2014

@stof the Validator 2.4/2.5/2.5_BC workaround is going to be removed in 3.0, so that test should be remove also, right?.

@stof
Copy link
Member

stof commented Dec 4, 2014

@mayeco only when the 2.4 API is removed. Until then, you need to keep it

@stof
Copy link
Member

stof commented Dec 4, 2014

And as I commented in some diff which has been hidden, you need to update the FrameworkExtension so that it updates the service class directly instead of setting the parameter

@mayeco
Copy link
Contributor Author

mayeco commented Dec 4, 2014

@stof for now lets keep the validator.class in this PR and I do other for the validator with the change in the parameter depending the api version, is that ok?

@stof
Copy link
Member

stof commented Dec 4, 2014

@mayeco you can already remove the validator.class parameter in this PR (making the work of this PR complete). The only requirement is that you update other places setting this parameter, not only the XML file (and check for other parameters as well to see whether we have some code setting them elsewhere)

@mayeco
Copy link
Contributor Author

mayeco commented Dec 6, 2014

@fabpot is done, but %validator.class% remain in validator.xml since change in runtime and I don't know internally the proper method to do it, and using decorator since is a very undocumented feature, is that ok with you @stof, can we leave this as it is and do a new PR for the proper way to change a container class.

@stof
Copy link
Member

stof commented Jan 4, 2015

@mayeco As I said previously, you should change the FrameworkExtension to get the service definition and change its class directly. This will let you get rid of the parameter. the class is not changed at runtime. It is changed at configuration time.

Decorators have nothing to do with this case.

and can you rebase your PR to fix conflicts (the master branch has changed since last month)

@fabpot
Copy link
Member

fabpot commented Mar 26, 2015

Rebasing is near to impossible, so I've created a new PR from scratch #14070

@fabpot fabpot closed this Mar 26, 2015
fabpot added a commit that referenced this pull request Mar 26, 2015
This PR was merged into the 3.0-dev branch.

Discussion
----------

removed all *.class parameters

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | yes
| Deprecations? | no
| Tests pass?   | no
| Fixed tickets | #11881, #12822
| License       | MIT
| Doc PR        | none

Commits
-------

8835d1a removed all *.class parameters
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
Morty Proxy This is a proxified and sanitized view of the page, visit original site.