#530 ~ Add behat test and change behavior of ItemDataProvider for uuid #533

Simperfit · 2016-05-07T15:22:59Z

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#530
License	MIT

This is WIP

Edit : This is ok for review

Finish the test behat
Add more test for composite
Modify the behaviour of ItemDataProvider to test the composite identifiers and to make uuid work

Simperfit · 2016-05-07T17:20:14Z

ping @dunglas.
This PR is ready for review ( @theofidry @polc).

theofidry · 2016-05-07T17:24:57Z

features/composite.feature

  @createSchema
  @dropSchema
-  Scenario: Get collection with composite identifiers
+  Scenario: Get a compisite item with composite identifiers


theofidry · 2016-05-07T17:36:11Z

src/Bridge/Doctrine/Orm/ItemDataProvider.php

            $placeholder = 'id_'.$propertyName;
+            $whereExpr[$i] = $queryBuilder->expr()->eq('o.'.$propertyName, ':'.$placeholder);
+
+            if ($i <= 0) {


you could use an iterator here instead

Ill look at it

foreach ($identifiers as $propertyName => $value) { $placeholder = 'id_'.$propertyName; $whereExpr[] = $queryBuilder->expr()->eq('o.'.$propertyName, ':'.$placeholder); $whereExprObj = new \ArrayObject($whereExpr); var_dump($whereExprObj); $whereIterator = $whereExprObj->getIterator(); var_dump($whereIterator->key()); if ($whereIterator->key() <= 0) { $queryBuilder ->where($whereIterator->current()); } else { $queryBuilder ->andWhere($whereIterator->current()); } $queryBuilder->setParameter($placeholder, $value); $whereIterator->next(); }

I can't make it working since the array is populated at the same time that I test it keys.
Do you see something wrong in what i've tried to achieve ?

I was thinking of an interator on $identifiers rather

I've missunderstand that comment line ;(

no worries :)

Like this ?

theofidry · 2016-05-07T19:59:29Z

src/Bridge/Doctrine/Orm/ItemDataProvider.php

-                ->setParameter($placeholder, $value);
+            ++$i;
+            $identifiersObjIterator->next();
        }


The point of the iterator is to get rid of $i. You can easily have something more readable by putting that into a private function:

$this->processIdentifiers($identifiers, $queryBuilder); private function processIdentifiers(array $identifiers, QueryBuilder $queryBuilder) { if (0 === count($identifiers)) { return; } $iterator = (new \ArrayObject($identifiers))->getIterator(); $queryBuilder->where( $queryBuilder->expr()->eq( 'o.'.$identifiersObjIterator->key(), ':id_'.$identifiersObjIterator->key() ) ); $queryBuilder->setParameter($'id_'.$identifiersObjIterator->key(), $identifiersObjIterator->current()); $iterator->next(); while($iterator->valid()) { $queryBuilder->andWhere( $queryBuilder->expr()->eq( 'o.'.$identifiersObjIterator->key(), ':id_'.$identifiersObjIterator->key() ) ); $queryBuilder->setParameter('id_'.$identifiersObjIterator->key(), $identifiersObjIterator->current()); $iterator->next(); } }

but to be fair, you could have another version where you check just if you're on the first property or not:

private function processIdentifiers(array $identifiers, QueryBuilder $queryBuilder) { if (0 === count($identifiers)) { return; } $firstIdentifier = reset($identifiers); foreach($identifiers as $identifier => $value) { $placeholder = ':id_'.$identifier; $expression = $queryBuilder->expr()->eq( 'o.'.$identifier, ':'.$placeholder ); if ($firstIdentifier === $identifier) { $queryBuilder->where($expression); } else { $queryBuilder->andWhere($expression); } $queryBuilder->setParameter($placeholder, $value); } }

I will prefer the iterator version, it's readable and fun to use ;).

WDYT ?

It's a fun case to play with the iterator which is why I mentioned it, but I must admit I find the second version more readable and accessible.

Simperfit · 2016-05-07T20:35:15Z

Thanks for the help @theofidry !

polc · 2016-05-08T01:43:41Z

I didn't looked into the code but this patch is working on my project 👍

dunglas · 2016-05-08T08:53:43Z

src/Bridge/Doctrine/Orm/ItemDataProvider.php

        }

-        $identifierValues = explode('-', $id);
+        $identifierValues = (array) $id;


Casting to an array is useless IIRC.

We use it as an array later in the code. Do we have to cast it in a other way ?
L130

