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

Jump to bottom

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

Merged
merged 2 commits into from
Sep 9, 2023
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2222054
Update delete path name to avoid route conflicts
loic425 Aug 18, 2023
c59467d
Fix PHPUnit tests
loic425 Aug 18, 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
21 changes: 21 additions & 0 deletions src/Component/Metadata/Api/ApiOperationInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
<?php

/*
* This file is part of the Sylius package.
*
* (c) Sylius Sp. z o.o.
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

declare(strict_types=1);

namespace Sylius\Component\Resource\Metadata\Api;

/**
* @experimental
*/
interface ApiOperationInterface
{
}
2 changes: 1 addition & 1 deletion src/Component/Metadata/Api/Delete.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
* @experimental
*/
#[\Attribute(\Attribute::TARGET_CLASS | \Attribute::IS_REPEATABLE)]
final class Delete extends HttpOperation implements DeleteOperationInterface
final class Delete extends HttpOperation implements DeleteOperationInterface, ApiOperationInterface
{
public function __construct(
?string $path = null,
Expand Down
2 changes: 1 addition & 1 deletion src/Component/Metadata/Api/Get.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
* @experimental
*/
#[\Attribute(\Attribute::TARGET_CLASS | \Attribute::IS_REPEATABLE)]
final class Get extends HttpOperation implements ShowOperationInterface
final class Get extends HttpOperation implements ShowOperationInterface, ApiOperationInterface
{
public function __construct(
?string $path = null,
Expand Down
2 changes: 1 addition & 1 deletion src/Component/Metadata/Api/GetCollection.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
* @experimental
*/
#[\Attribute(\Attribute::TARGET_CLASS | \Attribute::IS_REPEATABLE)]
final class GetCollection extends HttpOperation implements CollectionOperationInterface
final class GetCollection extends HttpOperation implements CollectionOperationInterface, ApiOperationInterface
{
public function __construct(
?string $path = null,
Expand Down
2 changes: 1 addition & 1 deletion src/Component/Metadata/Api/Patch.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
* @experimental
*/
#[\Attribute(\Attribute::TARGET_CLASS | \Attribute::IS_REPEATABLE)]
final class Patch extends HttpOperation implements UpdateOperationInterface
final class Patch extends HttpOperation implements UpdateOperationInterface, ApiOperationInterface
{
public function __construct(
?string $path = null,
Expand Down
2 changes: 1 addition & 1 deletion src/Component/Metadata/Api/Post.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
* @experimental
*/
#[\Attribute(\Attribute::TARGET_CLASS | \Attribute::IS_REPEATABLE)]
final class Post extends HttpOperation implements CreateOperationInterface
final class Post extends HttpOperation implements CreateOperationInterface, ApiOperationInterface
{
public function __construct(
?string $path = null,
Expand Down
2 changes: 1 addition & 1 deletion src/Component/Metadata/Api/Put.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
* @experimental
*/
#[\Attribute(\Attribute::TARGET_CLASS | \Attribute::IS_REPEATABLE)]
final class Put extends HttpOperation implements UpdateOperationInterface
final class Put extends HttpOperation implements UpdateOperationInterface, ApiOperationInterface
{
public function __construct(
?string $path = null,
Expand Down
6 changes: 2 additions & 4 deletions src/Component/Symfony/Routing/Factory/DeleteOperationRoutePathFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

namespace Sylius\Component\Resource\Symfony\Routing\Factory;

use Sylius\Component\Resource\Metadata\Api\ApiOperationInterface;
use Sylius\Component\Resource\Metadata\DeleteOperationInterface;
use Sylius\Component\Resource\Metadata\HttpOperation;

Expand All @@ -28,10 +29,7 @@ public function createRoutePath(HttpOperation $operation, string $rootPath): str
$identifier = $operation->getResource()?->getIdentifier() ?? 'id';

if ($operation instanceof DeleteOperationInterface) {
$path = match ($shortName) {
'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

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 •
edited
Loading

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


return sprintf('%s/{%s}%s', $rootPath, $identifier, $path);
}
Expand Down
11 changes: 9 additions & 2 deletions src/Component/spec/Symfony/Routing/Factory/DeleteOperationRoutePathFactorySpec.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,14 @@ function it_generates_route_path_for_delete_operations(): void
{
$operation = new Delete();

$this->createRoutePath($operation, '/dummies')->shouldReturn('/dummies/{id}');
$this->createRoutePath($operation, '/dummies')->shouldReturn('/dummies/{id}/delete');
}

function it_generates_route_path_for_delete_operations_with_custom_identifier(): void
{
$operation = (new Delete())->withResource(new Resource(identifier: 'code'));

$this->createRoutePath($operation, '/dummies')->shouldReturn('/dummies/{code}');
$this->createRoutePath($operation, '/dummies')->shouldReturn('/dummies/{code}/delete');
}

function it_generates_route_path_for_update_operations_with_custom_short_name(): void
Expand All @@ -59,4 +59,11 @@ function it_generates_route_path_for_api_delete_operations(): void

$this->createRoutePath($operation, '/dummies')->shouldReturn('/dummies/{id}');
}

function it_generates_route_path_for_api_delete_operations_with_custom_short_name(): void
{
$operation = new Api\Delete(shortName: 'remove');

$this->createRoutePath($operation, '/dummies')->shouldReturn('/dummies/{id}/remove');
}
}
2 changes: 1 addition & 1 deletion tests/Application/src/Subscription/Entity/Subscription.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,14 +39,14 @@
#[Index(grid: 'app_subscription')]
#[Create]
#[Update]
#[Delete]
#[BulkDelete]
#[ApplyStateMachineTransition(stateMachineTransition: 'accept')]
#[ApplyStateMachineTransition(stateMachineTransition: 'reject')]
#[BulkUpdate(
shortName: 'bulk_accept',
stateMachineTransition: 'accept',
)]
#[Delete]
#[Show(
template: 'subscription/show.html.twig',
twigContextFactory: ShowSubscriptionContextFactory::class,
Expand Down
4 changes: 2 additions & 2 deletions tests/Application/src/Tests/Controller/BoardGameUiTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,12 +50,12 @@ public function it_allows_browsing_board_games(): void
$this->assertStringContainsString('<td>Stone Age</td>', $content);
$this->assertStringContainsString(sprintf('<a href="/admin/board-games/%s">Show</a>', $boardGames['stone_age']->id()), $content);
$this->assertStringContainsString(sprintf('<a href="/admin/board-games/%s/edit">Edit</a>', $boardGames['stone_age']->id()), $content);
$this->assertStringContainsString(sprintf('<form action="/admin/board-games/%s" method="post">', $boardGames['stone_age']->id()), $content);
$this->assertStringContainsString(sprintf('<form action="/admin/board-games/%s/delete" method="post">', $boardGames['stone_age']->id()), $content);

$this->assertStringContainsString('<td>Ticket to Ride</td>', $content);
$this->assertStringContainsString(sprintf('<a href="/admin/board-games/%s">Show</a>', $boardGames['ticket_to_ride']->id()), $content);
$this->assertStringContainsString(sprintf('<a href="/admin/board-games/%s/edit">Edit</a>', $boardGames['ticket_to_ride']->id()), $content);
$this->assertStringContainsString(sprintf('<form action="/admin/board-games/%s" method="post">', $boardGames['ticket_to_ride']->id()), $content);
$this->assertStringContainsString(sprintf('<form action="/admin/board-games/%s/delete" method="post">', $boardGames['ticket_to_ride']->id()), $content);
}

/** @test */
Expand Down
6 changes: 3 additions & 3 deletions tests/Application/src/Tests/Controller/SubscriptionUiTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,17 +50,17 @@ public function it_allows_browsing_subscriptions(): void
$this->assertStringContainsString('<td>marty.mcfly@bttf.com</td>', $content);
$this->assertStringContainsString(sprintf('<a href="/admin/subscriptions/%s">Show</a>', $subscriptions['subscription_marty']->getId()), $content);
$this->assertStringContainsString(sprintf('<a href="/admin/subscriptions/%s/edit">Edit</a>', $subscriptions['subscription_marty']->getId()), $content);
$this->assertStringContainsString(sprintf('<form action="/admin/subscriptions/%s" method="post">', $subscriptions['subscription_marty']->getId()), $content);
$this->assertStringContainsString(sprintf('<form action="/admin/subscriptions/%s/delete" method="post">', $subscriptions['subscription_marty']->getId()), $content);

$this->assertStringContainsString('<td>doc.brown@bttf.com</td>', $content);
$this->assertStringContainsString(sprintf('<a href="/admin/subscriptions/%s">Show</a>', $subscriptions['subscription_doc']->getId()), $content);
$this->assertStringContainsString(sprintf('<a href="/admin/subscriptions/%s/edit">Edit</a>', $subscriptions['subscription_doc']->getId()), $content);
$this->assertStringContainsString(sprintf('<form action="/admin/subscriptions/%s" method="post">', $subscriptions['subscription_doc']->getId()), $content);
$this->assertStringContainsString(sprintf('<form action="/admin/subscriptions/%s/delete" method="post">', $subscriptions['subscription_doc']->getId()), $content);

$this->assertStringContainsString('<td>biff.tannen@bttf.com</td>', $content);
$this->assertStringContainsString(sprintf('<a href="/admin/subscriptions/%s">Show</a>', $subscriptions['subscription_biff']->getId()), $content);
$this->assertStringContainsString(sprintf('<a href="/admin/subscriptions/%s/edit">Edit</a>', $subscriptions['subscription_biff']->getId()), $content);
$this->assertStringContainsString(sprintf('<form action="/admin/subscriptions/%s" method="post">', $subscriptions['subscription_biff']->getId()), $content);
$this->assertStringContainsString(sprintf('<form action="/admin/subscriptions/%s/delete" method="post">', $subscriptions['subscription_biff']->getId()), $content);
}

/** @test */
Expand Down
Loading