[PropertyAccess] RFC: PropertyPath options #5013

webmozart · 2012-07-22T16:44:23Z

The PropertyPath class currently makes a list of assumptions:

1. Objects are treated by reference

When the path author.name is modified, the following methods are called:

getAuthor()
setName() // on the author

Note that setAuthor() is not called.

2. Adders and removers are used if they exist

If a collection is written into the path tags and the methods addTag() and removeTag() exist, these are used to merge into the old collection instead of accessing the old collection directly via its ArrayAccess interface. The following methods are called:

getTags()
addTag(...) // instead of $tags[] = ...
removeTag(...) // instead of unset($tags[...])

3. Adders and removers follow English singularization rules

If a property path is called children, the guessed adders and removers are addChild() and removeChild().

Limitations

The above assumptions do not hold in all applications:

Sometimes setters should be called (no by-reference handling, see [Form] by_reference when embedding a single object #6965)
Sometimes elements should be written directly into collections even though adders/removers exist ([2.1][Form] CollectionType / PropertyPath BC Break? #4158, [Form] Fixed undefined index in writeProperty when saving value arrays #5257)
For other languages than English, singularization does not work

So, these assumptions should be made configurable. The challenge is how to build this API in an intuitive way.

Alternative 1: Option arrays

The first alternative is to pass the options in an array separately from the property path. For example:

Path:    artikel.stichwörter (= article.tags)
Options: array(
             'artikel' => array(
                 'by_reference' => false,
             ),
             'artikel.stichwörter' => array(
                 'use_adders' => false,
                 // or
                 'singular' => 'stichwort',
             ),
         )

Alternative 2: Path flags

The second alternative is to build the configuration options into the path:

artikel{!byref}.stichwörter{!adders}
// or
artikel{!byref}.stichwörter{singular=stichwort}

Alternative 3: Your suggestions

Do you have any other ideas? What approach do you prefer?

The text was updated successfully, but these errors were encountered:

Sorien · 2012-07-22T17:30:59Z

maybe author.name should call setAuthor &author.name should call getAuthor()->setName() and other like use_adders and singular should be configured by your 1st alternative

cryptocompress · 2012-07-22T19:33:58Z

i dislike mapping arbitrary strings to methods. path or path parts should be collection keys.

if author.name is modified, the following methods should get called:
get("author.name") or get("author")->get("name")
set("author.name", "...") or get("author")->set("name", "...")

registerXy("author.name", function validate($name) { ... }); // called on ->set("author.name", "...")

get("tags")
add("tags", ...) // instead of $data['tags'][] = ...
remove("tags") // instead of unset($data['tags'])

stof · 2012-07-22T19:48:50Z

@cryptocompress the goal of the property path is to map to getters and setters, not to force the user to implement a get and a set method to handle it (the Form component should not force you to implement special methods in the underlying object you want to use)

cryptocompress · 2012-07-22T19:55:34Z

for generic get/set there is no need for a path at all... what am i missing?

stof · 2012-07-22T20:03:08Z

@cryptocompress why do you mean by generic get/set ?

cryptocompress · 2012-07-22T20:06:15Z

"the goal of the property path is to map to getters and setters"
this getters and setters are not implemented by user and so they are generic?

in the "article.tags" example i may want to normalize tags in setter. so i have to register my normalizeTag function somehow. otherwise only a "generic" setter is called with value and optional path as arguments.

i dont understand the whole complexity. my only point is: do not map strings to something else. let them be strings.

stof · 2012-07-22T21:07:13Z

@cryptocompress the complexity is that the property path is not only about the property of the object (which would be easier) but about object graphs (you can access a property of a nested object)

stof · 2012-07-22T21:08:32Z

and getters and setters are a generic convention whereas a get and set methods accepting a method name are not.

schmittjoh · 2012-07-22T21:13:13Z

Do we have a syntax for accessing by reflection already?

schmittjoh · 2012-07-22T21:16:15Z

I prefer the inline syntax btw. Maybe path also needs to become an object.

cryptocompress · 2012-07-22T21:18:35Z

@stof "methods accepting a method name" would be the other way around. a method converted to string. let methods be methods and strings strings.

stof · 2012-07-22T21:19:21Z

@cryptocompress the property path is about configuring which method should be called to access the values

cryptocompress · 2012-07-22T21:26:12Z

then some sort of xpath would be powerful: Alternative 2

can't see singular/plural problem though... take as is: authors -> getAuthors

mvrhov · 2012-07-23T05:02:36Z

am getAuthors is plural, so I'd expect that method to return some kind of collection either in form of an object or a plain array.

webmozart · 2012-07-23T06:22:05Z

There seems to be some confusion about what PropertyPath is about. That class helps to read values from and write values into object graphs, just like XPath does with XML. Reading is not a problem at all. For example, the path article.author.phoneNumbers[2].countryCode will lead to the following series of method calls:

getArticle()->getAuthor()->getPhoneNumbers()[2]->getCountryCode()

PropertyPath is a bit more flexible than that, as it also supports public properties, magic getters etc.

The above mentioned problems arise when we write into paths. Currently, when writing into the above path, PropertyPath does the following sequence of calls:

getArticle()->getAuthor()->getPhoneNumbers()[2]->setCountryCode(...)

This is usually sufficient, but some people also want setter methods for the ancestors in the path to be called, e.g.

addPhoneNumber(...) // here we have the singular vs. plural problem
setAuthor(...)
setArticle(...)

Baachi · 2012-07-23T06:32:50Z

I prefer the Alternative 1.
I think it's the best solution for this problem, because it's easy and simple.

Alternative 2 is to overhead.

Tobion · 2012-07-23T06:33:10Z

I think solution 2 is more readable. It has the drawback of beeing harder to create programmatically (which is rarely needed I guess). For such cases, the PropertyPathBuilder should be used I guess.
Btw, conflicting options like 'by_reference' => true, 'use_adders' => true should probably raise an exception.

Sorien · 2012-07-23T08:13:54Z

alternative #2 is more readable with few options but when number of options will raise it will become one big mess so i would prefer alternative 1.

webmozart · 2012-07-23T08:21:58Z

@Sorien: FYI I don't expect these options to be necessary in many occasions. I also don't expect the number of options to raise, as PropertyPath needs to remain as small and simple as possible.

fabpot · 2012-07-23T14:48:34Z

@bschussek Can you list the current (and probably the only) settings we will have? So that we can discuss the grammar if option 1 is chosen.

Tobion · 2012-07-23T15:17:32Z

Options 1 has the disadvantage of beeing redundant, i.e. you basically need to repeat the property path in the options array again (if you need it). And the property path is just some plain text, readable representation. Why not making use of that for the options, too? Esp. since they are highly correlated.

webmozart · 2012-07-23T16:00:47Z

@fabpot The three settings included in the above proposal are all that we have:

disable by-reference handling for objects, use setter (for properties) and ArrayAccess (for indices) instead
disable adder/remover method even if it exists and use ArrayAccess for collection modifications
configure the singular used for determining the adder/remover method names

jaugustin · 2012-07-25T21:26:19Z

@bschussek if we add new options is it possible to have one new added:

force getter/setter adder/remover call even if the propertyPath can't figure out that it's valid when there is a __call method [PropertyAccess] PropertyPath read/write property with __call #4683

jaugustin · 2012-07-25T21:37:58Z

+1 for alternative 1 because you could add future options without rewriting the syntax.

Who want this ?
article{!option1,option3=foo,!option4}.tags{option2=barTag,!option3}...

Imagine the face of a dev looking at a form with this propertyPath for the first time.
I think alternative 2 is not user friendly ;)

webmozart · 2012-07-26T05:43:55Z

@jaugustin Hopefully this example will never become reality. I would like to keep the options few and for edge cases only.

Another limitation of solution 1 just came to mind. In the Form component, the property path is guessed based on the context if it is not explicitely given. I.e.

$builder->add('name', 'text', array(
    // implicit 'property_path' => 'name',
    // or       'property_path' => '[name]',
));

Suppose he configures this path with solution 1:

$builder->add('name', 'text', array(
    'property_path_options' => array(
        'name' => array(
            'by_reference' => false,
        ),
    ),
));

If the context changes (i.e. the system does not guess name anymore but [name]) the options are not correct anymore. The only solution here is to check the option keys against the real path, which costs performance, or to force the developer to explicitly configure the property path:

$builder->add('name', 'text', array(
    'property_path' => 'name',
    'property_path_options' => array(
        'name' => array(
            'by_reference' => false,
        ),
    ),
));

I don't think this is more understandable than

$builder->add('name', 'text', array(
    'property_path' => 'name{!byref}'
));

jaugustin · 2012-07-26T08:06:59Z

@bschussek In the second case you have to specify the property path, maybe with the solution 1 we could also force the user to set the property path if he set a property path options.

webmozart · 2012-07-26T08:44:53Z

That's what I was saying with

or to force the developer to explicitly configure the property path

Do you prefer that version in terms of readabilitly?

jaugustin · 2012-07-26T10:22:57Z

I prefer the array of options for readability, and for the kill the magic philosophy of sf 2 ;)

