-
Notifications
You must be signed in to change notification settings - Fork 201
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
Add Data Producer plugins for creating, updating and deleting entities. #1212
base: 8.x-4.x
Are you sure you want to change the base?
Changes from 2 commits
c7f5d70
fbfc3c6
1aea77d
c7e59e5
acafe96
3253f08
80d91bb
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,92 @@ | ||
<?php | ||
|
||
namespace Drupal\graphql\Plugin\GraphQL\DataProducer\Entity; | ||
|
||
use Drupal\Core\Access\AccessResultReasonInterface; | ||
use Drupal\Core\Plugin\ContainerFactoryPluginInterface; | ||
use Drupal\graphql\Plugin\GraphQL\DataProducer\DataProducerPluginBase; | ||
use Drupal\graphql\Plugin\GraphQL\DataProducer\EntityValidationTrait; | ||
use Symfony\Component\DependencyInjection\ContainerInterface; | ||
|
||
/** | ||
* Creates an entity. | ||
* | ||
* @DataProducer( | ||
* id = "create_entity", | ||
* name = @Translation("Create Entity"), | ||
* produces = @ContextDefinition("entity", | ||
* label = @Translation("Entity") | ||
* ), | ||
* consumes = { | ||
* "entity_type" = @ContextDefinition("string", | ||
* label = @Translation("Entity Type"), | ||
* required = TRUE | ||
* ), | ||
* "values" = @ContextDefinition("any", | ||
* label = @Translation("Values to update"), | ||
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. Should be "Field values for creating the entity" |
||
* required = TRUE | ||
* ), | ||
* "entity_return_key" = @ContextDefinition("string", | ||
* label = @Translation("Entity Return Key"), | ||
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. Should be "Key name in the returned array where the entity will be placed" |
||
* required = TRUE | ||
* ), | ||
* "save" = @ContextDefinition("boolean", | ||
* label = @Translation("Save entity"), | ||
* required = FALSE, | ||
* default_value = TRUE, | ||
* ), | ||
* } | ||
* ) | ||
*/ | ||
class CreateEntity extends DataProducerPluginBase implements ContainerFactoryPluginInterface { | ||
|
||
use EntityValidationTrait; | ||
|
||
/** | ||
* The entity type manager. | ||
* | ||
* @var \Drupal\Core\Entity\EntityTypeManager | ||
*/ | ||
protected $entityTypeManager; | ||
|
||
/** | ||
* {@inheritdoc} | ||
*/ | ||
public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) { | ||
$instance = new static($configuration, $plugin_id, $plugin_definition); | ||
$instance->entityTypeManager = $container->get('entity_type.manager'); | ||
return $instance; | ||
} | ||
|
||
/** | ||
* Resolve the values for this producer. | ||
*/ | ||
public function resolve(string $entity_type, array $values, string $entity_return_key, ?bool $save, $context) { | ||
$storage = $this->entityTypeManager->getStorage($entity_type); | ||
$accessHandler = $this->entityTypeManager->getAccessControlHandler($entity_type); | ||
|
||
// Ensure the user has access to create this kind of entity. | ||
$access = $accessHandler->createAccess(NULL, NULL, [], TRUE); | ||
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 should pass the bundle here, so I think we need a bundle parameter for this resolver. |
||
$context->addCacheableDependency($access); | ||
if (!$access->isAllowed()) { | ||
return [ | ||
'errors' => [$access instanceof AccessResultReasonInterface && $access->getReason() ? $access->getReason() : 'Access was forbidden.'], | ||
]; | ||
} | ||
|
||
$entity = $storage->create($values); | ||
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. Does this take create field access into account? If yes we should add a code comment like "// The storage will check that only fields can be set that the user has access to." Otherwise we would have a security vulnerability here. Again looking at JSONAPI they return 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. Hm, the core API seems a little sketchy. From JSON API:
From the field access docs:
Kind of begs some additional questions like, is the API invoked before or after the field values are hydrated? In the JSON API case it looks like after, so I guess it makes sense to follow that approach. |
||
if ($violation_messages = $this->getViolationMessages($entity)) { | ||
return [ | ||
'errors' => $violation_messages, | ||
]; | ||
} | ||
|
||
if ($save) { | ||
$entity->save(); | ||
} | ||
return [ | ||
$entity_return_key => $entity, | ||
]; | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
<?php | ||
|
||
namespace Drupal\graphql\Plugin\GraphQL\DataProducer\Entity; | ||
|
||
use Drupal\Core\Access\AccessResultReasonInterface; | ||
use Drupal\Core\Entity\ContentEntityInterface; | ||
use Drupal\graphql\Plugin\GraphQL\DataProducer\DataProducerPluginBase; | ||
|
||
/** | ||
* Deletes an entity. | ||
* | ||
* @DataProducer( | ||
* id = "delete_entity", | ||
* name = @Translation("Delete Entity"), | ||
* produces = @ContextDefinition("entities", | ||
* label = @Translation("Entities") | ||
* ), | ||
* consumes = { | ||
* "entity" = @ContextDefinition("entity", | ||
* label = @Translation("Entity") | ||
* ), | ||
* } | ||
* ) | ||
*/ | ||
class DeleteEntity extends DataProducerPluginBase { | ||
|
||
/** | ||
* Resolve the values for this producer. | ||
*/ | ||
public function resolve(ContentEntityInterface $entity, $context) { | ||
$access = $entity->access('delete', NULL, TRUE); | ||
$context->addCacheableDependency($access); | ||
if (!$access->isAllowed()) { | ||
return [ | ||
'was_successful' => FALSE, | ||
'errors' => [$access instanceof AccessResultReasonInterface ? $access->getReason() : 'Access was forbidden.'], | ||
]; | ||
} | ||
|
||
$entity->delete(); | ||
return [ | ||
'was_successful' => TRUE, | ||
]; | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,78 @@ | ||
<?php | ||
|
||
namespace Drupal\graphql\Plugin\GraphQL\DataProducer\Entity; | ||
|
||
use Drupal\Core\Access\AccessResultReasonInterface; | ||
use Drupal\Core\Entity\ContentEntityInterface; | ||
use Drupal\graphql\Plugin\GraphQL\DataProducer\DataProducerPluginBase; | ||
use Drupal\graphql\Plugin\GraphQL\DataProducer\EntityValidationTrait; | ||
|
||
/** | ||
* Updates entity values. | ||
* | ||
* @DataProducer( | ||
* id = "update_entity", | ||
* name = @Translation("Update Entity"), | ||
* produces = @ContextDefinition("entities", | ||
* label = @Translation("Entities") | ||
* ), | ||
* consumes = { | ||
* "entity" = @ContextDefinition("entity", | ||
* label = @Translation("Entity") | ||
* ), | ||
* "values" = @ContextDefinition("any", | ||
* label = @Translation("Values to update"), | ||
* required = TRUE | ||
* ), | ||
* "entity_return_key" = @ContextDefinition("string", | ||
* label = @Translation("Entity Return Key"), | ||
* required = TRUE | ||
* ), | ||
* } | ||
* ) | ||
*/ | ||
class UpdateEntity extends DataProducerPluginBase { | ||
|
||
use EntityValidationTrait; | ||
|
||
/** | ||
* Resolve the values for this producer. | ||
*/ | ||
public function resolve(ContentEntityInterface $entity, array $values, string $entity_return_key, $context) { | ||
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. You are type hinting to content entities - then I think we should rename all data producers to update_content_entity etc.? 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. Hm, maybe some of them are using entity API in such a way that config entities wouldn't be off the table, but I haven't looked into that too deeply. At the very least, I can't imagine deleting a content and config entity would require any specific content entity functionality. |
||
// Ensure the user has access to perform an update. | ||
$access = $entity->access('update', NULL, TRUE); | ||
$context->addCacheableDependency($access); | ||
if (!$access->isAllowed()) { | ||
return [ | ||
'errors' => [$access instanceof AccessResultReasonInterface ? $access->getReason() : 'Access was forbidden.'], | ||
]; | ||
} | ||
|
||
// Filter out keys the user does not have access to update, this may include | ||
// things such as the owner of the entity or the ID of the entity. | ||
$update_fields = array_filter($values, function (string $field_name) use ($entity, $context) { | ||
$access = $entity->{$field_name}->access('edit', NULL, TRUE); | ||
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. this could throw fatal errors if the field does not exist, for example if the sender made a typo. We should probably return a readable error here? Could also be the case that the schema is defined wrong, but a clear error message would be helpful. 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. I went for an exception here, since I guess it better signals someone has made a code error vs some runtime resolvable thing, but I believe the module gracefully handles those too? |
||
$context->addCacheableDependency($access); | ||
return $access->isAllowed(); | ||
}, ARRAY_FILTER_USE_KEY); | ||
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. Is it a good idea to silently filter out fields that the user does not have edit access to? JSONAPI has more elaborate code + comments on this, check I think we need to do the same. 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. Maybe in this case it's better to simply return a list of errors if any fields are present the user does not have access to update, rather than to complete half the updates and fail on the other half. I don't know if this logic applies to graphql:
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. Yep, we should do the same as JSONAPI I think. Let the consumer submit special fields like ID but do not set them and ignore them. |
||
|
||
// Hydrate the entity with the values. | ||
foreach ($update_fields as $field_name => $field_value) { | ||
$entity->set($field_name, $field_value); | ||
} | ||
|
||
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. entity revision support is missing here like in JSONAPI EntityResource, but we don't have to do all the things in this PR, so fine for me if we skip it for now. |
||
if ($violation_messages = $this->getViolationMessages($entity)) { | ||
return [ | ||
'errors' => $violation_messages, | ||
]; | ||
} | ||
|
||
// Once access has been granted, the save can be committed and the entity | ||
// can be returned to the client. | ||
$entity->save(); | ||
return [ | ||
$entity_return_key => $entity, | ||
]; | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,39 @@ | ||
<?php | ||
|
||
namespace Drupal\graphql\Plugin\GraphQL\DataProducer; | ||
|
||
use Drupal\Core\Entity\ContentEntityInterface; | ||
|
||
/** | ||
* Trait for entity validation. | ||
* | ||
* Ensure the entity passes validation, any violations will be reported back | ||
* to the client. Validation will catch issues like invalid referenced entities, | ||
* incorrect text formats, required fields etc. Additional validation of input | ||
* should not be put here, but instead should be built into the entity | ||
* validation system, so the same constraints are applied in the Drupal admin. | ||
*/ | ||
trait EntityValidationTrait { | ||
|
||
/** | ||
* Get violation messages from an entity. | ||
* | ||
* @param \Drupal\Core\Entity\ContentEntityInterface $entity | ||
* An entity to validate. | ||
* | ||
* @return array|null | ||
* Get a list of violations or NULL if none were found. | ||
*/ | ||
public function getViolationMessages(ContentEntityInterface $entity) { | ||
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 should use a return type hint of array here and never return NULL to be more consistent. |
||
$violations = $entity->validate(); | ||
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 need to filter out validation errors that the user does not have access to, like EntityValidationTrait in the JSONAPI core module. 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. Done, but no test added just yet. |
||
if ($violations->count() > 0) { | ||
$violation_messages = []; | ||
foreach ($violations as $violation) { | ||
$violation_messages[] = sprintf('%s: %s', $violation->getPropertyPath(), strip_tags($violation->getMessage())); | ||
} | ||
return $violation_messages; | ||
} | ||
return NULL; | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,82 @@ | ||
<?php | ||
|
||
namespace Drupal\Tests\graphql\Kernel\DataProducer\Entity; | ||
|
||
use Drupal\node\Entity\NodeType; | ||
use Drupal\Tests\graphql\Kernel\GraphQLTestBase; | ||
use Drupal\Tests\graphql\Traits\DataProducerExecutionTrait; | ||
use Drupal\Tests\user\Traits\UserCreationTrait; | ||
|
||
/** | ||
* Test the CreateEntity producer. | ||
* | ||
* @group graphql | ||
*/ | ||
class CreateEntityTest extends GraphQLTestBase { | ||
|
||
use DataProducerExecutionTrait; | ||
use UserCreationTrait; | ||
|
||
/** | ||
* The plugin ID. | ||
* | ||
* @var string | ||
*/ | ||
protected $pluginId = 'create_entity'; | ||
|
||
/** | ||
* Test creating entities. | ||
*/ | ||
public function testCreateEntity() { | ||
$content_type = NodeType::create([ | ||
'type' => 'lorem', | ||
'name' => 'ipsum', | ||
]); | ||
$content_type->save(); | ||
|
||
$result = $this->executeDataProducer($this->pluginId, [ | ||
'entity_type' => 'node', | ||
'values' => [], | ||
'entity_return_key' => 'foo', | ||
]); | ||
$this->assertSame('Access was forbidden.', $result['errors'][0]); | ||
|
||
$this->setCurrentUser($this->createUser(['bypass node access', 'access content'])); | ||
|
||
$result = $this->executeDataProducer($this->pluginId, [ | ||
'entity_type' => 'node', | ||
'values' => [ | ||
'type' => 'lorem' | ||
], | ||
'entity_return_key' => 'foo', | ||
]); | ||
$this->assertSame([ | ||
'title: This value should not be null.', | ||
], $result['errors']); | ||
|
||
$result = $this->executeDataProducer($this->pluginId, [ | ||
'entity_type' => 'node', | ||
'save' => TRUE, | ||
'values' => [ | ||
'type' => 'lorem', | ||
'title' => 'bar', | ||
], | ||
'entity_return_key' => 'foo', | ||
]); | ||
$this->assertEquals('bar', $result['foo']->label()); | ||
$this->assertFalse($result['foo']->isNew()); | ||
|
||
$result = $this->executeDataProducer($this->pluginId, [ | ||
'entity_type' => 'node', | ||
'save' => FALSE, | ||
'values' => [ | ||
'type' => 'lorem', | ||
'title' => 'bar', | ||
], | ||
'entity_return_key' => 'foo', | ||
]); | ||
$this->assertEquals('bar', $result['foo']->label()); | ||
$this->assertTrue($result['foo']->isNew()); | ||
} | ||
|
||
} |
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.
As I understand it you are not producing an entity directly but an array that has the entity wrapped in it.
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.
Right, is "any" the most appropriate type here or can the array somehow be described?
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.
not that I know of, so "any" will have to do for now.