🇦🇹Austria @klausi

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.

The first alpha version was released, please test! https://www.drupal.org/project/graphql/releases/5.0.0-alpha1 →

Merged.

Add 📌 Merge AlterableComposableSchema into ComposableSchema Active

klausi → created an issue.

Looks good, thanks!

Uploading stable patch file for composer patches.

Thanks, uploading stable patch file for composer patches.

Uploading stable patch file for composer patches.

Sorry, this was already done in the dev version of the module.

klausi → created an issue.

Thanks, looks good! Uploading stable patch file for composer patches.

merge request created, uploading stable patch fiel for composer patches.

klausi → created an issue. See original summary → .

klausi → created an issue.

Turns out the remaining LoigicExceptions in code are correct, I don't think we can remove them.

I merged this now - we can do anything that is unclear or could be improved in follow-ups. Better we get this big change out of the way now so that we can work better on other improvements.

Merged, thanks!

@Andriy: I pushed some changes to the pull request, we can improve the AlterableSchemaTest to test the actual AlterableComposableSchema class.

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

Thanks, this will be merged soon.

@adamps: Thank you! Can you make a new release?

Currently we have the 4.x-dev version in composer, but Simplenews Scheduler does not like that when there is no version in Simplenews.

Thanks - sorry we don't provide compatibility guarantees for test code, downgrading to normal.

I think we can add those methods, but be aware that we don't have test coverage for this test code, so it could break again in the future. But I think that is ok, it is just test code afterall.

Thanks, uploading stable patch file for composer patches.

Looks good to me, thanks!

Uploading stable patch file for composer patches.

Looks good to me, thanks!

Uploading stable patch file for composer patches.

I also fixed another deprecated warning in the tests, I think this can stay RTBC.

Also uploading stable patch file for composer patches.

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

I pushed the empty array as default value instead, please review!

Also attaching stable patch file here for composer patches.

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

Merge request updated. Uploading stable patch file here for composer patches.

I think the tests are still missing here.

Thanks, uploading stable patch file for composer patches.

Thanks for reporting!

This happens when you have no blank line after the PHP open tag. Somehow the Squiz standard confuses Drupal file comments with the function comment. But it does not matter, you can fix that by having a blank line before the file comment.

Having a blank line after the PHP open tags is defined in our coding standards, for example https://www.drupal.org/docs/develop/standards/php/php-coding-standards#s... →

So I think there is nothing to fix here.

PHPCS 3.12 was released that contains my fix, I bumped the version in Coder's composer file.

Sure, but those annotation changes are not breaking changes? So we could do the them anytime later as well?

But if you would like to work on them I'm happy to include the conversions!

Updated the merge request. Uploading stbale patch file for composer patches.

thanks, uploading static file for composer patches.

Uploading static patch file for composer patches.

jjchinquist → credited klausi → .

This will be merged soon.

klausi → created an issue.