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.

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

Deprecate some AbstractAdmin methods #6885

Merged
merged 6 commits into from
Mar 13, 2021
Merged

Deprecate some AbstractAdmin methods #6885

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
aac9099
Deprecate calling build* methods
VincentLanglet Feb 21, 2021
a28aab1
Deprecate canAccessObject
VincentLanglet Feb 21, 2021
21a75ed
Remove configureActionButtons from public API
VincentLanglet Feb 21, 2021
762ccdd
Add update note
VincentLanglet Feb 22, 2021
340fa08
Add id check
VincentLanglet Feb 23, 2021
c533ae0
Review
VincentLanglet Mar 12, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions UPGRADE-3.x.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ UPGRADE 3.x
UPGRADE FROM 3.x to 3.x
=======================

### Deprecated `Sonata\AdminBundle\Model\AdminInterface::canAccessObject()` method.

Use `Sonata\AdminBundle\Admin\AdminInterface::hasAccess()` instead.

### `Sonata\AdminBundle\Controller\CRUDController::historyCompareRevisionsAction()`

- Deprecated route parameter "base_revision" in favor of "baseRevision";
Expand Down
124 changes: 100 additions & 24 deletions src/Admin/AbstractAdmin.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -741,10 +741,18 @@ public function getFilterParameters()
}

