Skip to content
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

make configuration compatible with sf3.0 #602

Closed
wants to merge 1 commit into from
Closed

make configuration compatible with sf3.0 #602

wants to merge 1 commit into from

Conversation

ip512
Copy link
Contributor

@ip512 ip512 commented May 15, 2015

As this will break compatibility with sf2.3, is it possible to put this change in a separate branch ? I'm not a version management expert, so if you have better suggestion, feel free to do something else

as this will break compatibility with sf2.3, is it possible to put this change in a separate branch ? I'm not a version management expert, so if you have better suggestion, feel free to do something else
@makasim
Copy link
Collaborator

makasim commented May 15, 2015

The current approach does not work in sf3.0, does it?

@ip512
Copy link
Contributor Author

ip512 commented May 15, 2015

Yes, the setFactoryClass method of Symfony\Component\DependencyInjection\Definition class is marked depracated and will be removed inf sf3.0.
With version 2.6+, there's some deprecated warnings only.

@makasim
Copy link
Collaborator

makasim commented May 15, 2015

so, maybe we can introduce a new imagine_sf30.xml file which is loaded only for sf3.0 version and overwrites services? wdyt?

In this case it will work for both 2.x and 3.x version at the same time

@ip512
Copy link
Contributor Author

ip512 commented May 15, 2015

I looked how it's managed in other bundles, in fosuser there is a test in the DI configuration :
https://github.com/FriendsOfSymfony/FOSUserBundle/blob/master/DependencyInjection/FOSUserExtension.php#L46-51
I think it's better to avoid config duplication.
In the meantime, I tried to launch the tests and I realized there's a lot of deprecated warnings and tests failures. Maybe this will require additional work to fix all that.

@Koc
Copy link
Contributor

Koc commented May 26, 2015

similar to #566

@lsmith77 lsmith77 added the State: Reviewing This item is being reviewed to determine if it should be accepted. label May 29, 2015
@lsmith77
Copy link
Contributor

seems like #566 is indeed a more BC friendly approach

@lsmith77 lsmith77 closed this May 29, 2015
@lsmith77 lsmith77 removed the State: Reviewing This item is being reviewed to determine if it should be accepted. label May 29, 2015
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