Dattaya · 2012-07-31T08:59:41Z

or as @Tobion said, PropertyPathBuilder could be used, for example:

<?php

use PropertyPathBuilder as Builder;

(new Builder)
    ->appendProperty('artikel', Builder::BY_VALUE) // second parameter - array or int
    ->appendProperty('stichwörter', Builder::USE_ADDERS, 'stichwort');

webmozart · 2012-07-31T14:52:43Z

@Dattaya PropertyPathBuilder is just a helper for constructing property paths. Everything added by this helper should be contained in the string representation of the path. Thus we end up with a string containing the options anyway.

We could also store the options internally in PropertyPath, but then one has to use PropertyPathBuilder to build property paths with options, which I don't like.

benglass · 2014-10-23T15:33:46Z

I see what you mean now, thanks for the clarification. It also makes it more clear why you would want to use annotations to define how this serialization would happen because it would be usable in multiple contexts.

I understand that if you were trying to use this serializer for multiple use cases (REST api and forms for example) that you would want to avoid duplicating the logic of how archived: true|false is transformed onto a model.

I'm concerned that this is not a good enough argument for disallowing people the flexibility of controlling how form data is bound via code for the following reasons

There are many people who will only be using forms and do not have a REST api
People with a rest API may be using one of the many existing serialization solutions and not intend to use this feature in the future
Allowing this new feature of a unified serializer for models to block giving users a fine grained way to control how the form synchronizes data to their model could result in this useful feature not getting added any time in the near future

