🇦🇹Austria @klausi

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.

Merged, thanks!

Cleaned up test fails in 🐛 Method Drupal\term_merge\Form\MergeTerms::__construct() invoked with 2 parameters, 3 required. Active .

Merged.

klausi → created an issue.

Thanks!

The tests fails look unrelated to me and should be cleaned up in a separate issue.

thanks, approach makes sense to me. Just one confusion about range().

Merged.

Fixed gitlab pipeline fails in 📌 Fix Gitlab pipeline in 4.x Active , merged that into the MR.

Put it on the merge train, should be committed soon.

klausi → made their first commit to this issue’s fork.

Merged.

klausi → created an issue.

Implemented casting to int and merged it, thanks!

OK sorry, can reproduce it now, but for some reason my merge request fixes it when I switch from "?int" to "int". Not sure why this works, but it does. Probably because of the new default value 0.

I thought about setting some non-zero default values for those limits, but they are really application specific and for me it is hard to come up with a value that will be good enough for any Drupal instance. Let me know if you have some inputs there!

New approach is in https://git.drupalcode.org/project/graphql/-/merge_requests/50

I'm super confused by this issue because I cannot reproduce it.

The error makes sense and I assume it would happen, but does not trigger on my local dev site.

What Drupal core version did you use? Which PHP version?

klausi → made their first commit to this issue’s fork.

This should not be needed anymore, closing.

Merging in the 5.x branch fixed the composer fail.

Merged, thanks!

Hi, not sure why we need to fix this, the pipeline on 5.x is passing: https://git.drupalcode.org/project/graphql/-/pipelines/558569

Where is it failing for you? Can you add a link to the issue summary?

Merged.

klausi → created an issue.

Thanks! As there are no PHP fatal errors in our tests I think we can commit this without adding test coverage. Just to make PHPStan happy!

Thanks! I think casting to string here is ok as the result of a dataproducer should probably not be a Translatable markup object.

Thanks, can you also fix examples/graphql_example/src/Wrappers/QueryConnection.php and cleanup up phpstan.neon so that all \Drupal ignores and the comment is removed?

Marking this as fixed, any further doc improvements can be done in a new issue.

Hm, please don't do rebases in merge requests. Now it is very hard for me to review your changes you added. Always do merges form the main branch in merge requests.

composer.lock looks good now, and if I interpret the rebase correctly there are no other changes. Looks ready!

Not sure why it would check JS files ... the default phpcs file from the gitlab template does not use JS files https://git.drupalcode.org/project/gitlab_templates/-/blob/main/assets/p...

Maybe try to copy that and list the extensions?

merge request created.

klausi → created an issue.

klausi → made their first commit to this issue’s fork.

Not sure why the default batch limit is set to 10, that is way too low.

In Drupal 7 we use 500 for the variable search_api_saved_searches_load_max.

Should we update the default value on install to 500?

Uploading stable patch file for composer patches.

Created MR, what do you think?

klausi → created an issue.

Agreed - please put any bad words in the cspell config of Drupal core and the cspell config of the gitlab template.

Thanks, looks good to me except drupal/core-recipe-unpack being added to composer.lock. Can you revert that?

Coder 8.3.30 released!

https://www.drupal.org/project/coder/releases/8.3.30 →

Please update that in composer, then we can move to RTBC I think.

Nice, looks all good to me.

Then I will make a new Coder release now and we can use that.

Sure, I'm currently not working on it!

One nice find with the improved concat sniff: this exposed a bug in test code, yay! The test used ".." but should be "." https://git.drupalcode.org/project/drupal/-/merge_requests/12018/diffs#6...

Found one more Coder issue: 🐛 Compley array<> expressions should be allowed in @return comments Active .

I think we should upgrade straight to Coder 8.3.30 here (not released yet).

I started to work through the newly reported issues and also removed deprecated sniffs. This will not pass yet, but let me know if you like the approach.

I tested with a git checkout of Coder in the vendor directory, to ensure I'm running on the latest version of Coder. Once everything is passing here as we want it I can make a new Coder release.

