Laravel-Backpack / community-forum

A workspace to discuss improvement and feature ideas, before they're actually implemented.
28 stars 0 forks source link

[Proposal] Installation - don't overwrite existing error views, when publishing #36

Open ziming opened 4 years ago

ziming commented 4 years ago

Currently whenever I do backpack:install it overrides all my existing error pages views

Will be good if backpack error pages (404, 403.etc) stays within backpack routes.

pxpm commented 4 years ago

This is how i think it's working, correct me if I am wrong:

Could we achieve what you say creating a different folder for backpack error pages, and create a new error handler that displays backpack errors only in backpack pages? How we'd check that we are in a backpack page ? Middleware ? Check if admin middleware is beeing used ?

Do you have any ideia how this could work ?

Best, Pedro

tabacitu commented 4 years ago

Hi @ziming & @pxpm - I agree this part of the framework could benefit from a little attention.

The reason those error pages exist is that:

What I understand @ziming is saying, is that his project already had 401, 402, 403 error pages. And then when he installed Backpack, those custom error pages were overwritten by the Backpack error pages. Which if true, might indeed be a problem. I think a good solution for this problem would be for Backpack to NOT overwrite an error page, if it already exists. Am I right @ziming ?

Either way, I haven't taken a look at how Laravel's error pages look & work in a while. It would be good to review the situation once again, and see if there's an alternative solution to publishing those error views.

ziming commented 4 years ago

Actually if there is a 404.blade.php file, Backpack doesn't override. I just tested it.

What happens is, I actually prefer laravel default error views over Backpack ones. I always thought those default views are in the errors folder in resources. Turns out I'm wrong, they are in vendor/...

still it is an unexpected change

tabacitu commented 4 years ago

Oh, I understand. I wonder if the Laravel team would be open to change the default Laravel views to:

  1. Show the custom message from the second parameter of abort(403, 'something') if present.
  2. If no error page is present for that HTTP error code, show an error page with the error code, instead of the "whoops" page.

It'd be a nice contribution to Laravel, I think. And if we fixed this at the framework level, we wouldn't need the error pages at all. I'll leave the issue open because this suggestion needs research (if it's been proposed in the past) and submitting an issue to Laravel.

If any of you has the time, and thinks it would be cool to make this small contribution to Laravel, please do it before me 😄

tabacitu commented 4 years ago

Looks like it's been discussed, but decided against: https://github.com/laravel/framework/pull/29775

pxpm commented 4 years ago

Yes @ziming, they go to resources if you publish them to edit, otherwise they are loaded from vendor.

Maybe we should NOT post error files by default, and instruct users how to publish them if they want backpack error pages ?

tabacitu commented 4 years ago

Hmm... I don't see a solution I like right now, so I'm going to postpone this change until the next version bump. Let's think about it some more.

Bronco84 commented 4 years ago

I'd also like the ability to use default laravel error pages, or the ability for them to only apply to the "admin" routes, or whatever route group is set up to handle backpack.

My reason is that I have an outward facing part of my application that I prefer the more general error pages to render when needed. For example, I display a pdf and if it isn't there I don't want the default backpack page to render because it is in an iframe and using the link to the homepage wouldn't work in that situation.

I think the backpack error pages themselves seem to be better suited to render when trying to access the backpack routes.

tabacitu commented 4 years ago

@Bronco84 in case you have better error views, just delete the resources/views/errors folder, or place the error views inside that folder, instead of the Backpack ones. That should work just fine. I agree with your use case - it's more important for the errors to look similar to your front-end, than to your admin panel.

pxpm commented 4 years ago

@tabacitu but wouldn't that also replace the error pages inside the backpack admin panel with the ones you setup for the frontend ?

Bronco84 commented 4 years ago

@tabacitu So I actually did something similar and it worked for my needs. I just moved the backpack error views to a subfolder in resources\views\errors and now laravel is using the default error pages.

@pxpm Yes, unfortunately when you move out the backpack error pages, any errors on backpack will render default laravel error pages. This was an ok trade off for me so I could use my own branded error pages meant for the external users on my website, but I think ideally it would be nice for backpack error pages to render only when on a backpack specific route. It doesn't have to be default behavior, but maybe through config variable.

tabacitu commented 4 years ago

I agree - that would be best - admin error pages for admin, front-end error pages for front-end. But as far as I know there's no clean way to achieve that.

The more I think about this, the more I think that we don't really need the admin panel to have its own error pages:

So the more I look at it, the more I think it's better to just remove the error pages entirely, from Backpack, and just use Laravel's default error pages - or any custom error pages the developer might have set up. What do you guys think? Is this really something Backpack should help with - error pages in production?

lotarbo commented 4 years ago

My opinion, using default error pages everywhere

Bronco84 commented 4 years ago

Yeah my opinion is to go with default Laravel error pages everywhere and drop the backpack error pages. The backpack error pages pages are very nice, but like you said there isn’t a really clean way to route specific error pages just for backpack routes and laravel has them already so it’s one less thing for the backpack team to worry about.

pxpm commented 4 years ago

Hello guys, from laravel docs:

/**
 * Render an exception into an HTTP response.
 *
 * @param  \Illuminate\Http\Request  $request
 * @param  \Throwable  $exception
 * @return \Illuminate\Http\Response
 */
public function render($request, Throwable $exception)
{
    if ($exception instanceof CustomException) {
        return response()->view('errors.custom', [], 500);
    }

    return parent::render($request, $exception);
}

if we have request, can't we separate backpack errors from others by grabing request url ? We are sure backpack is under domain.com/backpack-admin-prefix. Or am i missing something ?

tabacitu commented 4 years ago

@pxpm indeed, but afaik to include that kind of logic, we'd need to make changes inside App\Exceptions\Handler, in userland.

So basically, upon installation, we'd need to ask the user to make a change inside App\Exceptions\Handler. This would make the installation process less seamless, for a feature that... right now... seems unnecessary to me... That's why I said "fuck it" 😆

ajinzrathod commented 2 years ago

I think there is a hacky way to do this. This will not show the default error page of Laravel but we can hide the description of the Error and I think this looks cooler than the default one.

Open this file resources/views/errors/layout.blade.php

@if ($staff_designation == 'admin')  // <- add this line
    <small>
        @yield('description')
    </small>
@endif   // <- add this line

To check whether the user is Admin is not, I have used a custom service provider.

app/Providers/MyServiceProvider.php

    public function boot()
    {
        view()->composer('*', function($view) {
            $staff_designation = '';
            if (Auth::check()) {
                // write your admin logic here
                $id = auth()->user()->id;
                $staff_designation = auth()->user()->staff_designation;
            }
            return $view->with('staff_designation', $staff_designation);
        });
    }

Remember to edit the Admin Logic

And add the service provider in config/app.php

    'providers' => [
         // .....
        App\Providers\MyServiceProvider::class,
   ];

In this way, Error Description is viewed only to those who are admin. And all other users just get a 404 page without a Description.