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() #21144

Closed
gdinko opened this issue Sep 12, 2017 · 15 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.

Overwriting renderHttpException is not a solution if you writing package.
This was working super in versions before 5.5

@themsaid please stop closing this.

@Dylan-DPC-zz
Copy link

Isn't the previous issue enough? It is clearly mentioned that it isn't a bug. Stop creating new duplicates for no reason.

@gdinko
Copy link
Author

gdinko commented Sep 12, 2017

It's definitely a bug, something that was working < 5.5 and not working in 5.5 this is a bug and is hard coded. Please fix it. Why the path is hard coded and not get from the view paths? Now nobody can create themes because someone decided that the error views must be in the resources folder and nowhere else.

@Dylan-DPC-zz
Copy link

Dylan-DPC-zz commented Sep 12, 2017

something that was working < 5.5 and not working in 5.5 this is a bug

Sorry, but that is a "breaking change" and not necessarily a "bug".

@gdinko
Copy link
Author

gdinko commented Sep 12, 2017

Sorry, but that is a "breaking change" and not a "bug".

Aha it's not a bug it's future. haha

Ok, just tell me why the path to errors is hard coded and not get from the view paths? Is this "breaking change" too?

@themsaid
Copy link
Member

#21145

@themsaid
Copy link
Member

This PR will read the error views from the paths you define in your configuration file.

@gdinko
Copy link
Author

gdinko commented Sep 12, 2017

This is not ok. If you set the path dynamicially
like this in some ServiceProvider (if you have admin or package)
In AppServiceProvider->boot() add new views path view()->addLocation(realpath(base_path('resources/views/themes' . '/' . 'mytheme')));

this will not return all paths will return only config paths:

config('view.paths')

@gdinko
Copy link
Author

gdinko commented Sep 12, 2017

@themsaid please fix it like this: view()->getFinder()->getPaths() not like this config('view.paths')

@themsaid
Copy link
Member

If multiple packages are having error views, how do we know which one will be loaded?

@gdinko
Copy link
Author

gdinko commented Sep 12, 2017

Good question, but in Laravel < 5.5 the paths are extracted from the view finder not from config('view.paths'), and works just fine. Because if you set them dynamically they are not in the config('view.paths'). Maybe you need to discus this with your team.

@themsaid
Copy link
Member

The behaviour didn't change between 5.4 and 5.5

@gdinko
Copy link
Author

gdinko commented Sep 12, 2017

Ok, but it's wrong. Please fix it.

@themsaid
Copy link
Member

So it's not a bug or a breaking change but a feature request, your request is noted. Thanks.

@gdinko
Copy link
Author

gdinko commented Sep 12, 2017

Ok, Thanks

@gdinko
Copy link
Author

gdinko commented Sep 12, 2017

quick hack for dynamic set view paths for error pages:
view()->addLocation(realpath(base_path('resources/views/themes' . '/' . 'theme'))); config(['view.paths' => view()->getFinder()->getPaths()]);

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