Allow ItemDataProvider to throw more exceptions #455
Conversation
| @@ -22,8 +23,6 @@ | |||
| interface ItemDataProviderInterface | |||
| { | |||
| /** | |||
| * Retrieves an item. | |||
There was a problem hiding this comment.
Removed comment as is not very helpful IMO: we already have this description in the interface doc and the method name is quite explicit already.
There was a problem hiding this comment.
we already have this description in the interface
I don't get it.
There was a problem hiding this comment.
The interface description is "Retrieves items from a persistence layer.", so having "Retrieves an item." for this interface getItem() method description looks... obvious.
That being said it's just a method description... No strong feelings here :)
There was a problem hiding this comment.
I'm for keeping it, it will produce a nicer HTML PHPDoc.
theofidry
force-pushed
the
item-data-retriever
branch
from
49c12f8
to
b1e3e6e
Compare
| * @param string $resourceClass | ||
| * @param string|null $operationName | ||
| * @param int|string $id | ||
| * @param bool $fetchData | ||
| * | ||
| * @throws ResourceClassNotSupportedException |
There was a problem hiding this comment.
I would keep this one explicitly because this is a case that must be handled in many cases (for others, it's fine to let them be converted to an error displayed to the end user).
|
Updated, also apply those changes to |
theofidry
force-pushed
the
item-data-retriever
branch
from
3f6a5e0
to
0f77320
Compare
theofidry
force-pushed
the
item-data-retriever
branch
from
0f77320
to
c9922ba
Compare
|
ping @api-platform/core-team |
|
👍 |
Allow ItemDataProvider to throw more exceptions
I find the current very restrictive: it only allows an item provider to throw exceptions when it does not support the class or received an incorrect value. As the responsibility of an item provider is to retrieve an item from the persistent layer, it looks to me that there could be all kind of other problems: could not establish a connection with the persistence layer, the item could not be retrieve, etc.
If someone wants to add a custom provider implementing this interface and have to face one of the situation above, he will have little choice but to return
null. IMO, this is not helpful as thenullvalue should be used here for when no item was found. If there was an error while trying to retrieve this item, an exception should be fired. Hence my change in the interface.