/**
* NEXT_MAJOR: Change the visibility to protected (similar to buildShow, buildForm, ...).
* NEXT_MAJOR: Change the visibility to private.
*/
public function buildDatagrid()
VincentLanglet marked this conversation as resolved.
Show resolved Hide resolved
{
if ('sonata_deprecation_mute' !== (\func_get_args()[0] ?? null)) {
@trigger_error(sprintf(
'The %s() method is deprecated since sonata-project/admin-bundle 3.x'
.' and will become private in version 4.0.',
__METHOD__
), \E_USER_DEPRECATED);
}

if ($this->loaded['datagrid']) {
return;
}
Expand Down Expand Up @@ -1367,14 +1375,16 @@ public function getObject($id)

public function getForm()
{
$this->buildForm();
// NEXT_MAJOR: Remove the `'sonata_deprecation_mute'` param
$this->buildForm('sonata_deprecation_mute');

return $this->form;
}

public function getList()
{
$this->buildList();
// NEXT_MAJOR: Remove the `'sonata_deprecation_mute'` param
$this->buildList('sonata_deprecation_mute');

return $this->list;
}
Expand Down Expand Up @@ -1403,7 +1413,8 @@ public function createQuery($context = 'list')

public function getDatagrid()
{
$this->buildDatagrid();
// NEXT_MAJOR: Remove the `'sonata_deprecation_mute'` param
$this->buildDatagrid('sonata_deprecation_mute');

return $this->datagrid;
}
Expand Down Expand Up @@ -1741,14 +1752,16 @@ public function hasSubject()

public function getFormFieldDescriptions()
{
$this->buildForm();
// NEXT_MAJOR: Remove the `'sonata_deprecation_mute'` param
$this->buildForm('sonata_deprecation_mute');

return $this->formFieldDescriptions;
}

public function getFormFieldDescription($name)
{
$this->buildForm();
// NEXT_MAJOR: Remove the `'sonata_deprecation_mute'` param
$this->buildForm('sonata_deprecation_mute');

if (!$this->hasFormFieldDescription($name)) {
@trigger_error(sprintf(
Expand Down Expand Up @@ -1780,7 +1793,8 @@ public function getFormFieldDescription($name)
*/
public function hasFormFieldDescription($name)
{
$this->buildForm();
// NEXT_MAJOR: Remove the `'sonata_deprecation_mute'` param
$this->buildForm('sonata_deprecation_mute');

return \array_key_exists($name, $this->formFieldDescriptions);
}
Expand All @@ -1807,7 +1821,8 @@ public function removeFormFieldDescription($name)
*/
public function getShowFieldDescriptions()
{
$this->buildShow();
// NEXT_MAJOR: Remove the `'sonata_deprecation_mute'` param
$this->buildShow('sonata_deprecation_mute');

return $this->showFieldDescriptions;
}
Expand All @@ -1821,7 +1836,8 @@ public function getShowFieldDescriptions()
*/
public function getShowFieldDescription($name)
{
$this->buildShow();
// NEXT_MAJOR: Remove the `'sonata_deprecation_mute'` param
$this->buildShow('sonata_deprecation_mute');

if (!$this->hasShowFieldDescription($name)) {
@trigger_error(sprintf(
Expand All @@ -1846,7 +1862,8 @@ public function getShowFieldDescription($name)

public function hasShowFieldDescription($name)
{
$this->buildShow();
// NEXT_MAJOR: Remove the `'sonata_deprecation_mute'` param
$this->buildShow('sonata_deprecation_mute');

return \array_key_exists($name, $this->showFieldDescriptions);
}
Expand All @@ -1863,14 +1880,16 @@ public function removeShowFieldDescription($name)

public function getListFieldDescriptions()
{
$this->buildList();
// NEXT_MAJOR: Remove the `'sonata_deprecation_mute'` param
$this->buildList('sonata_deprecation_mute');

return $this->listFieldDescriptions;
}

public function getListFieldDescription($name)
{
$this->buildList();
// NEXT_MAJOR: Remove the `'sonata_deprecation_mute'` param
$this->buildList('sonata_deprecation_mute');

if (!$this->hasListFieldDescription($name)) {
@trigger_error(sprintf(
Expand All @@ -1896,7 +1915,8 @@ public function getListFieldDescription($name)

public function hasListFieldDescription($name)
{
$this->buildList();
// NEXT_MAJOR: Remove the `'sonata_deprecation_mute'` param
$this->buildList('sonata_deprecation_mute');

return \array_key_exists($name, $this->listFieldDescriptions);
}
Expand All @@ -1913,7 +1933,8 @@ public function removeListFieldDescription($name)

public function getFilterFieldDescription($name)
{
$this->buildDatagrid();
// NEXT_MAJOR: Remove the `'sonata_deprecation_mute'` param
$this->buildDatagrid('sonata_deprecation_mute');

if (!$this->hasFilterFieldDescription($name)) {
@trigger_error(sprintf(
Expand All @@ -1938,7 +1959,8 @@ public function getFilterFieldDescription($name)

public function hasFilterFieldDescription($name)
{
$this->buildDatagrid();
// NEXT_MAJOR: Remove the `'sonata_deprecation_mute'` param
$this->buildDatagrid('sonata_deprecation_mute');

return \array_key_exists($name, $this->filterFieldDescriptions);
}
Expand All @@ -1955,7 +1977,8 @@ public function removeFilterFieldDescription($name)

public function getFilterFieldDescriptions()
{
$this->buildDatagrid();
// NEXT_MAJOR: Remove the `'sonata_deprecation_mute'` param
$this->buildDatagrid('sonata_deprecation_mute');

return $this->filterFieldDescriptions;
}
Expand Down Expand Up @@ -2435,6 +2458,8 @@ public function getObjectIdentifier()
}

/**
* NEXT_MAJOR: Change visibility to protected.
*
* Return the list of permissions the user should have in order to display the admin.
*
* @param string $context
Expand Down Expand Up @@ -2485,7 +2510,8 @@ public function id($model)

public function getShow()
{
$this->buildShow();
// NEXT_MAJOR: Remove the `'sonata_deprecation_mute'` param
$this->buildShow('sonata_deprecation_mute');

return $this->show;
}
Expand Down Expand Up @@ -2701,7 +2727,10 @@ public function configureActionButtons($action, $object = null)

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

@franmomu franmomu Feb 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

  • 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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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).

Copy link
Member Author

Choose a reason for hiding this comment

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

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);
}

Copy link
Member Author

@VincentLanglet VincentLanglet Feb 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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

&& null !== $object
&& null !== $this->id($object)
// NEXT_MAJOR: Replace by `$this->hasAccess`
&& $this->canAccessObject('edit', $object, 'sonata_deprecation_mute')
) {
$list['edit'] = [
// NEXT_MAJOR: Remove this line and use commented line below it instead
Expand All @@ -2712,7 +2741,10 @@ public function configureActionButtons($action, $object = null)

if (\in_array($action, ['show', 'edit', 'acl'], true)
&& $this->hasRoute('history')
&& $this->canAccessObject('history', $object)
&& null !== $object
&& null !== $this->id($object)
// NEXT_MAJOR: Replace by `$this->hasAccess`
&& $this->canAccessObject('history', $object, 'sonata_deprecation_mute')
) {
$list['history'] = [
// NEXT_MAJOR: Remove this line and use commented line below it instead
Expand All @@ -2724,7 +2756,10 @@ public function configureActionButtons($action, $object = null)
if (\in_array($action, ['edit', 'history'], true)
&& $this->isAclEnabled()
&& $this->hasRoute('acl')
&& $this->canAccessObject('acl', $object)
&& null !== $object
&& null !== $this->id($object)
// NEXT_MAJOR: Replace by `$this->hasAccess`
&& $this->canAccessObject('acl', $object, 'sonata_deprecation_mute')
) {
$list['acl'] = [
// NEXT_MAJOR: Remove this line and use commented line below it instead
Expand All @@ -2735,7 +2770,10 @@ public function configureActionButtons($action, $object = null)

if (\in_array($action, ['edit', 'history', 'acl'], true)
&& $this->hasRoute('show')
&& $this->canAccessObject('show', $object)
&& null !== $object
&& null !== $this->id($object)
// NEXT_MAJOR: Replace by `$this->hasAccess`
&& $this->canAccessObject('show', $object, 'sonata_deprecation_mute')
&& \count($this->getShow()) > 0
) {
$list['show'] = [
Expand Down Expand Up @@ -2857,6 +2895,8 @@ final public function isDefaultFilter($name)
}

/**
* NEXT_MAJOR: Remove this method.
*
* Check object existence and access, without throw Exception.
*
* @param string $action
Expand All @@ -2868,6 +2908,14 @@ final public function isDefaultFilter($name)
*/
public function canAccessObject($action, $object)
{
if ('sonata_deprecation_mute' !== (\func_get_args()[2] ?? null)) {
@trigger_error(sprintf(
'The method %s() is deprecated since sonata-project/admin-bundle 3.x'
.' and will be removed an in 4.0. Use `hasAccess()` instead',
__METHOD__,
), \E_USER_DEPRECATED);
}

if (!\is_object($object)) {
return false;
}
Expand Down Expand Up @@ -3101,10 +3149,18 @@ protected function configureTabMenu(ItemInterface $menu, $action, ?AdminInterfac
}

/**
* build the view FieldDescription array.
* NEXT_MAJOR: Change the visibility to private.
*/
protected function buildShow()
{
if ('sonata_deprecation_mute' !== (\func_get_args()[0] ?? null)) {
@trigger_error(sprintf(
'The %s() method is deprecated since sonata-project/admin-bundle 3.x'
.' and will become private in version 4.0.',
__METHOD__
), \E_USER_DEPRECATED);
}

if ($this->loaded['show']) {
return;
}
Expand All @@ -3122,10 +3178,20 @@ protected function buildShow()
}

/**
* build the list FieldDescription array.
* NEXT_MAJOR: Change visibility to private.
*
* @deprecated since sonata-project/admin-bundle 3.x will be private in 4.0
*/
protected function buildList()
{
if ('sonata_deprecation_mute' !== (\func_get_args()[0] ?? null)) {
@trigger_error(sprintf(
'The %s() method is deprecated since sonata-project/admin-bundle 3.x'
.' and will become private in version 4.0.',
__METHOD__
), \E_USER_DEPRECATED);
}

if ($this->loaded['list']) {
return;
}
Expand Down Expand Up @@ -3179,10 +3245,20 @@ protected function buildList()
}

/**
* Build the form FieldDescription collection.
* NEXT_MAJOR: Change visibility to private.
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

Copy link
Member

Choose a reason for hiding this comment

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

What about buildShow()?

*
* @deprecated since sonata-project/admin-bundle 3.x will be private in 4.0
*/
protected function buildForm()
{
if ('sonata_deprecation_mute' !== (\func_get_args()[0] ?? null)) {
@trigger_error(sprintf(
'The %s() method is deprecated since sonata-project/admin-bundle 3.x'
.' and will become private in version 4.0.',
__METHOD__
), \E_USER_DEPRECATED);
}

if ($this->loaded['form']) {
return;
}
Expand Down
7 changes: 0 additions & 7 deletions src/Admin/AdminInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,9 @@
*
* NEXT_MAJOR: Add all these methods to the interface by uncommenting them.
*
* @method array configureActionButtons(string $action, ?object $object = null)
* @method string getSearchResultLink(object $object)
* @method array getDefaultFilterParameters()
* @method bool isCurrentRoute(string $name, ?string $adminCode)
* @method bool canAccessObject(string $action, object $object)
* @method mixed getPersistentParameter(string $name)
* @method string[] getExportFields()
* @method array getSubClasses()
Expand Down Expand Up @@ -732,11 +730,6 @@ public function setListMode($mode);
*/
public function getListMode();

/*
* Configure buttons for an action
*/
// public function configureActionButtons(string $action, ?object $object = null): array;

// NEXT_MAJOR: uncomment this method for 4.0
/*
* Returns the result link for an object.
Expand Down
9 changes: 8 additions & 1 deletion src/Resources/views/Button/acl_button.html.twig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,14 @@ For the full copyright and license information, please view the LICENSE
file that was distributed with this source code.

#}
{% if admin.isAclEnabled() and admin.canAccessObject('acl', object) and admin.hasRoute('acl') %}

{# NEXT_MAJOR: Replace canAccessObject by hasAccess #}
{% if admin.isAclEnabled()
and object is not null
and admin.id(object) is not null
and admin.canAccessObject('acl', object, 'sonata_deprecation_mute')
and admin.hasRoute('acl')
%}
<li>
<a class="sonata-action-element" href="{{ admin.generateObjectUrl('acl', object) }}">
<i class="fa fa-users" aria-hidden="true"></i>
Expand Down
7 changes: 6 additions & 1 deletion src/Resources/views/Button/edit_button.html.twig
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,12 @@ file that was distributed with this source code.

#}

{% if admin.canAccessObject('edit', object) and admin.hasRoute('edit') %}
{# NEXT_MAJOR: Replace canAccessObject by hasAccess #}
{% if object is not null
and admin.id(object) is not null
and admin.canAccessObject('edit', object, 'sonata_deprecation_mute')
and admin.hasRoute('edit')
%}
<li>
<a class="sonata-action-element" href="{{ admin.generateObjectUrl('edit', object) }}">
<i class="fa fa-edit" aria-hidden="true"></i>
Expand Down
Loading