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 admin template setters #6766

Merged
merged 2 commits into from
Jan 17, 2021

Conversation

wbloszyk
Copy link
Member

@wbloszyk wbloszyk commented Jan 11, 2021

Subject

This PR will revert some deprecations to allow override Admin templates in old way.
Also override templates by TemplateRegistry::setTemplates() should not replace but override it. People can call it in own code and break application working.

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

Changelog

### Added
- Added `Sonata\AdminBundle\Templating\MutableTemplateRegistryAwareInterface::setTemplate()`
- Added `Sonata\AdminBundle\Templating\MutableTemplateRegistryAwareInterface::setTemplates()`
### Removed
- Removed deprecations for `Sonata\AdminBundle\Admin\AbstractAdmin::setTemplate()`
- Removed deprecations for `Sonata\AdminBundle\Admin\AbstractAdmin::setTemplates()`

@wbloszyk wbloszyk marked this pull request as ready for review January 11, 2021 20:34
@wbloszyk
Copy link
Member Author

wbloszyk commented Jan 11, 2021

Mybe we can create MutableTemplateRegistry in AbstractAdmin::__constructor() and set templates by setTemplate() and setTemplates() instead creating new services? IMHO setTemplateRegistry() is unnesessery and add some confusing problems like how to set own templates, what is impossible for now.

In master (4.0) we dont use template registry services but take it directly from Admin.

$templateRegistryId = sprintf('%s.template_registry', $serviceId);
$templateRegistryDefinition = $container
->register($templateRegistryId, TemplateRegistry::class)
->addTag('sonata.admin.template_registry')
->setPublic(true); // Temporary fix until we can support service locators
if ($container->getParameter('sonata.admin.configuration.templates') !== $definedTemplates) {
$templateRegistryDefinition->addArgument($definedTemplates);
} else {
$templateRegistryDefinition->addArgument('%sonata.admin.configuration.templates%');
}
$definition->addMethodCall('setTemplateRegistry', [new Reference($templateRegistryId)]);

WDYT? @sonata-project/contributors

@wbloszyk wbloszyk requested a review from a team January 11, 2021 20:34
@VincentLanglet VincentLanglet requested a review from a team January 11, 2021 20:59
@VincentLanglet VincentLanglet added this to the 4.0 milestone Jan 11, 2021
VincentLanglet
VincentLanglet previously approved these changes Jan 11, 2021
@VincentLanglet VincentLanglet requested a review from a team January 11, 2021 22:39
tests/Templating/MutableTemplateRegistryTest.php Outdated Show resolved Hide resolved
src/Templating/MutableTemplateRegistry.php Outdated Show resolved Hide resolved
Update tests/Templating/MutableTemplateRegistryTest.php

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

Update src/Templating/MutableTemplateRegistry.php

Co-authored-by: Javier Spagnoletti <phansys@gmail.com>
@wbloszyk
Copy link
Member Author

@sonata-project/contributors @phansys Can we merge it?

phansys
phansys previously approved these changes Jan 15, 2021
src/Templating/MutableTemplateRegistry.php Outdated Show resolved Hide resolved
Co-authored-by: Javier Spagnoletti <phansys@gmail.com>
@VincentLanglet VincentLanglet merged commit c3bd1e0 into sonata-project:3.x Jan 17, 2021
@VincentLanglet
Copy link
Member

Thanks @wbloszyk

@wbloszyk wbloszyk deleted the fix_admin_template_setter branch January 17, 2021 12:13
@VincentLanglet
Copy link
Member

Mybe we can create MutableTemplateRegistry in AbstractAdmin::__constructor() and set templates by setTemplate() and setTemplates() instead creating new services? IMHO setTemplateRegistry() is unnesessery and add some confusing problems like how to set own templates, what is impossible for now.

In master (4.0) we dont use template registry services but take it directly from Admin.

$templateRegistryId = sprintf('%s.template_registry', $serviceId);
$templateRegistryDefinition = $container
->register($templateRegistryId, TemplateRegistry::class)
->addTag('sonata.admin.template_registry')
->setPublic(true); // Temporary fix until we can support service locators
if ($container->getParameter('sonata.admin.configuration.templates') !== $definedTemplates) {
$templateRegistryDefinition->addArgument($definedTemplates);
} else {
$templateRegistryDefinition->addArgument('%sonata.admin.configuration.templates%');
}
$definition->addMethodCall('setTemplateRegistry', [new Reference($templateRegistryId)]);

WDYT? @sonata-project/contributors

Not sure to understand everything and if it's related, but isn't it weird now to have a setTemplate method in both MutableTemplateRegistryAwareInterface and MutableTemplateRegistryInterface ?
I feel like the MutableTemplateRegistryInterface is useless now.

What would be the next @wbloszyk ?

@wbloszyk
Copy link
Member Author

Next step will be add MutableTemplateRegistryAwareInterface::setTemplatesByTemplateRegistry($registry, $override = true). Override false will be use to set global templates by compiler pass which is call after admin service configuration.

In 3.x template registry is created by compiler pass and can not be override(it is possibile but hard to do).

In 4.x template registry will be create by admin constructor. Override template registry will be not allowed but override templates by registry will be easy.

In 5.x I will try to present new templates registry system

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants