Closed nicolas-grekas closed 3 months ago
Thanks for the PR 😍
Define the SYMFONY_ENDPOINT
environment variable:
# On Unix-like (BSD, Linux and macOS)
export SYMFONY_ENDPOINT=https://raw.githubusercontent.com/symfony/recipes/flex/pull-1302/index.json
# On Windows
SET SYMFONY_ENDPOINT=https://raw.githubusercontent.com/symfony/recipes/flex/pull-1302/index.json
Install the package(s) related to this recipe:
composer req 'symfony/flex:^1.16'
composer req 'doctrine/doctrine-bundle:^2.12'
Don't forget to unset the SYMFONY_ENDPOINT
environment variable when done:
# On Unix-like (BSD, Linux and macOS)
unset SYMFONY_ENDPOINT
# On Windows
SET SYMFONY_ENDPOINT=
In order to help with the review stage, I'm in charge of computing the diff between the various versions of patched recipes. I'm going keep this comment up to date with any updates of the attached patch.
I am strongly against this: the recipe currently follows the default of the doctrine bundle, and was adjusted to not cause deprecation notices on new installations. This change should be put on hold, and only be merged if the default in the doctrine bundle changes.
@nicolas-grekas the auto-mapping feature causes WTF behavior when you have multiple entities involved in the signature of the controller, especially because it can silently load an unwanted entity. And this is not only theoretical: I've had silent bugs in production loading an unwanted entity because of this behavior. The auto-mapping indeed works nice for the very simple case (the one in the demo for instance), but its actual impact for more complex cases is totally bad DX.
Sure, I'm not negating all this.
What I'm saying is that the current changes are not enough. It's not enough to break DX for the simple cases because DX for complex cases. That's a bad compromise if I can say it this way.
We should find a way to improve DX in both simple and complex situations (or improve DX in complex situations without breaking DX in simple ones).
The current solution is just a shift from one evil to another. That's not what I call an improvement. We should seek for something better.
this auto-mapping feature is exactly "attempt loading the entity with a mapping containing all request attributes that match the name of a field of the entity".
The DX issue reported for the simple case is the fact that without this feature, you need to define the mapping explicitly (note that this is not for the most common case of loading the entity by primary key coming from the request attribute named either id
or with the same name than the argument, as this is done before trying with a mapping).
The DX issue for the complex case is the fact that the criteria with a mapping containing all request attributes that match the name of a field of the entity
can lead to unwanted mapping, even in some cases where the controller was expected to rely on the id-based lookup but one of the entity parameter is an optional one.
I don't see how to reconcile that if we want the simple mapping-based case to keep working without requiring any configuration on the controller.
So to clarify:
Are there any cases the previous auto-mapping breaks?
Are there any possible ideas to re-enable single-parameter auto-mapping (e.g. #[Route("/article/{slug}"]
), because I feel like this is the main concern. Maybe we could throw a exception whenever the scope of a "simple mapping" is exceeded (like to ambiguous parameters or to many wired entities)
I my opinion it makes sense to use the MapEntity
attribute in every other (more complex) situation.
@ebitkov the issue is how to guess the fact that you want to use the slug
request parameter to create a mapping to fill a product
argument based on its slug
property ?
Some cases being broken by the auto-mapping end up being broken exactly because of a guessed mapping that look like that.
@stof I'm not a Symfony Contributor with little to no knowledge about the inner magic of this framework, so bare with me.
In my mind, something like
class ArticleController
{
#[Route("/article/{slug}")
public function show(Article $article): Response
{
# ...
}
}
and Article
being a doctrine-managed entity with a property $slug
, looks to me like a clear intend of "map that single parameter to the single entity I configured", but I guess that's the same thought, that lead to the current problem.
Maybe we could create a "simple auto-mapping", that only supports single parameter to entity mapping and throw an exception when ever you try to do more. Maybe with a notice like "can't auto-map entity, use the attribute".
That would remove the need to map something like this manually:
class ArticleController
{
#[Route("/article/{slug}")
public function show(
#[MapEntity(mapping: ["slug" => "slug"])] Article $article
): Response
{
// ...
}
}
I guess cases like these are the problem why auto-mapping might be difficult:
class ArticleController
{
#[Route("/article/{id}/{slug}")
public function show(Article $article): Response
{
// ...
}
}
or
class ArticleController
{
#[Route("/article/{slug}/comment/{id}")
public function show(Article $article, Comment $comment): Response
{
// ...
}
}
The more I thing about it the more I feel like it actually does more sense to force developers map there entities themselves then have a half-cooked auto-mapper workaround just to save a single attribute.
On paper it sounds great to just put your entities into the arguments of your action method, but I rarely use this features, because I often need to check other fields (e.g. status, publishing date) before showing the entity and I prefer to do this via DQL/SQL then preemptively loading the entity and then check, if the field are correct.
Sooo... I don't know. I seems to be more work then use to me at this point.
What you think @nicolas-grekas?
If the recipe enables the feature by default, no developer will ever see that its default in the doctrine bundle is disabled,
And they don't have to care what the default is. The recipe makes the option discoverable.
no question
That's kind of not how I personally exchange arguments, but you do you.
At the very least this will need a comment with a warning
No strong opinion here. We can certainly add a clarifying comment with a link to the docs.
That's kind of not how I personally exchange arguments, but you do you.
Not an excuse, but having to type this from a phone while traveling doesn't help. I adjusted it to what it should have been 👍🏻
And they don't have to care what the default is.
True, but you should be able to rely on it having a value that is working correctly for all use cases, which is not the case when it has been set to true (as shown earlier).
The recipe makes the option discoverable.
Fair enough, but that was already the case before this proposed change.
I think we should take a step back here, and think about what the real issue is. Is it bad DX to default the developers to not rely on "broken" auto mapping, or is it bad DX because we changed the default and it now works differently for new projects/projects that have updated their recipes?
a value that is working correctly for all use cases
If that one-size-fits-all setting would exist, we wouldn't make it configurable.
"broken" auto mapping
This is probably the point where we disagree. The auto mapping feature is not "broken" per se. It's a DX feature that can be turned off if you need more control over how requests are mapped to entity identifiers.
Reverts the setting set in #1299
I'm not sure this is the solution we want to merge, but I'm sure the recent changes have not been made with DX in mind, and they're making the experience worse (see comments in https://github.com/symfony/recipes/pull/1299)
I'm kindly asking to redesign this change with DX in mind. :pray: If we don't have any better idea yet, I suggest reverting the change on DoctrineBundle until we have a DX-friendly solution.