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

Laravel's resource naming convention difference #47

Open
bilogic opened this issue Jun 1, 2021 · 5 comments
Open

Laravel's resource naming convention difference #47

bilogic opened this issue Jun 1, 2021 · 5 comments

Comments

@bilogic
Copy link
Contributor

bilogic commented Jun 1, 2021

Hi,

Any reason why we don't follow Laravel's convention?
It becomes one more thing to learn. Also, the VERB is not "restricted" in Orchid/Crud

Reason I'm looking at this is because I'm trying to create a Download button in a column of each row.

Thank you.

Action Laravel Orchid/Crud
list GET /admin/crud/documents ANY admin/crud/list/documents
create GET /admin/crud/documents/create ANY admin/crud/create/documents
view GET /admin/crud/documents/{id} ANY admin/crud/view/documents/{id}
edit GET /admin/crud/documents/{id}/edit ANY admin/crud/edit/documents/{id}

https://laravel.com/docs/8.x/controllers#actions-handled-by-resource-controller

@tabuna
Copy link
Member

tabuna commented Jun 1, 2021

Hi @bilogic. There is no reason for this.
I think we can change the routes to match the resource controller in laravel.
Would you like to work on this?

@bilogic
Copy link
Contributor Author

bilogic commented Jun 1, 2021

Ok, let me play around. Thanks.

@bilogic
Copy link
Contributor Author

bilogic commented Jul 5, 2021

@tabuna

The following is the best I could do, it was quite hard to understand how the handle() in the CRUD screens actually work.
Should I file a PR? There are several other PRs not merged yet, so I'm not sure.

Method Laravel (Desired) Method Orchid/Crud (Current)
GET/HEAD  crud/documents - index GET/HEAD  crud/documents - list
GET/HEAD  crud/documents/create - create GET/HEAD  crud/documents/create - create
POST   crud/documents - store POST  crud/documents/create/save - NO NAME
GET/HEAD  crud/documents/{id} - show GET/HEAD  crud/documents/{id} - view
GET/HEAD  crud/documents/{id}/edit - edit GET/HEAD  crud/documents/{id}/edit - edit
PUT/PATCH crud/documents/{id} - update POST  crud/documents/{id}/edit/update - NO NAME
DELETE crud/documents/{id} - destroy POST  crud/documents/{id}/edit/delete - NO NAME

This was referenced Jul 6, 2021
@bilogic
Copy link
Contributor Author

bilogic commented Jul 6, 2021

@tabuna

I thought it is better to discuss your feedback on #50 here.

First, I also starting out thinking it was just a simple rename of the routes, but as I delve deeper, there were other important differences

Route name and order

Laravel defines routes in the following order

  1. index (CRUD's list)
  2. create
  3. store
  4. show (CRUD's view)
  5. edit
  6. update
  7. destroy

CRUD defines routes in the following order

Action Priority Orchid/Crud Priority Laravel
create 1 ANY /crud/create/documents 2 GET/HEAD /crud/documents/create
view 2 ANY /crud/view/documents/{id} 4 GET/HEAD /crud/documents/{id}
edit 3 ANY /crud/edit/documents/{id} 5 GET/HEAD /crud/documents/{id}/edit
list 4 ANY /crud/list/documents 1 GET/HEAD /crud/documents
  • I did not quite understand the reasons for routing everything via the handle(...) method found in platform's Screen.php
  • As store is missing, and could potentially conflict with index, I decided to take the approach of making CRUD exactly the same as Laravel at the route and HTTP level.
  • I think this might take a few more refactoring to complete, for now, I just want to fix as much as I can while not breaking current CRUD tests/functionality
  • The remaining differences is best shown below
# CURRENT
list     GET|HEAD|POST    /admin/crud/documents
create   GET|HEAD|POST    /admin/crud/documents/create
         POST             /admin/crud/documents/create/save
view     GET|HEAD         /admin/crud/documents/{id}
edit     GET|HEAD|POST    /admin/crud/documents/{id}/edit
         POST             /admin/crud/documents/{id}/edit/update
         POST             /admin/crud/documents/{id}/edit/delete

# DESIRED
index    GET|HEAD         /admin/crud/documents
create   GET|HEAD         /admin/crud/documents/create
store    POST	          /admin/crud/documents
show     GET|HEAD         /admin/crud/documents/{id}
edit     GET|HEAD         /admin/crud/documents/{id}/edit
update   PUT|PATCH        /admin/crud/documents/{id}
destroy  DELETE           /admin/crud/documents/{id}

Route HTTP methods

  • Laravel accepts very specific HTTP methods for each route
  • CRUD accepts ANY for all routes
  • Thus, I restricted CRUD's HTTP methods.

Next, I'm agreeable to extend the screen macro instead of redefining another macro. (I did not know macros can be extended)

The benefits of following Laravel's naming, priority and route convention will make it easier to pick up Orchid and extend it. For some time now, I could only use Orchid, I could not really improve it as a number of design differ from Laravel.

Do you have any other concerns?

@bilogic
Copy link
Contributor Author

bilogic commented Jul 29, 2021

@tabuna the current CRUD routes can't be just mapped to Laravel because Laravel's update and delete rely on PUT/PATCH/DELETE HTTP action, but CRUD is using ANY for all HTTP actions today.

Hope you can come to a decision on this and #50. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants