8000 [PropertyAccess] add naming strategy by DavidBadura · Pull Request #23789 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[PropertyAccess] add naming strategy #23789

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[PropertyAccess] add naming strategy #23789

wants to merge 1 commit into from

Conversation

DavidBadura
Copy link
Contributor
@DavidBadura DavidBadura commented Aug 4, 2017 •
edited
Loading
Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #22454, #21569, #9336, #5219 and others
License MIT
Doc PR todo

I'm from issue #23656 which is about a problem with the PropertyAccessor. The problem is that the PropertyAccessor always camelizes properties before searching for the right methods.

The other referenced issues (above) are pointing out a lot more problems about how the PropertyAccessor creates the method names:

  • wrong singularizing of properties
  • only singluarizing in English
  • hardcoded method prefixes
  • etc.

#22190 & #22191 should fix all this, but:

  • it's a huge PR and a big intervention for PropertyAccessor
  • you need to add an annotation for all properties, if you want to change how it singularizes

The simplest way to solve these problems is to extract how the method names are created. The advatages of this method are:

Simple override example:

class MySpecialStrategy implements NamingStrategy 
{
    private $defaultStrategy;
    
    public function __constructor(NamingStrategyInterface $defaultStrategy) 
    {
        $this->defaultStrategy = $defaultStrategy;
    }
 
    public function getAddersAndRemovers($class, $property)
    {
        if ($class === 'Group' && $property == 'members') {
            return ['joinMember', 'leaveMember'];
        }
  
        return $this->defaultStrategy->getAddersAndRemovers($class, $property);
    }
    //...
}

An underscore strategy or a yaml based configuration strategy would also be possible.

What do you think about this solution?

Todos:

  • update PropertyAccessorBuilder
  • make it configurable
  • add CamelCaseStrategy tests
  • add PropertyAccess tests with another strategy
  • add PropertyAccessBuilder tests
  • rebase on master
  • add typehints

@DavidBadura
Copy link
Contributor Author

I have added the configuration and few tests.

@Koc
Copy link
Contributor
Koc commented Aug 5, 2017

You are the genius!

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Aug 5, 2017
@nicolas-grekas nicolas-grekas requested a review from dunglas August 5, 2017 14:08
@DavidBadura
Copy link
Contributor Author

i added more tests

@nicolas-grekas
Copy link
Member

Moving to 4.1 as review is pending (ping @symfony/deciders)
Rebase on master might be needed, where PHP 7.1 features can be used btw.

@nicolas-grekas nicolas-grekas modified the milestones: 3.4, 4.1 Oct 8, 2017
@xabbuh
Copy link
Member
xabbuh commented Oct 8, 2017

Could the changes proposed in #22190 be relevant here? Do we need to think of ways to make both PRs interact?

This was referenced Oct 8, 2017
Closed
Closed
@DavidBadura DavidBadura changed the base branch from 3.4 to master October 24, 2017 18:39
@DavidBadura
Copy link
Contributor Author

rebased on master & typehints added

@DavidBadura DavidBadura reopened this Oct 24, 2017
ro0NL
ro0NL reviewed Nov 10, 2017
View reviewed changes
src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php Outdated
@@ -1113,6 +1113,10 @@ private function registerPropertyAccessConfiguration(array $config, ContainerBui
->replaceArgument(0, $config['magic_call'])
->replaceArgument(1, $config['throw_exception_on_invalid_index'])
;

if (isset($config['naming_strategy']) && $config['naming_strategy']) {
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps add a cannotBeEmpty() in config, so only isset() is needed here.

Copy link
Contributor

Choose a reason for hiding this comment

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

no need for isset() actually as the node does not seem to be removed if not set (hence will have an empty value). A cannot be empty would break BC IIRC.

Copy link
Contributor

Choose a reason for hiding this comment

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

A cannot be empty would break BC IIRC.

Why so? This is a new config node.

No real opinion about isset($config['node']) vs. null !== $config['node'].

ro0NL
ro0NL reviewed Nov 10, 2017
View reviewed changes
Copy link
Contributor
@ro0NL ro0NL left a comment

Choose a reason for hiding this comment

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

👍 for this approach

@DavidBadura
Copy link
Contributor Author

comments addressed

@nicolas-grekas nicolas-grekas modified the milestones: 4.1, next Apr 20, 2018
@GoodGuyMarco
Copy link

What's the status here? 🤔

Koc
Koc reviewed Sep 16, 2018
View reviewed changes
src/Symfony/Component/PropertyAccess/NamingStrategy/NamingStrategyInterface.php
* @param string $class
* @param string $property
*
* @return array
Copy link
Contributor
8000

Choose a reason for hiding this comment

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

What structure should be used for return? Maybe better split for getAdders and getRemovers and return array of strings for both methods?

Koc
Koc reviewed Sep 16, 2018
View reviewed changes
src/Symfony/Component/PropertyAccess/NamingStrategy/NamingStrategyInterface.php
* Returns all possible setters.
*
* @param string $class
* @param string $property
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this PHPDocs has no sense: they just duplicates typehints. But we can better describe return: @return string[].

@nicolas-grekas nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@javiereguiluz javiereguiluz javiereguiluz left review comments

@Koc Koc Koc left review comments

@Taluu Taluu Taluu left review comments

@ro0NL ro0NL ro0NL left review comments

@DanielBadura DanielBadura DanielBadura approved these changes

@dunglas dunglas Awaiting requested review from dunglas

Assignees
No one assigned
Labels
Feature PropertyAccess Status: Reviewed
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.
10 participants
@DavidBadura @Koc @nicolas-grekas @xabbuh @GoodGuyMarco @javiereguiluz @Taluu @ro0NL @DanielBadura @carsonbot
0