From fdbf86a3491a6b1fe23283061c663b84205606ca Mon Sep 17 00:00:00 2001 From: Klaus Purer Date: Thu, 30 May 2024 14:43:59 +0200 Subject: [PATCH 01/12] fix(dataproducer): Populate context default values before resolving --- .../DataProducer/DataProducerPluginBase.php | 17 ++++++++++- .../Kernel/DataProducer/DefaultValueTest.php | 28 +++++++++++++++++++ 2 files changed, 44 insertions(+), 1 deletion(-) create mode 100644 tests/src/Kernel/DataProducer/DefaultValueTest.php diff --git a/src/Plugin/GraphQL/DataProducer/DataProducerPluginBase.php b/src/Plugin/GraphQL/DataProducer/DataProducerPluginBase.php index 8ed6a429b..14f4d5f1a 100644 --- a/src/Plugin/GraphQL/DataProducer/DataProducerPluginBase.php +++ b/src/Plugin/GraphQL/DataProducer/DataProducerPluginBase.php @@ -51,11 +51,26 @@ public function resolveField(FieldContext $field) { throw new \LogicException('Missing data producer resolve method.'); } - $context = $this->getContextValues(); + $context = $this->getContextValuesWithDefaults(); return call_user_func_array( [$this, 'resolve'], array_values(array_merge($context, [$field])) ); } + /** + * Initializes all contexts and populates default values. + * + * We cannot use ::getContextValues() here because it does not work with + * default_value. + */ + public function getContextValuesWithDefaults() { + $values = []; + foreach ($this->getContextDefinitions() as $name => $definition) { + $values[$name] = $this->getContext($name)->getContextValue(); + } + + return $values; + } + } diff --git a/tests/src/Kernel/DataProducer/DefaultValueTest.php b/tests/src/Kernel/DataProducer/DefaultValueTest.php new file mode 100644 index 000000000..d1d0f6df7 --- /dev/null +++ b/tests/src/Kernel/DataProducer/DefaultValueTest.php @@ -0,0 +1,28 @@ +container->get('plugin.manager.graphql.data_producer'); + $plugin = $manager->createInstance('entity_load'); + // Only type is required. + $plugin->setContextValue('type', 'node'); + $context_values = $plugin->getContextValuesWithDefaults(); + $this->assertSame(TRUE, $context_values['access']); + $this->assertSame('view', $context_values['access_operation']); + } + +} From 804e45aa46dae71b5a33c7591a041cb35f653037 Mon Sep 17 00:00:00 2001 From: Klaus Purer Date: Thu, 30 May 2024 14:47:19 +0200 Subject: [PATCH 02/12] cleanup test case --- tests/src/Kernel/DataProducer/EntityMultipleTest.php | 6 ------ 1 file changed, 6 deletions(-) diff --git a/tests/src/Kernel/DataProducer/EntityMultipleTest.php b/tests/src/Kernel/DataProducer/EntityMultipleTest.php index 3d9598db0..ad8cca956 100644 --- a/tests/src/Kernel/DataProducer/EntityMultipleTest.php +++ b/tests/src/Kernel/DataProducer/EntityMultipleTest.php @@ -78,10 +78,6 @@ public function testResolveEntityLoadMultiple(): void { 'type' => $this->node1->getEntityTypeId(), 'bundles' => [$this->node1->bundle(), $this->node2->bundle()], 'ids' => [$this->node1->id(), $this->node2->id(), $this->node3->id()], - // @todo We need to set these default values here to make the access - // handling work. Ideally that should not be needed. - 'access' => TRUE, - 'access_operation' => 'view', ]); $nids = array_values(array_map(function (NodeInterface $item) { @@ -104,8 +100,6 @@ public function testResolveEntityLoadWithNullId(): void { $result = $this->executeDataProducer('entity_load_multiple', [ 'type' => $this->node1->getEntityTypeId(), 'ids' => [NULL], - 'access' => TRUE, - 'access_operation' => 'view', ]); $this->assertSame([], $result); From e11b3adce4c32788be4894755ad4e5bcbbd8ca82 Mon Sep 17 00:00:00 2001 From: Klaus Purer Date: Thu, 30 May 2024 14:55:26 +0200 Subject: [PATCH 03/12] fix phpstan --- src/Plugin/GraphQL/DataProducer/DataProducerPluginBase.php | 2 +- tests/src/Kernel/DataProducer/DefaultValueTest.php | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Plugin/GraphQL/DataProducer/DataProducerPluginBase.php b/src/Plugin/GraphQL/DataProducer/DataProducerPluginBase.php index 14f4d5f1a..13209ede9 100644 --- a/src/Plugin/GraphQL/DataProducer/DataProducerPluginBase.php +++ b/src/Plugin/GraphQL/DataProducer/DataProducerPluginBase.php @@ -64,7 +64,7 @@ public function resolveField(FieldContext $field) { * We cannot use ::getContextValues() here because it does not work with * default_value. */ - public function getContextValuesWithDefaults() { + public function getContextValuesWithDefaults(): array { $values = []; foreach ($this->getContextDefinitions() as $name => $definition) { $values[$name] = $this->getContext($name)->getContextValue(); diff --git a/tests/src/Kernel/DataProducer/DefaultValueTest.php b/tests/src/Kernel/DataProducer/DefaultValueTest.php index d1d0f6df7..754eae6a8 100644 --- a/tests/src/Kernel/DataProducer/DefaultValueTest.php +++ b/tests/src/Kernel/DataProducer/DefaultValueTest.php @@ -21,7 +21,7 @@ public function testEntityLoadDefaultValue(): void { // Only type is required. $plugin->setContextValue('type', 'node'); $context_values = $plugin->getContextValuesWithDefaults(); - $this->assertSame(TRUE, $context_values['access']); + $this->assertTrue($context_values['access']); $this->assertSame('view', $context_values['access_operation']); } From d8fb3af9e2ed445de10c80a2a831a5fea5f25779 Mon Sep 17 00:00:00 2001 From: Klaus Purer Date: Thu, 30 May 2024 15:14:51 +0200 Subject: [PATCH 04/12] fix cs --- tests/src/Kernel/DataProducer/DefaultValueTest.php | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/src/Kernel/DataProducer/DefaultValueTest.php b/tests/src/Kernel/DataProducer/DefaultValueTest.php index 754eae6a8..67149abbf 100644 --- a/tests/src/Kernel/DataProducer/DefaultValueTest.php +++ b/tests/src/Kernel/DataProducer/DefaultValueTest.php @@ -11,7 +11,6 @@ */ class DefaultValueTest extends GraphQLTestBase { - /** * Test that the entity_load data producer has the correct default values. */ From 0b4d348388f414428e5c1df860c4ce41ae93269a Mon Sep 17 00:00:00 2001 From: Klaus Purer Date: Sat, 15 Jun 2024 10:04:44 +0200 Subject: [PATCH 05/12] implement configuration flag --- config/install/graphql.settings.yml | 1 + config/schema/graphql.schema.yml | 10 ++++++++++ graphql.install | 12 ++++++++++++ src/Plugin/DataProducerPluginManager.php | 19 +++++++++++++++++++ .../DataProducer/DataProducerPluginBase.php | 4 ++-- 5 files changed, 44 insertions(+), 2 deletions(-) create mode 100644 config/install/graphql.settings.yml diff --git a/config/install/graphql.settings.yml b/config/install/graphql.settings.yml new file mode 100644 index 000000000..abdb9497a --- /dev/null +++ b/config/install/graphql.settings.yml @@ -0,0 +1 @@ +dataproducer_populate_default_values: true diff --git a/config/schema/graphql.schema.yml b/config/schema/graphql.schema.yml index f90dc52f0..cec9ca533 100644 --- a/config/schema/graphql.schema.yml +++ b/config/schema/graphql.schema.yml @@ -66,3 +66,13 @@ graphql.default_persisted_query_configuration: plugin.plugin_configuration.persisted_query.*: type: graphql.default_persisted_query_configuration + +graphql.settings: + type: config_object + label: "GraphQL Settings" + mapping: + # @todo Remove in GraphQL 5. + dataproducer_populate_default_values: + type: boolean + label: "Populate dataproducer context default values" + description: "Legacy setting: Populate dataproducer context default values before executing the resolve method. Set this to true to be future-proof. This setting is deprecated and will be removed in a future release." diff --git a/graphql.install b/graphql.install index dcc9c47bc..bf79cf1cd 100644 --- a/graphql.install +++ b/graphql.install @@ -69,3 +69,15 @@ function graphql_update_8001(): void { */ function graphql_update_8400() :void { } + +/** + * Preserve dataproducer default value behavior for old installations. + * + * Set dataproducer_populate_default_values to TRUE after you verified that your + * dataproducers are still working with the new default value behavior. + */ +function graphql_update_10400() :void { + \Drupal::configFactory()->getEditable('graphql.settings') + ->set('dataproducer_populate_default_values', FALSE) + ->save(); +} diff --git a/src/Plugin/DataProducerPluginManager.php b/src/Plugin/DataProducerPluginManager.php index 549749f0e..407118ba5 100644 --- a/src/Plugin/DataProducerPluginManager.php +++ b/src/Plugin/DataProducerPluginManager.php @@ -35,6 +35,13 @@ class DataProducerPluginManager extends DefaultPluginManager { */ protected $resultCacheBackend; + /** + * Backwards compatibility flag to populate context defaults or not. + * + * @todo Remove in 5.x. + */ + protected bool $populateContextDefaults = TRUE; + /** * DataProducerPluginManager constructor. * @@ -83,6 +90,18 @@ public function __construct( $this->requestStack = $requestStack; $this->contextsManager = $contextsManager; $this->resultCacheBackend = $resultCacheBackend; + + // We don't use dependency injection here to avoid a constructor signature + // change. + $this->populateContextDefaults = \Drupal::config('graphql.settings')->get('dataproducer_populate_default_values', TRUE); + } + + /** + * {@inheritdoc} + */ + public function createInstance($plugin_id, array $configuration = []) { + $configuration['dataproducer_populate_default_values'] = $this->populateContextDefaults; + return parent::createInstance($plugin_id, $configuration); } /** diff --git a/src/Plugin/GraphQL/DataProducer/DataProducerPluginBase.php b/src/Plugin/GraphQL/DataProducer/DataProducerPluginBase.php index 13209ede9..b21348cbc 100644 --- a/src/Plugin/GraphQL/DataProducer/DataProducerPluginBase.php +++ b/src/Plugin/GraphQL/DataProducer/DataProducerPluginBase.php @@ -50,8 +50,8 @@ public function resolveField(FieldContext $field) { if (!method_exists($this, 'resolve')) { throw new \LogicException('Missing data producer resolve method.'); } - - $context = $this->getContextValuesWithDefaults(); + $populateDefaulktValues = $this->configuration['dataproducer_populate_default_values'] ?? TRUE; + $context = $populateDefaulktValues ? $this->getContextValuesWithDefaults() : $this->getContextValues(); return call_user_func_array( [$this, 'resolve'], array_values(array_merge($context, [$field])) From 6a6bf308587d7ffce03d125d430ad955b1e7a07e Mon Sep 17 00:00:00 2001 From: Klaus Purer Date: Sat, 15 Jun 2024 17:43:08 +0200 Subject: [PATCH 06/12] fix phpstan --- src/Plugin/DataProducerPluginManager.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Plugin/DataProducerPluginManager.php b/src/Plugin/DataProducerPluginManager.php index 407118ba5..b32a98851 100644 --- a/src/Plugin/DataProducerPluginManager.php +++ b/src/Plugin/DataProducerPluginManager.php @@ -92,8 +92,8 @@ public function __construct( $this->resultCacheBackend = $resultCacheBackend; // We don't use dependency injection here to avoid a constructor signature - // change. - $this->populateContextDefaults = \Drupal::config('graphql.settings')->get('dataproducer_populate_default_values', TRUE); + // change. @phpstan-ignore-next-line + $this->populateContextDefaults = \Drupal::config('graphql.settings')->get('dataproducer_populate_default_values') ?? TRUE; } /** From 937667d5ce5484c2e9e4fbbd5c0a8754265ef7ea Mon Sep 17 00:00:00 2001 From: Klaus Purer Date: Sat, 15 Jun 2024 19:31:20 +0200 Subject: [PATCH 07/12] test 1 --- .../Kernel/DataProducer/DefaultValueTest.php | 34 +++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/tests/src/Kernel/DataProducer/DefaultValueTest.php b/tests/src/Kernel/DataProducer/DefaultValueTest.php index 67149abbf..428a131cb 100644 --- a/tests/src/Kernel/DataProducer/DefaultValueTest.php +++ b/tests/src/Kernel/DataProducer/DefaultValueTest.php @@ -2,7 +2,13 @@ namespace Drupal\Tests\graphql\Kernel\DataProducer; +use Drupal\Core\Session\AccountInterface; +use Drupal\graphql\GraphQL\Execution\FieldContext; +use Drupal\graphql\Plugin\GraphQL\DataProducer\Entity\EntityLoad; +use Drupal\media_test_source\Plugin\media\Source\Test; use Drupal\Tests\graphql\Kernel\GraphQLTestBase; +use GraphQL\Deferred; +use PHPUnit\Framework\Assert; /** * Context default value test. @@ -24,4 +30,32 @@ public function testEntityLoadDefaultValue(): void { $this->assertSame('view', $context_values['access_operation']); } + /** + * Test that the legacy dataproducer_populate_default_values setting works. + */ + public function testLegacyDefaultValueSetting(): void { + $this->container->get('config.factory')->getEditable('graphql.settings') + ->set('dataproducer_populate_default_values', FALSE) + ->save(); + $manager = $this->container->get('plugin.manager.graphql.data_producer'); + + // Manipulate the plugin definitions to use our test class for entity_load. + $definitions = $manager->getDefinitions(); + $definitions['entity_load']['class'] = TestLegacyEntityLoad::class; + $reflection = new \ReflectionClass($manager); + $property = $reflection->getProperty('definitions'); + $property->setAccessible(TRUE); + $property->setValue($manager, $definitions); + + $this->executeDataProducer('entity_load', ['type' => 'node']); + } + +} + +class TestLegacyEntityLoad extends EntityLoad { + public function resolve($type, $id, ?string $language, ?array $bundles, ?bool $access, ?AccountInterface $accessUser, ?string $accessOperation, FieldContext $context): ?Deferred { + Assert::assertNull($access); + Assert::assertNull($accessOperation); + return NULL; + } } From a507079cd7cc732a58c4b9ab4abc57b63a01f993 Mon Sep 17 00:00:00 2001 From: Klaus Purer Date: Sat, 15 Jun 2024 19:47:10 +0200 Subject: [PATCH 08/12] Finish test coverage with data provider --- .../Kernel/DataProducer/DefaultValueTest.php | 45 +++++++++++++++++-- 1 file changed, 41 insertions(+), 4 deletions(-) diff --git a/tests/src/Kernel/DataProducer/DefaultValueTest.php b/tests/src/Kernel/DataProducer/DefaultValueTest.php index 428a131cb..2185d2e7b 100644 --- a/tests/src/Kernel/DataProducer/DefaultValueTest.php +++ b/tests/src/Kernel/DataProducer/DefaultValueTest.php @@ -5,7 +5,6 @@ use Drupal\Core\Session\AccountInterface; use Drupal\graphql\GraphQL\Execution\FieldContext; use Drupal\graphql\Plugin\GraphQL\DataProducer\Entity\EntityLoad; -use Drupal\media_test_source\Plugin\media\Source\Test; use Drupal\Tests\graphql\Kernel\GraphQLTestBase; use GraphQL\Deferred; use PHPUnit\Framework\Assert; @@ -32,16 +31,18 @@ public function testEntityLoadDefaultValue(): void { /** * Test that the legacy dataproducer_populate_default_values setting works. + * + * @dataProvider settingsProvider */ - public function testLegacyDefaultValueSetting(): void { + public function testLegacyDefaultValueSetting(bool $populate_setting, string $testClass): void { $this->container->get('config.factory')->getEditable('graphql.settings') - ->set('dataproducer_populate_default_values', FALSE) + ->set('dataproducer_populate_default_values', $populate_setting) ->save(); $manager = $this->container->get('plugin.manager.graphql.data_producer'); // Manipulate the plugin definitions to use our test class for entity_load. $definitions = $manager->getDefinitions(); - $definitions['entity_load']['class'] = TestLegacyEntityLoad::class; + $definitions['entity_load']['class'] = $testClass; $reflection = new \ReflectionClass($manager); $property = $reflection->getProperty('definitions'); $property->setAccessible(TRUE); @@ -50,12 +51,48 @@ public function testLegacyDefaultValueSetting(): void { $this->executeDataProducer('entity_load', ['type' => 'node']); } + /** + * Data provider for the testLegacyDefaultValueSetting test. + */ + public function settingsProvider(): array { + return [ + [FALSE, TestLegacyEntityLoad::class], + [TRUE, TestNewEntityLoad::class], + ]; + } + } +/** + * Helper class to test the legacy behavior. + */ class TestLegacyEntityLoad extends EntityLoad { + + /** + * {@inheritdoc} + */ public function resolve($type, $id, ?string $language, ?array $bundles, ?bool $access, ?AccountInterface $accessUser, ?string $accessOperation, FieldContext $context): ?Deferred { + // Old behavior: no default values applied, so we get NULL here. Assert::assertNull($access); Assert::assertNull($accessOperation); return NULL; } + +} + +/** + * Helper class to test the new behavior. + */ +class TestNewEntityLoad extends EntityLoad { + + /** + * {@inheritdoc} + */ + public function resolve($type, $id, ?string $language, ?array $bundles, ?bool $access, ?AccountInterface $accessUser, ?string $accessOperation, FieldContext $context): ?Deferred { + // New behavior: default values are applied. + Assert::assertTrue($access); + Assert::assertSame('view', $accessOperation); + return NULL; + } + } From 0cfef48eacbdefec96488749be61f012dbff0d54 Mon Sep 17 00:00:00 2001 From: Klaus Purer Date: Sat, 15 Jun 2024 19:54:11 +0200 Subject: [PATCH 09/12] ignore phpcs line --- src/Plugin/DataProducerPluginManager.php | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/Plugin/DataProducerPluginManager.php b/src/Plugin/DataProducerPluginManager.php index b32a98851..c89111b5b 100644 --- a/src/Plugin/DataProducerPluginManager.php +++ b/src/Plugin/DataProducerPluginManager.php @@ -92,7 +92,8 @@ public function __construct( $this->resultCacheBackend = $resultCacheBackend; // We don't use dependency injection here to avoid a constructor signature - // change. @phpstan-ignore-next-line + // change. + // @phpcs:ignore $this->populateContextDefaults = \Drupal::config('graphql.settings')->get('dataproducer_populate_default_values') ?? TRUE; } From 256d225ff6b872c33a24397d91dd29aa7d46fd3c Mon Sep 17 00:00:00 2001 From: Klaus Purer Date: Sat, 15 Jun 2024 19:57:30 +0200 Subject: [PATCH 10/12] Also ignore PHPstan --- src/Plugin/DataProducerPluginManager.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Plugin/DataProducerPluginManager.php b/src/Plugin/DataProducerPluginManager.php index c89111b5b..66420948c 100644 --- a/src/Plugin/DataProducerPluginManager.php +++ b/src/Plugin/DataProducerPluginManager.php @@ -93,7 +93,7 @@ public function __construct( // We don't use dependency injection here to avoid a constructor signature // change. - // @phpcs:ignore + // @phpcs:ignore @phpstan-ignore-next-line $this->populateContextDefaults = \Drupal::config('graphql.settings')->get('dataproducer_populate_default_values') ?? TRUE; } From 50305872b6d5b00bcd5aba9c9780156b7327c80f Mon Sep 17 00:00:00 2001 From: Klaus Purer Date: Sat, 15 Jun 2024 20:09:51 +0200 Subject: [PATCH 11/12] argh! --- src/Plugin/DataProducerPluginManager.php | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/Plugin/DataProducerPluginManager.php b/src/Plugin/DataProducerPluginManager.php index 66420948c..9f05658bb 100644 --- a/src/Plugin/DataProducerPluginManager.php +++ b/src/Plugin/DataProducerPluginManager.php @@ -93,8 +93,10 @@ public function __construct( // We don't use dependency injection here to avoid a constructor signature // change. - // @phpcs:ignore @phpstan-ignore-next-line + // @phpcs:disable + // @phpstan-ignore-next-line $this->populateContextDefaults = \Drupal::config('graphql.settings')->get('dataproducer_populate_default_values') ?? TRUE; + // @phpcs:enable } /** From 2c09f29dc4ec58e7714f0a1008ee281251dc840d Mon Sep 17 00:00:00 2001 From: Klaus Purer Date: Sun, 16 Jun 2024 11:52:38 +0200 Subject: [PATCH 12/12] fix typo --- src/Plugin/GraphQL/DataProducer/DataProducerPluginBase.php | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Plugin/GraphQL/DataProducer/DataProducerPluginBase.php b/src/Plugin/GraphQL/DataProducer/DataProducerPluginBase.php index b21348cbc..945d9c571 100644 --- a/src/Plugin/GraphQL/DataProducer/DataProducerPluginBase.php +++ b/src/Plugin/GraphQL/DataProducer/DataProducerPluginBase.php @@ -50,8 +50,8 @@ public function resolveField(FieldContext $field) { if (!method_exists($this, 'resolve')) { throw new \LogicException('Missing data producer resolve method.'); } - $populateDefaulktValues = $this->configuration['dataproducer_populate_default_values'] ?? TRUE; - $context = $populateDefaulktValues ? $this->getContextValuesWithDefaults() : $this->getContextValues(); + $populateDefaultValues = $this->configuration['dataproducer_populate_default_values'] ?? TRUE; + $context = $populateDefaultValues ? $this->getContextValuesWithDefaults() : $this->getContextValues(); return call_user_func_array( [$this, 'resolve'], array_values(array_merge($context, [$field]))