Enable entity automapping for controllers again #1302
Conversation
|
Thanks for the PR 😍 How to test these changes in your application
Diff between recipe versionsIn order to help with the review stage, I'm in charge of computing the diff between the various versions of patched recipes. doctrine/doctrine-bundle1.6 vs 1.12diff --git a/doctrine/doctrine-bundle/1.6/config/packages/doctrine.yaml b/doctrine/doctrine-bundle/1.12/config/packages/doctrine.yaml
index 2f611de..30d710d 100644
--- a/doctrine/doctrine-bundle/1.6/config/packages/doctrine.yaml
+++ b/doctrine/doctrine-bundle/1.12/config/packages/doctrine.yaml
@@ -10,9 +10,12 @@ doctrine:
charset: utf8mb4
default_table_options:
collate: utf8mb4_unicode_ci
+
+ # backtrace queries in profiler (increases memory usage per request)
+ #profiling_collect_backtrace: '%kernel.debug%'
orm:
auto_generate_proxy_classes: true
- naming_strategy: doctrine.orm.naming_strategy.underscore
+ naming_strategy: doctrine.orm.naming_strategy.underscore_number_aware
auto_mapping: true
mappings:
App:
diff --git a/doctrine/doctrine-bundle/1.6/config/packages/prod/doctrine.yaml b/doctrine/doctrine-bundle/1.12/config/packages/prod/doctrine.yaml
index 0a7c53b..084f59a 100644
--- a/doctrine/doctrine-bundle/1.6/config/packages/prod/doctrine.yaml
+++ b/doctrine/doctrine-bundle/1.12/config/packages/prod/doctrine.yaml
@@ -2,26 +2,14 @@ doctrine:
orm:
auto_generate_proxy_classes: false
metadata_cache_driver:
- type: service
- id: doctrine.system_cache_provider
+ type: pool
+ pool: doctrine.system_cache_pool
query_cache_driver:
- type: service
- id: doctrine.system_cache_provider
+ type: pool
+ pool: doctrine.system_cache_pool
result_cache_driver:
- type: service
- id: doctrine.result_cache_provider
-
-services:
- doctrine.result_cache_provider:
- class: Symfony\Component\Cache\DoctrineProvider
- public: false
- arguments:
- - '@doctrine.result_cache_pool'
- doctrine.system_cache_provider:
- class: Symfony\Component\Cache\DoctrineProvider
- public: false
- arguments:
- - '@doctrine.system_cache_pool'
+ type: pool
+ pool: doctrine.result_cache_pool
framework:
cache:
diff --git a/doctrine/doctrine-bundle/1.6/manifest.json b/doctrine/doctrine-bundle/1.12/manifest.json
index 04da3d1..2d3a40a 100644
--- a/doctrine/doctrine-bundle/1.6/manifest.json
+++ b/doctrine/doctrine-bundle/1.12/manifest.json
@@ -11,8 +11,8 @@
"#2": "IMPORTANT: You MUST configure your server version, either here or in config/packages/doctrine.yaml",
"#3": "",
"#4": "DATABASE_URL=\"sqlite:///%kernel.project_dir%/var/data.db\"",
- "#5": "DATABASE_URL=\"mysql://app:!ChangeMe!@127.0.0.1:3306/app?serverVersion=8\"",
- "DATABASE_URL": "postgresql://app:!ChangeMe!@127.0.0.1:5432/app?serverVersion=16&charset=utf8"
+ "#5": "DATABASE_URL=\"mysql://app:!ChangeMe!@127.0.0.1:3306/db_name?serverVersion=8\"",
+ "DATABASE_URL": "postgresql://app:!ChangeMe!@127.0.0.1:5432/db_name?serverVersion=16&charset=utf8"
},
"dockerfile": [
"RUN apk add --no-cache --virtual .pgsql-deps postgresql-dev && \\",1.12 vs 2.0diff --git a/doctrine/doctrine-bundle/1.12/config/packages/doctrine.yaml b/doctrine/doctrine-bundle/2.0/config/packages/doctrine.yaml
index 30d710d..365fef1 100644
--- a/doctrine/doctrine-bundle/1.12/config/packages/doctrine.yaml
+++ b/doctrine/doctrine-bundle/2.0/config/packages/doctrine.yaml
@@ -5,14 +5,7 @@ doctrine:
# IMPORTANT: You MUST configure your server version,
# either here or in the DATABASE_URL env var (see .env file)
#server_version: '16'
-
- # only needed for MySQL
- charset: utf8mb4
- default_table_options:
- collate: utf8mb4_unicode_ci
-
- # backtrace queries in profiler (increases memory usage per request)
- #profiling_collect_backtrace: '%kernel.debug%'
+ use_savepoints: true
orm:
auto_generate_proxy_classes: true
naming_strategy: doctrine.orm.naming_strategy.underscore_number_aware
diff --git a/doctrine/doctrine-bundle/1.12/manifest.json b/doctrine/doctrine-bundle/2.0/manifest.json
index 2d3a40a..04da3d1 100644
--- a/doctrine/doctrine-bundle/1.12/manifest.json
+++ b/doctrine/doctrine-bundle/2.0/manifest.json
@@ -11,8 +11,8 @@
"#2": "IMPORTANT: You MUST configure your server version, either here or in config/packages/doctrine.yaml",
"#3": "",
"#4": "DATABASE_URL=\"sqlite:///%kernel.project_dir%/var/data.db\"",
- "#5": "DATABASE_URL=\"mysql://app:!ChangeMe!@127.0.0.1:3306/db_name?serverVersion=8\"",
- "DATABASE_URL": "postgresql://app:!ChangeMe!@127.0.0.1:5432/db_name?serverVersion=16&charset=utf8"
+ "#5": "DATABASE_URL=\"mysql://app:!ChangeMe!@127.0.0.1:3306/app?serverVersion=8\"",
+ "DATABASE_URL": "postgresql://app:!ChangeMe!@127.0.0.1:5432/app?serverVersion=16&charset=utf8"
},
"dockerfile": [
"RUN apk add --no-cache --virtual .pgsql-deps postgresql-dev && \\",2.0 vs 2.3diff --git a/doctrine/doctrine-bundle/2.0/config/packages/prod/doctrine.yaml b/doctrine/doctrine-bundle/2.3/config/packages/prod/doctrine.yaml
index 084f59a..17299e2 100644
--- a/doctrine/doctrine-bundle/2.0/config/packages/prod/doctrine.yaml
+++ b/doctrine/doctrine-bundle/2.3/config/packages/prod/doctrine.yaml
@@ -1,9 +1,6 @@
doctrine:
orm:
auto_generate_proxy_classes: false
- metadata_cache_driver:
- type: pool
- pool: doctrine.system_cache_pool
query_cache_driver:
type: pool
pool: doctrine.system_cache_pool
diff --git a/doctrine/doctrine-bundle/2.3/config/packages/test/doctrine.yaml b/doctrine/doctrine-bundle/2.3/config/packages/test/doctrine.yaml
new file mode 100644
index 0000000..2ace640
--- /dev/null
+++ b/doctrine/doctrine-bundle/2.3/config/packages/test/doctrine.yaml
@@ -0,0 +1,4 @@
+doctrine:
+ dbal:
+ # "TEST_TOKEN" is typically set by ParaTest
+ dbname: 'main_test%env(default::TEST_TOKEN)%'
diff --git a/doctrine/doctrine-bundle/2.0/manifest.json b/doctrine/doctrine-bundle/2.3/manifest.json
index 04da3d1..26e52b9 100644
--- a/doctrine/doctrine-bundle/2.0/manifest.json
+++ b/doctrine/doctrine-bundle/2.3/manifest.json
@@ -11,13 +11,13 @@
"#2": "IMPORTANT: You MUST configure your server version, either here or in config/packages/doctrine.yaml",
"#3": "",
"#4": "DATABASE_URL=\"sqlite:///%kernel.project_dir%/var/data.db\"",
- "#5": "DATABASE_URL=\"mysql://app:!ChangeMe!@127.0.0.1:3306/app?serverVersion=8\"",
+ "#5": "DATABASE_URL=\"mysql://app:!ChangeMe!@127.0.0.1:3306/app?serverVersion=8&charset=utf8mb4\"",
"DATABASE_URL": "postgresql://app:!ChangeMe!@127.0.0.1:5432/app?serverVersion=16&charset=utf8"
},
"dockerfile": [
- "RUN apk add --no-cache --virtual .pgsql-deps postgresql-dev && \\",
- "\tdocker-php-ext-install -j$(nproc) pdo_pgsql && \\",
- "\tapk add --no-cache --virtual .pgsql-rundeps so:libpq.so.5 && \\",
+ "RUN apk add --no-cache --virtual .pgsql-deps postgresql-dev; \\",
+ "\tdocker-php-ext-install -j$(nproc) pdo_pgsql; \\",
+ "\tapk add --no-cache --virtual .pgsql-rundeps so:libpq.so.5; \\",
"\tapk del .pgsql-deps"
],
"docker-compose": {2.3 vs 2.4diff --git a/doctrine/doctrine-bundle/2.3/config/packages/doctrine.yaml b/doctrine/doctrine-bundle/2.4/config/packages/doctrine.yaml
index 365fef1..e517e07 100644
--- a/doctrine/doctrine-bundle/2.3/config/packages/doctrine.yaml
+++ b/doctrine/doctrine-bundle/2.4/config/packages/doctrine.yaml
@@ -13,7 +13,32 @@ doctrine:
mappings:
App:
is_bundle: false
- type: annotation
dir: '%kernel.project_dir%/src/Entity'
prefix: 'App\Entity'
alias: App
+
+when@test:
+ doctrine:
+ dbal:
+ # "TEST_TOKEN" is typically set by ParaTest
+ dbname_suffix: '_test%env(default::TEST_TOKEN)%'
+
+when@prod:
+ doctrine:
+ orm:
+ auto_generate_proxy_classes: false
+ proxy_dir: '%kernel.build_dir%/doctrine/orm/Proxies'
+ query_cache_driver:
+ type: pool
+ pool: doctrine.system_cache_pool
+ result_cache_driver:
+ type: pool
+ pool: doctrine.result_cache_pool
+
+ framework:
+ cache:
+ pools:
+ doctrine.result_cache_pool:
+ adapter: cache.app
+ doctrine.system_cache_pool:
+ adapter: cache.system
diff --git a/doctrine/doctrine-bundle/2.3/config/packages/prod/doctrine.yaml b/doctrine/doctrine-bundle/2.3/config/packages/prod/doctrine.yaml
deleted file mode 100644
index 17299e2..0000000
--- a/doctrine/doctrine-bundle/2.3/config/packages/prod/doctrine.yaml
+++ /dev/null
@@ -1,17 +0,0 @@
-doctrine:
- orm:
- auto_generate_proxy_classes: false
- query_cache_driver:
- type: pool
- pool: doctrine.system_cache_pool
- result_cache_driver:
- type: pool
- pool: doctrine.result_cache_pool
-
-framework:
- cache:
- pools:
- doctrine.result_cache_pool:
- adapter: cache.app
- doctrine.system_cache_pool:
- adapter: cache.system
diff --git a/doctrine/doctrine-bundle/2.3/config/packages/test/doctrine.yaml b/doctrine/doctrine-bundle/2.3/config/packages/test/doctrine.yaml
deleted file mode 100644
index 2ace640..0000000
--- a/doctrine/doctrine-bundle/2.3/config/packages/test/doctrine.yaml
+++ /dev/null
@@ -1,4 +0,0 @@
-doctrine:
- dbal:
- # "TEST_TOKEN" is typically set by ParaTest
- dbname: 'main_test%env(default::TEST_TOKEN)%'
diff --git a/doctrine/doctrine-bundle/2.3/manifest.json b/doctrine/doctrine-bundle/2.4/manifest.json
index 26e52b9..88bac7a 100644
--- a/doctrine/doctrine-bundle/2.3/manifest.json
+++ b/doctrine/doctrine-bundle/2.4/manifest.json
@@ -44,5 +44,8 @@
" - \"5432\""
]
}
+ },
+ "conflict": {
+ "symfony/framework-bundle": "<5.3"
}
}2.4 vs 2.8diff --git a/doctrine/doctrine-bundle/2.4/config/packages/doctrine.yaml b/doctrine/doctrine-bundle/2.8/config/packages/doctrine.yaml
index e517e07..c00894c 100644
--- a/doctrine/doctrine-bundle/2.4/config/packages/doctrine.yaml
+++ b/doctrine/doctrine-bundle/2.8/config/packages/doctrine.yaml
@@ -8,6 +8,7 @@ doctrine:
use_savepoints: true
orm:
auto_generate_proxy_classes: true
+ enable_lazy_ghost_objects: true
naming_strategy: doctrine.orm.naming_strategy.underscore_number_aware
auto_mapping: true
mappings:
diff --git a/doctrine/doctrine-bundle/2.4/manifest.json b/doctrine/doctrine-bundle/2.8/manifest.json
index 88bac7a..addfd20 100644
--- a/doctrine/doctrine-bundle/2.4/manifest.json
+++ b/doctrine/doctrine-bundle/2.8/manifest.json
@@ -11,12 +11,13 @@
"#2": "IMPORTANT: You MUST configure your server version, either here or in config/packages/doctrine.yaml",
"#3": "",
"#4": "DATABASE_URL=\"sqlite:///%kernel.project_dir%/var/data.db\"",
- "#5": "DATABASE_URL=\"mysql://app:!ChangeMe!@127.0.0.1:3306/app?serverVersion=8&charset=utf8mb4\"",
+ "#5": "DATABASE_URL=\"mysql://app:!ChangeMe!@127.0.0.1:3306/app?serverVersion=8.0.32&charset=utf8mb4\"",
+ "#6": "DATABASE_URL=\"mysql://app:!ChangeMe!@127.0.0.1:3306/app?serverVersion=10.11.2-MariaDB&charset=utf8mb4\"",
"DATABASE_URL": "postgresql://app:!ChangeMe!@127.0.0.1:5432/app?serverVersion=16&charset=utf8"
},
"dockerfile": [
"RUN apk add --no-cache --virtual .pgsql-deps postgresql-dev; \\",
- "\tdocker-php-ext-install -j$(nproc) pdo_pgsql; \\",
+ "\tdocker-php-ext-install -j\"$(nproc)\" pdo_pgsql; \\",
"\tapk add --no-cache --virtual .pgsql-rundeps so:libpq.so.5; \\",
"\tapk del .pgsql-deps"
],
@@ -46,6 +47,8 @@
}
},
"conflict": {
+ "doctrine/orm": "<2.14",
+ "symfony/dependency-injection": "<6.2",
"symfony/framework-bundle": "<5.3"
}
}2.8 vs 2.9diff --git a/doctrine/doctrine-bundle/2.8/manifest.json b/doctrine/doctrine-bundle/2.9/manifest.json
index addfd20..1d8996a 100644
--- a/doctrine/doctrine-bundle/2.8/manifest.json
+++ b/doctrine/doctrine-bundle/2.9/manifest.json
@@ -16,10 +16,7 @@
"DATABASE_URL": "postgresql://app:!ChangeMe!@127.0.0.1:5432/app?serverVersion=16&charset=utf8"
},
"dockerfile": [
- "RUN apk add --no-cache --virtual .pgsql-deps postgresql-dev; \\",
- "\tdocker-php-ext-install -j\"$(nproc)\" pdo_pgsql; \\",
- "\tapk add --no-cache --virtual .pgsql-rundeps so:libpq.so.5; \\",
- "\tapk del .pgsql-deps"
+ "RUN install-php-extensions pdo_pgsql"
],
"docker-compose": {
"docker-compose.yml": {2.9 vs 2.10diff --git a/doctrine/doctrine-bundle/2.9/config/packages/doctrine.yaml b/doctrine/doctrine-bundle/2.10/config/packages/doctrine.yaml
index c00894c..d42c52d 100644
--- a/doctrine/doctrine-bundle/2.9/config/packages/doctrine.yaml
+++ b/doctrine/doctrine-bundle/2.10/config/packages/doctrine.yaml
@@ -5,14 +5,19 @@ doctrine:
# IMPORTANT: You MUST configure your server version,
# either here or in the DATABASE_URL env var (see .env file)
#server_version: '16'
+
+ profiling_collect_backtrace: '%kernel.debug%'
use_savepoints: true
orm:
auto_generate_proxy_classes: true
enable_lazy_ghost_objects: true
+ report_fields_where_declared: true
+ validate_xml_mapping: true
naming_strategy: doctrine.orm.naming_strategy.underscore_number_aware
auto_mapping: true
mappings:
App:
+ type: attribute
is_bundle: false
dir: '%kernel.project_dir%/src/Entity'
prefix: 'App\Entity'2.10 vs 2.12diff --git a/doctrine/doctrine-bundle/2.10/config/packages/doctrine.yaml b/doctrine/doctrine-bundle/2.12/config/packages/doctrine.yaml
index d42c52d..75ec9e8 100644
--- a/doctrine/doctrine-bundle/2.10/config/packages/doctrine.yaml
+++ b/doctrine/doctrine-bundle/2.12/config/packages/doctrine.yaml
@@ -22,6 +22,8 @@ doctrine:
dir: '%kernel.project_dir%/src/Entity'
prefix: 'App\Entity'
alias: App
+ controller_resolver:
+ auto_mapping: true
when@test:
doctrine: |
|
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. |
|
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". 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. I my opinion it makes sense to use the |
|
@ebitkov the issue is how to guess the fact that you want to use the |
|
@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 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? |
There was a problem hiding this comment.
I agree that we should merge this PR, although I still believe that DoctrineBundle is on the right track.
The auto mapping feature is background magic that can have surprising side effects as others have elaborated already. Turning the feature off by default (which is what doctrine/DoctrineBundle#1762 is prepared) is the safe option.
By enabling the feature in the recipe, we improve the developer experience for new projects. At the same time, we give developers a very obvious way to disable the feature, should it ever cause trouble for them.
There was a problem hiding this comment.
If the recipe enables the feature by default, no developer will ever see that its default in the doctrine bundle is disabled, untill it hits the unwanted behaviour already described causing them to look for this. Having this option enabled in the recipe is therefore bad DX, that's no question for me.
If you do read the documentation (as you should), and you want to use this while being informed as to how it works (which the documentation in its current state does not), you can enable it.
At the very least this will need a comment with a warning regarding the bad behaviour of the auto mapping feature, leading the developer to the documentation update that I have prepared for this: symfony/symfony-docs#19671.
And they don't have to care what the default is. The recipe makes the option discoverable.
That's kind of not how I personally exchange arguments, but you do you.
No strong opinion here. We can certainly add a clarifying comment with a link to the docs. |
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 👍🏻
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).
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? |
If that one-size-fits-all setting would exist, we wouldn't make it configurable.
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 #1299)
I'm kindly asking to redesign this change with DX in mind. 🙏 If we don't have any better idea yet, I suggest reverting the change on DoctrineBundle until we have a DX-friendly solution.