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

Fix TemplateRegistry usages #6387

Merged
merged 1 commit into from
Sep 15, 2020

Conversation

wbloszyk
Copy link
Member

@wbloszyk wbloszyk commented Sep 14, 2020

Subject

Based on #6383 (comment) I made some research and I found what can be improve.

  • service: admin_code // Admin object
  • service admin_code.template_registry // TemplateRegistry object dedicated for specify Admin

It mean We should be able to get TemplateRegistry object directly from admin, becouse we can get it from container.

I also remove some unnecessary NEXT_MAJOR

I am targeting this branch, because this change respect BC.

Changelog

### Added
- `Sonata\AdminBundle\Admin\AbstractAdmin::hasTemplateRegistry(): bool`
### Deprecated
- `Sonata\AdminBundle\Admin\AbstractAdmin::setTemplates()`
- `Sonata\AdminBundle\Admin\AbstractAdmin::setTemplate()`

core23
core23 previously requested changes Sep 14, 2020
src/Admin/AbstractAdmin.php Show resolved Hide resolved
src/Admin/AbstractAdmin.php Show resolved Hide resolved
@jordisala1991
Copy link
Member

This should probably be done on master to avoid BC breaks

@wbloszyk
Copy link
Member Author

@jordisala1991

  • change protected to public in getTemplateRegistry() is BC becouse it is final
  • throw LogicException is BC too becouse otherwise error You cannot call ... on null will be throw

I think I should revert NEXT_MAJOR. Someone can override getTemplate method.

jordisala1991
jordisala1991 previously approved these changes Sep 14, 2020
@wbloszyk
Copy link
Member Author

@core23 Can we merge it?

src/Admin/AbstractAdmin.php Outdated Show resolved Hide resolved
src/Admin/AbstractAdmin.php Outdated Show resolved Hide resolved
@wbloszyk
Copy link
Member Author

@phansys Sorry for another request for review. I click it before icon refresh. :)

phansys
phansys previously approved these changes Sep 14, 2020
@franmomu
Copy link
Member

If the plan is to call getTemplateRegistry from the CRUDController shouldn't be added to the AdminInterface?

src/Admin/AdminInterface.php Outdated Show resolved Hide resolved
{
return $query;
return $this->templateRegistry instanceof MutableTemplateRegistryInterface;
Copy link
Member

Choose a reason for hiding this comment

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

Is there any constraint requiring setTemplate() or setTemplates() to be defined? If not, I think we should use TemplateRegistryInterface instead of MutableTemplateRegistryInterface.

Copy link
Member Author

Choose a reason for hiding this comment

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

In 3.x branch:

    public function setTemplates(array $templates)
    {
        // NEXT_MAJOR: Remove this line
        $this->templates = $templates;

        $this->getTemplateRegistry()->setTemplates($templates);
    }

Copy link
Member

Choose a reason for hiding this comment

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

I understand that's the case for 3.x, but then we should add a NEXT_MAJOR comment explaining how to proceed in the next major release.
The changes at AdminInterface should refer to TemplateRegistryInterface, right?

Copy link
Member Author

@wbloszyk wbloszyk Sep 14, 2020

Choose a reason for hiding this comment

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

$admin->getTemplateRegistry() should be equal to admin_code.template_registry service. It is created on:

final class TemplateRegistry implements MutableTemplateRegistryInterface

So MutableTemplateRegistryInterface will be correct.

Copy link
Member

Choose a reason for hiding this comment

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

IMO we should check against 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.

Checking will be add in master. We can't throw exeption becouse someone compare getTemplateRegistry() to null. What we can do in 3.x is only add custom description for $this->getTemplateRegistry()->setTemplates($templates); exception.

Copy link
Member

Choose a reason for hiding this comment

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

I mean: return $this->... !== 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.

done

Copy link
Member

Choose a reason for hiding this comment

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

I think @phansys is right, there is no need to return MutableTemplateRegistryInterface if we are not using setTemplates or setTemplate.

Copy link
Member Author

Choose a reason for hiding this comment

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

Update src/Admin/AbstractAdmin.php

Co-authored-by: Javier Spagnoletti <phansys@gmail.com>

Update src/Admin/AbstractAdmin.php

Co-authored-by: Javier Spagnoletti <phansys@gmail.com>

Update src/Admin/AbstractAdmin.php

Co-authored-by: Javier Spagnoletti <phansys@gmail.com>
* @method array configureActionButtons(string $action, ?object $object = null)
* @method string getSearchResultLink(object $object)
* @method void showMosaicButton(bool $isShown)
* @method bool isDefaultFilter(string $name) // NEXT_MAJOR: Remove this
Copy link
Member

Choose a reason for hiding this comment

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

Not related to this PR, but we can remove this line

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 many change on this PR. Lets do it in another PR. Mybe we should create "Clean up" PR? 😅

@wbloszyk wbloszyk mentioned this pull request Sep 15, 2020
@wbloszyk wbloszyk dismissed stale reviews from phansys and core23 September 15, 2020 07:44

@phansys sugestion is made and accepted by @jordisala1991.

@wbloszyk wbloszyk merged commit 00bf1d6 into sonata-project:3.x Sep 15, 2020
@wbloszyk wbloszyk deleted the fix_template_registry branch September 15, 2020 07:48
}

protected function configureQuery(ProxyQueryInterface $query): ProxyQueryInterface
final public function hasTemplateRegistry(): bool
Copy link
Member

Choose a reason for hiding this comment

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

Is this method used outside the AbstractAdmin? If so, should be added to the interface?

Copy link
Member Author

Choose a reason for hiding this comment

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

It is new method, so it is not used for now. IMO all setTemplateRegistry, getTemplateRegistry and hasTemplateRegistry should be add to seperate TemplateRegistryOwnerInterface. AdminInterface should inplemented it.

phansys added a commit to phansys/SonataAdminBundle that referenced this pull request Oct 24, 2020
…()` and `setTemplates()` in `AbstractAdmin`

PR sonata-project#6387 deprecates methods `AbstractAdmin::setTemplate()` and `AbstractAdmin::setTemplates()`,
but the documentation still recommends to use them.
These calls were updated in order to use the template registry services.
phansys added a commit to phansys/SonataAdminBundle that referenced this pull request Oct 24, 2020
…()` and `setTemplates()` in `AbstractAdmin`

PR sonata-project#6387 deprecates methods `AbstractAdmin::setTemplate()` and `AbstractAdmin::setTemplates()`,
but the documentation still recommends to use them.
These calls were updated in order to use the template registry services.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants