Skip to content

[HttpKernel] Use TypeInfo for #[MapRequestPayload] type resolution #58036

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 1 commit into
base: 7.4
Choose a base branch
from

Conversation

chalasr
Copy link
Member

@chalasr chalasr commented Aug 19, 2024

Q A
Branch? 7.2
Bug fix? no
New feature? yes
Deprecations? yes
Issues -
License MIT

Simplifying mapping request parameters to object lists. Before/After:

    /**
     * @param Product[] $products
     */
    #[Route(path: "/products", methods: "POST")]
    public function __invoke(
-        #[MapRequestPayload(type: Product::class)] array $products,
+        #[MapRequestPayload] array $products,
    ): Response {
    }

/cc @yceruto @mtarld

@carsonbot carsonbot added this to the 7.2 milestone Aug 19, 2024
@chalasr chalasr force-pushed the typeinfo-argument-resolvers branch 2 times, most recently from 45b01c9 to 7cba23b Compare August 20, 2024 01:40
@chalasr chalasr force-pushed the typeinfo-argument-resolvers branch from 7cba23b to 0643d9a Compare August 20, 2024 07:46
if ($this->typeResolver) {
$type = $this->typeResolver->resolve($parameter);

if ($type instanceof CollectionType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We have to "unwrap" a potential nullable type here I think

@yceruto
Copy link
Member

yceruto commented Aug 20, 2024

+   /**
+    * @param Product[] $products
+    */
    #[Route(path: "/products", methods: "POST")]
    public function __invoke(
-        #[MapRequestPayload(type: Product::class)] array $products,
+        #[MapRequestPayload] array $products,
    ): Response {
    }

Honestly, when you mentioned this feature during the type option discussion, I had higher expectations. But now that I see the diff, I’m not so sure. The combination of attributes and PHPDoc has always bothered me—things like the duplication of argument names like $products, the need for additional dependencies to make it work, and possible performance drawbacks with the current implementation make me question if it’s really worth it...

@chalasr
Copy link
Member Author

chalasr commented Aug 20, 2024

I get your concern, yet that phpdoc is needed anyway for type detection in IDEs/static analyzers to work.
So we actually end up duplicating that type information in a Symfony-specific way, that's what make me think it is worth it.

Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Makes sense but IMHO:

  1. I don't think the deprecation is needed nor desired
  2. This adds advanced type resolution to the hot-path, while this info might not be used; we should look for a way to make this parsing lazy

@chalasr
Copy link
Member Author

chalasr commented Aug 21, 2024

I don't think the deprecation is needed nor desired

Not strictly needed indeed but: it's not obvious what$type is about, anything else than arrays are auto detected through type declarations. That makes the API rather confusing IMHO, only for a special case that this PR is going to handles pretty much 100% of the time (#[MapRequestPayload requires serializer which requires type-info). Unneeded complexity to me. Still not convinced?

This adds advanced type resolution to the hot-path, while this info might not be used; we should look for a way to make this parsing lazy

👍 I will wrap that logic in a closure and pass it to ArgumentMetadata::$type for late execution

@nicolas-grekas
Copy link
Member

Still not convinced about the deprecation :)

@fabpot fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024
@fabpot fabpot modified the milestones: 7.3, 7.4 May 26, 2025
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.

6 participants