Add DocumentValueResolver and MapDocument #774

franmomu · 2023-06-29T07:45:55Z

Alternative to #773

Since we cannot use EntityValueResolver and MapEntity, this is an alternative to make it work.

The problem is that since Symfony 6.3, because targeted resolvers, MapEntity cannot be used when using ORM + ODM.

Closes #770
Closes #773

GromNaN · 2023-06-29T10:08:07Z

What is the actual issue that requires a dedicated DocumentValueResolver? In your proposition, this resolver is a proxy to EntityValueResolver and does nothing different.

The #[MapEntity] works as expected in my demo project on Doctrine ODM.
https://github.com/GromNaN/symfony-demo/blob/60eb116cf2b6a0f9acc0bc6591d70f4a4c9f2c69/src/Controller/BlogController.php#L103

Is it to allow using both ORM and ODM in the same project and avoiding the autowiring alias on Doctrine\Persistence\ManagerRegistry: '@doctrine_mongodb' :
https://github.com/GromNaN/symfony-demo/blob/60eb116cf2b6a0f9acc0bc6591d70f4a4c9f2c69/config/services.yaml#L40

franmomu · 2023-06-29T10:30:41Z

Is it to allow using both ORM and ODM in the same project and avoiding the autowiring alias on Doctrine\Persistence\ManagerRegistry: '@doctrine_mongodb' : https://github.com/GromNaN/symfony-demo/blob/60eb116cf2b6a0f9acc0bc6591d70f4a4c9f2c69/config/services.yaml#L40

Exactly, the problem is having both ORM and ODM and since MapEntity is tied to EntityValueResolver by its FQCN I couldn't come up with something better 😞

GromNaN · 2023-06-29T10:48:51Z

An alternative could be to have a ManagerRegistry aggregator embedding both ORM and ODM implementations that can be used whether it's an entity or a document.

All in all, I find your approach interesting because it avoids having to use "entity" word for "document". And it doesn't require a lot of specific code.

GromNaN · 2023-06-29T13:45:44Z

Resources/config/mongodb.xml

@@ -208,13 +208,5 @@
        <service id="doctrine_mongodb.odm.symfony.fixtures.loader" class="Doctrine\Bundle\MongoDBBundle\Loader\SymfonyFixturesLoader" public="false">
            <argument type="service" id="service_container" />
        </service>
-
-        <service id="doctrine_mongodb.odm.entity_value_resolver" class="Symfony\Bridge\Doctrine\ArgumentResolver\EntityValueResolver">


It looks like a breaking change if someone is already using the MapEntity attribute with the ODM. Not a big deal but it must be documented in the UPGRADE file.

hmmm not sure about the BC break, these services were added in 4.6.x and never been released, so if someone is using it is because EntityValueResolver was registered as a service manually, in that case I guess it will continue working fine.

and never been released

We're fine changing logic given this argument :)

boskos-q · 2023-07-03T10:44:25Z

composer.json

        "symfony/stopwatch": "^5.4|^6.2",
        "symfony/validator": "^5.4|^6.2",


Suggested change

"symfony/stopwatch": "^5.4|^6.2",

"symfony/validator": "^5.4|^6.2",

"symfony/stopwatch": "^5.4|^6.3",

"symfony/validator": "^5.4|^6.3",

I don't want to rush ahead of time, but sf 6.2 is supported till the end of July. Maybe to bump version to ^6.3, tho this can also be done later on as well ;)

thanks for the suggestion! but yeah, better to do that in another PR later

malarzm

Would it be possible to have few functional tests here in the bundle? Just so we know that decorating original classes still works

malarzm · 2023-07-17T19:36:41Z

Resources/doc/first_steps.rst

+    }
+
+That's it! The bundle uses the ``{id}`` from the route to query for the ``Product``
+by the ``id`` column. If it's not found, a 404 page is generated.


Suggested change

by the ``id`` column. If it's not found, a 404 page is generated.

by the ``id`` field. If it's not found, a 404 page is generated.

malarzm · 2023-07-17T19:38:20Z

Resources/doc/first_steps.rst

+Fetch Automatically
+~~~~~~~~~~~~~~~~~~~
+
+If your route wildcards match properties on your document, then the resolver


Suggested change

If your route wildcards match properties on your document, then the resolver

If your route wildcards match properties in your document, then the resolver

