Skip to content

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Doctrine Parameter Converter injecting first collection member when no value is provided #47166

Answered by nicolas-grekas
janklan asked this question in Q&A
Discussion options

I tripped over a peculiar behaviour involving the SensioFrameworkExtraBundle's DoctrineParamConverter, and now also Entity Argument Resolver in #43854.

In a common controller URL parameter conversion scenario, it goes like this:

  1. If you have a route: #[Route('/test/{parentEntity<\d+>}/load/{childEntity<\d+>?})], and
  2. If your controller has a signature of public function action(Parent $parentEntity, ?Child $childEntity), then
  3. When you call /test/50/load/, the childEntity should be null, but instead it is whatever is the first member of the child collection in the parentEntity is.

The root cause of this is the piece of code I linked above:

if (null === $mapping = $options->mapping) {
    $keys = $request->attributes->keys();
    $mapping = $keys ? array_combine($keys, $keys) : [];
}

// ... 

$metadata = $objectManager->getClassMetadata($class); // Child entity metadata
foreach ($mapping as $attribute => $field) {
    if (!$metadata->hasField($field) && (!$metadata->hasAssociation($field) || !$metadata->isSingleValuedAssociation($field))) {
        continue;
    }

    $criteria[$field] = $request->attributes->get($attribute);
}

// ...

try {
    return $objectManager->getRepository($class)->findOneBy($criteria);
} catch (NoResultException|ConversionException) {
    return null;
}
  1. The $mapping contains the current request attributes, so for instance, ['parentEntity' => 50, '_route' => '_route', ...].
  2. Here is the problem: If the $child happens to have an association called parentEntity, the code above slaps ['parentEntity' => 50] to $criteria.
  3. $criteria is later passed to findOneBy. Since the criteria only contains the parentEntity's ID, it will find one childEntity according to the $criteria - the first that belongs to the parent with ID 50. But we did not pass any childEntity ID - we did not want the converter to load anything

I think the original idea is not bad - the end effect is that if there are multiple related entities, they are guaranteed to belong together, because the $criteria contains more than just a single primary key. Even though it's magic that depends on consistent naming conventions, let's roll with it.

The problem I see is that if the childEntity value is not present in the $mapping array (= the value is missing in the Request), none of this code should have been executed in the first place.

This behaviour can be suppressed by providing the id param name in options, but I'm wondering if this should not be the default for optional parameters?

You must be logged in to vote

Replies: 3 comments · 4 replies

Comment options

The problem I see is that if the childEntity value is not present in the $mapping array (= the value is missing in the Request), none of this code should have been executed in the first place.

Only you can know this so I don’t think the default behavior will change!

The converter will attempt to do a findOneBy() fetch by using all of the wildcards in your route that are actually properties on your entity.

https://symfony.com/bundles/SensioFrameworkExtraBundle/current/annotations/converters.html#1-fetch-automatically

There are options like id to address use cases where automatic fetching does not work, do not be afraid to use them 👌


Your route’s pattern is also weird because you can get the parent from the child so only one parameter would be needed?

You must be logged in to vote
4 replies
@janklan
Comment options

Only you can know this so I don’t think the default behavior will change!

It works well when both arguments are present, but the default is to load "something" (= first child of a parent) when the value is "nothing" (= null).

Your route’s pattern is also weird because you can get the parent from the child so only one parameter would be needed?

I have good reasons for doing so. The bottom line is that the current default seems to be not ideal. If the child argument is not provided, it should stay null.

@MatTheCat
Comment options

Okay I understand now (sorry I wasn’t aware a route parameter was considered an ID if its name matches an entity’s).

Indeed the fact null and false are treated the same in

if (false === $id || null === $id) {
return false;
}
feels weird. I’ll check that out.


Sooo no tests are covering this behavior (well testApplyGuessOptional would complain, but it seems unrelated).

I opened #47242

@nicolas-grekas
Comment options

If the child argument is not provided, it should stay null.

Could be worth considering. Would you mind sending a PR that does so?

@MatTheCat
Comment options

@nicolas-grekas I opened #47242 but GitHub won't link discussions 😔

Comment options

Fixed by #47242 from @MatTheCat 🙏

You must be logged in to vote
0 replies
Answer selected by nicolas-grekas
Comment options

Thanks @MatTheCat

FYI the real-world scenario for my weird routes: I have multi-tenant app where I prefix every URL with tenant's slug. Say there was Child entity owned by a Tenant. In my routes, I used /{tenant}/{child<\d+>?}. When I called /tenant-id entity, I wanted to show some Tenant-related page. When I called /tenant-id/child-id, I wanted to show the same page, but then include some additional info about that selected child.

Since Child had ManyToOne relation to Tenant, the param converter always tried to find the first child when the ID was null.

I'm using the Tenant entity in the URL for access control. One of my early event subscribers (kernel.controller with 4096 priority) figures out what organisation is the user accessing, checks permission to do so for the user in session, and activates row level security in PostgreSQL so that data of other tenants does no longer show up, and then lets the lest of the request go trhough, incl. other route parameter conversion etc.

I decided I have the tenant present in every tenant-aware route instead of session, where it created some complexities that were not worth the hassle.

You must be logged in to vote
0 replies
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Category
🙏
Q&A
Labels
None yet
3 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.