Rework of Doctrine ItemDataProvider #458

theofidry · 2016-03-12T22:52:52Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	no
Fixed tickets	#457
License	MIT
Doc PR	none

The objective of this PR is to:

Rework the code to make it a bit more explicit and declarative
Tackle the concerns risen by Review of Doctrine ItemDataProvider #457

theofidry · 2016-03-12T22:53:51Z

src/Bridge/Doctrine/Orm/ItemDataProvider.php

+        }
+
+        if ($resourceManager instanceof EntityManagerInterface) {
+            // TODO: check if $resourceClass is good here


TODO to be checked: @dunglas you're probably more familiar than me on that one

could also check against the method, not sure

theofidry · 2016-03-16T11:48:36Z

In blocked state:

waiting for input on Rework of Doctrine ItemDataProvider #458 (comment)
waiting on Upgrade to reflection-docblock 3 phpspec/prophecy#263 for tests

theofidry · 2016-03-16T12:41:04Z

src/Bridge/Doctrine/Orm/ItemDataProvider.php

+    {
+        $repository = $resourceManager->getRepository($resourceClass);
+
+        if ($repository instanceof  EntityRepository) {


@dunglas should I check against EntityRepository or try if it has the createQueryBuilder() method?

Prefer class_exists it will allow custom implementations to work too.

theofidry · 2016-03-19T13:04:55Z

ping @dunglas updated the querybuilder part, tell me if it's good for you and I'll add unit tests afterwards

soyuka · 2016-03-23T13:37:57Z

@theofidry I made a use case that broke in v1 due to composite identifiers, feel free to use the behat test case I wrote :).

theofidry · 2016-03-23T13:42:09Z

@soyuka neat, will look into it as soon as I can :)

soyuka · 2016-03-23T13:47:54Z

src/Bridge/Doctrine/Orm/ItemDataProvider.php

     * @return \Doctrine\ORM\QueryBuilder
     */
    private function retrieveQueryBuilder(ObjectManager $resourceManager, string $resourceClass)
    {
        $repository = $resourceManager->getRepository($resourceClass);

-        if ($repository instanceof  EntityRepository) {
+        if ($repository instanceof EntityRepository) {
            return $repository->createQueryBuilder('o');
        }

        if ($resourceManager instanceof EntityManagerInterface) {


Can't you add an or condition to avoid code duplication below? (same goes for the repository method_exists).

not really because the order matter here:

try with an EntityRepository: optimal case

try with a EntityManagerInterface: other optimal case, but requires to do things differently

fallback to check if the repository still have this createQueryBuilder() method, and assumes it's the right one (technically there is no guarantee here)

last fallback with the manager before throwing an error

I find this hack pretty ugly but maybe I missed something, it's been some time since I used Doctrine, would be happy to find a better solution

Hmm I don't understand why EntityManagerInterface would take priority upon EntityRepository if it has a createQueryBuilder method.

if ($repository instanceof EntityRepository || method_exists($repository, 'createQueryBuilder')) { return $repository->createQueryBuilder('o'); } if ($resourceManager instanceof EntityManagerInterface || method_exists($resourceManager, 'createQueryBuilder')) { return $resourceManager->createQueryBuilder() ->select('o') ->from($resourceClass, 'o'); } throw new RuntimeException();

I'd add that I find it weird to test that the $resourceManager has the createQueryBuilder, especially if it's not implementing the EntityManagerInterface. IMO this will almost always be false, what's the edge case that I'm missing?

IMO there is no better way of returning a query builder with a select clause on the correct entity, and it's a fine way of doing it.

Hmm I don't understand why EntityManagerInterface would take priority upon EntityRepository if it has a createQueryBuilder method.

My thinking was trying to get the most reliable solution first, assuming that the EntityReopsitory::createQueryBuilder() is right is a baseless assumption with no means to check if it's true. At least with the EntityManagerInterface it's guaranteed to work.

I'd add that I find it weird to test that the $resourceManager has the createQueryBuilder, especially if it's not implementing the EntityManagerInterface. IMO this will almost always be false, what's the edge case that I'm missing?

$resourceManager could simply be a ObjectManager right?

IMO there is no better way of returning a query builder with a select clause on the correct entity, and it's a fine way of doing it.

I don't have any strong feeling on this, but if you and the others find it better I would be happy to switch to that instead

$resourceManager could simply be a ObjectManager right?

Indeed, and the ObjectManager doesn't provide the createQueryBuilder method. EntityManagerInterface extends ObjectManager, and the createQueryBuilder method belongs to the EntityManagerInterface.

As I'm no Doctrine expert, I've no further opinion on this. Hope others will :).

Well, we got someone who wrote a book on it right @dunglas?

I've read one more time and it does make sense in some way :). Does this patch also applies to the CollectionDataProvider?

theofidry · 2016-03-30T07:18:51Z

ping @dunglas

theofidry · 2016-04-06T11:53:29Z

@api-platform/core-team there is one last point for which I'm not sure of the behaviour we should adopt, see the discussion with soyuka. Could you check that so that I fix this and rebase this PR?

theofidry · 2016-04-14T22:51:09Z

ping @api-platform/core-team: there is just one part left, could you guys check it please?

soyuka · 2016-04-15T06:50:27Z

src/Bridge/Doctrine/Orm/ItemDataProvider.php


    /**
     * @param ManagerRegistry                        $managerRegistry
     * @param PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory
     * @param PropertyMetadataFactoryInterface       $propertyMetadataFactory
     * @param QueryItemExtensionInterface[]          $itemExtensions
-     * @param ItemDataProviderInterface|null         $decorated
+     * @param ItemDataProviderInterface|null         $decoratedProvider


We're naming this decorated in every other class, why change it here?

theofidry · 2016-04-23T16:27:17Z

Updated

theofidry · 2016-07-15T15:56:24Z

@dunglas is that still relevant? i.e. is it still worth to rebase it?

dunglas · 2016-08-19T22:36:11Z

@theofidry Honestly I don't know. WDYT?

theofidry · 2016-08-19T22:42:03Z

Let me check it when I'm back in London (Tuesday), any opinion before that
would not be very reasonable ^^

On Saturday, 20 August 2016, Kévin Dunglas notifications@github.com wrote:

@theofidry https://github.com/theofidry Honestly I don't know. WDYT?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#458 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AE76gUkGQ0wTou3A4AmCM6pAZlMB-QF5ks5qhi_cgaJpZM4HvaKr
.

soyuka · 2016-09-30T09:58:32Z

Maybe we can close this one?

theofidry · 2016-09-30T11:23:31Z

I don't really have time to take a look at it again so yeah let's do that. The problem may not be solved but I don't see any API change so that's something that could easily come as a patch if a problem were to occur.

theofidry reviewed Mar 12, 2016
View reviewed changes

theofidry reviewed Mar 16, 2016
View reviewed changes

theofidry force-pushed the doctrine-item-provider branch 3 times, most recently from 24d1fbc to 2239a39 Compare March 19, 2016 13:03

soyuka reviewed Mar 23, 2016
View reviewed changes

soyuka mentioned this pull request Mar 23, 2016

[WIP] Composite identifiers #387

Closed

soyuka reviewed Apr 15, 2016
View reviewed changes

theofidry added 3 commits April 23, 2016 15:38

WIP ~ Rework of Doctrine ItemDataProvider

a167a5f

Fix TODO for querybuilder

4ba8b6d

Rebased

6b6c5a8

theofidry force-pushed the doctrine-item-provider branch from 2239a39 to 6b6c5a8 Compare April 23, 2016 16:23

theofidry changed the title ~~WIP ~ Rework of Doctrine ItemDataProvider~~ Rework of Doctrine ItemDataProvider Apr 23, 2016

theofidry closed this Sep 30, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework of Doctrine ItemDataProvider #458

Rework of Doctrine ItemDataProvider #458

theofidry commented Mar 12, 2016 •

edited

theofidry Mar 12, 2016

theofidry Mar 16, 2016

theofidry commented Mar 16, 2016

theofidry Mar 16, 2016

dunglas Mar 16, 2016

theofidry commented Mar 19, 2016

soyuka commented Mar 23, 2016

theofidry commented Mar 23, 2016

soyuka Mar 23, 2016

theofidry Mar 23, 2016

theofidry Mar 23, 2016

soyuka Mar 23, 2016

theofidry Mar 23, 2016

soyuka Mar 23, 2016

theofidry Mar 23, 2016

soyuka Apr 15, 2016 •

edited

theofidry commented Mar 30, 2016

theofidry commented Apr 6, 2016

theofidry commented Apr 14, 2016

soyuka Apr 15, 2016

theofidry commented Apr 23, 2016

theofidry commented Jul 15, 2016

dunglas commented Aug 19, 2016

theofidry commented Aug 19, 2016

soyuka commented Sep 30, 2016

theofidry commented Sep 30, 2016

Rework of Doctrine ItemDataProvider #458

Rework of Doctrine ItemDataProvider #458

Conversation

theofidry commented Mar 12, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theofidry commented Mar 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

theofidry commented Mar 19, 2016

soyuka commented Mar 23, 2016

theofidry commented Mar 23, 2016

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

soyuka Apr 15, 2016 • edited

Choose a reason for hiding this comment

theofidry commented Mar 30, 2016

theofidry commented Apr 6, 2016

theofidry commented Apr 14, 2016

Choose a reason for hiding this comment

theofidry commented Apr 23, 2016

theofidry commented Jul 15, 2016

dunglas commented Aug 19, 2016

theofidry commented Aug 19, 2016

soyuka commented Sep 30, 2016

theofidry commented Sep 30, 2016

theofidry commented Mar 12, 2016 •

edited

soyuka Apr 15, 2016 •

edited