not 100% about this one, but on sounds weird when translating in my head :)

malarzm · 2023-07-17T19:39:33Z

Resources/doc/first_steps.rst

+     * Fetch via primary key because {id} is in the route.
+     */
+    #[Route('/product/{id}')]
+    public function showByPk(Post $post): Response


MongoDB does not have a primary key concept, should we just say identifier or is there more to itfunctionality-wise?

The method name still uses Pk part :)

Oops, thanks! fixed

malarzm · 2023-07-17T19:42:06Z

Resources/doc/first_steps.rst

+
+``id``
+    If an ``id`` option is configured and matches a route parameter, then
+    the resolver will find by the primary key:


Suggested change

the resolver will find by the primary key:

the resolver will find by the identifier:

I've seen primary key in previous chapter as well, best to check for it globally

malarzm · 2023-07-17T19:44:18Z

Resources/doc/first_steps.rst

+    ``null`` will not be used for the query.
+
+``objectManager``
+    By default, the ``DocumentValueResolver`` uses the *default*


Not sure how this works in ORM but I'd expect passing entity/document to a ManagerRegistry to figure out which object manager should be used by default?

https://github.com/symfony/symfony/blob/503a7b3cb62fb6de70176b07bd1c4242e3addc5b/src/Symfony/Bridge/Doctrine/ArgumentResolver/EntityValueResolver.php#L82-L95 I think you mean this, if no parameter is passed, it does what you said.

Cool, so the feature works as I've expected it's just the wording that confused me as default is often name of a default OM. How about

Suggested change

By default, the ``DocumentValueResolver`` uses the *default*

By default, the ``DocumentValueResolver`` will choose DocumentManager that has the class registered for it

or maybe we have something better for when we're talking about multiple document managers? We rarely do (or at least did when I was working in such environment) so I'm not sure

Changed to what you suggested, I haven't found any text in the docs, I think it is fine now and more understandable, thanks

malarzm · 2023-07-17T19:45:18Z

Resources/doc/first_steps.rst

+
+``evictCache``
+    If true, forces Doctrine to always fetch the document from the database
+    instead of cache.


ODM does not provide native cache solutions, I believe we shouldn't be documenting (or exposing) the functionality for now

Yes, you're right, I've removed the documentation about this and cannot be configured

malarzm

Thanks for introducing full-blown functional tests! 🎉

malarzm · 2023-07-25T18:08:10Z

Resources/config/value_resolver.xml

+    xsi:schemaLocation="http://symfony.com/schema/dic/services http://symfony.com/schema/dic/services/services-1.0.xsd">
+
+    <services>
+        <service id="doctrine_mongodb.odm.entity_value_resolver" class="Symfony\Bridge\Doctrine\ArgumentResolver\EntityValueResolver">


Should this and other services use document instead of entity?

I've changed the other ones, but not sure about this one since it's for the EntityValueResolver

malarzm · 2023-07-25T18:10:15Z

Resources/doc/first_steps.rst

+     * Fetch via primary key because {id} is in the route.
+     */
+    #[Route('/product/{id}')]
+    public function showByPk(Post $post): Response


The method name still uses Pk part :)

malarzm · 2023-07-25T18:12:05Z

Tests/DocumentValueResolverFunctionalTest.php

+
+/**
+ * @requires PHP 8.0
+ * @requires function \Symfony\Bridge\Doctrine\Attribute\MapEntity::__construct


oh this is cool, didn't know one can use @requires this way!

https://docs.phpunit.de/en/9.6/incomplete-and-skipped-tests.html#incomplete-and-skipped-tests-requires-tables-api yeah, never used before to be honest 😬

pepeh · 2023-09-28T11:22:34Z

ArgumentResolver/DocumentValueResolver.php

+use Symfony\Component\HttpKernel\Controller\ValueResolverInterface;
+use Symfony\Component\HttpKernel\ControllerMetadata\ArgumentMetadata;
+
+/** @internal */


Why It's marked as @internal if the original EntityValueResolver is not @internal?

Hi @pepeh, in this case we provide the feature of fetching objects from the controller automatically, I decided to mark this class as @internal since to me it was an implementation detail, if we could use the EntityValueResolver, we could remove this class.

Are you using this class directly?

