Move normalizeIdentifiers method to a new Util class IdentifierManager #907
Conversation
| */ | ||
| public function __construct(ManagerRegistry $managerRegistry, PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, array $itemExtensions = []) |
There was a problem hiding this comment.
why btw? Interface hasn't been impacted or is it because the class isn't "final" and so can be extended?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I agree this could be a trait
There was a problem hiding this comment.
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?
| */ | ||
| public function __construct(ManagerRegistry $managerRegistry, PropertyNameCollectionFactoryInterface $propertyNameCollectionFactory, PropertyMetadataFactoryInterface $propertyMetadataFactory, array $itemExtensions = []) |
| use ApiPlatform\Core\Metadata\Property\Factory\PropertyNameCollectionFactoryInterface; | ||
| use Doctrine\Common\Persistence\ObjectManager; | ||
|
|
||
| class IdentifierManager implements IdentifierManagerInterface |
There was a problem hiding this comment.
CompositeIdentifierTrait?
There was a problem hiding this comment.
Not really related to composite identifiers
|
Trait it is 🕺 |
|
👍 |
|
can we merge this? |
| use ApiPlatform\Core\Exception\PropertyNotFoundException; | ||
| use Doctrine\Common\Persistence\ObjectManager; | ||
|
|
||
| trait IdentifierManagerTrait |
There was a problem hiding this comment.
Can you make it @internal for now?
soyuka
force-pushed
the
move-normalize-identifier
branch
from
5e66aac to
1f72a9b
Compare
|
Thanks @soyuka |
| /** | ||
| * @internal | ||
| */ | ||
| trait IdentifierManagerTrait |
There was a problem hiding this comment.
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?!).
There was a problem hiding this comment.
What about IdentifierNormalizerTrait?
…fier Move normalizeIdentifiers method to a new Util class IdentifierManager
…1014 Backport api-platform#1014 (also includes api-platform#907)
Better SRP + cleaner code/tests regarding the ItemDataProvider.