Skip to content

Move normalizeIdentifiers method to a new Util class IdentifierManager #907

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dunglas merged 2 commits into from
Jan 20, 2017
Merged

Move normalizeIdentifiers method to a new Util class IdentifierManager #907

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4074faa
Move normalizeIdentifiers method to a new Util class IdentifierManager
Jan 12, 2017
1f72a9b
Move IdentifierManager to a trait to avoid BC break
Jan 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
63 changes: 6 additions & 57 deletions src/Bridge/Doctrine/Orm/ItemDataProvider.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,14 @@

use ApiPlatform\Core\Bridge\Doctrine\Orm\Extension\QueryItemExtensionInterface;
use ApiPlatform\Core\Bridge\Doctrine\Orm\Extension\QueryResultItemExtensionInterface;
use ApiPlatform\Core\Bridge\Doctrine\Orm\Util\IdentifierManagerTrait;
use ApiPlatform\Core\Bridge\Doctrine\Orm\Util\QueryNameGenerator;
use ApiPlatform\Core\DataProvider\ItemDataProviderInterface;
use ApiPlatform\Core\Exception\PropertyNotFoundException;
use ApiPlatform\Core\Exception\ResourceClassNotSupportedException;
use ApiPlatform\Core\Exception\RuntimeException;
use ApiPlatform\Core\Metadata\Property\Factory\PropertyMetadataFactoryInterface;
use ApiPlatform\Core\Metadata\Property\Factory\PropertyNameCollectionFactoryInterface;
use Doctrine\Common\Persistence\ManagerRegistry;
use Doctrine\Common\Persistence\ObjectManager;
use Doctrine\ORM\EntityManagerInterface;
use Doctrine\ORM\QueryBuilder;

Expand All @@ -38,11 +37,12 @@ class ItemDataProvider implements ItemDataProviderInterface
private $propertyMetadataFactory;
private $itemExtensions;

use IdentifierManagerTrait;

/**
* @param ManagerRegistry $managerRegistry
* @param PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory
* @param PropertyMetadataFactoryInterface $propertyMetadataFactory
* @param QueryItemExtensionInterface[] $itemExtensions
* @param ManagerRegistry $managerRegistry
* @param IdentifierManagerInterface $identifierManager
* @param QueryItemExtensionInterface[] $itemExtensions
*/
public function __construct(ManagerRegistry $managerRegistry, PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, array $itemExtensions = [])

@dunglas dunglas Jan 12, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problem: this is a BC break...

@soyuka soyuka Jan 12, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

raw

@soyuka soyuka Jan 12, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why btw? Interface hasn't been impacted or is it because the class isn't "final" and so can be extended?

@dunglas dunglas Jan 12, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's because if someone initializes this class by himself (and some people including me do) it will break.
This document is a nice reference of what is a BC break or not: http://symfony.com/doc/current/contributing/code/bc.html

@soyuka soyuka Jan 12, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice link! To bad though, this is a nice improvement IMO.

I can make this work without BC by still injecting both metadata factories and calling new IdentifierManager manually in the constructor, not sure that it'd be accepted though.

@dunglas dunglas Jan 12, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a good solution for now.

@dunglas dunglas Jan 12, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Btw, maybe should we make this new class internal as this logic should not be exposed or replaced. Maybe can we just use a trait too.

@Simperfit Simperfit Jan 12, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree this could be a trait

@soyuka soyuka Jan 12, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lol @Simperfit still we don't need the factories in the data provider but they're needed to get the identifiers. What would be the best to inject those?

@teohhanhui teohhanhui Jan 13, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use a trait.

{
Expand Down Expand Up @@ -114,55 +114,4 @@ private function addWhereForIdentifiers(array $identifiers, QueryBuilder $queryB
$queryBuilder->setParameter($placeholder, $value);
}
}

