-
-
Notifications
You must be signed in to change notification settings - Fork 912
[Proposal] Add Stages #2978
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.
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
[Proposal] Add Stages #2978
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
<?php | ||
|
||
/* | ||
* This file is part of the API Platform project. | ||
* | ||
* (c) Kévin Dunglas <[email protected]> | ||
* | ||
* For the full copyright and license information, please view the LICENSE | ||
* file that was distributed with this source code. | ||
*/ | ||
|
||
declare(strict_types=1); | ||
|
||
namespace ApiPlatform\Core\Exception; | ||
|
||
/** | ||
* @author Alan Poulain <[email protected]> | ||
*/ | ||
final class NotFoundException extends \Exception implements ExceptionInterface | ||
{ | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,94 @@ | ||
<?php | ||
|
||
/* | ||
* This file is part of the API Platform project. | ||
* | ||
* (c) Kévin Dunglas <[email protected]> | ||
* | ||
* For the full copyright and license information, please view the LICENSE | ||
* file that was distributed with this source code. | ||
*/ | ||
|
||
declare(strict_types=1); | ||
|
||
namespace ApiPlatform\Core\Stage; | ||
|
||
use ApiPlatform\Core\DataProvider\CollectionDataProviderInterface; | ||
use ApiPlatform\Core\DataProvider\ItemDataProviderInterface; | ||
use ApiPlatform\Core\DataProvider\OperationDataProviderTrait; | ||
use ApiPlatform\Core\DataProvider\SubresourceDataProviderInterface; | ||
use ApiPlatform\Core\Exception\InvalidIdentifierException; | ||
use ApiPlatform\Core\Exception\NotFoundException; | ||
use ApiPlatform\Core\Exception\RuntimeException; | ||
use ApiPlatform\Core\Identifier\IdentifierConverterInterface; | ||
use ApiPlatform\Core\Metadata\Resource\Factory\ResourceMetadataFactoryInterface; | ||
use ApiPlatform\Core\Metadata\Resource\ToggleableOperationAttributeTrait; | ||
|
||
/** | ||
* Retrieves data from the applicable data provider. | ||
* | ||
* @author Alan Poulain <[email protected]> | ||
*/ | ||
final class ReadStage implements ReadStageInterface | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why isn't this class also used by the GraphQL resolver? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would like to merge both too but sadly they are very different. I don't think it will be doable easily. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But then it doesn’t fix the issue of having good extensions points working both for REST and GraphQL. Which is one of the goals. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not having different ones? The need to use both REST and GraphQL is not common. It would just mean having two different service names to use. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To me it's very common, implementing both. That's actually what drove me to API Platform in the first place, I thought it would provide a(nother) way to DRY my Controllers and Resolvers. If you feel it's too much work for this MR, that's of course your prerogative, but please don't sacrifice forever the goal of unification. Also, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We could introduce stage for both REST and GraphQL, but the service will be an empty shell with practically no code IMO. It will be just there to be decorated. |
||
{ | ||
use OperationDataProviderTrait; | ||
use ToggleableOperationAttributeTrait; | ||
|
||
public function __construct(CollectionDataProviderInterface $collectionDataProvider, ItemDataProviderInterface $itemDataProvider, SubresourceDataProviderInterface $subresourceDataProvider = null, IdentifierConverterInterface $identifierConverter = null, ResourceMetadataFactoryInterface $resourceMetadataFactory = null) | ||
{ | ||
$this->collectionDataProvider = $collectionDataProvider; | ||
$this->itemDataProvider = $itemDataProvider; | ||
$this->subresourceDataProvider = $subresourceDataProvider; | ||
$this->identifierConverter = $identifierConverter; | ||
$this->resourceMetadataFactory = $resourceMetadataFactory; | ||
} | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public function apply(array $attributes, array $parameters, ?array $filters, string $method, array $normalizationContext) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of this long list of unstructured and untyped parameters, shouldn't we introduce a new Basically, the responsibility of the Symfony Event Listener, or of the Laravel middleware, or of the GraphQL resolver would be to create this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've always wondered if it would be interesting to introduce this kind of class instead of having a lot of parameters and a |
||
{ | ||
if (!isset($attributes['resource_class']) | ||
|| !$attributes['receive'] | ||
|| ('POST' === $method && isset($attributes['collection_operation_name'])) | ||
|| $this->isOperationAttributeDisabled($attributes, self::OPERATION_ATTRIBUTE_KEY) | ||
) { | ||
return null; | ||
} | ||
|
||
$context = null === $filters ? [] : ['filters' => $filters]; | ||
$context += $normalizationContext ?? []; | ||
|
||
if (isset($attributes['collection_operation_name'])) { | ||
return $this->getCollectionData($attributes, $context); | ||
} | ||
|
||
$data = []; | ||
|
||
if ($this->identifierConverter) { | ||
$context[IdentifierConverterInterface::HAS_IDENTIFIER_CONVERTER] = true; | ||
} | ||
|
||
try { | ||
$identifiers = $this->extractIdentifiers($parameters, $attributes); | ||
|
||
if (isset($attributes['item_operation_name'])) { | ||
$data = $this->getItemData($identifiers, $attributes, $context); | ||
} elseif (isset($attributes['subresource_operation_name'])) { | ||
if (null === $this->subresourceDataProvider) { | ||
throw new RuntimeException('No subresource data provider.'); | ||
} | ||
|
||
$data = $this->getSubresourceData($identifiers, $attributes, $context); | ||
} | ||
} catch (InvalidIdentifierException $e) { | ||
throw new NotFoundException('Not found, because of an invalid identifier configuration', 0, $e); | ||
} | ||
|
||
if (null === $data) { | ||
throw new NotFoundException('Not Found'); | ||
} | ||
|
||
return $data; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
<?php | ||
|
||
/* | ||
* This file is part of the API Platform project. | ||
* | ||
* (c) Kévin Dunglas <[email protected]> | ||
* | ||
* For the full copyright and license information, please view the LICENSE | ||
* file that was distributed with this source code. | ||
*/ | ||
|
||
declare(strict_types=1); | ||
|
||
namespace ApiPlatform\Core\Stage; | ||
|
||
/** | ||
* Retrieves data from the applicable data provider. | ||
* | ||
* @author Alan Poulain <[email protected]> | ||
*/ | ||
interface ReadStageInterface | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wonder if this new concept is really necessary. Does this logic belong to the DataProvider? Or should we create a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think their responsibility is clear:
We could add the second point inside an And I don't know where we could put the other parts. Maybe having a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also IMHO, it's easier for the user to understand and decorate this kind of service than to use the data provider / persister and the serializer. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I’m really not sure about the last point! It’s more concepts to learn with no clear responsibilities. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess it depends of the developer and their way to think. |
||
{ | ||
public const OPERATION_ATTRIBUTE_KEY = 'read'; | ||
|
||
/** | ||
* @return object|iterable|null | ||
*/ | ||
public function apply(array $attributes, array $parameters, ?array $filters, string $method, array $normalizationContext); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Which one do you have in mind? Something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Stage sounds like “util” or “service” to me: too generic and not very descriptive There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But it is something kind of generic. It's a step or an operation. How would you call the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make it a callable? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea! WDYT @dunglas? |
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be backported on 2.4 as a bugfix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not really a bugfix. It's because now this method is always called whereas before it was not called if the operation was a collection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An issue has been reported for non-cloneable objects IIRC
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, without this change, the tests wouldn't pass.