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

ErrorController::beforeRender not called #17603

Closed
scarabedore opened this issue Mar 4, 2024 · 12 comments
Closed

ErrorController::beforeRender not called #17603

scarabedore opened this issue Mar 4, 2024 · 12 comments

Comments

@scarabedore
Copy link

Description

We recently made the switch from Cake 4 to Cake 5. Since then, our 404 page stopped working.

How to reproduce

// app.php
'Error' => [
    'errorLevel' => E_ALL,
    'skipLog' => [
        'Cake\Http\Exception\MissingControllerException',
        'Cake\Routing\Exception\MissingRouteException',
        'Cake\Controller\Exception\MissingActionException',
        'Authorization\Exception\ForbiddenException',
        'Cake\Http\Exception\UnauthorizedException',
    ],
    'log' => true,
    'trace' => true,
    'ignoredDeprecationPaths' => [
        //'vendor/josegonzalez/cakephp-version/src/Model/Behavior/VersionBehavior.php',
    ],
    // Having queryString set on exceptions is deprecated.
    // See https://github.com/cakephp/cakephp/pull/16971
    // In this context 'queryString' would be a property of \Cake\Database\Log\LoggingStatement.
    'convertStatementToDatabaseException' => true,
],
// bootstrap.php
/*
 * Register application error and exception handlers.
 */
(new ErrorTrap(Configure::read('Error')))->register();
(new ExceptionTrap(Configure::read('Error')))->register();
// Application.php
public function middleware(MiddlewareQueue $middlewareQueue): MiddlewareQueue
{
    $middlewareQueue
        // Catch any exceptions in the lower layers,
        // and make an error page/response
        ->add(new ErrorHandlerMiddleware(Configure::read('Error')))
        ...
}
// templates/Error/error500.php
<?php
echo get_class($this);
?>
// src/Controller/ErrorController.php
<?php
declare(strict_types=1);
namespace App\Controller;
use Cake\Event\EventInterface;

class ErrorController extends AppController
{
    public function beforeRender(EventInterface $event)
    {
        \Cake\Log\Log::debug('event');
        parent::beforeRender($event);

        $this->viewBuilder()->setTemplatePath('Error');
        $this->viewBuilder()->setClassName(\App\View\InternalView::class);
    }
}
// vendor/cakephp/cakephp/src/Error/Middleware/ErrorHandlerMiddleware.php
public function handleException(Throwable $exception, ServerRequestInterface $request): ResponseInterface
{
    ...
    \Cake\Log\Log::debug('before event');  // not in original source code
    $event = $this->dispatchEvent(
        'Exception.beforeRender',
        ['exception' => $exception, 'request' => $request],
        $trap
    );
    \Cake\Log\Log::debug('after event');  // not in original source code
    ...
}

produces

// logs/debug.log
2024-03-04 12:05:20 debug: before event
2024-03-04 12:05:20 debug: after event

// HTML output
Cake\View\View

Expected behavior

To see event in the logs, and to see App\View\InternalView in the HTML output.

CakePHP Version

5.0.5

PHP Version

8.1.25

@scarabedore
Copy link
Author

Could it be in any way related to #17599 ?

@ADmad
Copy link
Member

ADmad commented Mar 4, 2024

ErrorController::beforeRender() is called for the Controller.beforeRender event, not Exception.beforeRender event. You need to better explain what you mean by "404 page stopped working.".

@ADmad ADmad added this to the 5.0.6 milestone Mar 4, 2024
@ADmad ADmad added support and removed defect labels Mar 4, 2024
@scarabedore
Copy link
Author

Sure. For this file:

// src/Controller/ErrorController.php
<?php
declare(strict_types=1);
namespace App\Controller;
use Cake\Event\EventInterface;

class ErrorController extends AppController
{
    public function beforeRender(EventInterface $event)
    {
        \Cake\Log\Log::debug('event');
        parent::beforeRender($event);

        $this->viewBuilder()->setTemplatePath('Error');
        $this->viewBuilder()->setClassName(\App\View\InternalView::class);
    }
}