/**
* Transform and check the identifier, composite or not.
*
* @param int|string $id
* @param ObjectManager $manager
* @param string $resourceClass
*
* @throws PropertyNotFoundException
*
* @return array
*/
private function normalizeIdentifiers($id, ObjectManager $manager, string $resourceClass): array
{
$identifierValues = [$id];
$doctrineMetadataIdentifier = $manager->getClassMetadata($resourceClass)->getIdentifier();

if (count($doctrineMetadataIdentifier) >= 2) {
$identifiers = explode(';', $id);
$identifiersMap = [];

// first transform identifiers to a proper key/value array
foreach ($identifiers as $identifier) {
$keyValue = explode('=', $identifier);
$identifiersMap[$keyValue[0]] = $keyValue[1];
}
}

$identifiers = [];
$i = 0;

foreach ($this->propertyNameCollectionFactory->create($resourceClass) as $propertyName) {
$propertyMetadata = $this->propertyMetadataFactory->create($resourceClass, $propertyName);

$identifier = $propertyMetadata->isIdentifier();
if (null === $identifier || false === $identifier) {
continue;
}

$identifier = !isset($identifiersMap) ? $identifierValues[$i] ?? null : $identifiersMap[$propertyName] ?? null;

if (null === $identifier) {
throw new PropertyNotFoundException(sprintf('Invalid identifier "%s", "%s" has not been found.', $id, $propertyName));
}

$identifiers[$propertyName] = $identifier;
++$i;
}

return $identifiers;
}
}
71 changes: 71 additions & 0 deletions src/Bridge/Doctrine/Orm/Util/IdentifierManagerTrait.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
<?php

/*
* This file is part of the API Platform project.
*
* (c) Kévin Dunglas <dunglas@gmail.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace ApiPlatform\Core\Bridge\Doctrine\Orm\Util;

use ApiPlatform\Core\Exception\PropertyNotFoundException;
use Doctrine\Common\Persistence\ObjectManager;

/**
* @internal
*/
trait IdentifierManagerTrait

@dunglas dunglas Jan 18, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you make it @internal for now?

@soyuka soyuka Jan 20, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@teohhanhui teohhanhui Jan 20, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think IdentifierManagerTrait is a bad name. CompositeIdentifierTrait makes sense to me, because this is a trait that helps with handling composite identifiers (even when the identifier is not composite).

Or perhaps another, because the "manager" part of IdentifierManagerTrait definitely does not inspire confidence that it is not violating SRP (a manager trait?!).

@soyuka soyuka Jan 20, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about IdentifierNormalizerTrait?

{
/**
* Transform and check the identifier, composite or not.
*
* @param int|string $id
* @param ObjectManager $manager
* @param string $resourceClass
*
* @throws PropertyNotFoundException
*
* @return array
*/
public function normalizeIdentifiers($id, ObjectManager $manager, string $resourceClass): array
{
$identifierValues = [$id];
$doctrineMetadataIdentifier = $manager->getClassMetadata($resourceClass)->getIdentifier();

if (2 <= count($doctrineMetadataIdentifier)) {
$identifiers = explode(';', $id);
$identifiersMap = [];

// first transform identifiers to a proper key/value array
foreach ($identifiers as $identifier) {
$keyValue = explode('=', $identifier);
$identifiersMap[$keyValue[0]] = $keyValue[1];
}
}

$identifiers = [];
$i = 0;

foreach ($this->propertyNameCollectionFactory->create($resourceClass) as $propertyName) {
$propertyMetadata = $this->propertyMetadataFactory->create($resourceClass, $propertyName);

$identifier = $propertyMetadata->isIdentifier();
if (null === $identifier || false === $identifier) {
continue;
}

$identifier = !isset($identifiersMap) ? $identifierValues[$i] ?? null : $identifiersMap[$propertyName] ?? null;
if (null === $identifier) {
throw new PropertyNotFoundException(sprintf('Invalid identifier "%s", "%s" has not been found.', $id, $propertyName));
}

$identifiers[$propertyName] = $identifier;
++$i;
}

return $identifiers;
}
}
Loading