Merged.

klausi → created an issue.

While testing the latest Coder dev version on core I detected another phpcs ignore inconvenience and fixed it in 📌 Allow phpcs:ignore before param comments Active .

Merged.

klausi → created an issue.

remove outdated broken homebrew instructions

CSS support has been removed from Coder

Note that CSS support has been removed from Coder. To check and fix CSS files please use Stylelint and use the Drupal core .stylelintrc.json configuration file.

Note that CSS support has been removed from Coder. To check and fix CSS files please use Stylelint and use the Drupal core .stylelintrc.json configuration file.

Note that CSS support has been removed from Coder. To check and fix CSS files please use Stylelint and use the Drupal core .stylelintrc.json configuration file.

Note that CSS support has been removed from Coder. To check and fix CSS files please use Stylelint and use the Drupal core .stylelintrc.json configuration file.

Yeah, I set it to hidden. Also added the PR link in the issue summary.

klausi → changed the visibility of the branch 3524264-deprecate-all-css to hidden.

We use cspell now to check for spelling, so another approach would be to add more bad flagWords into core's .cspell.json file. But I think that has the downside that it would not propagate to Drupal contrib projects, they would have to add the same flagWords to their cspell config? Not sure how the cspell gitlab job works.

So a sniff in Coder could be the more universal approach.

Fully agreed. Merged, thanks!

Alright, merged that now. Thank you all!

Thanks, did some manual test runs myself and realized we cannot implement DeprecatedSniff. The problem is that we cannot silence the deprecation from our ruleset.xml file. That means all users will always get the deprecated message which they cannot fix.

My workaround is now to just make the deprecated sniffs empty so that they don't check anything anymore. Not great, but at least it will not break the CI pipelines for people that still reference those sniffs in their config.

I updated the issue summary with instructions for the release notes.

I see, and is your global "phpcs" command pointing to the dev version of your Coder install?

What does "which phpcs" return? Is it the path where you installed Coder 8.3.x from git and PHPCS with it in the vendor directory?

Sorry, I used the wrong words. I meant PHP attributes:

/**
 * Test with attribute before.
 */
// @phpstan-ignore-next-line
#[ExampleAttribute('foo', 'bar')]
public function ignore_phpstan_comment() {
}

and

/**
 * Test with attribute after.
 */
#[ExampleAttribute('foo', 'bar')]
// @phpstan-ignore-next-line missingType.return
public function ignore_phpstan_comment() {
}

Thanks, approach looks good to me!

The test-only way would be to open a new pull request just with the test additions, can't think of a better way right now.

Yes, please add the other test cases!

Thanks for testing!

Do you have those sniffs enabled in your phpcs.xml config file? If yes then the deprecations make sense. How do you invoke phpcs?

Thanks for reporting!

Can you put a full code example where the problem triggers in the issue summary?

I tried to reproduce with this, but works fine for me:

use Foo\Bar\Testi;

/**
 * Example.
 *
 * @phpstan-type UserAddress array{street: string, city: string, zip: string}
 */
class Test {

  /**
   * Address in the form of an array, defined on the class comment.
   *
   * @var UserAddress
   */
  protected $address;

  /**
   * Getter.
   *
   * @return UserAddress
   *   The address.
   */
  public function getAddress(): array {
    /** @var UserAddress $otherAddress */
    $otherAddress = [new Testi()];
    return $this->address;
  }

}

What am I doing wrong?

Side note: Coder cannot check if classes exist. It is coding standards checker that cannot access/process other PHP files to check if classes exist. We are deliberately leaving that to PHPStan.

As far as I understand our DataTypeNamespaceSniff it only checks that if a type matches the use statement, then the FQN should be used.

Merged that one.

