Add accessor property instead of code

[VincentLanglet]

Subject

I am targeting this branch, because BC.

Closes #6831.
Closes #6741.

Changelog

### Added
- `FieldDescription` `accessor` option

### Deprecated
- `FieldDescription` `code` option

[franmomu]

would this work with a path? like

'accessor' => 'children.name'

[VincentLanglet]

would this work with a path? like

'accessor' => 'children.name'

Like it's implemented, no.
It handle a callable or the name of the callable. countChildren for instance.
But the code option does not support path.

We could add support for path, but:

• 'accessor' => 'countChildren' could lead to unexpected behavior if someone has both a method countChildren and getCountChildren (my example is stupid but you get the idea). Which one are you expecting first ?
• Why not passing ->add('children.name') directly ? Do you have a usecase in mind ?

Should I rename the option to accessor_method to make it clearer ?

[franmomu]

I understand the idea, but I think that's unusual, what I would expect is to behave the same way that the form component does, some examples that I think they should work:

->add('address.name');
->add('addressName', null, ['accessor' => 'address.name']);
->add('firstAddressName', null, ['accessor' => 'addresses[0].name']);
->add('thumbnail', null, ['accessor' => 'image[thumbnail]']);

[VincentLanglet]

I understand the idea, but I think that's unusual, what I would expect is to behave the same way that the form component does, some examples that I think they should work:

->add('address.name');
->add('addressName', null, ['accessor' => 'address.name']);
->add('firstAddressName', null, ['accessor' => 'addresses[0].name']);
->add('thumbnail', null, ['accessor' => 'image[thumbnail]']);

Should we allow

• either a method
• either a propertyPath
  And stop using the method name ? @sonata-project/contributors

[VincentLanglet]

@vladyslavstartsev I know you're using this property (or at least you tried).
What sounds natural to you ?

[vv12131415]

IMO
I think that method is more natural. Yes, it's still "magic", but it's still less "magic" than propertyPath

[vv12131415]

but, since this option will void SRP, maybe it will be better to do, 2 separate options

accessor_method and accessor_path

WDYT?

[VincentLanglet]

but, since this option will void SRP, maybe it will be better to do, 2 separate options

accessor_method and accessor_path

WDYT?

With two options we have to handle the case when both are set.

I don't think it's again SRP, for instance symfony allow both a property path or a method for the choice_value option

[VincentLanglet]

The only things to decide here is

Allowing Method + PropertyPath VS Allowing Method + Method name

[vv12131415]

my vote is for Method name

[VincentLanglet]

my vote is for Method name

Do you have examples in your code, where property path wouldn't work ?

[VincentLanglet]

my vote is for Method name

Do you have examples in your code, where property path wouldn't work ?

Since I don't have in mind (and this can still be done by passing the method), I allowed property path instead of method name @franmomu

[VincentLanglet]

Sorry @dmaicher I forgot to update the doc ^^'

[SonataCI]

Could you please rebase your PR and fix merge conflicts?

[franmomu]

Since code is always set in the persistence bundles, this is always triggered: https://github.com/sonata-project/SonataDoctrineORMAdminBundle/runs/1903176971?check_suite_focus=true#step:8:108

[VincentLanglet]

I think we should remove the code in the persistence bundle.

If the name is getfoo and the fieldName is bar, because we're passing the code, the value will be $object->getFoo() instead of relying on bar ; this seems to be a bug to me.

[simplegr]

👍

[toooni]

@VincentLanglet I've got an issue because of this change. On PHP8 this gives me an error date(): Argument #1 ($format) must be of type string, App\Entity\Invoice given. This is happening because of return $accessor($object); where 'date' is will be used as the function name.

    protected function configureListFields(ListMapper $listMapper): void
    {
        $listMapper->add('date', 'date', ['label' => 'Datum']);
    }

EDIT: This is happening because the name of the field is also a function in PHP (date in this case). (https://github.com/sonata-project/SonataAdminBundle/pull/6837/files#diff-d470daa9a101ba3f927eb0b89745013568722e7e3cc00dc0a340438b64b06d32R445)

[VincentLanglet]

@toooni This should have been fixed by https://github.com/sonata-project/SonataAdminBundle/pull/6859/files

Can you be more explicit ?
What is your entity ? Which getter is supposed to be called ?

[toooni]

@VincentLanglet Oh, sorry. I've updated the day before the fix.