Change controller resolver auto mapping default configuration value

[bobvandevijver]

As I have been bitten by this again in one of my projects, and I saw a related issue in the Symfony repository (https://github.com/symfony/symfony/issues/50739 and a comment from @stof), I propose to change the default with the next major. The deprecation will only be triggered when not explicitly configured.

In the future, this will prevent new users from unexpected behaviour, but keeps the existing functionality available for anyone that wants to use this. And, with the new MapEntity attributes it would also be trivial to change the mapping configuration for a single controller argument, which is in my opinion preferable to have this wildcard match enabled globally.

If this is to be merged, the documentation on https://symfony.com/doc/current/doctrine.html#fetch-automatically needs to be updated as well. I can do that as well.

[ostrolucky]

I'll need a feedback from other maintainers here, I can't decide on this. I think this might be controversial.

[dmaicher]

~~It will however still be true for new projects then using the flex recipe~~

~~Regarding changing the default: I'm indifferent on this. I checked 2 of my work projects and disabled the option: nothing changed. Seems we are not relying on that auto mapping feature. We never experienced any issues with it being enabled either though.~~

[bobvandevijver]

@dmaicher You are referring to the other auto_mapping option (yes, there are two options with the same name). This change is specifically for the controller resolver, not for the entity managers. The flex recipe does not define a default for controller resolver option.

In simple situations either option behaves the same. But there are more complex situations where the auto mapping can bite you unexpectedly. For example, if you have multiple parameters in your route where both should be mapped to an entity. Consider the following example:

#[Route('/event/{event<\d+>}/activity/{activity<\d+>}`)]
public function show(Event $event, Activity $activity): Response {
}

If you would try to access this for a non-existing Activity, you would expect a 404 as the activity with that id does not exist. However, due to the auto mapping feature, the resolver will use all other parameters to try find something that might match. So, if you have a relation with Event in your Activity, it would instead resolve the first found Activity with that event, and thus not come with the expected 404.

[dmaicher]

@bobvandevijver ah of course :man_facepalming: I confused the two options. Sorry. I tested again and we are indeed relying on it :sweat_smile: A small number of tests failing when I disable it. They can be fixed by explicitly adding MapEntity with a mapping.

@stof WDYT?

[ostrolucky]

pls add UPGRADE-2.12.md file where this is documented too

[bobvandevijver]

@ostrolucky Something like this?

[ostrolucky]

yes that looks very good! thanks

[dmaicher]

Should we also adjust the recipe? So that new projects don't start with a deprecation?

[bobvandevijver]

Seems reasonable to me: I'm currently in progress of updating the Symfony documentation, will look into the recipe after that 👍🏻

[stof]

Totally in favor of this change. I've advocated for years that this feature should not exist at all because it is a total footgun. When introducing the argument value resolver to replace the DoctrineParamConverter of SensioFrameworkExtraBundle, I managed to make this feature configurable (it was not in DoctrineParamConverter) but I did not manage to make it disabled by default or non-existent. Disabling this is literally one of the first things I'm doing when starting a new project, so I'm glad it is off by default.

[ostrolucky]

Well it's not off by default yet, that will happen in next major version

[dmaicher]

Well it's not off by default yet, that will happen in next major version

well at least with the recipe change its off by default for new projects with flex :smile:

[bobvandevijver]

Even with the recipe change it still requires DoctrineBundle 2.12 to be released first 😄

[dmaicher]

@bobvandevijver I noticed that we now have those deprecations when running tests on 2.12.x

Remaining self deprecation notices (98)

  98x: Since doctrine/doctrine-bundle 2.12: The default value of "doctrine.orm.controller_resolver.auto_mapping" will be changed from `true` to `false`. Explicitly configure `true` to keep existing behaviour.
    6x in DoctrineExtensionTest::testCacheConfiguration from Doctrine\Bundle\DoctrineBundle\Tests\DependencyInjection
    3x in DoctrineExtensionTest::testAutomapping from Doctrine\Bundle\DoctrineBundle\Tests\DependencyInjection
    2x in DoctrineExtensionTest::testDependencyInjectionConfigurationDefaults from Doctrine\Bundle\DoctrineBundle\Tests\DependencyInjection
    2x in DoctrineExtensionTest::testSingleEntityManagerWithEmptyConfiguration from Doctrine\Bundle\DoctrineBundle\Tests\DependencyInjection
    2x in DoctrineExtensionTest::testControllerResolver from Doctrine\Bundle\DoctrineBundle\Tests\DependencyInjection

Would you have time by any chance to look into fixing those? If not I can also take a look.

[seb-jean]

Hi, Since the default value of setting doctrine.orm.controller_resolver.auto_mapping is false, I get the following error:

Cannot autowire argument $user of "App\Controller\ProfileController::show()": it needs an instance of "App\Entity\User" but this type has been excluded in "config/services.yaml".

My controller:

// src/Controller/ProfileController.php
namespace App\Controller;

use App\Entity\User;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\Routing\Attribute\Route;

class ProfileController
{
    #[Route('/profile/{ulid}')]
    public function show(Request $request, User $user): Response
    {
        // ...
    }
}

[ostrolucky]

Default value isn't false

[nicolas-grekas]

I'm proposing to revert this change in https://github.com/symfony/recipes/pull/1302: this breaks the DX, we need to find a better way.

[nicolas-grekas]

To me this change doesn't make sense BTW: automapping is supposed to be a DX helper. If it is disabled by default, why would anyone want to re-enable it to get a clumsy behavior in the end.

The right way that'd be consistent with this change would be to remove automapping altogether.

BUT, this would still break DX, so that I'm NOT proposing to remove automapping. I'd rather revert this change, and possibly improve automapping instead if we can improve the edge cases.

[bobvandevijver]

For me the automapper was bad DX from the start, and you do not even need it for the most common scenarios. For those other specific scenarios you are always better of with the MapEntity attribute.

And if you do want to use it, you are free to do so. I have updated the documentation (which has not yet been merged) to make clear where it can be used for, and what bad DX you get in return.

In my opinion relying on entity property names in your routing parameters is bad design anyways (not refactor safe).

[ostrolucky]

We did this change to fix the DX. I'm not sure about disabling this by default in recipe, but I think having to be explicit about it is better too. Sorry @nicolas-grekas but looks like it's 4v1. Deprecation stays.