For Drupal core you have 3 options:
1) Add a return comment to your @return docs, remove phpcs:ignore (best solution IMO)
2) Globally ignore Drupal.Commenting.FunctionComment.MissingReturnComment in the phpcs.xml.dist config file when updating Coder, fix those instances later
3) Wait for Coder 8.3.30 with this fix and upgrade to that, but you will still have to move the phpcs:ignore statements into the doc comment before the @return as in the example here.

Thanks for reporting!

We improved the handling of phpcs ignores, and now the exact line where the problem is (the @return line in the doc comment) is returned. I assumed it would be enough to move "phpcs:ignore Drupal.Commenting.FunctionComment.MissingReturnComment" into the doc comment, but turns out that this results in a different error:

/**
 * Provides a collection of condition plugins.
 */
class ConditionPluginCollection extends DefaultLazyPluginCollection {

  /**
   * {@inheritdoc}
   *
   * phpcs:ignore Drupal.Commenting.FunctionComment.MissingReturnComment
   * @return \Drupal\Core\Condition\ConditionInterface
   */
  public function &get($instance_id) {
    return 'x';
  }

}

Opened a PR to make that work as well: https://github.com/pfrenssen/coder/pull/265/files

Of course it would be so much better if you just add the missing return comment. Then you don't need confusing phpcs ignore statements littered in Drupal core. Example:

/**
 * Provides a collection of condition plugins.
 */
class ConditionPluginCollection extends DefaultLazyPluginCollection {

  /**
   * {@inheritdoc}
   *
   * @return \Drupal\Core\Condition\ConditionInterface
   *   The condition for the given instance ID.
   */
  public function &get($instance_id) {
    return 'x';
  }

}

Thanks for reporting!

This was already fixed in 📌 fix(FunctionComment): Allow PHPCS ignore directives before functions Active . Please upgrade to the latest version of Coder.

There is a similar issue for PHPStan which is not fixed yet ( 🐛 @phpstan-ignore-next-line annotation collides with PHPCS Active ), but the example phpcs ignore in the issue summary is working.

Right, thanks for reporting! I'm wrapping this into 🐛 Deprecated sniffs Active where I'm cleaning up all deprecations and remove JS and CSS handling code.

This is ready for review now. We are preserving backwards compatibility by keeping the sniffs but deprecating them.

I also added a sentence to the Coder project page: "Note that CSS support has been removed. To check and fix CSS files please use Stylelint and use the Drupal core .stylelintrc.json configuration file."

Let me know if you see any problem with this approach!

Good question - I think you can put it in a public static function, if that is possible. Then all classes that need it can call into that. Not the most obvious solution but should work for now, we can always try to expose it better in the future.

Started at https://github.com/pfrenssen/coder/pull/264

Thanks, I think the approach makes sense!

Please add test cases to good.php:
1. phpstan ignore before function
2. phpstan ignore before annotation and function
3. phpstan ignore between annotation and function

Thanks for reporting!

I think you can use PHPStan ignores like this:

class Schema extends BaseMySqlSchema {

  /**
   * {@inheritdoc}
   *
   * @phpstan-ignore-next-line missingType.return 
   */
  public function addField($table, $field, $spec, $keys_new = []) {
    if ...

Can you try if that works? I would like avoid writing special code to detect and ignore PHPStan comments as that would make a lot of our sniffs more complicated.

Downgrading priority to normal, I don't think this is a major issue as you can do this workaround.

Clarified the problem.

Duplicate of 📌 Can we enforce case statements ending in a : instead of a ; Active .

PHP-CS-Fixer is a different system than PHPCS. Coder uses PHPCS sniffs.

Integrating or running PHP-CS-Fixer is a bit out of scope for this Coder issue, but I'm open to review approaches how we could leverage PHP-CS-Fixer as parallel system to PHPCS in Coder.

Hm, the point in 4.x was to have it explicitly as separate class, see the discussion in https://github.com/drupal-graphql/graphql/issues/1301 .

So I would not backport this. For graphql compose I assume you need to start a new git branch anyway? There are some small typing changes that could be incompatible between graphql 4.x and 5.x. Not sure if it is possible to write graphql compose to be compatible with both versions.