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.

Sign up for GitHub

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

Make setup of details row routes optional #5397

Merged
merged 5 commits into from
Dec 7, 2023
Merged

Make setup of details row routes optional #5397

Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e6df259
allow developer to prevent details row setup
pxpm Dec 6, 2023
61a36ca
Apply fixes from StyleCI
StyleCIBot Dec 6, 2023
3eff0da
simplify if
pxpm Dec 6, 2023
7ae1655
rename variable
pxpm Dec 6, 2023
7d9e5f1
use singular
pxpm Dec 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 7 additions & 5 deletions src/app/Http/Controllers/Operations/ListOperation.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,13 @@ protected function setupListRoutes($segment, $routeName, $controller)
'operation' => 'list',
]);

Route::get($segment.'/{id}/details', [
'as' => $routeName.'.showDetailsRow',
'uses' => $controller.'@showDetailsRow',
'operation' => 'list',
]);
if (! isset($this->setupDetailsRowRoutes) || $this->setupDetailsRowRoutes === true) {
Copy link
Member

@tabacitu tabacitu Dec 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

@tabacitu tabacitu Dec 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. 👍

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok great.

Route::get($segment.'/{id}/details', [
'as' => $routeName.'.showDetailsRow',
'uses' => $controller.'@showDetailsRow',
'operation' => 'list',
]);
}
}

/**
Expand Down