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

[RFC] Use only the native template overriding mechanism of Symfony? #1749

Closed
javiereguiluz opened this issue Aug 9, 2017 · 5 comments
Closed
Labels

Comments

@javiereguiluz
Copy link
Collaborator

This is a proposal from @yceruto explained in this comment and copied below:


What about to use only the standard (native) overriding templates mechanism?

For example, installing a fresh EasyAdminBundle with the next configuration:

easy_admin:
    entities:
        - AppBundle\Entity\User

I want to customize the User list template just creating the path convention:

app/Resources/EasyAdminBundle/views/User/list.html.twig

Later with nothing more the bundle should be able to load@EasyAdmin/User/list.html.twig template for User/list action and so on.

To achieve it, the next template paths should be enough to cover all cases:

  • @EasyAdmin/<entityName>/<templateName>.html.twig (set only if exists else use default template)
  • @EasyAdmin/default/<templateName>.html.twig

Also, instead of hardcode the easy_admin directory (AFAIR workaround to well known issue symfony/symfony#17557) you can add a second Twig's namespace automatically for EasyAdminBundle/Resources/views e.g. '!EasyAdmin' as you're suggested.

Why?
Less paths to do the same => less issues, less options to document & remember, less steps, less code to maintain, bundle-less methods in favor of native methods.

WDYT? Thanks!

See patch (without BC layer):
diff --git a/Configuration/TemplateConfigPass.php b/Configuration/TemplateConfigPass.php
index e121416..0c00649 100644
--- a/Configuration/TemplateConfigPass.php
+++ b/Configuration/TemplateConfigPass.php
@@ -20,8 +20,7 @@ namespace JavierEguiluz\Bundle\EasyAdminBundle\Configuration;
  */
 class TemplateConfigPass implements ConfigPassInterface
 {
-    private $templatesDir;
-
+    private $templateLoader;
     private $defaultBackendTemplates = array(
         'layout' => '@EasyAdmin/default/layout.html.twig',
         'menu' => '@EasyAdmin/default/menu.html.twig',
@@ -63,13 +62,16 @@ class TemplateConfigPass implements ConfigPassInterface
         'label_undefined' => '@EasyAdmin/default/label_undefined.html.twig',
     );
 
-    public function __construct($templatesDir)
+    public function __construct(\Twig_Loader_Filesystem $templateLoader)
     {
-        $this->templatesDir = $templatesDir;
+        $this->templateLoader = $templateLoader;
     }
 
     public function process(array $backendConfig)
     {
+        // Workaround to allow override default templates partially
+        $this->templateLoader->addPath(__DIR__.'/../Resources/views', '!EasyAdmin');
+
         $backendConfig = $this->processEntityTemplates($backendConfig);
         $backendConfig = $this->processDefaultTemplates($backendConfig);
         $backendConfig = $this->processFieldTemplates($backendConfig);
@@ -91,20 +93,7 @@ class TemplateConfigPass implements ConfigPassInterface
         // first, resolve the general template overriding mechanism
         foreach ($backendConfig['entities'] as $entityName => $entityConfig) {
             foreach ($this->defaultBackendTemplates as $templateName => $defaultTemplatePath) {
-                // 1st level priority: easy_admin.entities.<entityName>.templates.<templateName> config option
-                if (isset($entityConfig['templates'][$templateName])) {
-                    $template = $entityConfig['templates'][$templateName];
-                // 2nd level priority: easy_admin.design.templates.<templateName> config option
-                } elseif (isset($backendConfig['design']['templates'][$templateName])) {
-                    $template = $backendConfig['design']['templates'][$templateName];
-                // 3rd level priority: app/Resources/views/easy_admin/<entityName>/<templateName>.html.twig
-                } elseif (file_exists($this->templatesDir.'/easy_admin/'.$entityName.'/'.$templateName.'.html.twig')) {
-                    $template = 'easy_admin/'.$entityName.'/'.$templateName.'.html.twig';
-                // 4th level priority: app/Resources/views/easy_admin/<templateName>.html.twig
-                } elseif (file_exists($this->templatesDir.'/easy_admin/'.$templateName.'.html.twig')) {
-                    $template = 'easy_admin/'.$templateName.'.html.twig';
-                // 5th level priority: @EasyAdmin/default/<templateName>.html.twig
-                } else {
+                if (!$this->templateLoader->exists($template = '@EasyAdmin/'.$entityName.'/'.$templateName.'.html.twig')) {
                     $template = $defaultTemplatePath;
                 }
 
@@ -129,14 +118,8 @@ class TemplateConfigPass implements ConfigPassInterface
                             @trigger_error(sprintf('Passing a template path without the ".html.twig" extension is deprecated since version 1.11.7 and will be removed in 2.0. Use "%s" as the value of the "template" option for the "%s" field in the "%s" view of the "%s" entity.', $templatePath, $fieldName, $view, $entityName), E_USER_DEPRECATED);
                         }
 
-                        // before considering $templatePath a regular Symfony template
-                        // path, check if the given template exists in any of these directories:
-                        // * app/Resources/views/easy_admin/<entityName>/<templatePath>
-                        // * app/Resources/views/easy_admin/<templatePath>
-                        if (file_exists($this->templatesDir.'/easy_admin/'.$entityName.'/'.$templatePath)) {
-                            $templatePath = 'easy_admin/'.$entityName.'/'.$templatePath;
-                        } elseif (file_exists($this->templatesDir.'/easy_admin/'.$templatePath)) {
-                            $templatePath = 'easy_admin/'.$templatePath;
+                        if (!$this->templateLoader->exists($templatePath = '@EasyAdmin/'.$entityName.'/'.$templatePath)) {
+                            $templatePath = '@EasyAdmin/default/'.$templatePath;
                         }
                     } else {
                         // At this point, we don't know the exact data type associated with each field.
@@ -168,14 +151,7 @@ class TemplateConfigPass implements ConfigPassInterface
     private function processDefaultTemplates(array $backendConfig)
     {
         foreach ($this->defaultBackendTemplates as $templateName => $defaultTemplatePath) {
-            // 1st level priority: easy_admin.design.templates.<templateName> config option
-            if (isset($backendConfig['design']['templates'][$templateName])) {
-                $template = $backendConfig['design']['templates'][$templateName];
-            // 2nd level priority: app/Resources/views/easy_admin/<templateName>.html.twig
-            } elseif (file_exists($this->templatesDir.'/easy_admin/'.$templateName.'.html.twig')) {
-                $template = 'easy_admin/'.$templateName.'.html.twig';
-            // 3rd level priority: @EasyAdmin/default/<templateName>.html.twig
-            } else {
+            if (!$this->templateLoader->exists($template = '@EasyAdmin/default/'.$templateName.'.html.twig')) {
                 $template = $defaultTemplatePath;
             }
@javiereguiluz
Copy link
Collaborator Author

I like that:

  • It would simplify a bit our code and the learning curve.

I don't like that:

  • We're "abusing" the native mechanism because we're overriding templates that don't exist (because they include the EntityName in their paths) and that doesn't work in the default Symfony.
  • We lose the ability of defining a template to override that same template in all entities.

I don't know:

  • If there would be problems with overriding the default templates and extending from them. It's a well-known Symfony issue. I know that @yceruto has made something to fix or improve this ... but I don't know the details.

@gabiudrescu
Copy link
Contributor

We lose the ability of defining a template to override that same template in all entities.

so basically, if I want to override the view template for all entities, I will no longer be able to do so?

@yceruto
Copy link
Collaborator

yceruto commented Aug 9, 2017

We're "abusing" the native mechanism because we're overriding templates that don't exist (because they include the EntityName in their paths) and that doesn't work in the default Symfony.

I don't understand what do you mean really, but it's just as works the current path convention for app/Resources/views/easy_admin/<entityName>/<templateName>, my proposal is deprecate it and move this convention to app/Resources/EasyAdminBundle/views/<entityName>/<templateName> nothing more. Thus, we're using the native mechanism of Symfony to override templates.

... we're overriding templates that don't exist

What template doesn't exists? :/

We lose the ability of defining a template to override that same template in all entities.

I'm not sure what case is it, you're refering to default templates? it'll be possible in the same way that now.

If there would be problems with overriding the default templates and extending from them. It's a well-known Symfony issue.

No problem anymore, this issue can be resolved adding a second Twig's namespace for views path of the bundle: symfony/symfony#17557 (comment). So instead of say: "you should to create this namespace manually and then ...", my proposal is add it automatically and say "you should use this another namespace !EasyAdmin..." to override templates and extending from them. Tecnically this:

twig:
    paths:
        vendor/javiereguiluz/easyadmin-bundle/Resources/views: !EasyAdmin

then in app/Resources/EasyAdminBundle/views/default/layout.html.twig:

{% extends '@!EasyAdmin/default/layout.html.twig' %}

More info about which is the source of the issue here https://stackoverflow.com/questions/45220467/how-can-i-override-partially-a-third-party-template


My intention is make the overriding template mechanism of the bundle like native. Currently, "without configuration" we can override any template from any bundle, only we need make the path convention: %kernel.root_dir%/Resources/<bundleName>/views/<templateName>. With that, we can override all default templates in EasyAdminBundle too.

So it implies that easy_admin.design.templates.<templateName> config option is useless and app/Resources/views/easy_admin/<templateName>.html.twig path convention too.

If you want a different place to override these templates then use Twig's paths:

twig:
    paths:
        # syntax <relative or realpath> : <bundle namespace>
        custom/path: EasyAdmin

and it's enough to override any default template from another location (custom/path/default/layout.html.twig). The possibilities are unlimited with Twig's paths.

Now, how can I override a template from one entity? (Currently)

  • with configuration: Creates the template file anywhere, then use easy_admin.entities.<entityName>.templates.<templateName> config option
  • with convention: Creates the template file in app/Resources/views/easy_admin/<entityName>/<templateName>.html.twig (path convention of the bundle)

In this case the convention wins (one step less), but you need a new dir easy_admin as convention and mixed with the app templates dirs. It's why I proposed move it to app/Resources/EasyAdminBundle/views/<entityName>/<templateName>.html.twig as well-known native convention. It works as before, still more clear, all my templates are in one place and all them will be named as @EasyAdmin/...html.twig.

I hope it help!

@yceruto
Copy link
Collaborator

yceruto commented Aug 9, 2017

I know that @yceruto has made something to fix or improve this ... but I don't know the details.

In the next release of twig an exception should be thrown on circular template reference twigphp/Twig#2530 :)

@javiereguiluz
Copy link
Collaborator Author

I'm closing this issue because we're starting a new phase in the history of this bundle (see #2059). We've moved it into a new GitHub organization and we need to start from scratch: no past issues, no pending pull requests, etc.

I understand if you are angry or disappointed by this, but we really need to "reset" everything in order to reignite the development of this bundle.

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

No branches or pull requests

3 participants