The subquery is needed because a JOIN to itself can also be slow when you have many revisions. I analyzed that in π Entity queries querying the latest revision very slow with lots of revisions Needs work where this regression was introduced.
We have 2 problems I see:
* The self join is very bad for entities with thousands of revisions
* The subquery is very bad for sites with millions of entities
Coder 9.0.0-alpha1 released, but I plan to check some more sniffs we could remove by upstream sniffs. https://www.drupal.org/project/coder/releases/9.0.0-alpha1 β
Thanks, can I also get credit on this issue? I adapted Coder over the last 10 years to support what drupal core needs π
The biggest chunk of work was done in π Update php_codesniffer to 4.0 Needs work .
I started to replace some sniffs with sniffs from Slevomat, which is nice to reduce the code to maintain in Coder.
I would like to keep Drupal 7 support as I'm still running lots of Drupal 7 sites and use Coder as part of the dev process :)
I committed the work you started to 9.x, continuing at https://github.com/pfrenssen/coder/pull/272
Merged https://github.com/pfrenssen/coder/pull/270 . Used the wrong issue number in the commit to π Typo in WidgetInterface.php Active , but should not matter.
I tweaked the regex to allow 3 dots at the end and only target 2 dots.
As this coding standard should apply to all Drupal code we should do the same Slevomat sniff with config in Coder's ruleset.xml file.
Core is getting out of sync with Coder rules again ... please always make pull requests against Coder as well when you change core's phpcs.xml.dist file, so that we have the same standards everywhere.
I'm open to pull requests at https://github.com/pfrenssen/coder if you want to unify this.
Thanks, approach makes sense. Left one minor comment on the PR https://github.com/pfrenssen/coder/pull/268
Hey thanks!
I think we should probably make a new Coder 9.x branch when we adopt this major upgrade.
Feel free to continue working in a pull request again 8.3.x in the meantime. We do pull requests on Github, please open one against https://github.com/pfrenssen/coder
Thanks, merged!
Usually I try to make releases every 3 months if changes were merged in that period.
I think I can make a new release at Drupalcon on contribution day next Friday.
Thanks, changes look good to me, the only thing missing is now the test fixing.
klausi β made their first commit to this issueβs fork.
This change caused a critical performance regression, the query is factor 1000 slower on a big site with 2 million paragraph entities.
You can find my analysis in the critical follow-up issue π Updating to 10.5.3 causes gateway timeouts on revisioned content Active .
We have 2 problems I see:
* The old query is very bad for entities with thousands of revisions
* The new query is very bad for sites with millions of entities
For comparison the reverted query, it takes 0.2ms to run (factor 1000 faster):
SELECT base_table.revision_id AS revision_id, base_table.id AS id
FROM
paragraphs_item_revision base_table
LEFT OUTER JOIN paragraphs_item_revision base_table_2 ON base_table.id = base_table_2.id AND base_table.revision_id < base_table_2.revision_id
INNER JOIN paragraphs_item_field_data paragraphs_item_field_data ON paragraphs_item_field_data.id = base_table.id
WHERE (base_table_2.id IS NULL) AND (paragraphs_item_field_data.id = '6091304')
EXPLAIN output showing that an index can be used for all parts:
Having an unlimited SELECT subquery is not a good idea, this resulting query for 2 million paragraphs items takes 2 seconds to run:
SELECT base_table.revision_id AS revision_id, base_table.id AS id
FROM
paragraphs_item_revision base_table
INNER JOIN paragraphs_item_field_data paragraphs_item_field_data ON paragraphs_item_field_data.id = base_table.id
WHERE (base_table.revision_id IN (SELECT MAX(base_table.revision_id) AS expression
FROM
paragraphs_item_revision base_table
GROUP BY base_table.id)) AND (paragraphs_item_field_data.id = '6091304');
EXPLAIN output shows the subquery as the only part that does not use a DB index:
Uploading the revert for 10.5.x as stable patch file for composer patches.
Thanks! Can you open a PR at https://github.com/pfrenssen/coder/pulls ? We are still doing the pull requests there for now.
Merge request: https://git.drupalcode.org/project/feeds_http_auth_fetcher/-/merge_reque... (seems broken, not sure why)
Attaching stable patch for composer patches.
Here is an additional update function to switch the DB schema to blob for existing sites.
GraphQL 5.0.0-beta1 released, please test!
Nice, thanks!
Uploading stable patch file for composer patches.
Sorry, this was merged after alpha3, please test the dev version.
I will make a new release soon as well.
Ah sorry, it was merged, just the commit does not show up here. Best include the issue number in the commit message next time.
This issue was not fixed yet, right? Let's leave it at RTBC until the fix is merged and committed.
looks good, thanks!
typos
started preparing the API changes for the 5.0.0 release notes.
Should probably be moved to a change record at some point.
I'm planning to release a first beta version soon, thanks a lot for testing the alpha release (2 issues on that fixed already in 5.x dev).
Merged the small fixes in the pull request, thanks @almunnings!
I'm not sure that this will fix the cache issue here, please test if this helps.
Then I can make a new 4.x release soon.
Merged.
merge request created.
Merged that one already, let me know if you still see a problem!
This should do it, please test!
Aha good point, I should have checked Drupal core plugins.
Is it enough to just add this property to the attribute classes? Or do we have to do anything on the plugin managers as well? Probably not, as you say the property is defined on the parent Plugin attribute class.
Thanks for reporting!
I had the same problem in our installation because the 2 int properties were missing in our graphql entity server config:
query_depth: 20
query_complexity: 0
I wrote a custom update function to work around that:
/**
* Set the missing query_depth and query_complexity values.
*/
function jobiqo_graphql_update_10103(): void {
\Drupal::configFactory()
->getEditable('graphql.graphql_servers.jobiqo_schema')
->set('query_depth', 20)
->set('query_complexity', 0)
->save();
}
The int values were introduced in π 5.x fatal error creating new severs Active .
We have 2 options to solve this:
1. Implement constructor on the Server entity and always cast those 2 to int
2. Write an update function to update all graphql server entities that miss those 2 properties, where they are not int.
I'm inclined to go with option 2, so that we don't have to maintain workaround code in the constructor.
GraphQL 5.0.0-alpha3 released!
Mostly refactoring to use PHP attributes and some bug fixes.
Thank you Daniel Bosen, Al Munnings, Andrii Aleksandrov and Ben Stallings for contributing fixes!
https://www.drupal.org/project/graphql/releases/5.0.0-alpha3 β
I think we can do the first beta soon, let me know of any blockers you see.
Thanks, merged!
Thanks, merged!
Not the best test as it would be better to test a real entity, but I think we don't care.
merged.
Thanks, I think we need to expand the test coverage to test with string and TranslatableMarkup for the description.
Ah ok, I don't think we need a functional test, we can just use a real entity. Can you create a node in the test and use that instead of a mock? We have a database connection available in our kernel tests.
Thanks, that is surprising! Can you add a test case? Looks like test coverage was missing before, but we should ensure the code is tested.
Pull request ready.
This was the final prompt for AI: "convert the doc block annotation to php attributes. use statement: "use Drupal\graphql\Attribute\DataProducer;". For ContextDefinition use Drupal\Core\Plugin\Context\ContextDefinition. keep the top doc comment sentence. use named parameters for all ContextDefinition constructors, the data type property is called "data_type". Use TranslatableMarkup for @Translation. make sure the use statements on top of the file are sorted alphabetically. for each property of ContextDefintion use a new line. If the data type of a context definition starts with "entity:" then use the class EntityContextDefinition with use statement Drupal\Core\Plugin\Context\EntityContextDefinition. If the data type of a context definition is exactly "entity" then use the class ContextDefinition with use statement Drupal\Core\Plugin\Context\ContextDefinition. Make sure that all multiline array elements have a comma at the end. Ensure that all context definitions in "consumes" are present in the attribute conversion and have the same name. the number of entries in "consumes" needs to be the same. If the class already has an attribute then skip the file."
I took me 5 hours to create this pull request. Was I really faster with AI? Maybe a bit, but fiddling and reviewing changes also takes time. This might be a typical case where a developer thinks they are more productive with AI and suddenly 5 hours have passed.
At least I enjoyed the refactoring more as I did not have to type out or copy/paste all the modifications. I would estimate that 95% of the changes the AI created correctly and the 5% rest I had to fix manually.
I'm very thankful for the automated tests, which caught a lot of small mistakes that I then had to feed back into the prompt.
I used Github copilot in agent mode with model Gemini 2.5 Pro.
klausi β made their first commit to this issueβs fork.
Merged.
klausi β made their first commit to this issueβs fork.
Merged.
klausi β made their first commit to this issueβs fork.
Merged.
klausi β made their first commit to this issueβs fork.
Nice, thank you!
Merged.
I will try to convert the annotations to attributes with AI, as the Drupal rector tool did not work for me. Issue: π Use attributes to define DataProducer plugins Active
Merged.
Is the rector code supposed to work with contrib plugin annotations and attributes supposed to work? Tried to convert graphql dataproducer plugins, but I get no output.
Rector config:
<?php
declare(strict_types=1);
use Drupal\graphql\Attribute\DataProducer;
use DrupalRector\Drupal10\Rector\Deprecation\AnnotationToAttributeRector;
use DrupalRector\Drupal10\Rector\ValueObject\AnnotationToAttributeConfiguration;
use Rector\Config\RectorConfig;
return static function (RectorConfig $rectorConfig): void {
$rectorConfig->ruleWithConfiguration(AnnotationToAttributeRector::class, [
new AnnotationToAttributeConfiguration('10.0.0', '10.0.0', 'DataProducer', DataProducer::class),
]);
if (class_exists('DrupalFinder\DrupalFinderComposerRuntime')) {
$drupalFinder = new DrupalFinder\DrupalFinderComposerRuntime();
} else {
$drupalFinder = new DrupalFinder\DrupalFinder();
$drupalFinder->locateRoot(__DIR__);
}
$drupalRoot = $drupalFinder->getDrupalRoot();
$rectorConfig->autoloadPaths([
$drupalRoot . '/core',
$drupalRoot . '/modules',
$drupalRoot . '/profiles',
$drupalRoot . '/themes'
]);
$rectorConfig->skip(['*/upgrade_status/tests/modules/*']);
$rectorConfig->fileExtensions(['php', 'module', 'theme', 'install', 'profile', 'inc', 'engine']);
$rectorConfig->importNames(true, false);
$rectorConfig->importShortClasses(false);
};
Invocation:
vendor/bin/rector process modules/graphql --dry-run
output:
klausi@drupal-11-web:/var/www/html$ vendor/bin/rector process modules/graphql
36/68 [ββββββββββββββββββββββββββββ] 52%
In ProcessPool.php line 30:
Process "qabrfesumt" not found.
Looks like rector is crashing here?
Did anyone convert contrib annotation successfully with an automated tool?
klausi β created an issue.
klausi β created an issue.
Merged, thanks!
Merged!
Thanks for reporting - I would like to take a slightly different approach by refactoring this config setup into a common method on the schema plugin manager.
klausi β made their first commit to this issueβs fork.
Resolved merge conflict, uploading stable patch file for composer patches.
klausi β made their first commit to this issueβs fork.
Merged in the 2.0 branch and resolved conflicts.
Uploading stable patch file for composer patches.