Add a BC layer on data providers and deprecation errors #1913
Conversation
| @@ -35,10 +35,10 @@ public function getItem(string $resourceClass, $id, string $operationName = null | |||
| { | |||
| // Retrieve the blog post item from somewhere | |||
| $cnr = new ContainNonResource(); | |||
| $cnr->id = $id['id']; | |||
| $cnr->id = $id; | |||
There was a problem hiding this comment.
I reverted Antoine's changes in this custom dataprovider so it proves it will continue to work in behat tests "the old way"
|
This can be reviewed but should not be merged in 2.2, it should target master when it will be cleaned. |
| return $dataProvider->getItem($resourceClass, $id, $operationName, $context); | ||
| $identifier = $id; | ||
| if (!$dataProvider instanceof DenormalizedIdentifiersAwareItemDataProviderInterface) { | ||
| @trigger_error(sprintf('Receiving "$id" as non-array in an item data provider is deprecated, your item data providers must implement "%s".', DenormalizedIdentifiersAwareItemDataProviderInterface::class), E_USER_DEPRECATED); |
There was a problem hiding this comment.
Is it mandatory to trigger a deprecation notice? In most case, you'll not use composed identifiers (it's not really reliable), and implementing this new interface looks like an annoyance for most people to handle specific use cases.
I suggest to trigger a deprecation only if \count($identifier) > 1. WDYT?
There was a problem hiding this comment.
@dunglas it is your call, I've no strong opinion on it.
My argument is the change affects all data providers, even if you don't use composite identifiers. In 2.2 you would get "2" passed as $id and now you will get ['id' => 2] so in an ideal world, every data provider needs to be updated. Not triggering the deprecation error is ok, it just means that we won't be able to remove this piece of code in the next major version
There was a problem hiding this comment.
Making it easy for the user to add new data providers matter, so I suggest to trigger the deprecation only if \count($identifier) > 1.
There was a problem hiding this comment.
Ok I'll update acordingly this morning
| namespace ApiPlatform\Core\DataProvider; | ||
|
|
||
| /** | ||
| * Retrieves items from a persistence layer. |
There was a problem hiding this comment.
Can you change this sentence by something like Marks data providers able to deal with complex identifiers denormalized as an array?
antograssiot
force-pushed
the
dataprovider-bc-layer
branch
2 times, most recently
from
bf307ae to
cb4f7e2
Compare
antograssiot
force-pushed
the
dataprovider-bc-layer
branch
from
cb4f7e2 to
7e78dbf
Compare
antograssiot
force-pushed
the
dataprovider-bc-layer
branch
from
7e78dbf to
32dea18
Compare
| @trigger_error(sprintf('Receiving "$id" as non-array in an item data provider is deprecated in 2.3 in favor of implementing "%s".', DenormalizedIdentifiersAwareItemDataProviderInterface::class), E_USER_DEPRECATED); | ||
| $identifier = http_build_query($identifier, '', ';'); | ||
| } else { | ||
| $identifier = current($identifier); |
There was a problem hiding this comment.
Update done @dunglas, do you want me to move this statement up to avoid the else ?
There was a problem hiding this comment.
Not it's ok like this (it prevents a useless call to current)
|
Thanks @antograssiot! |
The introduction of identifier denormalization changes the type of the passed
$idto thegetItem()functions in custom data providers from a string to an array.This patch allow to continue to use old data providers and will warn users that they have to update their code meanwhile.
WDYT @soyuka @dunglas ?