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

Allow schema caching per server when using a common base schema #1224

Open
wants to merge 11 commits into
base: 8.x-4.x
Choose a base branch
from

Conversation

darvanen
Copy link
Contributor

@darvanen darvanen commented Jul 22, 2021

I am NOT by any means suggesting this at the fix approach, it merely shows where the problem lies so a discussion can be had regarding how to fix it.
Updated to a viable patch, though there are other ways we could approach it.

@darvanen
Copy link
Contributor Author

See #1223 for initial notes

@darvanen
Copy link
Contributor Author

I'm sorry, I went to try and create a test to demonstrate this but it's completely beyond me.

@darvanen
Copy link
Contributor Author

Ok, now this PR contains a potential fix using a setter on the plugin to provide the server ID.

We could probably also do it by putting another key on the configuration that gets passed to the plugin but this seemed safer to me.

@darvanen darvanen changed the title 1223-schema-caching demonstrate problem Allow schema caching per server when using a common base schema Jul 23, 2021
@darvanen
Copy link
Contributor Author

Alright I give up. The last test failure is a conflict between PHPStan and phpcs

PHPStan:

 ------ ------------------------------------------------------------------ 
  Line   src/Plugin/SchemaPluginInterface.php                              
 ------ ------------------------------------------------------------------ 
  23     PHPDoc tag @param has invalid value (string                       
           The machine name of the server using this plugin.): Unexpected  
         token "\n   * ", expected variable at offset 84                   
 ------ ------------------------------------------------------------------ 

phpcs:

FILE: ...odules/graphql/src/Plugin/GraphQL/Schema/SdlSchemaPluginBase.php
----------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
----------------------------------------------------------------------
 47 | ERROR | [x] Do not append variable name "$serverId" to the type
    |       |     declaration in a member variable comment
----------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

@darvanen darvanen marked this pull request as ready for review July 23, 2021 02:49
src/Plugin/GraphQL/Schema/SdlSchemaPluginBase.php Outdated Show resolved Hide resolved
src/Plugin/SchemaPluginInterface.php Outdated Show resolved Hide resolved
@klausi klausi added the 4.x label Oct 7, 2021
@darvanen darvanen requested a review from Kingdutch September 3, 2024 02:13
Comment on lines +121 to +127
/**
* {@inheritdoc}
*/
public function setServerId(string $serverId): void {
$this->serverId = $serverId;
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of using a setter to configure the server ID, I'm wondering if it wouldn't be better to make the plugin configurable by implementing ConfigurableInterface. In fact, both schema plugins we ship are already configurable (but the base class SdlSchemaPluginBase isn't, and changing this would be a B/C break).

Comment on lines -182 to +196
$cid = "schema:{$this->getPluginId()}";
$cid = "server:{$this->serverId}:schema:{$this->getPluginId()}";

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This assumes that the setter has always been called, but it might not have been, and $this->serverId might be NULL.

Probably using a setter is not the best approach. Since the server ID is required for the plugin to work in the current it is essential that the setter is always called. The setter is not part of the SchemaPluginInterface and it also should not be, since it is not essential for all schema plugins, only for the ones that use caching.

Maybe we can add a server_name key to the standard plugin configuration and check that this is populated in the plugin constructor? That would also enable us to gracefully introduce this change. We can emit a deprecation warning if it is missing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants