[DoctrineBridge] Deprecate auto-mapping of entities in favor of mapped route parameters

[nicolas-grekas]

Q	A
Branch?	7.1
Bug fix?	no
New feature?	yes
Deprecations?
Issues	Fix #50739
License	MIT

Auto-mapping of entities on controllers is a foot-gun when multiple entities are listed on the signature of the controller.
This is described extensively by e.g. @stof in the linked issue and in a few others.

The issue is that we try to use all request attributes to call a findOneBy, but when there are many entities, there can be an overlap in the naming of the field/associations of both entities.

In this PR, I propose to deprecate auto-mapping and to replace it with mapped route parameters, as introduced in #54720.

If we go this way, people that use auto-mapping to e.g. load a $conference based on its {slug} will have to declare the mapping by using {slug:conference} instead. That makes everything explicit and keeps a nice DX IMHO, by not forcing a #[MapEntity] attribute for simple cases.

[nicolas-grekas]

I just confirmed on a real app that this solves all ambiguities while providing nice DX.

[chalasr]

rebase needed and 👍

[bobvandevijver]

@nicolas-grekas I absolutely love the solution you've come up with, this is awesome! 😃

I tested this by backporting the changes to a 6.4 project, everything seems to be working great there, and the old, now deprecated, automapping is still disabled as expected.

[fabpot]

Thank you @nicolas-grekas.

[kevinpapst]

In https://symfony.com/blog/new-in-symfony-7-1-mapped-route-parameters it says

Given that this new feature provides a nice DX (developer experience) and is very concise, we've decided to deprecate the automatic mapping of route parameters into Doctrine entities starting from Symfony 7.1.

I counter that argument. Deprecating a feature that works since years is not a great DX. This one feels different than other deprecations, as it requires so many route to be manually adjusted. I understand it if there are multiple Entities and parameters, but if only one {id} is set and only one Entity is used, isn't it obvious which one is meant?

[stof]

the usage of {id} to find by primary key was not part of the auto-mapping feature. @nicolas-grekas I think this should deserve a separate deprecation message (and probably a separate changelog entry)

[kevinpapst]

My feedback is purely based on this example in the Blog entry:

Do I misunderstand something? Is it deprecated or not?

Considering the note:

Given that this new feature provides a nice DX

I honestly don't see how this change is a win from DX side. Looks more like unnecessary repetition to me.
I really fear having to adjust and test hundreds of routes across several projects.

[nicolas-grekas]

The {id:document} mapping in the last example in the blog post is not needed indeed.
What matters is having explicit mapping and using {document} should be the way to go.
/cc @javiereguiluz FYI

[kevinpapst]

@nicolas-grekas thanks for the update. I agree that {document} is better to understand and should be used. But in the past {id} was documented and is likely used in tenth of thousands of routes out there.
So my question and hope remains: {id} keeps on working without a deprecation?

[kevinpapst]

Follow up at #57211

[javiereguiluz]

@nicolas-grekas @kevinpapst from the point of view of generating URLs in templates:

<a href="{{ path('document_show', {id: document.id}) }}"> ... </a>

This looks better than the following:

<a href="{{ path('document_show', {document: document.id}) }}"> ... </a>

A document is not the same as document ID. That's why I kept the {id} parameter in the route and mapped it to the controller's document argument.

But maybe I'm missing something here.