With Cake 4.4.18, I enter a non existing URL in the browser, ErrorController::beforeRender() runs (I see the log, and the proper View is set).

With Cake 5.0.5, I enter a non existing URL in the browser (same one as above), ErrorController::beforeRender() does not run (I don't see the log and the wrong View is set).

@ADmad
Copy link
Member

ADmad commented Mar 5, 2024

I am unable to reproduced this, ErrorController::beforeRender() gets called as expected. Maybe in your case an exception is being generated when the exception renderer tries to create an ErrorController instance and it ends up using a Controller instance instead.

@scarabedore
Copy link
Author

Which files should I look into to find out more about what is going on?

@scarabedore
Copy link
Author

scarabedore commented Mar 6, 2024

Commenting out the ErrorHandlerMiddleware brought Cake 4's behavior back, i.e.

// Application.php

public function middleware(MiddlewareQueue $middlewareQueue): MiddlewareQueue
{
    $middlewareQueue
        // Catch any exceptions in the lower layers,
        // and make an error page/response
        // ->add(new ErrorHandlerMiddleware(Configure::read('Error')))
        ...
// bootstrap.php, unchanged

(new ErrorTrap(Configure::read('Error')))->register();
(new ExceptionTrap(Configure::read('Error')))->register();

I'll try to reproduce this issue on a new project.

@scarabedore
Copy link
Author

I successfully reproduced it on a new project:

composer create-project --prefer-dist cakephp/app:5.* cms
composer require cakephp/authentication
// app_local.php
'debug' => false,

Apply this diff
diff.patch

Call URL http://localhost:8765/few/asd

There is no event log inside of debug.log.

@scarabedore
Copy link
Author

On the reproduction repo (previous comment), commenting Authentication stuff made ErrorController::beforeRender() runs as expected, i.e.

// AppController.php
public function initialize(): void
{
    parent::initialize();

    $this->loadComponent('Flash');
    // $this->loadComponent('Authentication.Authentication');
}
// PagesController.php
public function initialize(): void
{
    parent::initialize();
    // $this->Authentication->allowUnauthenticated(['display']);
}

@ADmad
Copy link
Member

ADmad commented Mar 6, 2024

Since you added a parent::initialize() calls in ErrorController::initialize() you also need to add extra code to skip auth checks for the ErrorController:

if ($this->components()->has('Authentication')) {
    $this->Authentication->allowUnauthenticated([$this->request->getParam('action')]);
}

@scarabedore
Copy link
Author

That's right.
However this check was not needed with Cake 4, and updating to Cake 5 broke out the app without notice. So I think "something" should be done, either using Cake 4's behavior, or showing some warning somewhere.

@markstory markstory modified the milestones: 5.0.6, 5.0.7 Mar 9, 2024
@markstory
Copy link
Member

So I think "something" should be done, either using Cake 4's behavior, or showing some warning somewhere.

We could have the error rendering process log an error when using the application's ErrorController fails and a fallback needs to be used. Restoring the cake4 behavior could break applications that have already been moved to 5.x

markstory added a commit that referenced this issue Mar 10, 2024
When error rendering fails it can be tedious to diagnose what the
problem is. Having some logging when rendering fails could help triage
the problem and get to a solution quicker.

Refs #17603
markstory added a commit that referenced this issue Mar 10, 2024
When error rendering fails it can be tedious to diagnose what the
problem is. Having some logging when rendering fails could help triage
the problem and get to a solution quicker.

Refs #17603
@markstory markstory modified the milestones: 5.0.7, 5.0.8 Apr 6, 2024
@markstory markstory modified the milestones: 5.0.8, 5.0.9 May 11, 2024
@markstory
Copy link
Member

We've improved the logging here, so closing for now. If we need further improvements we can have new issues for those topics.

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

No branches or pull requests

3 participants