Skip to content
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

[PropertyAccess] Allow different prefixes than "add" and "remove" #9336

Closed
webmozart opened this issue Oct 18, 2013 · 14 comments
Closed

[PropertyAccess] Allow different prefixes than "add" and "remove" #9336

webmozart opened this issue Oct 18, 2013 · 14 comments

Comments

@webmozart
Copy link
Contributor

Currently, adders and removers must be prefixed with "add" and "remove". When applying DDD, this sometimes doesn't make sense, for example:

class Contact
{
    public function addGroup(ContactGroup $group) { }
    public function removeGroup(ContactGroup $group) { }
}

Here it would make much more sense to prefix the methods with "join" and "leave":

class Contact
{
    public function joinGroup(ContactGroup $group) { }
    public function leaveGroup(ContactGroup $group) { }
}

The PropertyAccessor should provide a way to use these methods.

@webmozart
Copy link
Contributor Author

ref #5013

@stof
Copy link
Member

stof commented Oct 18, 2013

@bschussek it should actually allow defining the full name for adders and removers, not only the prefix. This will allow people to customize it in case they write their method names in a different language than English, for whch the inflection rules will fail to find the singular. See #9205 for the report

@dolbertz
Copy link

I would like to take on this ticket as my first active involvment with Symfony.

In which branch should I make this contribution?
And what about the best approach to this? Adding the name of the getter/setter as another parameter to PropertyAccessor::get/setValue() seems a bit harsh. Maybe an $options array Parameter? With 'callback' as key?

@dolbertz
Copy link

After looking further into this and into Ticket #5013 I don't think this is such an Easy Pick. Adding the ability to the current setValue/getValue through an $options array would lead to a lot of changes just so that PropertyAccessor::findAdderAndRemover() can use those settings.

That could call for a similar approach as the enableMagicCall(). Maybe something like enableCallbacks() which takes an array of callback names for add/remove/get/set.

I really would like to get some feedback on this.

@TomasVotruba
Copy link
Contributor

@webmozart Is this still relevant?

I've been using this component so this might be easy.

What's the final decision? I would be for custom callbacks. Because there might be loads off different prefixes like has/join/leave/remove/attach/add/get/set etc... (just from the top of my head).

Any relevant sources?

@TomasVotruba
Copy link
Contributor

Ping @webmozart

@webmozart webmozart removed the Good first issue Ideal for your first contribution! (some Symfony experience may be required) label Feb 29, 2016
@webmozart
Copy link
Contributor Author

@TomasVotruba Indeed this is not such an easy pick. This post in #9336 and the following posts explain how this could be solved in a nice manner.

The difficulty is that, if we use annotations to configure property paths, we also have to provide XML and YAML mappings and their respective loaders, as we do in other places. That's quite a bit of work and duplicating what we already have in other components. A solution for this problem (at least for XML) would be native support for XML-based annotations in Doctrine, and as far as I know (ping @Ocramius) this is WIP.

Second, we could refactor all the different metadata loaders into a separate component. There is a library that does that already, but it's not maintained anymore and I'm not sure whether it applies to our use cases.

The benefit is that the PropertyAccess-based serializer would become more powerful, as would the Form component.

@lrlopez
Copy link
Contributor

lrlopez commented Mar 5, 2016

@webmozart I've been studying how metadata gets loaded by other Symfony components (i.e. Validator, Routing, etc.) and you're right: there is a lot of overlapping. I'm about to try replicating the loader code from the Validator component while taking out any unneeded feature as a first approach.

Later we can figure out how to extract all metadata loaders their own independent component. The library you pointed out, jms/metadata could be used as a base (it seems to be rather stable and supports pluggable caching) so we'd just have to implement the annotation, YAML and XML drivers. Then we could refactor all the current code that need metadata info to use the new component.

@fabpot
Copy link
Member

fabpot commented Mar 6, 2016

Just a quick note: jms/metadata cannot be used due to licensing issues.

The loader from the Validator component is the one that we need to get rid of. All the other components are already using a standardized way.

@Ocramius
Copy link
Contributor

Ocramius commented Mar 6, 2016

and as far as I know (ping @Ocramius) this is WIP.

It's not something for ORM 2.x anyway. We are crunching on it, but far from getting anywhere soon, sorry.

@sstok
Copy link
Contributor

sstok commented Mar 6, 2016

https://github.com/rollerworks/metadata/ MIT licensed, documentation is missing currently.
You can see a working implementation in https://github.com/rollerworks/search-metadata

Feel free to reuse my work and move it Symfony 👍

@lrlopez
Copy link
Contributor

lrlopez commented Mar 6, 2016

Thanks @fabpot for the hint. I had already changed my mind and used the Serializer mapping as the implementation reference because I found its code much cleaner. Anyway, it hurt a lot copying so many LOCs, it just doesn't seem right.

@sstok, I'll have a look into it, I really appreciate your offer.

lyrixx added a commit that referenced this issue Jan 31, 2020
…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
@lyrixx
Copy link
Member

lyrixx commented Jan 31, 2020

@Korbeil #30704 has been merged. Does it help this issue?

@Korbeil
Copy link
Contributor

Korbeil commented Jan 31, 2020

@Korbeil #30704 has been merged. Does it help this issue?

Yes, now you can instantiate PropertyAccessor with custom $writeInfoExtractor and you can give him: new ReflectionExtractor(null, null, ['join', 'leave']); to use theses prefixes.

In services.yaml, you'll have to do:

CustomWriteInfoExtractor:
  class: Symfony\Component\PropertyAccess\PropertyAccessor
  arguments:
    $arrayMutatorPrefixes: ['join', 'leave']

Symfony\Component\PropertyAccess\PropertyAccessorInterface: 
  arguments:
    $writeInfoExtractor: '@CustomWriteInfoExtractor'

wouterj added a commit to symfony/symfony-docs that referenced this issue Nov 5, 2020
…orbeil)

This PR was merged into the 5.1 branch.

Discussion
----------

[PropertyAccess] Non-standard adder/remover methods

Will add documentation about non-standard adder/remover methods for PropertyAccessor.

Related to #13023 and symfony/symfony#9336
This is not a complete documentation for related issues, but this will introduce one of the possible new use.

Commits
-------

bf6cca9 Non-standard adder/remover methods
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests