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

[Fix] Update delete path name to avoid route conflicts #740

Merged

Conversation

loic425
Copy link
Member

@loic425 loic425 commented Aug 18, 2023

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Related tickets
License MIT

1/ Delete path name can have some conflicts depending on operation sorting.
On previous routing system, these routing sorting system was hard-coded.
If we place the Bulk delete operation after the delete operation, the bulk delete is broken due to path conflicts.

2/ On current version of Symfony, http method overrides is not enabled by default and then these paths have another issue with same path for delete and edit operation (with POST method).
symfony/recipes#892

Before

app_backend_contact_create               GET|POST        ANY      ANY    /admin/contacts/new                             
app_backend_contact_update               GET|PUT         ANY      ANY    /admin/contacts/{id}/edit                       
app_backend_contact_bulk_delete          DELETE          ANY      ANY    /admin/contacts/bulk_delete                     
app_backend_contact_delete               DELETE          ANY      ANY    /admin/contacts/{id}                          
app_backend_contact_show                 GET             ANY      ANY    /admin/contacts/{id}                            
app_backend_contact_index                GET             ANY      ANY    /admin/contacts 

After

app_backend_contact_create               GET|POST        ANY      ANY    /admin/contacts/new                             
app_backend_contact_update               GET|PUT         ANY      ANY    /admin/contacts/{id}/edit                       
app_backend_contact_bulk_delete          DELETE          ANY      ANY    /admin/contacts/bulk_delete                     
app_backend_contact_delete               DELETE          ANY      ANY    /admin/contacts/{id}/delete                           
app_backend_contact_show                 GET             ANY      ANY    /admin/contacts/{id}                            
app_backend_contact_index                GET             ANY      ANY    /admin/contacts 

@loic425 loic425 requested a review from a team as a code owner August 18, 2023 12:10
@loic425 loic425 changed the title Update delete path name to avoid route conflicts [Fix] Update delete path name to avoid route conflicts Aug 18, 2023
'delete' => '',
default => '/' . $shortName,
};
$path = $operation instanceof ApiOperationInterface && 'delete' === $shortName ? '' : '/' . $shortName;
Copy link
Contributor

Choose a reason for hiding this comment

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

What about adding the delete string injected in this service, allowing developers to customize the url slug can maybe be a thing.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hum interesting, but if we do that for this one, we have to do that for every operations.

Copy link
Contributor

Choose a reason for hiding this comment

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

It was just an idea, it will need a dedicated PR

'delete' => '',
default => '/' . $shortName,
};
$path = $operation instanceof ApiOperationInterface && 'delete' === $shortName ? '' : '/' . $shortName;
Copy link
Member

Choose a reason for hiding this comment

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

I have mixed feelings about it. The fix is needed, but in API this delete suffix should be required only in non API context. Once we are in API context, it is okay to have exactly the same URL but with different HTTP method. This logic is required only when we have a form, that will send a POST request with hidden DELETE notation.

What I would recommend is to reverse this condition so we add delete only for non ApiOperationInterface

Copy link
Member Author

@loic425 loic425 Aug 29, 2023

Choose a reason for hiding this comment

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

With my proposal, we have these behaviors.

Non API

#[Delete]
#[Delete(name: 'remove')]
app_book_delete             DELETE        /admin/books/delete 
app_book_remove             DELETE        /admin/books/remove 

API

#[Delete]
#[Delete(name: 'remove')]
app_book_delete             DELETE        /admin/books 
app_book_remove             DELETE        /admin/books/remove 

IMHO, this is ok like this.
Don't think about hidden field, cause we'll need to support POST method with non api routes.

With the next PR, it will be

Non API

#[Delete]
#[Delete(name: 'remove')]
app_book_delete             POST, DELETE        /admin/books/delete 
app_book_remove             POST, DELETE        /admin/books/remove 

API

#[Delete]
#[Delete(name: 'remove')]
app_book_delete             DELETE        /admin/books 
app_book_remove             DELETE        /admin/books/remove 

@Zales0123 Zales0123 added the Bug Confirmed bugs or bugfixes. label Sep 9, 2023
@lchrusciel lchrusciel merged commit 45121e9 into 1.11 Sep 9, 2023
87 checks passed
@lchrusciel
Copy link
Member

Thank you, @loic425!

@lchrusciel lchrusciel deleted the fix/update-delete-path-name-to-avoid-route-conflicts branch September 9, 2023 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Confirmed bugs or bugfixes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants