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

[DX] A simpler way to define custom error pages #24049

Closed
javiereguiluz opened this issue Aug 31, 2017 · 10 comments
Closed

[DX] A simpler way to define custom error pages #24049

javiereguiluz opened this issue Aug 31, 2017 · 10 comments
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony)

Comments

@javiereguiluz
Copy link
Member

javiereguiluz commented Aug 31, 2017

Q A
Bug report? no
Feature request? yes
BC Break report? no
RFC? no
Symfony version 3.4

How to Customize Error Pages in Symfony apps has bugged me since day one. In the past we could understand why it worked that way. But now that Twig is a first class citizen, in-app bundles are gone and bundle inheritance is in question we could reconsider this.

The Proposal

Make Symfony/Twig/TwigBundle look for in the templates/error/ directory for custom error pages, and keep using the same logic to pick the template (error500.json.twig, error.xml.twig, etc.)

This change could be made for Symfony 4.0, which introduces the new templates/ directory at project root, but we may need to deprecate or change something in Symfony 3.4 in preparation for that.

@javiereguiluz javiereguiluz added the DX DX = Developer eXperience (anything that improves the experience of using Symfony) label Aug 31, 2017
@Pierstoval
Copy link
Contributor

Pierstoval commented Aug 31, 2017

If the templates/error/ directory is customizable via an option in the TwigBundle, I'm totally ok 👍

By the way, this could be done for 3.4 too: if this option is not set in the TwigBundle, use classic bundle inheritance, else, load error templates from the resolved directory. This should be possible with a compiler pass

@yceruto
Copy link
Member

yceruto commented Aug 31, 2017

I think it should be a general convention, I means, not only for TwigBundle's errors templates. Although each bundle is free to add its own custom Twig's path.

There is an open PR #23339 proposing to move them to templates/bundles/Twig/Exception/*.html.twig by convention.

@javiereguiluz
Copy link
Member Author

@yceruto I agree with what you say, but considering that 100% of real projects need to customize the error pages, why not making that simpler? If the only way to customize is templates/bundles/Twig/Exception/*.html then I need to learn that errors are in fact exceptions, that they are handled by "a thing" called TwigBundle, I need to learn what a bundle is, I need to learn how to override the templates of a bundle, etc.

This proposal could be compatible with your proposal too. Want to customize error pages fast? Create templates/error/... Do you prefer a more strict approach? Use the native Symfony mechanism to override templates: templates/bundles/Twig/Exception/*.html

That way we could solve @Pierstoval issue too: you can either use the simple and fast path (templates/error/) or you can use the standard overriding mechanims (but no other option is provided).

@Pierstoval
Copy link
Contributor

Pierstoval commented Aug 31, 2017

There is something really important too:

If templates/error becomes customizable (and maybe configured by default in TwigBundle's recipe for DX), you will be able to load the "parent" template or blocks, which you cannot do when using Symfony's inheritance system (because then @Twig/Exception/error.html.twig is always resolved as app/Resources/TwigBundle/views/Exception/error.html.twig).

Proposing a customizable extension point with a simple configuration option is to me the BEST possible solution for templates overrides.

EasyAdminBundle uses something similar: auto-discovery in app/Resources/easy_admin, and configuration-based template sources. Each of them allow extending base EasyAdmin templates, which standard override does not allow.

@Pierstoval
Copy link
Contributor

Just as a reminder, people tend to use bundle templates inheritance systems, while it should be much better to use twig paths.

IMO this practice should be encouraged, and maybe a new syntax could be used to resolve a bundle view directory just to create a new namespace for it and allow better overriding systems:

Before:

# app/config/config.yml
twig:
    paths:
        '%kernel.project_dir%/vendor/symfony/twig-bundle/Resources/views/Exception': Error
        '%kernel.project_dir%/vendor/friendsofsymfony/user-bundle/Resources/views': FOS

After:

# app/config/config.yml
twig:
    paths:
        '@TwigBundle/Exception': Error
        '@FOSUserBundle': User

@yceruto
Copy link
Member

yceruto commented Aug 31, 2017

@javiereguiluz I see your point and I'm in favor of simplest way to do this and I think there is some way to reach it if what we want is templates/error/*.twig without do anything more to be able to override
TwigBundle/Resources/views/Exception/*.twig templates.

  1. We need move all TwigBundle/Resources/views/Exception/*.twig templates to the root views TwigBundle/Resources/views/*.twig otherwise we can't omit Exception directory in templates/error/.
  2. Update all references from @Twig\Exception\*.twig to @Twig\*.twig.
  3. Add a new Twig's path in TwigBundle to register (before the parent one) the new convention:
    if (file_exists($dir = $container->getParameter('kernel.project_dir').'/templates/error')) {
        $twigFilesystemLoaderDefinition->addMethodCall('addPath', array($dir, 'Twig'));
    }
    $container->addResource(new FileExistenceResource($dir));

Thus, when a @Twig\error500.html.twig template is called you can override it in templates/error/error500.html.twig automatically.

It's just an idea, something might be missing and of course, I did ignored BC breaks.

@yceruto
Copy link
Member

yceruto commented Sep 4, 2017

Also it could be confused, since it puts the overridden bundle templates in a location that can conflict with the app templates inside the main templates/ directory.

@ghost
Copy link

ghost commented Sep 15, 2017

@Pierstoval gave the correct purpose imo.

# app/config/config.yml
twig:
    paths:
        '@TwigBundle/Exception': Error
        '@FOSUserBundle': User

Are we sticking with that solution or is the solution from @javiereguiluz the way to go? Or are both solutions possible?

@yceruto
Copy link
Member

yceruto commented Sep 15, 2017

IMHO @Pierstoval's proposal doesn't help to solve this issue :/ because the TwigBundle/Resources/views/Exception/* templates are referenced as @Twig/Exception/* internally:

protected function getTemplate()
{
return '@Twig/Exception/'.($this->debug ? 'exception' : 'error').'.html.twig';
}

protected function findTemplate(Request $request, $format, $code, $showException)
{
$name = $showException ? 'exception' : 'error';
if ($showException && 'html' == $format) {
$name = 'exception_full';
}
// For error pages, try to find a template for the specific HTTP status code and format
if (!$showException) {
$template = sprintf('@Twig/Exception/%s%s.%s.twig', $name, $code, $format);
if ($this->templateExists($template)) {
return $template;
}
}
// try to find a template for the given format
$template = sprintf('@Twig/Exception/%s.%s.twig', $name, $format);
if ($this->templateExists($template)) {
return $template;
}
// default to a generic HTML exception
$request->setRequestFormat('html');
return sprintf('@Twig/Exception/%s.html.twig', $showException ? 'exception_full' : $name);
}

and this proposal '@TwigBundle/Exception': Error only would create a new namespace @Error for a new path vendor/symfony/twig-bundle/Resources/views/Exception that would allow to load TwigBundle templates as @Error\error.html.twig in userland. It seems more related to another issue #17557.

For now, the approved convention for 3.4/4.0 is <default_path>/bundles/<BundleName> (#24179).

@javiereguiluz
Copy link
Member Author

Let's close because thanks to @yceruto overriding bundles templates is now much simpler. Also, even the error pages path is a bit weird, it's the standard way of doing things in Symfony, so it's better to not introduce yet another way of doing this. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
DX DX = Developer eXperience (anything that improves the experience of using Symfony)
Projects
None yet
Development

No branches or pull requests

3 participants