Deprecate some AbstractAdmin methods #6885

VincentLanglet · 2021-02-21T22:25:45Z

Subject

I am targeting this branch, because BC

Changelog

### Deprecated
- `AbstractAdmin::buildForm()`
- `AbstractAdmin::buildDatagrid()`
- `AbstractAdmin::buildShow()`
- `AbstractAdmin::buildList()`
- `AbstractAdmin::canAccessObject()` in favor of `AbstractAdmin::hasAccess()`
- `AdminInterface::canAccessObject()`
- `AdminInterface::configureActionButtons()` and `AbstractAdmin::configureActionButtons()` will become protected

franmomu · 2021-02-22T23:29:45Z

src/Admin/AbstractAdmin.php

@@ -2690,7 +2716,9 @@ public function configureActionButtons($action, $object = null)

        if (\in_array($action, ['show', 'delete', 'acl', 'history'], true)
            && $this->hasRoute('edit')
-            && $this->canAccessObject('edit', $object)


Do we have to remove this one?

IMO looks better to use a method to check this rather than substitute this call with null !== $object && $this->hasAccess('x', $object) everywhere.

There is also this check inside the canAccessObject method: if (!$this->id($object)) { }, that I don't know if can be removed.

EDIT: Looking at the calls, I think we can even add in that method a check for $this->hasRoute('route') to make those if smaller.

There is a lot of methods in the AdminInterface. I don't think we should exposing two methods with almost the same behavior.

We're mixing the access notion in both the AccessRegistryInterface and the AdminInterface

Both can be called with an object (hasAccess('edit', $object) and canAccessObject('edit', $object)). How to know which one I should use ? If you look at the code, sometimes we were using one, sometimes the second one, this is really confusing.

Maybe deprecating this method is not the right thing to do.

Changing AccessRegistryInterface::hasAccess($action, $object) to AccessRegistryInterface::hasAccess($action) ? This way it's clear, if you're concerned about the action access, use hasAccess ; if you're concerned about the object, use canAccessObject.

Renaming the canAccessObject method ?

And we could also move the canAccessObject method to the AccessRegistryInterface.

I take a new look at the code.

Firstly, the check $this->id($object) is needed.

Secondly, when you look at the following code

{% if admin.canAccessObject('edit', object) and admin.hasRoute('edit') %} <li> <a class="sonata-action-element" href="{{ admin.generateObjectUrl('edit', object) }}"> <i class="fa fa-edit" aria-hidden="true"></i> {{ 'link_action_edit'|trans({}, 'SonataAdminBundle') }} </a> </li> {% endif %}

Nothing can guarantee that

the object is not null. User could have a different implementation of canAccessObject, and then generateObjectUrl will give a TypeError.

canAccessObject and generateObjectUrl are related. There are in different interfaces.

So I still think it's better to deprecate canAccessObject and using explicit checks.

To be honest I don't understand the reasoning behind removing a meaningful method name in favor of explicit checks:

if ($this->hasRoute('acl') && null !== $object && null !== $this->id($object) && $this->hasAccess('acl', $object) )

vs:

if ($this->canAccessObject('acl', object))

Every time there is a call to canAccessObject, there is also a hasRoute call, so I guess that method (don't know if canAccessObject or other) could also check that.

IMHO (for the sake of maintainability) it is better to have a method as a concept that the developer can understand when looking at it rather than several checks.

the object is not null. User could have a different implementation of canAccessObject, and then generateObjectUrl will give a TypeError.

I think this is different, in this case if there is a call to generateObjectUrl that needs an object, makes sense to check if that exists.

To be honest I don't understand the reasoning behind removing a meaningful method name in favor of explicit checks:

if ($this->hasRoute('acl') && null !== $object && null !== $this->id($object) && $this->hasAccess('acl', $object) )

vs:

if ($this->canAccessObject('acl', object))

You're not always calling $this->canAccessObject, you're often calling $admin->canAccessObject, so you're calling this in the AdminInterface, not the AbstractAdmin. You don't know how the developer will implement this method.

I should be allowed to have this implementation:

public function canAccessObject($action, $object) { return true; }

Does the SonataAdmin code still works with this implementation ? No. We rely on a specific implementation. So this method shouldn't be in the interface. It could be in a service, but since we're not using it a lot, using specific checks seems enough to me.

Every time there is a call to canAccessObject, there is also a hasRoute call, so I guess that method (don't know if canAccessObject or other) could also check that.
IMHO (for the sake of maintainability) it is better to have a method as a concept that the developer can understand when looking at it rather than several checks.

Still, If you do this, you'll rely on a specific implementation of canAccessObject which is against the purpose of an interface.
If a method should exist, it would be canGenerateObjectUrl and be in the UrlGeneratorInterface.

the object is not null. User could have a different implementation of canAccessObject, and then generateObjectUrl will give a TypeError.

I think this is different, in this case if there is a call to generateObjectUrl that needs an object, makes sense to check if that exists.

With the implementation I gave you, the twig template is giving a TypeError.

the object is not null. User could have a different implementation of canAccessObject, and then generateObjectUrl will give a TypeError.

I think this is different, in this case if there is a call to generateObjectUrl that needs an object, makes sense to check if that exists.

With the implementation I gave you, the twig template is giving a TypeError.

This is precisely what I meant with my previous comment, in case we need to check that object is not null (because there is call that needs that object), make sense to add that extra check (admin.whatever and object != null).

I misunderstood that part. But the rest of my message is still valid.
We're relying on the implementation of canAccessObject when it's not even in the same interface.

For a list, we're doing hasRoute and hasAccess.
If for a show, you're doing hasRoute and canAccessObject or worst, just doing canAccessObject it won't be consistent with the list, it could lead to weird behavior if I override the canAccessObject method.

That's why I think it's better to keep the checks we need instead of hiding them inside a method which is in the public API. Given that fact, only the $this->id($object) !== null is useful to keep inside this method. Does it worth a method ? I'm not sure about this.

If you look more at it, you can even see that the $this->id($object) !== null check is wrong. Since generateObjectUrl is relying on getUrlSafeIdentifier.

If we want to keep abstraction, the only method to provide would be

public function canGenerateObjectUrl(object $object) { return nul !== $this->getUrlSafeIdentifier($object); }

Friendly ping @franmomu @sonata-project/contributors WDYT ?

src/Admin/AbstractAdmin.php

core23 · 2021-02-27T10:07:09Z

Are there any replacements for this methods?

VincentLanglet · 2021-02-27T10:12:31Z

Are there any replacements for this methods?

build* methods are going to be private. They are not meant to be called by the developer.
AbstractAdmin::configureActionButtons is going to be protected, so it's removed from the interface. There is already getActionButtons.

core23 · 2021-02-27T10:42:55Z

Are there any replacements for this methods?

build* methods are going to be private. They are not meant to be called by the developer.
AbstractAdmin::configureActionButtons is going to be protected, so it's removed from the interface. There is already getActionButtons.

This information should be added to the CHANGELOG.

VincentLanglet · 2021-02-28T11:55:11Z

The changelog should be better now.

SonataCI · 2021-03-10T22:14:35Z

Could you please rebase your PR and fix merge conflicts?

dmaicher

LGTM.

I also had a look and from what I can tell it does not impact any of my projects where I'm using SonataAdminBundle.

dmaicher · 2021-03-12T16:04:05Z

@franmomu @core23 could you also have another look? 😊

hasAccess can be used instead

phansys

GitHub is responding with 502.
At src/Admin/AbstractAdmin.php:2914:

                .' and will be removed an in 4.0. Use `hasAccess()` instead.',

phansys · 2021-03-12T18:27:52Z

src/Admin/AbstractAdmin.php

@@ -3179,10 +3243,18 @@ protected function buildList()
    }

    /**
-     * Build the form FieldDescription collection.
+     * NEXT_MAJOR: Change visibility to private.


Shouldn't we add a @deprecated annotation, just in case the users are overriding this method without calling parent?

What about buildShow()?

src/Admin/AbstractAdmin.php

phansys

IMO, the chengelog must explicitly tell which deprecated methods will reduce its visibility.

VincentLanglet · 2021-03-12T20:43:32Z

IMO, the chengelog must explicitly tell which deprecated methods will reduce its visibility.

If it's private, it's the same no ?

phansys · 2021-03-12T21:33:18Z

If it's private, it's the same no ?

Yes, I mean the cases where we are deprecating a method but not for removal, but for reduced visibility.

VincentLanglet · 2021-03-12T21:34:10Z

I did:

AdminInterface::configureActionButtons() and AbstractAdmin::configureActionButtons() will become protected

phansys · 2021-03-12T21:41:27Z

These methods will reduce its visibility from public/protected to private, but the changelog just states that they are deprecated:

AbstractAdmin::buildDatagrid()
AbstractAdmin::buildShow()
AbstractAdmin::buildList()
AbstractAdmin::buildForm()

That's what I mean.

VincentLanglet · 2021-03-12T22:00:28Z

These methods will reduce its visibility from public/protected to private, but the changelog just states that they are deprecated:

AbstractAdmin::buildDatagrid()

AbstractAdmin::buildShow()

AbstractAdmin::buildList()

AbstractAdmin::buildForm()

That's what I mean.

But since private and removed have the same impact for the developer, I don't think we should be more precise.
The developer won't be able to use it, that's all. He shouldn't know about our implementation.
I could rename buildDatagrid() to a private method with another name and stop using a private method.

phansys · 2021-03-12T22:10:41Z

He shouldn't know about our implementation.

I agree with this fact. Dismiss my suggestion.

VincentLanglet force-pushed the work branch 2 times, most recently from d4bf913 to df65be8 Compare February 22, 2021 08:41

VincentLanglet marked this pull request as ready for review February 22, 2021 08:48

VincentLanglet requested a review from a team February 22, 2021 08:48

VincentLanglet added this to the 4.0 milestone Feb 22, 2021

phansys previously approved these changes Feb 22, 2021

View reviewed changes

VincentLanglet requested a review from a team February 22, 2021 21:25

franmomu added the minor label Feb 22, 2021

franmomu reviewed Feb 22, 2021

View reviewed changes

VincentLanglet marked this pull request as draft February 23, 2021 01:43

core23 reviewed Feb 23, 2021

View reviewed changes

src/Admin/AbstractAdmin.php Show resolved Hide resolved

phansys marked this pull request as ready for review February 23, 2021 14:48

phansys marked this pull request as draft February 23, 2021 14:48

VincentLanglet dismissed phansys’s stale review via 65e6f93 February 23, 2021 19:32

VincentLanglet marked this pull request as ready for review February 23, 2021 19:57

VincentLanglet requested review from franmomu, phansys and a team February 23, 2021 19:57

VincentLanglet requested review from core23, wbloszyk, dmaicher, greg0ire and OskarStark February 28, 2021 11:55

OskarStark previously approved these changes Mar 7, 2021

View reviewed changes

SonataCI added the pending author label Mar 10, 2021

dmaicher previously approved these changes Mar 12, 2021

View reviewed changes

VincentLanglet added 5 commits March 12, 2021 17:11

Deprecate calling build* methods

aac9099

Deprecate canAccessObject

a28aab1

hasAccess can be used instead

Remove configureActionButtons from public API

21a75ed

Add update note

762ccdd

Add id check

340fa08

VincentLanglet dismissed stale reviews from dmaicher and OskarStark via 340fa08 March 12, 2021 16:12

VincentLanglet force-pushed the work branch from 65e6f93 to 340fa08 Compare March 12, 2021 16:12

SonataCI removed the pending author label Mar 12, 2021

dmaicher previously approved these changes Mar 12, 2021

View reviewed changes

dmaicher requested a review from OskarStark March 12, 2021 16:25

phansys requested changes Mar 12, 2021

View reviewed changes

phansys reviewed Mar 12, 2021

View reviewed changes

src/Admin/AbstractAdmin.php Outdated Show resolved Hide resolved

Review

c533ae0

VincentLanglet dismissed dmaicher’s stale review via c533ae0 March 12, 2021 20:01

dmaicher approved these changes Mar 12, 2021

View reviewed changes

phansys approved these changes Mar 12, 2021

View reviewed changes

VincentLanglet merged commit 733dc72 into sonata-project:3.x Mar 13, 2021

VincentLanglet deleted the work branch March 13, 2021 10:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate some AbstractAdmin methods #6885

Deprecate some AbstractAdmin methods #6885

VincentLanglet commented Feb 21, 2021 •

edited

franmomu Feb 22, 2021 •

edited

VincentLanglet Feb 23, 2021

VincentLanglet Feb 23, 2021

franmomu Feb 23, 2021

VincentLanglet Feb 23, 2021

franmomu Feb 23, 2021

VincentLanglet Feb 23, 2021

VincentLanglet Feb 26, 2021 •

edited

core23 commented Feb 27, 2021

VincentLanglet commented Feb 27, 2021

core23 commented Feb 27, 2021

VincentLanglet commented Feb 28, 2021

SonataCI commented Mar 10, 2021

dmaicher left a comment

dmaicher commented Mar 12, 2021

phansys left a comment

phansys Mar 12, 2021

VincentLanglet Mar 12, 2021

phansys Mar 12, 2021

phansys left a comment

VincentLanglet commented Mar 12, 2021

phansys commented Mar 12, 2021

VincentLanglet commented Mar 12, 2021

phansys commented Mar 12, 2021 •

edited

VincentLanglet commented Mar 12, 2021

phansys commented Mar 12, 2021

Deprecate some AbstractAdmin methods #6885

Deprecate some AbstractAdmin methods #6885

Conversation

VincentLanglet commented Feb 21, 2021 • edited

Subject

Changelog

franmomu Feb 22, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VincentLanglet Feb 26, 2021 • edited

Choose a reason for hiding this comment

core23 commented Feb 27, 2021

VincentLanglet commented Feb 27, 2021

core23 commented Feb 27, 2021

VincentLanglet commented Feb 28, 2021

SonataCI commented Mar 10, 2021

dmaicher left a comment

Choose a reason for hiding this comment

dmaicher commented Mar 12, 2021

phansys left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phansys left a comment

Choose a reason for hiding this comment

VincentLanglet commented Mar 12, 2021

phansys commented Mar 12, 2021

VincentLanglet commented Mar 12, 2021

phansys commented Mar 12, 2021 • edited

VincentLanglet commented Mar 12, 2021

phansys commented Mar 12, 2021

VincentLanglet commented Feb 21, 2021 •

edited

franmomu Feb 22, 2021 •

edited

VincentLanglet Feb 26, 2021 •

edited

phansys commented Mar 12, 2021 •

edited