From 2a1b3f554b94a5bcfb93c4dd1dc327e1d614b4da Mon Sep 17 00:00:00 2001 From: Markus Podar Date: Sat, 7 Jan 2023 15:44:31 +0100 Subject: [PATCH] Remove optional support for non-lazy loading of types Lazy loading has been introduced in 2.0.0 (2019-08) and has been enabled by default in 8.0.0 (2021-11). From now on the type loader is always used when resolving types and the impact mostly means that you can't have types registered using an alias different from their name. As can be seen by th commit, this reduces complexity and maintenance overhead of the library. --- .github/workflows/tests.yml | 7 +--- CHANGELOG.md | 9 ++++ README.md | 41 ------------------- composer.json | 6 +-- config/config.php | 5 --- phpstan-baseline.neon | 15 ------- src/GraphQL.php | 14 ++----- tests/Support/Objects/ExampleType2.php | 26 ------------ .../Objects/ExamplesConfigAliasQuery.php | 40 ------------------ tests/TestCase.php | 8 ---- tests/Unit/GraphQLQueryTest.php | 30 -------------- ...ueriesAndTypesEachInTheirOwnSchemaTest.php | 5 --- tests/Unit/TypesInSchemas/TypesTest.php | 5 --- 13 files changed, 15 insertions(+), 196 deletions(-) delete mode 100644 tests/Support/Objects/ExampleType2.php delete mode 100644 tests/Support/Objects/ExamplesConfigAliasQuery.php diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml index bc548d43..a560e0d7 100644 --- a/.github/workflows/tests.yml +++ b/.github/workflows/tests.yml @@ -35,7 +35,6 @@ jobs: matrix: php: ${{ fromJson(needs.supported-versions-matrix.outputs.version) }} laravel: [^6.0, ^8.0, ^9.0] - lazy_types: ['false', 'true'] exclude: - php: 7.4 laravel: ^9.0 @@ -45,7 +44,7 @@ jobs: laravel: ^6.0 - php: 8.2 laravel: ^6.0 - name: P=${{ matrix.php }} L=${{ matrix.laravel }} Lazy types=${{ matrix.lazy_types }} + name: P=${{ matrix.php }} L=${{ matrix.laravel }} runs-on: ubuntu-latest env: COMPOSER_NO_INTERACTION: 1 @@ -79,9 +78,5 @@ jobs: - run: composer update --prefer-dist --no-progress - - name: Enable lazy types conditionally - run: echo "TESTS_ENABLE_LAZYLOAD_TYPES=1" >> $GITHUB_ENV - if: matrix.lazy_types == 'true' - - name: Run tests run: composer tests diff --git a/CHANGELOG.md b/CHANGELOG.md index 59fdd458..1919f30f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,15 @@ CHANGELOG [Next release](https://github.com/rebing/graphql-laravel/compare/8.5.0...master) -------------- +## Breaking changes +### Removed +- Remove support for eager loading (=non-lazy loading) of types\ + Lazy loading has been introduced in 2.0.0 (2019-08) and has been made the + default since 8.0.0 (2021-11).\ + The practical impact is that types are always going to be resolved using a + type loader and therefore cannot use aliases anymore. Types and their type + name have to match. + 2023-01-13, 8.5.0 ----------------- ### Added diff --git a/README.md b/README.md index 74dbc8ef..3e8883cd 100644 --- a/README.md +++ b/README.md @@ -126,8 +126,6 @@ The default GraphiQL view makes use of the global `csrf_token()` helper function - [Upgrading from v1 to v2](#upgrading-from-v1-to-v2) - [Migrating from Folklore](#migrating-from-folklore) - [Performance considerations](#performance-considerations) - - [Lazy loading of types](#lazy-loading-of-types) - - [Example of aliasing **not** supported by lazy loading](#example-of-aliasing-not-supported-by-lazy-loading) - [Wrap Types](#wrap-types) - [GraphQL testing clients](#graphql-testing-clients) @@ -278,12 +276,6 @@ them, in addition to the global middleware. For example: 'default' => [ 'query' => [ ExampleQuery::class, - // It's possible to specify a name/alias with the key - // but this is discouraged as it prevents things - // like improving performance with e.g. `lazyload_types=true` - // It's recommended to specify just the class here and - // rely on the `'name'` attribute in the query / type. - 'someQuery' => AnotherExampleQuery::class, ], 'mutation' => [ ExampleMutation::class, @@ -447,18 +439,6 @@ Alternatively you can: GraphQL::addType(\App\GraphQL\Types\UserType::class); ``` -As with queries/mutations, you can use an alias name (though again this prevents -it from taking advantage of lazy type loading): -```php -'schemas' => [ - 'default' => [ - // ... - - 'types' => [ - 'Useralias' => App\GraphQL\Types\UserType::class, - ], -``` - Then you need to define a query that returns this type (or a list). You can also specify arguments that you can use in the resolve method. ```php namespace App\GraphQL\Queries; @@ -2696,9 +2676,6 @@ To prevent such scenarios, you can add the `UnusedVariablesMiddleware` to your - `batching`\ - 'enable'\ Whether to support GraphQL batching or not -- `lazyload_types`\ - The types will be loaded on demand. Enabled by default as it improves - performance. Cannot be used with type aliasing. - `error_formatter`\ This callable will be passed the Error object for each errors GraphQL catch. The method should return an array representing the error. @@ -2824,24 +2801,6 @@ The following is not a bullet-proof list but should serve as a guide. It's not a ## Performance considerations -### Lazy loading of types - -Lazy loading of types is a way of improving the start up performance. - -If you are declaring types using aliases, this is not supported and you need to -set `lazyload_types` set to `false`. - -#### Example of aliasing **not** supported by lazy loading - -I.e. you cannot have a query class `ExampleQuery` with the `$name` property -`example` but register it with a different one; this will **not** work: - -```php -'query' => [ - 'aliasedExample' => ExampleQuery::class, -], -``` - ### Wrap Types You can wrap types to add more information to the queries and mutations. Similar as the pagination is working you can do the same with your extra data that you want to inject ([see test examples](https://github.com/rebing/graphql-laravel/tree/master/tests/Unit/WithTypeTests)). For instance, in your query: diff --git a/composer.json b/composer.json index 6fa2c2d9..4bf246a5 100644 --- a/composer.json +++ b/composer.json @@ -69,11 +69,7 @@ "phpstan-baseline": "phpstan analyse --memory-limit=512M --generate-baseline", "lint": "php-cs-fixer fix --diff --dry-run", "fix-style": "php-cs-fixer fix", - "tests": "phpunit --colors=always --verbose", - "tests-all": [ - "TESTS_ENABLE_LAZYLOAD_TYPES=0 phpunit --colors=always --verbose", - "TESTS_ENABLE_LAZYLOAD_TYPES=1 phpunit --colors=always --verbose" - ] + "tests": "phpunit --colors=always --verbose" }, "extra": { "branch-alias": { diff --git a/config/config.php b/config/config.php index 551e9775..03ddeddc 100644 --- a/config/config.php +++ b/config/config.php @@ -111,11 +111,6 @@ // \Rebing\GraphQL\Support\UploadType::class, ], - // The types will be loaded on demand. Default is to load all types on each request - // Can increase performance on schemes with many types - // Presupposes the config type key to match the type class name property - 'lazyload_types' => true, - // This callable will be passed the Error object for each errors GraphQL catch. // The method should return an array representing the error. // Typically: diff --git a/phpstan-baseline.neon b/phpstan-baseline.neon index e1f39142..055b7eef 100644 --- a/phpstan-baseline.neon +++ b/phpstan-baseline.neon @@ -865,21 +865,6 @@ parameters: count: 1 path: tests/Support/Objects/ExamplesAuthorizeQuery.php - - - message: "#^Method Rebing\\\\GraphQL\\\\Tests\\\\Support\\\\Objects\\\\ExamplesConfigAliasQuery\\:\\:resolve\\(\\) has no return type specified\\.$#" - count: 1 - path: tests/Support/Objects/ExamplesConfigAliasQuery.php - - - - message: "#^Method Rebing\\\\GraphQL\\\\Tests\\\\Support\\\\Objects\\\\ExamplesConfigAliasQuery\\:\\:resolve\\(\\) has parameter \\$args with no type specified\\.$#" - count: 1 - path: tests/Support/Objects/ExamplesConfigAliasQuery.php - - - - message: "#^Method Rebing\\\\GraphQL\\\\Tests\\\\Support\\\\Objects\\\\ExamplesConfigAliasQuery\\:\\:resolve\\(\\) has parameter \\$root with no type specified\\.$#" - count: 1 - path: tests/Support/Objects/ExamplesConfigAliasQuery.php - - message: "#^Method Rebing\\\\GraphQL\\\\Tests\\\\Support\\\\Objects\\\\ExamplesFilteredQuery\\:\\:resolve\\(\\) has parameter \\$args with no type specified\\.$#" count: 1 diff --git a/src/GraphQL.php b/src/GraphQL.php index b350a5fe..5496569f 100644 --- a/src/GraphQL.php +++ b/src/GraphQL.php @@ -242,11 +242,7 @@ public function getType(string $name, bool $fresh = false): Type } if (!isset($this->types[$name])) { - $error = "Type $name not found."; - - if ($this->config->get('graphql.lazyload_types', true)) { - $error .= "\nCheck that the config array key for the type matches the name attribute in the type's class.\nIt is required when 'lazyload_types' is enabled"; - } + $error = "Type $name not found. Check that the config array key for the type matches the name attribute in the type's class."; throw new TypeNotFound($error); } @@ -401,11 +397,9 @@ public function buildSchemaFromConfig(array $schemaConfig): Schema return $types; }, - 'typeLoader' => $this->config->get('graphql.lazyload_types', true) - ? function ($name) { - return $this->type($name); - } - : null, + 'typeLoader' => function ($name) { + return $this->type($name); + }, ]); } diff --git a/tests/Support/Objects/ExampleType2.php b/tests/Support/Objects/ExampleType2.php deleted file mode 100644 index f42e7d55..00000000 --- a/tests/Support/Objects/ExampleType2.php +++ /dev/null @@ -1,26 +0,0 @@ - 'Example2', - 'description' => 'An example', - ]; - - public function fields(): array - { - return [ - 'test' => [ - 'type' => Type::string(), - 'description' => 'A test field', - ], - 'test_validation' => ExampleValidationField::class, - ]; - } -} diff --git a/tests/Support/Objects/ExamplesConfigAliasQuery.php b/tests/Support/Objects/ExamplesConfigAliasQuery.php deleted file mode 100644 index bd159e48..00000000 --- a/tests/Support/Objects/ExamplesConfigAliasQuery.php +++ /dev/null @@ -1,40 +0,0 @@ - 'examplesAlias', - ]; - - public function type(): Type - { - return Type::listOf(GraphQL::type('ExampleConfigAlias')); - } - - public function args(): array - { - return [ - 'index' => ['name' => 'index', 'type' => Type::int()], - ]; - } - - public function resolve($root, $args) - { - $data = include __DIR__ . '/data.php'; - - if (isset($args['index'])) { - return [ - $data[$args['index']], - ]; - } - - return $data; - } -} diff --git a/tests/TestCase.php b/tests/TestCase.php index d852791d..b11f1c6c 100644 --- a/tests/TestCase.php +++ b/tests/TestCase.php @@ -18,13 +18,11 @@ use Rebing\GraphQL\Tests\Support\Objects\ExamplesAuthorizeMessageQuery; use Rebing\GraphQL\Tests\Support\Objects\ExamplesAuthorizeQuery; use Rebing\GraphQL\Tests\Support\Objects\ExampleSchema; -use Rebing\GraphQL\Tests\Support\Objects\ExamplesConfigAliasQuery; use Rebing\GraphQL\Tests\Support\Objects\ExamplesFilteredQuery; use Rebing\GraphQL\Tests\Support\Objects\ExamplesMiddlewareQuery; use Rebing\GraphQL\Tests\Support\Objects\ExamplesPaginationQuery; use Rebing\GraphQL\Tests\Support\Objects\ExamplesQuery; use Rebing\GraphQL\Tests\Support\Objects\ExampleType; -use Rebing\GraphQL\Tests\Support\Objects\ExampleType2; use Rebing\GraphQL\Tests\Support\Objects\UpdateExampleMutation; use Symfony\Component\Console\Tester\CommandTester; @@ -43,10 +41,6 @@ protected function setUp(): void protected function getEnvironmentSetUp($app): void { - if ('0' === env('TESTS_ENABLE_LAZYLOAD_TYPES')) { - $app['config']->set('graphql.lazyload_types', false); - } - $app['config']->set('graphql.schemas.default', [ 'query' => [ 'examples' => ExamplesQuery::class, @@ -55,7 +49,6 @@ protected function getEnvironmentSetUp($app): void 'examplesMiddleware' => ExamplesMiddlewareQuery::class, 'examplesPagination' => ExamplesPaginationQuery::class, 'examplesFiltered' => ExamplesFilteredQuery::class, - 'examplesConfigAlias' => ExamplesConfigAliasQuery::class, ], 'mutation' => [ 'updateExample' => UpdateExampleMutation::class, @@ -75,7 +68,6 @@ protected function getEnvironmentSetUp($app): void $app['config']->set('graphql.types', [ 'Example' => ExampleType::class, - 'ExampleConfigAlias' => ExampleType2::class, 'ExampleFilterInput' => ExampleFilterInputType::class, ]); diff --git a/tests/Unit/GraphQLQueryTest.php b/tests/Unit/GraphQLQueryTest.php index a0731316..209ceec1 100644 --- a/tests/Unit/GraphQLQueryTest.php +++ b/tests/Unit/GraphQLQueryTest.php @@ -20,36 +20,6 @@ public function testQueryAndReturnResult(): void ]); } - public function testConfigKeysIsDifferentFromTypeClassNameQuery(): void - { - if (app('config')->get('graphql.lazyload_types')) { - self::markTestSkipped('Skipping test when lazyload_types=true'); - } - - $result = GraphQL::queryAndReturnResult($this->queries['examplesWithConfigAlias']); - - self::assertObjectHasAttribute('data', $result); - - self::assertEquals($result->data, [ - 'examplesConfigAlias' => $this->data, - ]); - } - - public function testConfigKeyIsDifferentFromTypeClassNameNotSupportedInLazyLoadingOfTypes(): void - { - if (false === app('config')->get('graphql.lazyload_types')) { - self::markTestSkipped('Skipping test when lazyload_types=false'); - } - - $result = GraphQL::queryAndReturnResult($this->queries['examplesWithConfigAlias']); - self::assertObjectHasAttribute('errors', $result); - - $expected = "Type Example2 not found. -Check that the config array key for the type matches the name attribute in the type's class. -It is required when 'lazyload_types' is enabled"; - self::assertSame($expected, $result->errors[0]->getMessage()); - } - public function testQuery(): void { $resultArray = GraphQL::query($this->queries['examples']); diff --git a/tests/Unit/TypesInSchemas/QueriesAndTypesEachInTheirOwnSchemaTest.php b/tests/Unit/TypesInSchemas/QueriesAndTypesEachInTheirOwnSchemaTest.php index d9dbd886..2c1d937c 100644 --- a/tests/Unit/TypesInSchemas/QueriesAndTypesEachInTheirOwnSchemaTest.php +++ b/tests/Unit/TypesInSchemas/QueriesAndTypesEachInTheirOwnSchemaTest.php @@ -25,11 +25,6 @@ protected function getEnvironmentSetUp($app): void SchemaTwo\Type::class, ], ]); - - // To still properly support dual tests, we thus have to add this - if ('0' === env('TESTS_ENABLE_LAZYLOAD_TYPES')) { - $app['config']->set('graphql.lazyload_types', false); - } } public function testQueriesAndTypesEachInTheirOwnSchema(): void diff --git a/tests/Unit/TypesInSchemas/TypesTest.php b/tests/Unit/TypesInSchemas/TypesTest.php index 9df8526d..f355a298 100644 --- a/tests/Unit/TypesInSchemas/TypesTest.php +++ b/tests/Unit/TypesInSchemas/TypesTest.php @@ -11,11 +11,6 @@ class TypesTest extends TestCase protected function getEnvironmentSetUp($app): void { // Note: deliberately not calling parent to start with a clean config - - // To still properly support dual tests, we thus have to add this - if ('0' === env('TESTS_ENABLE_LAZYLOAD_TYPES')) { - $app['config']->set('graphql.lazyload_types', false); - } } public function testQueryAndTypeInDefaultSchema(): void