fix #740 [Fix] Update delete path name to avoid route conflicts (loic…

…425) This PR was merged into the 1.11 branch. Discussion ---------- | 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 ``` Commits ------- 2222054 Update delete path name to avoid route conflicts c59467d Fix PHPUnit tests
Sylius · Sep 9, 2023 · 45121e9 · 45121e9
2 parents affba10 + c59467d
commit 45121e9
Show file tree

Hide file tree

Showing 12 changed files with 44 additions and 18 deletions.
diff --git a/src/Component/Metadata/Api/ApiOperationInterface.php b/src/Component/Metadata/Api/ApiOperationInterface.php
@@ -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
+{
+}
diff --git a/src/Component/Metadata/Api/Delete.php b/src/Component/Metadata/Api/Delete.php
@@ -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,

diff --git a/src/Component/Metadata/Api/Get.php b/src/Component/Metadata/Api/Get.php
@@ -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,

diff --git a/src/Component/Metadata/Api/GetCollection.php b/src/Component/Metadata/Api/GetCollection.php
@@ -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,

diff --git a/src/Component/Metadata/Api/Patch.php b/src/Component/Metadata/Api/Patch.php
@@ -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,

diff --git a/src/Component/Metadata/Api/Post.php b/src/Component/Metadata/Api/Post.php
@@ -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,

diff --git a/src/Component/Metadata/Api/Put.php b/src/Component/Metadata/Api/Put.php
@@ -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,

diff --git a/src/Component/Symfony/Routing/Factory/DeleteOperationRoutePathFactory.php b/src/Component/Symfony/Routing/Factory/DeleteOperationRoutePathFactory.php
@@ -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;
 
@@ -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;
 
             return sprintf('%s/{%s}%s', $rootPath, $identifier, $path);
         }

diff --git a/src/Component/spec/Symfony/Routing/Factory/DeleteOperationRoutePathFactorySpec.php b/src/Component/spec/Symfony/Routing/Factory/DeleteOperationRoutePathFactorySpec.php
@@ -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
@@ -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');
+    }
 }
diff --git a/tests/Application/src/Subscription/Entity/Subscription.php b/tests/Application/src/Subscription/Entity/Subscription.php
@@ -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,

diff --git a/tests/Application/src/Tests/Controller/BoardGameUiTest.php b/tests/Application/src/Tests/Controller/BoardGameUiTest.php
@@ -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 */

diff --git a/tests/Application/src/Tests/Controller/SubscriptionUiTest.php b/tests/Application/src/Tests/Controller/SubscriptionUiTest.php
@@ -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 */