8000 [PropertyAccess]: Allow custom singularify class (WIP) by lrlopez · Pull Request #15766 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[PropertyAccess]: Allow custom singularify class (WIP) #15766

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 1 commit into from

Conversation

lrlopez
Copy link
Contributor
@lrlopez lrlopez commented Sep 11, 2015
Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #13721, #13723
License MIT
Doc PR WIP
  • Write docs about the new feature

I'm going to teach Symfony to some Spanish students. We are using models created in other subjects into our test projects, and so we have to keep the original entities names and properties, which are in Spanish.

When implementing Doctrine X-to-many relationships, Symfony calls StringUtils::singularify() for property access. As the method only works for English nouns, awkward methods names (for a native speaker) have to be created for the app to work.

I know this has been discussed multiple times, but I'm proposing a different approach to the problem. What about allowing to override the default behaviour by using custom singularify classes? It would be backward compatible while allowing to plug-in different languages, should it be needed. The PR implements a new configuration parameter inside property_access called property_singularify that now can point to an alternative class that implements StringSingularifyInterface. The interface only defines a singularify() method. I still have to run some benchmarks using Blackfire.io to see if there is any noticeable performance degradation, although I don't expect much.

This is my first Symfony contribution so this PR could be (well, it will be) wrong at many levels, so all comments are welcome. Maybe just the idea is bogus, but I want to give it a try.

I'd rather like to see it accepted for the 2.8/3.0 release, I hope is not too late for it.

Thanks guys!

| Q             | A
| ------------- | ---
| Bug fix?      | no
| New feature?  | yes
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | symfony#13721, symfony#13723
| License       | MIT
| Doc PR        | WIP

- [ ] Document new feature
@linaori
Copy link
Contributor
linaori commented Sep 12, 2015

My guess is that the idea behind it is to program in English, the same "language" as PHP is. Not to mention that if 1 language is supported additionally, people want all of them. Language rules are complex and hard to maintain. Not to mention that they might change over time.

@javiereguiluz
Copy link
Member

@iltar if I understand @lrlopez right, he's proposing that Symfony allows anyone to plug in their own classes to support their own languages. Symfony would still ship with English support only.

@linaori
Copy link
Contributor
linaori commented Sep 12, 2015

@javiereguiluz That would mean people would install additional packages just because they prefer to program in another "language", I know this is beyond symfony, but to me that sounds like writing a spellings correction library where you add things like define('treu', true).

I don't think that making this configurable is the right way to go. I'm fine with people programming in their native "language" (so please don't get me wrong), I just don't see a lot of good coming from this.

@javiereguiluz
Copy link
Member

The truth is that some developers already use their own languages for some elements (e.g. entities). In those cases, Symfony makes hard-to-understand mistakes (see #13721 for example).

@wouterj
Copy link
Member
wouterj commented Sep 12, 2015

There is already a possibility to do this, which is built in into the PropertyAccess component. This feature is disabled however: #5011

The reason is that we need to fix this in a more generalized way (See #5013). We should instead finish the discussion in #5013 and enable the feature again (with the decided syntax).

Having a globally configured setting with a class doing this kind of thing doesn't seem right (a 3rd party library can require the English Singularization rules, while app code relies on some custom rules).

@webda2l
Copy link
webda2l commented Sep 12, 2015

Didn't met #5011 before.
👍 for @wouterj to finish and re-enable existing feature.

@lrlopez
Copy link
Contributor Author
lrlopez commented Sep 12, 2015

@iltar Knowing English is no-way a requirement to be a good PHP programmer. In fact, I've had students that were real good PHP programmers and had a rather basic English knowledge.

My point is that allowing people to use properties in other languages could benefit people that is no fluent in English. In fact, I do prefer using English properties in my projects, but I cannot force my students to do the same because I teach programming, not language.

By the way, what would be wrong with people publishing bundles offering other singularization rules? They wouldn't be officially supported anyway, but the community could. The key here is flexibility: if you want to use an alternate set of rules, at least you can.

Anyway @wouterj has a point with the globally configured setting. It would break other bundles/libraries expecting the default behaviour and that is really bad. That could be worked out allowing multiple singularization classes and merging the output of all of them, but I feels somewhat hacky.

I really don't 8000 mind how is implemented as long the override feature is included in the core, even if you must explicitly annotate each property. I agree that PR #5013 should be solved before 2.8 is launched. It will be very difficult to implement big changes after the release for some time.

If we move the discussion to #5013 I'll be glad to close this issue.

Thank you for your comments!

@linaori
Copy link
Contributor
linaori commented Sep 12, 2015

@lrlopez What I think is wrong with requiring packages that provide that, would be people acknowledging packages in production which are only used for development. It's not wrong to develop in another language, I just don't agree with having code to fix code because of a design choice.

@lrlopez
Copy link
Contributor Author
lrlopez commented Sep 12, 2015

@iltar I see your reasoning there, but I think that is unavoidable because PropertAccess takes place in runtime. So it is not only used in development but also when accessing properties.

Every now and then an issue is open because singularify() doesn't work with a particular English plural form and a code fix must be issued. That code fix goes to production because it gets run on production.

Anyway, if there was an override setting anywhere in the model definition (like #5013 proposes) there would be no need for any of this.

@linaori
Copy link
Contributor
linaori commented Sep 12, 2015

Isn't there something that can be re-used from the translation component here?

@lrlopez
Copy link
Contributor Author
lrlopez commented Sep 12, 2015

Hmmm... I don't think it would be aplicable to this case. Anyway, requiring the translation component for property access just doesn't seem right to me.

@DavidBadura
Copy link
Contributor

Big +1 for this. We have customers with special terms that simply can not be translated into english. If we create entities or forms, what should we do? We can not disable the singularify without override the whole PropertyAccess. And it is imposible to translate these terms into english without confusing. So, now we have misspelled methods to meet PropertyAccess singularify. And that's wrong for me. Actually PropertyAccess should adapt to our needs.

@fabpot
Copy link
Member
fabpot commented Sep 22, 2015

Closing in favor of #5013

@fabpot fabpot closed this Sep 22, 2015
@lrlopez
Copy link
Contributor Author
lrlopez commented Sep 24, 2015

@fabpot Fabien, PR #5013 seems to be stalled since October 2014 which is almost a whole year. Can anyone from the team have a look into it? I'd really like to have a mechanism embedded into PropertyAccess that allows overriding how it guess method names from the model before next major Symfony version gets released. If I can help somehow, just let me now. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants
0