@franmomu there is option to specify value resolver manually
#[ValueResolver(DocumentValueResolver::class)]
https://symfony.com/doc/current/controller/value_resolver.html#managing-value-resolvers
But if class is @internal IDE will show warning

franmomu added the Feature label Jun 29, 2023

franmomu added this to the 4.6.0 milestone Jun 29, 2023

GromNaN reviewed Jun 29, 2023

View reviewed changes

franmomu force-pushed the targeted_resolvers branch 3 times, most recently from 9163f3c to 29a9c78 Compare July 1, 2023 12:53

boskos-q approved these changes Jul 3, 2023

View reviewed changes

boskos-q reviewed Jul 3, 2023

View reviewed changes

franmomu requested review from alcaeus, IonBazan and malarzm July 10, 2023 06:50

malarzm reviewed Jul 17, 2023

View reviewed changes

franmomu force-pushed the targeted_resolvers branch 4 times, most recently from ee43b7c to c40ef51 Compare July 23, 2023 18:56

franmomu mentioned this pull request Jul 23, 2023

Allow doctrine/annotations 2 #767

Merged

franmomu force-pushed the targeted_resolvers branch from c40ef51 to 8a9aba6 Compare July 23, 2023 19:03

malarzm approved these changes Jul 25, 2023

View reviewed changes

franmomu force-pushed the targeted_resolvers branch from 8a9aba6 to 481ee19 Compare July 26, 2023 08:36

franmomu added 2 commits July 26, 2023 14:16

Add Symfony 6.3 to the test matrix

038e742

Introduce DocumentValueResolver and MapDocument

0492beb

franmomu force-pushed the targeted_resolvers branch from 481ee19 to 0492beb Compare July 26, 2023 12:18

franmomu merged commit 294b0fb into doctrine:4.6.x Jul 26, 2023

franmomu deleted the targeted_resolvers branch July 26, 2023 13:41

pepeh reviewed Sep 28, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add DocumentValueResolver and MapDocument #774

Add DocumentValueResolver and MapDocument #774

franmomu commented Jun 29, 2023 •

edited

Loading

GromNaN commented Jun 29, 2023

franmomu commented Jun 29, 2023

GromNaN commented Jun 29, 2023 •

edited

Loading

GromNaN Jun 29, 2023 •

edited

Loading

franmomu Jul 1, 2023

malarzm Jul 17, 2023

boskos-q Jul 3, 2023 •

edited

Loading

franmomu Jul 26, 2023

malarzm left a comment

malarzm Jul 17, 2023

malarzm Jul 17, 2023

malarzm Jul 17, 2023

malarzm Jul 25, 2023

franmomu Jul 26, 2023

malarzm Jul 17, 2023

malarzm Jul 17, 2023

franmomu Jul 23, 2023

malarzm Jul 25, 2023

franmomu Jul 26, 2023

malarzm Jul 17, 2023

franmomu Jul 23, 2023

malarzm left a comment

malarzm Jul 25, 2023

franmomu Jul 26, 2023

malarzm Jul 25, 2023

malarzm Jul 25, 2023

franmomu Jul 26, 2023

pepeh Sep 28, 2023

franmomu Sep 29, 2023

pepeh Oct 1, 2023

		"symfony/stopwatch": "^5.4\|^6.2",
		"symfony/validator": "^5.4\|^6.2",

	by the ``id`` column. If it's not found, a 404 page is generated.
	by the ``id`` field. If it's not found, a 404 page is generated.

	If your route wildcards match properties on your document, then the resolver
	If your route wildcards match properties in your document, then the resolver

	the resolver will find by the primary key:
	the resolver will find by the identifier:

	By default, the ``DocumentValueResolver`` uses the default
	By default, the ``DocumentValueResolver`` will choose DocumentManager that has the class registered for it

Add DocumentValueResolver and MapDocument #774

Add DocumentValueResolver and MapDocument #774

Conversation

franmomu commented Jun 29, 2023 • edited Loading

GromNaN commented Jun 29, 2023

franmomu commented Jun 29, 2023

GromNaN commented Jun 29, 2023 • edited Loading

GromNaN Jun 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boskos-q Jul 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malarzm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

malarzm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franmomu commented Jun 29, 2023 •

edited

Loading

GromNaN commented Jun 29, 2023 •

edited

Loading

GromNaN Jun 29, 2023 •

edited

Loading

boskos-q Jul 3, 2023 •

edited

Loading