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

Introduce ability to override default error view paths #51293

Closed

Conversation

ahmedabdel3al
Copy link
Contributor

@ahmedabdel3al ahmedabdel3al commented May 5, 2024

This PR introduces the capability to override the default error view paths through the withExceptions callback:

->withExceptions(function (Exceptions $exceptions) {
      $exceptions->registerErrorViewPathsUsing(function () {
          View::replaceNamespace('errors', collect(config('view.error_view_path'))->all());
      });
  })

After Laravel 11 released , we can still override it using a withSingletons , by making a new handler class extend the framework one then override registerErrorViewPaths

->withSingletons([
        \Illuminate\Contracts\Debug\ExceptionHandler::class => \App\Exceptions\Handler::class,
    ])

IMO ,This method of overriding follows the new approach of error handling.

@ahmedabdel3al ahmedabdel3al force-pushed the register-error-paths branch from 6c2b082 to bbccebb Compare May 5, 2024 01:16
@ahmedabdel3al ahmedabdel3al changed the title add ability to override the default error view paths Introduce ability to override default error view paths via withExceptions callback May 5, 2024
@ahmedabdel3al ahmedabdel3al changed the title Introduce ability to override default error view paths via withExceptions callback Introduce ability to override default error view paths May 5, 2024
@taylorotwell
Copy link
Member

Thanks for your pull request to Laravel!

Unfortunately, I'm going to delay merging this code for now. To preserve our ability to adequately maintain the framework, we need to be very careful regarding the amount of code we include.

If possible, please consider releasing your code as a package so that the community can still take advantage of your contributions!

If you feel absolutely certain that this code corrects a bug in the framework, please "@" mention me in a follow-up comment with further explanation so that GitHub will send me a notification of your response.

@ahmedabdel3al
Copy link
Contributor Author

hey @taylorotwell
This explanation outlines the reason behind this PR. I am sorry that I didn't provide it in the PR description

If someone wishes to customize error pages, they can do

php artisan vendor:publish --tag=laravel-errors

However, if the intention is to handle all exceptions using only 4xx.blade.php and 5xx.blade.php only

views/errors/ {4xx.blade.php / 5xx.blade.php }

this will not work for all exceptions , for example 404 will still display from the vendor path because laravel still searching in all paths assigned to errors

$hits = [
   'errors'=> [
      resources/views/errors, // 404 not here 
      vendor/laravel/framework/src/Illuminate/Foundation/Exceptions/views  //404.blade is here 
   ]
]

In another case of an e-commerce application with multiple portals such as seller dashboard, buyer view, and admin panel, the views directory structure might appear as follows:

views
  - sellers/errors
  - buyers/errors
  - panel/errors

In such scenarios, overriding the default behavior of Laravel becomes necessary to prevent it from searching inside the vendor path and conditionally rendering error views based on the current request in case of e-commerce

After Laravel 11 was released, we had to make a new handler class to override this registerErrorViewPaths

class Handler extends ExceptionHandler
{
    /**
     * Register the error template hint paths.
     */
    protected function registerErrorViewPaths(): void
    {
        //logic here 
    }
}

This might lead to inconsistency because we have two different places to handle expectations.
1- inside the new Handler class
2- inside withExceptions callback in bootstrap/app.php

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

Successfully merging this pull request may close these issues.

2 participants