Skip to content
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

WIP: Symfony 7 and GraphQLite 8 #1

Closed
wants to merge 32 commits into from
Closed

WIP: Symfony 7 and GraphQLite 8 #1

wants to merge 32 commits into from

Conversation

andrew-demb
Copy link
Owner

@andrew-demb andrew-demb commented Nov 16, 2024

Continuation of thecodingmachine#203

@andrew-demb
Copy link
Owner Author

Refs: thecodingmachine/graphqlite#708 (problem with parameter annotations and validator annotations)

@andrew-demb
Copy link
Owner Author

Refs: thecodingmachine/graphqlite#707 (stable release for commits about improving performance and dropping doctrine annotations support)

@SCIF
Copy link

SCIF commented Nov 17, 2024

TBH, I spent some time but couldn't narrow down the root of the problem with inability to find a type for UserInterface. @andrew-demb , were you successful in this?

@andrew-demb
Copy link
Owner Author

@SCIF I will try to investigate it today later. After 15 minutes yesterday I failed to find the reason.

There is more than one problem remaining

@andrew-demb
Copy link
Owner Author

TBH, I spent some time but couldn't narrow down the root of the problem with inability to find a type for UserInterface. @andrew-demb , were you successful in this?

@SCIF it is related to the in-memory cache and ordering of type mappers ().
If I would patch in vendor \TheCodingMachine\GraphQLite\SchemaFactory::createSchema to ensure, that \TheCodingMachine\GraphQLite\Mappers\StaticClassListTypeMapperFactory::create will be added to the composite at first (before the ClassFinderTypeMapper with configured graphqlite namespace from the userland) - tests will be passed.

I need to investigate further about the latest performance improvements that was added to the graphqlite to find the root cause.

Here's a monkey patch:

diff --git a/SchemaFactory.php b/SchemaFactory.php
index 3991341..8f4c7e7 100644
--- a/SchemaFactory.php
+++ b/SchemaFactory.php
@@ -462,20 +462,6 @@ class SchemaFactory
         $inputTypeUtils = new InputTypeUtils($annotationReader, $namingStrategy);
         $inputTypeGenerator = new InputTypeGenerator($inputTypeUtils, $fieldsBuilder, $this->inputTypeValidator);
 
-        if ($this->namespaces) {
-            $compositeTypeMapper->addTypeMapper(new ClassFinderTypeMapper(
-                $classFinder,
-                $typeGenerator,
-                $inputTypeGenerator,
-                $inputTypeUtils,
-                $this->container,
-                $annotationReader,
-                $namingStrategy,
-                $recursiveTypeMapper,
-                $classFinderComputedCache,
-            ));
-        }
-
         if (! empty($this->typeMapperFactories) || ! empty($this->queryProviderFactories)) {
             $context = new FactoryContext(
                 $annotationReader,
@@ -507,6 +493,19 @@ class SchemaFactory
             $compositeTypeMapper->addTypeMapper($typeMapper);
         }
 
+        if ($this->namespaces) {
+            $compositeTypeMapper->addTypeMapper(new ClassFinderTypeMapper(
+                $classFinder,
+                $typeGenerator,
+                $inputTypeGenerator,
+                $inputTypeUtils,
+                $this->container,
+                $annotationReader,
+                $namingStrategy,
+                $recursiveTypeMapper,
+                $classFinderComputedCache,
+            ));
+        }
         $compositeTypeMapper->addTypeMapper(new PorpaginasTypeMapper($recursiveTypeMapper));
 
         $queryProviders = [];

@andrew-demb
Copy link
Owner Author

I need to investigate further about the latest performance improvements that was added to the graphqlite to find the root cause.

The root cause, is that there're at least two instances of \TheCodingMachine\GraphQLite\Mappers\ClassFinderTypeMapper in graphqlite, with different implementations/instances of \TheCodingMachine\GraphQLite\Discovery\ClassFinder (\TheCodingMachine\GraphQLite\Mappers\ClassFinderTypeMapper::__construct(classFinder)), but ClassFinderTypeMapper generates the same cache key for \TheCodingMachine\GraphQLite\Discovery\Cache\ClassFinderComputedCache::compute - 'classToAnnotations'

This obviously should be fixed in the graphqlite

@andrew-demb
Copy link
Owner Author

OK, I managed with the proper solution. I will create a PR in the graphqlite about avoiding the reuse of the same cache key for different class finders today.

@SCIF
Copy link

SCIF commented Nov 18, 2024

OK, I managed with the proper solution. I will create a PR in the graphqlite about avoiding the reuse of the same cache key for different class finders today.

Great news! I found out the caching mechanism extremely weird during debugging and requiring high focus on details.

@andrew-demb
Copy link
Owner Author

I temporarily switched to the own fork of graphqlite to show the fully green CI.

Awaits upstream graphqlite PRs and a stable release:

After that - this PR can be recreated to the upstream graphqlite-bundle instead of thecodingmachine#203

@andrew-demb
Copy link
Owner Author

I need a stable release of graphqlite (it will be 8.0.0 thecodingmachine/graphqlite#708 (comment)) and to create PR in https://github.com/thecodingmachine/graphqlite-symfony-validator-bridge to make it compatible with the future 8.0.0

@enricobono
Copy link

graphqlite 8.0.0 has been tagged 🎉 😊

Ref:

@andrew-demb andrew-demb changed the title WIP: Symfony 7 WIP: Symfony 7 and GraphQLite 8 Dec 18, 2024
@andrew-demb
Copy link
Owner Author

I need to rebase this PR over this one: thecodingmachine#227

And will land PR to the origin (Symfony 7 + GraphQLite 8)

fogrye and others added 22 commits December 18, 2024 13:15
…ions, simplify phpunit config and to improve autoloader configuration
…opped support for doctrine annotations. Drop support for doctrine annotations
# Conflicts:
#	.travis.yml
#	composer.json
@andrew-demb
Copy link
Owner Author

PR landed in upstream thecodingmachine#229

@andrew-demb andrew-demb closed this Jan 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants