Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[FrameworkBundle] Serializer groups support#13107
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
9de8a0f to47012cfCompare475e6ce to4ecb81eComparedunglas commentedDec 25, 2014
This PR is now ready to be reviewed. |
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.
IMO 4 lines do not qualify for that.
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.
Ok I'll remove it.
4ecb81e to72bfc01Compare72bfc01 to106fe6fComparedunglas commentedMar 3, 2015
Made |
dunglas commentedMar 3, 2015
ping @symfony/deciders |
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.
Do we have other examples in Symfony where such a "convention" is harcoded? I know it's not the case for the routing, but probably done the same way for the translator.
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.
JMSSerializerBundle which is probably the most used use theserializer directory name for configs (see conf) , but it can be changed for any bundle manually. I think we should look for resource existence before searching for a default one.
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.
I'm not saying that we should make it configurable. As a matter of fact, I think there is no need to make it configurable, I just wanted to be sure that we behave like other parts of the framework.
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.
- Same for the validator (this code is based on the validator code):https://github.com/symfony/symfony/blob/2.7/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php#L786-L794
- For translation it's the same approach but directory based instead of file based:https://github.com/symfony/symfony/blob/2.7/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php#L670-L679
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.
The point is to have a coherent behavior for both components (Validation, Translation, Routing...). If we allow to create a file by class (why not, I personally use annotations), it must be the same at least for validation mappings.
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.
For this PR, I'd prefer to keep what we have in other parts of the framework.
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 Validator component just got the ability to read the configuration from a directory (see#13855). Why not adapt this approach here too?
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.
Thanks for the pointer@xabbuh. I had not seen this PR.
Support forResources/config/serialization/*[.xml|.yml] added.
106fe6f to290d50bComparedunglas commentedMar 10, 2015
Rebased. |
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.
Could this cause issues if cache is warmed up in one place, and later moved to another directory? I know we not necessarily support such case, but I'd rather avoid adding one more obstacle here.
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.
Probably but this is already the case for the validator:https://github.com/symfony/symfony/blob/2.7/src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php#L749
If we change this behavior, we must change it everywhere in another PR.
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.
For the record, though it still looks a little odd to me to have this service here that may not be used, I do like it. As I mentioned before, it's private, so will be removed if it's not being used. And I think it ends up being quite usable. 👍
dunglas commentedMar 16, 2015
Is this PR ok to be merged? |
fabpot commentedMar 18, 2015
Thank you@dunglas. |
…glas)This PR was merged into the 2.7 branch.Discussion----------[FrameworkBundle] Fix regression introduced by#13107| Q | A| ------------- | ---| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | n/a| License | MIT| Doc PR | n/aI've introduced a bug in83b56f6. An error will be throw if the serializer is enabled but no cache is set.This is the fix.Commits-------efadac0 [FrameworkBundle] Fix regression introduced by#13107
This PR was squashed before being merged into the 2.7 branch (closes#764).Discussion----------Serializer groups support| Q | A| ------------- | ---| Bug fix? | no| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| License | MITEnable serializer groups (related tosymfony/symfony#13107).Commits-------3561a7c Serializer groups support
This PR enables serializer groups in the full stack framework (including XML, YAML, annotations and caching).