[FrameworkBundle] Integrate custom property accessors into the framework #22191
Conversation
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php
Outdated
Show resolved
Hide resolved
|
@dunglas Done! |
|
Minor correction to RegisterComponentMapping so it can be merged to 3.4. Failures in the builds seem to be unrelated to the PR. |
|
@dunglas time for another review? I'm leaving for holidays in a few days and I'd like to close this PR along the way |
|
@fabpot Fabien, are you still interested in including this feature in Symfony 3.4? Kévin seems to be busy and feature-freeze deadline is almost here. I'd rather not miss it again... Should that be the case I'll rework the PR so it can be rebased to the 3.4 branch. Thanks! |
087747a to
e948620
Compare
|
I'm working on turning the builds green... |
bc95a83 to
fe9b784
Compare
|
I'm puzzled with the failed test on PHP 7.x. Can somebody throw some light on how debug it? |
|
You will need to add the PropertyAccess component to the |
|
I've tried that, but the failed test is still there. Well, changes to |
|
Our Travis config makes sure that the components are taken from the PR for the |
7fcdcd8 to
544e466
Compare
|
@xabbuh , |
| @@ -15,6 +15,12 @@ | |||
| "homepage": "https://symfony.com/contributors" | |||
| } | |||
| ], | |||
| "repositories": [ | |||
There was a problem hiding this comment.
These changes are only temporary until the PR is accepted or until PR #22190 is merged...
|
@nicolas-grekas #22190 does not introduce any BC issues (or at least it should not). There is an additional (and optional) parameter into the constructor of a #22190 was already reviewed by @HeahDude and was only pending on some changes that are already addressed. This feature will be useful to my students, as they are not proficient in English and find problems with the current singularization rules when creating entities. They'll be very grateful it could be merged into 3.4 which is LTS and still works with PHP 5.6/7.0. I'll commit it as many personal time as I'm able to if necessary to help you on that during the stabilization phase. Thanks! |
For that case, wouldn't #23789 be of use for them too? This makes me wonder if we really need both PRs #22190 and #23789. And if we don't need both we should make a careful decision of which PR to merge. |
Ok, I see the struggle here. The problem with #23789 is that you must create a new naming strategy per project even if you're interested in just overriding just a pair of accessors. It would be a little overkill for my students, but is a viable solution (annotations are more intuitive for them, though). In fact, my first approach a couple of years ago was similar to David's one (see PR #15766) but Fabien decided that defining PropertyAccess options was the way to go (#5013). That was the seed of #22190. Both #23789 and #22190 address some issues in a different way (i.e. singularization) and in any scenario one could be better than the other one, but there is no silver bullet that works best on every scenario. Each of the approaches have different pros & cons. Let's start with some of them and we can add more as the discussion moves forward: Custom naming-strategy (#23789)Pros:
Cons:
Custom property accessors (#22190)Pros:
Cons:
Anyway, I don't think both approaches are incompatible but in fact complement each other. Granularity is the key here: sometimes a custom (and global) naming strategy could be the best solution and in others changing a couple of accessors could be just enough. |
|
Another use case for NamingStratgy: controlling of camelizing (exists this Word?) I use the Serializer (ObjectNormalizer) a lot. There is a problem with the interplay of PropertyInfo, PropertyAccess and Serializer (see PR #23656). My suggestion was to add a new option "don't camelize", so you can create a special PropertyAccessor for the Serializer component. But one flag for one special usecase is bad. Instead of adding a flag, with #23789 you can change the strategy. But as @lrlopez said, the features complement each other perfectly. I also have use cases where annotation is much easier and better. And would love to see this in the core. But not everything can be solved well with annotation. Actually, everything you want to change Global (for one PropertyAccess instance). |
|
Until a decision is made, do I need to make any additional changes to the PR? The "Needs work" label is still assigned to the issue |
544e466 to
148985f
Compare
148985f to
91d9d74
Compare
|
Status updated and milestone changed to 4.1 where PHP7.1 features can be used. |
This is the last of the two PR related to #18016. It implements changes on
FrameworkBundleso it integrates with the new features ofPropertyAccess.Builds will fail because the new component code has not been merged in this branch in order to ease the review. Should the review be positive, I will rebase this with #22190 so they can work together.