$firstIdentifier = reset($identifiers); foreach ($identifiers as $identifier => $value) {

I'd be happy if this whole identifiers business can be moved into a separate method:

$identifiers = $this->normalizeIdentifiers($id);

In the same class ?

As a first step, yes. We could certainly extract it out later if that makes sense...

It's ok like this ?

teohhanhui · 2016-05-10T06:54:49Z

src/Bridge/Doctrine/Orm/ItemDataProvider.php

+     * @param array        $identifiers
+     * @param QueryBuilder $queryBuilder
+     *
+     * Populate the query with where when needed.


This is not valid PHPDoc. It assume you forgot to clean up here.

Anyway, I suggest to rephrase it like this:

Add WHERE conditions to the query for one or more identifiers (simple or composite).

Simperfit · 2016-05-11T20:11:03Z

src/Bridge/Symfony/Routing/IriConverter.php


            if ($propertyMetadata->isIdentifier()) {
                $identifierValues[] = $this->propertyAccessor->getValue($item, $propertyName);
+                $identifierValuesComposite[] = $propertyName.'='.$this->propertyAccessor->getValue($item, $propertyName);


THIS, feels like a hack.

Just change the above line to:

$identifiers[$propertyName] = $this->propertyAccessor->getValue($item, $propertyName);

Then do this after the loop:

if (1 === count($identifiers)) { $identifiers = array_map(function ($identifierValue) { return rawurlencode($identifierValue); }, $identifiers); } else { $identifiers = array_map(function ($identifierName, $identifierValue) { return sprintf('%s=%s', $identifierName, rawurlencode($identifierValue)); }, array_keys($identifiers), $identifiers); }

And finally:

return $this->router->generate($routeName, ['id' => implode(',', $identifiers)], $referenceType);

Thanks for the help ! Ill modify that asap

…g uuid feature. and fix composite

teohhanhui · 2016-05-12T13:57:08Z

tests/Fixtures/TestBundle/Entity/CompositeRelation.php

    public function getId()
    {
-        return sprintf('%s-%s', $this->compositeItem->getId(), $this->compositeLabel->getId());
+        return sprintf('%s;%s', $this->compositeItem->getId(), $this->compositeLabel->getId());


What is the purpose of this method? I don't see it being used anywhere...

My mistake, I think this is some leftovers from my tests with api-platform 1.x ref #282 (comment)

I asked myself too, but :
If we remove that :

{ "@id": "\/composite_relations\/compositeItem=1;compositeLabel=1", "@type": "CompositeRelation", "id": null, "value": "somefoobardummy" }

Where id is null, but when this exist, the ID is correct :
"id": "1;1"

A separate id property? Where does it come from when there's no getId method?

I don't understand, there is one, we are talking about that method no ?

public function getId() { ...

Yes, is it still a problem if we remove this method from CompositeRelation (and from the related tests too, of course)?

I agree, it's useless

dunglas · 2016-05-14T08:31:59Z

Thank you @Simperfit

api-platform#530 ~ Add behat test and change behavior of ItemDataProvider for uuid

Simperfit force-pushed the fix-uuid-530 branch 5 times, most recently from 55aa7d1 to 67e1333 Compare May 7, 2016 17:17

Simperfit changed the title ~~[WIP] #530 ~ Add behat test and change behavior of ItemDataProvider for uuid~~ #530 ~ Add behat test and change behavior of ItemDataProvider for uuid May 7, 2016

theofidry reviewed May 7, 2016
View reviewed changes

Simperfit force-pushed the fix-uuid-530 branch from 67e1333 to 1be8f44 Compare May 7, 2016 17:26

theofidry reviewed May 7, 2016
View reviewed changes

Simperfit force-pushed the fix-uuid-530 branch 4 times, most recently from 1996b64 to f67ceca Compare May 7, 2016 19:48

theofidry reviewed May 7, 2016
View reviewed changes

Simperfit force-pushed the fix-uuid-530 branch 3 times, most recently from b0a9590 to 6ed35e7 Compare May 7, 2016 20:27

dunglas reviewed May 8, 2016
View reviewed changes

Simperfit force-pushed the fix-uuid-530 branch 4 times, most recently from 9812b8a to 69e9988 Compare May 10, 2016 06:22

teohhanhui reviewed May 10, 2016
View reviewed changes

Simperfit force-pushed the fix-uuid-530 branch 2 times, most recently from ddf2461 to e2ab7cc Compare May 11, 2016 20:10

Simperfit reviewed May 11, 2016
View reviewed changes

Add behat test and change behavior of ItemDataProvider.php when usin…

5ab3dab

…g uuid feature. and fix composite

Simperfit force-pushed the fix-uuid-530 branch 2 times, most recently from ec18af3 to aabd844 Compare May 12, 2016 12:00

teohhanhui reviewed May 12, 2016
View reviewed changes

fix api-platform#530 - Support id;id for composite

1025bbe

Simperfit force-pushed the fix-uuid-530 branch from aabd844 to 1025bbe Compare May 13, 2016 19:44

dunglas merged commit b3cf3b7 into api-platform:master May 14, 2016

soyuka mentioned this pull request May 24, 2016

(WIP) Bug OneToMany => ManyToOne composite identifiers #550

Closed

teohhanhui mentioned this pull request May 26, 2016

Fix identifiers behavior bugs #552

Merged

magarzon pushed a commit to magarzon/core that referenced this pull request Feb 12, 2017

Merge pull request api-platform#533 from Simperfit/fix-uuid-530

31b16c7

api-platform#530 ~ Add behat test and change behavior of ItemDataProvider for uuid

Uh oh!

#530 ~ Add behat test and change behavior of ItemDataProvider for uuid #533

#530 ~ Add behat test and change behavior of ItemDataProvider for uuid #533

Uh oh!

Conversation

Simperfit commented May 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Simperfit commented May 7, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Simperfit May 7, 2016 • edited by theofidry Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theofidry May 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

theofidry May 7, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Simperfit commented May 7, 2016

Uh oh!

polc commented May 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teohhanhui May 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teohhanhui May 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teohhanhui May 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soyuka May 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Simperfit commented May 7, 2016 •

edited

Loading

Simperfit May 7, 2016 •

edited by theofidry

Loading

theofidry May 7, 2016 •

edited

Loading

theofidry May 7, 2016 •

edited

Loading

teohhanhui May 9, 2016 •

edited

Loading

teohhanhui May 12, 2016 •

edited

Loading

teohhanhui May 12, 2016 •

edited

Loading

soyuka May 12, 2016 •

edited

Loading