Skip to content

Add MapRequestPayload tips with built-in types #19590

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

Open
wants to merge 3 commits into
base: 7.1
Choose a base branch
from

Conversation

AurelienPillevesse
Copy link
Contributor

@AurelienPillevesse AurelienPillevesse commented Feb 23, 2024

Added a tip for using built-in types only with MapRequestPayload, to avoid revealing the internal implementation of your applications

Edit : I tried to target to 7.0 because feature is already available but lot of commits involved. Currently stay for 7.1, let you decide (or I can make a new PR to target 6.4 if you want)

@carsonbot carsonbot added this to the 7.1 milestone Feb 23, 2024
@AurelienPillevesse AurelienPillevesse changed the title Add MapRequestPayload tips with built-in typed Add MapRequestPayload tips with built-in types Feb 23, 2024
@AurelienPillevesse AurelienPillevesse changed the base branch from 7.1 to 7.0 February 26, 2024 12:57
@AurelienPillevesse AurelienPillevesse changed the base branch from 7.0 to 7.1 February 26, 2024 13:07
@AurelienPillevesse
Copy link
Contributor Author

If you have any feedback @nicolas-grekas @fabpot @dunglas

@nicolas-grekas
Copy link
Member

Shouldn't we not expose internal info instead? This recommendation looks scary without really providing the solution nor the why so I don't think it's how we want to deal with this. Maybe worth opening an issue on symfony/symfony? Or a PR if you have a proposal on the topic?

@AurelienPillevesse
Copy link
Contributor Author

AurelienPillevesse commented Feb 27, 2024

Currently, the solution with an Enum in a #[MapRequestPayload] class is to do like this :

#[Assert\Choice(callback: [YourEnum::class, 'values'])
public string $myInputAttribute;

And in the enum :

public static function values(): array
{
    return array_column(self::cases(), 'value');
}

This solution seems OK for me as we want to map a string from JSON (for example) to a string in our input. This string in our input can have all values from our Enum.

Would you like me to add this example to the "tips" in the case of an enum in a #[MapRequestPayload] class?

@AurelienPillevesse
Copy link
Contributor Author

AurelienPillevesse commented Feb 27, 2024

Is it better now ?

@alexandre-daubois
Copy link
Member

I would rephrase this like this 🙂 Tell me what you think!

diff --git a/controller.rst b/controller.rst
diff --git a/controller.rst b/controller.rst
index 392ed827b..255ec6246 100644
--- a/controller.rst
+++ b/controller.rst
@@ -539,10 +539,18 @@ if you want to map a nested array of specific DTOs::
         ) {}
     }
 
-.. tip::
+.. caution::
+
+    If you're using typed properties with ``MapRequestPayload```, it is
+    recommended to use built-in types like ``int``, ``bool`` or ``string`` for
+    mapping. Using custom types could expose your application implementation in
+    errors during denormalization. For example, validating an enum when using
+    ``#[MapRequestPayload]`` could look like this::
 
-    If using typed properties with `MapRequestPayload`, it's recommanded to use built-in types (like `int`, `bool`, `string`...) for mapping. Using custom types can expose your application implementation outside with errors during denormalization.
-    Validating an Enum in your `#[MapRequestPayload]` class should look like this::
+        // src/Controller/LuckyController.php
+        use App\Model\MyInput;
+        use Symfony\Component\HttpFoundation\Response;
+        use Symfony\Component\HttpKernel\Attribute\MapRequestPayload;
 
         class LuckyController
         {
@@ -553,12 +561,14 @@ if you want to map a nested array of specific DTOs::
             }
         }
 
+        // src/Model/MyInput.php
         class MyInput
         {
             #[Assert\Choice(callback: [MyEnum::class, 'values'])]
             public string $myInputAttribute;
         }
 
+        // src/Model/MyEnum.php
         enum MyEnum: string
         {
             case FIRST_CASE = 'first_case';

@AurelienPillevesse
Copy link
Contributor Author

AurelienPillevesse commented Mar 12, 2024

@alexandre-daubois seems good for me, I applied the changes
What do you think about releasing it to 6.4 or 7.0 to be visible on documentation when validated ?

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

Successfully merging this pull request may close these issues.

4 participants