Tobion · 2014-10-23T15:54:10Z

I agree with @bvleur that the enabled/disabler can simply be implemented in user code as he showed. So there is no urgent need to support that in core.

webmozart · 2014-10-23T15:58:36Z

@bvleur I agree!

@benglass' original request was to be able to map the archive()/unarchive() methods directly, without the indirection over setArchived(). The drawback of setArchived() is that you have to add a method just to make the domain model work with PropertyAccess instead of letting PropertyAccess adapt to whatever model you design.

But then again, does it matter? Probably not.

thundo · 2015-08-24T13:43:21Z

Adders and removers follow English singularization rules

Crashed on this one. A model property may be related to a language-specific domain. The singularization rules produce really funky results which totally break the readbility and coehrence of the model code.

Any help like symfony/symfony-docs#1227 to set custom or language-specific rules would be much appreciated.

lrlopez · 2015-09-13T10:29:43Z

@webmozart What is the status of this? I really like the idea of the model-specific configuration through annotations, XML, YAML or PHP code. This could fix a lot of issues at once (custom getter/setter methods, custom singularizing, by value/reference problems, array access, etc.) and would be a really nice feature to be added to Symfony 3.0.

The approach allows BC and is future-proof, as more options can be added later.

Could we agree to the syntax and move this forward?

lrlopez · 2015-09-20T21:27:58Z

@wouterj I'd like to move this forward as there is a lot potential in this approach. @webmozart seems to be missing (or just busy). So, is there anyone from the Symfony team willing to take the lead on this? How can I help?

webmozart · 2016-02-29T08:11:38Z

@lrlopez I don't have time to work on the implementation myself, but I'm happy to support you if you want to work on this!

See also #9336 for some thoughts about this issue.

lrlopez · 2016-03-04T21:45:50Z

Thank you! I'll give it a try, it's an excellent opportunity to dig into the Symfony internals. As I stated before, I think this feature can be rather useful to many people.

lrlopez · 2016-03-05T00:49:34Z

@webmozart Can you check PR #18016 when you have some spare time? I've implemented custom getters/setters/adders/removers through property annotations instead method ones. There is still much work to do (i.e. supporting YAML/XML/PHP metadata), but I think is a good starting point.

