Make setup of details row routes optional

[pxpm]

WHY

BEFORE - What was wrong? What was happening before this PR?

This was reported in Laravel-Backpack/community-forum#774 and @karandatwani92 brought this to my attention in #5395

TLDR: If you use ListOperation in your CrudController, the details row routes are always setup, even if you don't enableDetailsRow(). So technically you can access admin/monster/1/details even when details row is not enabled.

AFTER - What is happening after this PR?

This PR adds a way for developers to disable the details row setup if they don't plan to use it in their List Operation.

HOW

How did you achieve that, in technical terms?

The correct way to go about this would be to separate the details row into a new operation, that you can add when you need. That way routes would only be setup when you add the operation to the controller.

That would be a BC at the moment. So I thought about the other ways we could go about this.

a) We could create a new ListOperation without the details row. Like SimpleList or something similar.

• I don't think this is a good solution, because traits cannot be extended so we would have a lot of duplicated code.

b) We could create a php attribute #[WithoutDetailsInList] or something similar for example an interface to implement in the controller implements WithoutDetailsRow

• I don't dislike neither ways to be used as a configuration mechanism, but to make this non-breaking we would be using them on the "negative". I wouldn't have nothing against if we were talking about WithDetailsRow, but it being negative makes it a not so good option.

c) use a property as config so that developer can prevent the route setup.

• Indeed, this looks the easiest and cleanest way out. Setting protect $setupDetailsRowRoutes = false in your controller would work similar as if you didn't include that operation in that controller.

Is it a breaking change?

No I don't think so.

How can we test the before & after?

You can go to any controller where you don't have the enableDetailsRow(), and access entity/{id}/details.
docs: Laravel-Backpack/docs#528

[tabacitu]

Why add a new way to configure things, as a property on CrudController?

All other configurations are done using CRUD::set(), what makes this one so different, that it requires a different paradigm? The fact that it needs to be present on route setup? And making this configuration in setup() or setupListOperation() is already too late... right?

[tabacitu]

So it sounds like a good way to do it, yes.

To properly support this property, let's:

• add this property to the ListOperation trait, public setupDetailsRowRoute = true;
• no longer test if it exits, only if it's true or false;
• let's make the property singural instead of plural (setupDetailsRowRoute, not setupDetailsRowRoutes); that way it's different than the setup route methods;

That way I think it's more clear, the trait comes with the property.

[tabacitu]

Good job here Pedro, I really couldn't think of a better way out of this hole.

[pxpm]

I agree with your last point but not with the first two.

The first can't be done. You cannot initialize a property in a trait and overwrite it in the "used class".
The second is there because the first one can't be done. So when it doesn't exist, it means the default true.

I just did the 3rd. 👍

[tabacitu]

Ok great.

[tabacitu]

I completely agree with the reasoning, excellent job there Pedro! Loved reading it too, reads really really nicely.

[tabacitu]

Maybe there's another option?

(D) Seeing your conditional made me think... that the details row functionality is only useful if properly set up. How do you set it up? According to the docs:

• Enable the functionality: CRUD::enableDetailsRow()
• Overwrite the showDetailsRow($id) method; Alternative: overwrite views/backpack/crud/details_row.blade.php which is called by the default showDetailsRow($id) functionality;

I hate that we wrote that "alternative". Because that's a bad way to do that, it would apply to ALL details rows, for all entities. If we hadn't suggested that, we could have just changed the signature of showDetailsRow() and make it return false by default. But we can't do that, because of the above and because... it would be a breaking change.

---

So no, there's no D.

[pxpm]

Maybe there's another option?

(D) Seeing your conditional made me think... that the details row functionality is only useful if properly set up. How do you set it up? According to the docs:

• Enable the functionality: CRUD::enableDetailsRow()
• Overwrite the showDetailsRow($id) method; Alternative: overwrite views/backpack/crud/details_row.blade.php which is called by the default showDetailsRow($id) functionality;

I hate that we wrote that "alternative". Because that's a bad way to do that, it would apply to ALL details rows, for all entities. If we hadn't suggested that, we could have just changed the signature of showDetailsRow() and make it return false by default. But we can't do that, because of the above and because... it would be a breaking change.

So no, there's no D.

Actually I didn't went to the docs. I know that I need to enableDetailsRow() and setDetailsRowView(). Wasn't aware of that issue in the docs.

It may be some very very old docs, from the image too. Better update them no ?

Cheers

[tabacitu]

It may be some very very old docs, from the image too. Better update them no ?

Yes please.