Config error on controller/error_pages.html #11163
Conversation
|
I think there has been some confusion here. The controller defined in To implement this, you need to create both the controller ( |
|
I think you are right Javier and by the way if this change is valid, xml and php code examples needs the same change |
|
@javiereguiluz I'm comfortable with what the documentation is describing. The overall concept is clear. The goal of this section on error pages is to override the native functionality of the Twig ExceptionController in the TwigBundle of Symfony. However, that controller does not have a "showException" method to override. This is confusing, as the method responsible for rendering and managing the exception behaviour is Symfony\Bundle\TwigBundle\Controller\ExceptionController::showAction(). It seems then that I should expect, as someone attempting to override the native functionality, that I can declare my own controller in the twig config, wire up my service (if needed), and restart my page, that there should be no changes at all (assuming that I have made no changes and simply inherit from twig's ExceptionController). Following the example's explicitly will instead produce an error complaining that there is no showException() method, and that I should be calling showAction() instead. If this assumption is incorrect, then the documentation needs to include information about what the showException() method in our overriding class should do (return values, parent methods to call), because as it stands, there is no parent functionality in that method at all from the base class. |
|
The section is indeed talking about overriding the controller, which meant: Create a new controller (Extending from nothing or just the Maybe we should make it more explicit (by showing an example action) that error handling is just another controller returning a nice Response. I.e: // src/Controller/ExceptionController.php
namespace App\Controller;
use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
use Symfony\Component\Debug\Exception\FlattenException;
use Symfony\Component\HttpFoundation\Request;
use Psr\Log\LoggerInterface;
class ExceptionController extends AbstractController
{
public function showException(Request $request, FlattenException $exception, LoggerInterface $logger = null)
{
$message = ...; // create a nice message based on $exception
// return a Response that is sent to the browser
return $this->render('error.html.twig', ['message' => $message]);
}
}If you agree, please update this PR, so we can give you full credits! 😃 Thanks for your very detailed explanation of your "documentation experience". |
|
In that case, perhaps changing the language used in the section would be prudent. The title of the passage is
What is being explaining above I would consider "replacing" the default ExceptionController (as "Overriding" as the operative word has a very specific meaning). Additionally, if you're simply going to be replacing the default ExceptionController and not extending it in any way, it doesn't really feel like this course of action is any different than just building an event listener and catching the kernel.exception event yourself (which is given significantly more detail in the next section of the documentation). In that case, I think it makes more sense to dedicate this middle portion to actually extending the ExceptionController provided by Symfony, instead of starting from scratch and assuming you understand what it should be doing. This flow seems to make more sense as well within the scope of the documentation, as the processes go from the simplest solution where developers have the least control, to the most complicated and most control. |
|
@javiereguiluz how can we proceed here? @brandinchiu are you willing to finish this PR then? |
|
I totally forgot about this since it hasn't really had any movement in the last few months. I have no problems appending any changes, but I also want to make sure that what I proposed on March 16th actually makes sense to people. The initial comment from @javiereguiluz seems to indicate that my understanding of what the documentation is trying to explain is incorrect, and therefore no change should be needed at all. If that's the case, We can simply close this issue. Otherwise, I'd like to have some kind of confirmation that my interpretation and suggestion have some value before committing anything. |
Hmm, can you show exactly what is that error?
I agree that "Replacing" is better than "Overriding" for this case. Also I'd suggest to remove the |
Starting from a blank Symfony 4.3 project and following the instructions on the page, I've made only the following changes:
twig:
...
exception_controller: App\Controller\ExceptionController:showException
...
# add more service definitions when explicit configuration is needed
# please note that last definitions always *replace* previous ones
App\Controller\ExceptionController:
public: true
arguments:
$debug: '%kernel.debug%'
<?php
namespace App\Controller;
use Symfony\Component\Debug\Exception\FlattenException;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpKernel\Log\DebugLoggerInterface;
/**
* Class ExceptionController
* @package App\Controller
*/
class ExceptionController extends \Symfony\Bundle\TwigBundle\Controller\ExceptionController
{
public function showAction(Request $request, FlattenException $except
8000
ion, DebugLoggerInterface $logger = null)
{
return parent::showAction($request, $exception, $logger); // TODO: Change the autogenerated stub
}
public function findTemplate(Request $request, $format, $code, $showException)
{
return parent::findTemplate($request, $format, $code, $showException); // TODO: Change the autogenerated stub
}
}If I then simply throw any exception from my default route, I'm greeted with this error:
This is of course because the twig configuration is telling me to do one thing, and the paragraphs immediately after it are telling me to do something else. This is the part that's confusing. The section is called:
And the first step is modifying the twig config. The example shows me using the The proposition here is only a change in language. The part that I'd like to know is if senior contributors of the documentation even agree with this interpretation at all, and if so, from which angle they would like to address it?
|
This appears to be a typo, as showException gives an error on Symfony 4.2 that reports confusion between showAction and showException. Setting the twig config to showAction fixes the problem and displays the correct result.
This PR was submitted for the 4.2 branch but it was merged into the 3.4 branch instead (closes #11163). Discussion ---------- Config error on controller/error_pages.html This appears to be a typo, as showException gives an error on Symfony 4.2 that reports confusion between showAction and showException. Setting the twig config to showAction fixes the problem and displays the correct result. <!-- If your pull request fixes a BUG, use the oldest maintained branch that contains the bug (see https://symfony.com/roadmap for the list of maintained branches). If your pull request documents a NEW FEATURE, use the same Symfony branch where the feature was introduced (and `master` for features of unreleased versions). --> Commits ------- 6071e6c Update error_pages.rst
|
@brandinchiu thanks for the super detailed explanation! Everything is clear to me now ... so we've merged this ... but we did that in 3.4 branch, which is still supported and contains the same bug. We'll merge in all the other branches too automatically. Thanks. |
|
That's great @javiereguiluz! I'm happy to have been helpful! |
This appears to be a typo, as showException gives an error on Symfony 4.2 that reports confusion between showAction and showException. Setting the twig config to showAction fixes the problem and displays the correct result.