-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Update docs in order to avoid calling deprecated methods setTemplate()
and setTemplates()
in AbstractAdmin
#6538
Conversation
Documentation is not that different from code: it does deserve separate commits for grammar fixes and for other things. Also, it does deserve descriptive commit messages that explain what you are doing, not just which area of the docs you are working on. Typically, this should appear in your commit message:
|
setTemplate()
and setTemplates()
in AbstractAdmin
The commit and the PR description were updated. Please, let me know if it accomplish your expectations. |
is available through ``$this->getTemplateRegistry()`` within the ``Admin``. | ||
Using the service container the template registries can be accessed outside an ``Admin``. | ||
Use the ``Admin`` code + ``.template_registry`` as the service ID (i.e. | ||
"app.admin.post" uses the template registry "app.admin.post.template_registry"). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd use code markup here, but it's funny, that's the second time I have this discussion today, so don't hesitate to tell me if you feel strongly otherwise.
"app.admin.post" uses the template registry "app.admin.post.template_registry"). | |
``app.admin.post`` uses the template registry ``app.admin.post.template_registry``). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there is no established rule for the case where we need to refer to service names, parameter names, package names, etc. (since they are names, sometimes it feels natural to use double quotes and treat them a literal strings).
Personally, I try to be consistent with this, but in some cases it depends on the context, I think.
In the same reasoning, I try to use backticks when referencing code snippets (with partial or complete fragments, IE AdminInterface::id()
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI the discussion I linked resolved in using backticks (there was no debate, we needed both backticks and quotes). I tend to use backticks for everything. So to me you can write "admin class", you can also write Admin
class, but you can't write Admin class, because now you are using a technical name as it appears in the code, and names that appear in code may contain special characters that have meaning in markdown like underscores or stars, but also in normal language, for example dots. When you use backticks, a monospace font will be used, and the reader knows you are talking about something that will appear in the code, and not approximately referring to something using a high level term like "unit of work" or "repository interface".
…()` 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.
I am trying to test this change locally and I cannot make it work as I supposed. |
I busy today. I have to finish some of my commercial works. I will try to find some time for it tomorrow. IMO it is problem with template registry which is created by compiler pass and do not exist when config is loaded. |
I agree with that suspicion. |
My idea is create one, global admin template registry and force people to create own (by override default) if they want override templates. <service id="sonata.media.admin.media.template_registry">
<tag name="sonata.teplate_registry"/>
<call method="setTemplates">
...
</call>
</service>
<service id="sonata.media.admin.media">
<tag name="sonata.admiun"/>
<call method="setTemplateRegistry">
<argument type="service" id="sonata.media.admin.media.template_registry">
</call>
</service> Im feature will be nice to add multiple registry for one admin. Support for role, destiny (chooseable show as table or thumbnail etc.) |
I understand. But I also think that we shouldn't deprecate |
Agree with that, IMO we should undeprecate this until a fully working solution can be provided, and only if it is better than simply calling setTemplate |
I will try do it tomorrow. |
Miss click 😆 |
@phansys At second: At third: |
Could you please share an example about the required DI configuration to customize an admin template? IE:
|
Compiler pass will auto inject BEFORE: admin.foo:
class: App\FooAdmin
arguments: [null, App\Entity\Foo, null]
calls:
- [setTemplate, ['edit', 'admin/foo/edit.html.twig']] AFTER: admin.foo.template_registry:
class: Sonata\AdminBundle\Templating\MutableTemplatingRegistry # IMO this can be set by tag, then class will be optional
tags:
- [name: "sonata.template_registry"]
calls:
- [setTemplate, ['edit', 'admin/foo/edit.html.twig']] |
Currently, the custom definition of |
After reading all this discussion, I just feel like this feature is not ready and was maybe not discuss enough. This PR:
IMHO, we should just revert it and delay these changes for 5.0 if it's not ready and fully working. Plus now I (maybe) start to understand it, I don't see the interest in this feature.
to
does not seem a good thing to me. Setting a templating is actually so easy that it doesn't need a service for this.
Can just be done this way
and I find this simpler. Personally when I override show/edit template, it's alway for one single Admin. Asking me to create a service everytime I need to override a template would be counter productive. Did I missunderstood something @wbloszyk @phansys ? Maybe both way should co-exist:
|
I can confirm it isn't working. You could check locally by introspecting the compiled debug container in your environment (
I understand exactly the same. Please, note that I've created this PR by guessing what was done at #6387, but I'm not have certainty about the required steps to replace the deprecated feature. |
#6556 should in 3.x. Other step in master or in 4.x. IMO keep this methods is bad decision. Templates should be manage by templates registry. But we can keep calling setters by catching this args, inject it by argument and remove and remove calling in compiler pass. WDYT? |
Why should it be ?
I'm not sure to fully understand. Do you want to auto-generate a templateRegistry if we provide Can you provide a way to have both this config
and this config
working ? |
I just thought about a special case
With template registry, you need to create 3 services because admin.foo1 can't set multiple templateRegistry.
|
IMO keep
Agree. That why we should keep
I can provide something like this:
Update for @VincentLanglet |
@phansys I can fix in the evening if you agree with #6538 (comment) |
I agree that it could be a working solution, but I'm in doubt since the template registry approach was not working in any of the related PRs yet. |
Not sure if this hack will be natural for developer who won't understand why adding a
I agree. Best would be a
Seems great
Ok. |
Maybe we could transmit the "magic" concept by using |
🤔 I found the best solution: We can use additional |
Can you give an example of the config you have in mind @wbloszyk ? If it's on
|
Of course in 3.x |
Seems great. With this, I think we don't have to revert the deprecation @phansys. |
Agreed. |
I will try to do it today. Additional this will add the same functionality to |
Some update: #6566
Thursday is family day. I will finish it in Friday. |
Me seams that its too complex to programming. |
What about configure templates in php code? In <?php
declare(strict_types=1);
namespace App\Admin;
use App\Entity\FooBar;
class FooBarAdmin extends AbstractAdmin
{
public function __construct()
{
parent::__construct('foo-bar', FooBar::class);
}
public function configure(): void
{
$this->getTemplateRegistry()->setTemplates([
'list' => '@FooBar/list.html.twig',
]);
}
} |
You can do it with configure indeed. But it's not bad to allow user to use |
Its possible add my example to documentation? |
Whyt ? protected function configureTemplateRegistry(MutableTemplateRegistryInterface $registry): void
{
$registry->setTemplate('edit', '@FooBar/FooBarAdmin/edit.html.twig');
} |
Also found problem: When call |
There is no real need to discuss about this PR before #6566 |
Subject
Update docs about templates.
PR #6387 deprecates methods
AbstractAdmin::setTemplate()
andAbstractAdmin::setTemplates()
, but the documentation still recommends to use them.These calls were updated in order to use the template registry services.
I am targeting this branch, because these changes respect BC.
Related to #6387.
Changelog