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

[5.5.3] Please fix hard coded path in ExceptionHandler->renderHttpException() #21138

Closed
gdinko opened this issue Sep 12, 2017 · 7 comments
Closed

Comments

@gdinko
Copy link

gdinko commented Sep 12, 2017

  • Laravel Version: 5.5.3
  • PHP Version:
  • Database Driver & Version:

Description:

Hard coded path in Exception Handler renderHttpException

When additional path is added to view (let's say if we implements themes), the exception handler seeks the 404.blade.php in resources/errors folder, not in the themes folder, and this is hard coded.

Steps To Reproduce:

  1. In AppServiceProvider->boot() add new views path
    view()->addLocation(realpath(base_path('resources/views/themes' . '/' . 'mytheme')));
  2. In the new path create errors/404.blade.php
  3. In the browser call non existing path
  4. default 404.blade.php is rendered

Please fix this:
`protected function renderHttpException(HttpException $e)
{
$status = $e->getStatusCode();

    view()->replaceNamespace('errors', [
        resource_path('views/errors'),
        __DIR__.'/views',
    ]);

    if (view()->exists($view = "errors::{$status}")) {
        return response()->view($view, ['exception' => $e], $status, $e->getHeaders());
    }

    return $this->convertExceptionToResponse($e);
}`

to this:
`protected function renderHttpException(HttpException $e)
{
$status = $e->getStatusCode();

    $hints = view()->getFinder()->getPaths();
    foreach($hints as $k => $v){
        $hints[$k] = $v . '/errors';
    }
    $hints[] = __DIR__.'/views';

    view()->replaceNamespace('errors', $hints);

    if (view()->exists($view = "errors::{$status}")) {
        return response()->view($view, ['exception' => $e], $status, $e->getHeaders());
    }

    return $this->convertExceptionToResponse($e);
}`

If you don't fix these the all idea about themes is useless.

@Dylan-DPC-zz
Copy link

I don't see this as a bug. You can open a proposal in laravel/internals or just shoot a PR

@themsaid
Copy link
Member

You can override renderHttpException in your exception handler as you wish depending on your specific use case.

@gdinko
Copy link
Author

gdinko commented Sep 12, 2017

@themsaid please stop closing this. It's not ok to overwrite renderHttpException. Is this your work?

@themsaid
Copy link
Member

@gdinko what's wrong with overriding? you can adapt the framework as per your use case, overriding makes it easier to adapt :)

@gdinko
Copy link
Author

gdinko commented Sep 12, 2017

@themsaid please stop closing this. It's not ok to overwrite renderHttpException.
Because if i create a package i can't overwrite the renderHttpException.

what's wrong with overriding? you can adapt the framework as per your use case, overriding makes it easier to adapt :)

Before 5.5 version, this was working ok, please fix it, and stop closing this bug or i will contact someone else!!!

@Dylan-DPC-zz
Copy link

@gdinko kindly respect the people here. He has every right to close the issue if he wants. As he said you can override the handler if you want. Also since you are building a package, it shouldn't be rendering any exceptions and relying on the "target" application to render

@themsaid
Copy link
Member

No problem guys, here's a PR: #21145

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

No branches or pull requests

3 participants