…rface and implementation on reflection (joelwurtz, Korbeil) This PR was merged into the 5.1-dev branch. Discussion ---------- [PropertyInfo] Add accessor and mutator extractor interface and implementation on reflection | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #30248, partially: #22190, #18016, #5013, #9336, #5219, | License | MIT | Doc PR | TODO This PR brings accessor / mutator extraction on the PropertyInfo component, There is no link to existing code, as IMO it should be in another PR as this will add a dependency on property access to the property info component and not sure this is something wanted (although, it will reduce a lot of code base on the property access component as a lot of code seems to be duplicated) Code is extracted from #30248 also there is some new features (that can be removed if not wanted) * Allow extracting private accessor / mutator (will do a new PR that improve private extraction on reflection latter) * Allow extracting static accessor / mutators * Allow extracting constructor mutators Current implementation try to be as close as the PropertyAccess implementation and i did not reuse some methods already available in the class as there is some differences in implementation, but maybe it will be a good time to make this consistent (Looking forward to your input) ? Things that should be done in a new PR: * Linking property info to property access to remove a lot of duplicate code * Add a new system that allow adding Virtual Property based on this extractor Commits ------- 0a92dab Rebase, fix tests, review & update CHANGELOG fc25086 [PropertyInfo] Add accessor and mutator extractor interface and implementation on reflection

carsonbot · 2021-02-18T13:01:09Z

Thank you for this issue.
There has not been a lot of activity here for a while. Has this been resolved?

carsonbot · 2021-03-04T13:04:33Z

Just a quick reminder to make a comment on this. If I don't hear anything I'll close this.

carsonbot · 2021-03-18T13:09:32Z

Hey,

I didn't hear anything so I'm going to close it. Feel free to comment if this is still relevant, I can always reopen!

webmozart mentioned this issue Oct 23, 2014

[Validator] Simplify XML and YAML syntax #7720

Closed

webda2l mentioned this issue Feb 18, 2015

[PropertyAccess] Add french singular for words like typologies > typologie #13721

Closed

stof mentioned this issue Apr 23, 2015

[StringUtil] Fixed singularification of 'selfies' #14452

Merged

wouterj mentioned this issue Sep 12, 2015

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

Closed

1 task

lrlopez mentioned this issue Mar 5, 2016

[PropertyAccess] Allow custom methods on property accesses #18016

Closed

4 tasks

stof mentioned this issue Feb 8, 2017

Collection adder/remover name - English dependency #21569

Closed

lrlopez mentioned this issue Mar 27, 2017

[PropertyAccess] Allow custom property accessors #22190

Closed

ro0NL mentioned this issue Apr 17, 2017

Collection element adder/remover naming in an entity #22454

Closed

lrlopez mentioned this issue Oct 9, 2017

[FrameworkBundle] Integrate custom property accessors into the framework #22191

Closed

joelwurtz mentioned this issue Mar 26, 2019

[PropertyInfo] Add accessor and mutator extractor interface and implementation on reflection #30704

Merged

javiereguiluz added the ⭐️ EU-FOSSA Hackathon label Apr 1, 2019

fabpot removed the ⭐️ EUFOSSA Hackathon label Aug 17, 2020

lrlopez mentioned this issue Oct 11, 2020

[PropertyAccess] [For reference only] Allow custom property accessors #38515

Closed

VincentLanglet mentioned this issue Oct 23, 2020

Improve the way we append parent object if any sonata-project/SonataAdminBundle#6171

Merged

carsonbot added the Stalled label Feb 18, 2021

carsonbot closed this as completed Mar 18, 2021

janopae mentioned this issue Nov 29, 2023

[PropertyAccess] Add a predictable convention for Adders and Removers #52809

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PropertyAccess] RFC: PropertyPath options #5013

[PropertyAccess] RFC: PropertyPath options #5013

[PropertyAccess] RFC: PropertyPath options #5013

[PropertyAccess] RFC: PropertyPath options #5013

Comments

1. Objects are treated by reference

2. Adders and removers are used if they exist

3. Adders and removers follow English singularization rules

Limitations

Alternative 1: Option arrays

Alternative 2: Path flags

Alternative 3: Your suggestions