🇦🇹 Vienna
Account created on 19 March 2008, over 17 years ago
#

Merge Requests

More

Recent comments

🇦🇹Austria klausi 🇦🇹 Vienna

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!

🇦🇹Austria klausi 🇦🇹 Vienna

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?

🇦🇹Austria klausi 🇦🇹 Vienna

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

🇦🇹Austria klausi 🇦🇹 Vienna

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?

🇦🇹Austria klausi 🇦🇹 Vienna

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

🇦🇹Austria klausi 🇦🇹 Vienna

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

🇦🇹Austria klausi 🇦🇹 Vienna

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.

🇦🇹Austria klausi 🇦🇹 Vienna

Nice, looks all good to me.

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

🇦🇹Austria klausi 🇦🇹 Vienna

Sure, I'm currently not working on it!

🇦🇹Austria klausi 🇦🇹 Vienna

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...

🇦🇹Austria klausi 🇦🇹 Vienna

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.

🇦🇹Austria klausi 🇦🇹 Vienna

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 .

🇦🇹Austria klausi 🇦🇹 Vienna

remove outdated broken homebrew instructions

🇦🇹Austria klausi 🇦🇹 Vienna

CSS support has been removed from Coder

🇦🇹Austria klausi 🇦🇹 Vienna

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.

🇦🇹Austria klausi 🇦🇹 Vienna

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.

🇦🇹Austria klausi 🇦🇹 Vienna

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.

🇦🇹Austria klausi 🇦🇹 Vienna

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

🇦🇹Austria klausi 🇦🇹 Vienna

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

🇦🇹Austria klausi 🇦🇹 Vienna

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.

🇦🇹Austria klausi 🇦🇹 Vienna

Fully agreed. Merged, thanks!

🇦🇹Austria klausi 🇦🇹 Vienna

Alright, merged that now. Thank you all!

🇦🇹Austria klausi 🇦🇹 Vienna

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.

🇦🇹Austria klausi 🇦🇹 Vienna

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?

🇦🇹Austria klausi 🇦🇹 Vienna

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() {
}
🇦🇹Austria klausi 🇦🇹 Vienna

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!

🇦🇹Austria klausi 🇦🇹 Vienna

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?

🇦🇹Austria klausi 🇦🇹 Vienna

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.

🇦🇹Austria klausi 🇦🇹 Vienna

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.

🇦🇹Austria klausi 🇦🇹 Vienna

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';
  }

}
🇦🇹Austria klausi 🇦🇹 Vienna

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.

🇦🇹Austria klausi 🇦🇹 Vienna

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.

🇦🇹Austria klausi 🇦🇹 Vienna

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!

🇦🇹Austria klausi 🇦🇹 Vienna

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.

🇦🇹Austria klausi 🇦🇹 Vienna

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

🇦🇹Austria klausi 🇦🇹 Vienna

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.

🇦🇹Austria klausi 🇦🇹 Vienna

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.

🇦🇹Austria klausi 🇦🇹 Vienna

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.

🇦🇹Austria klausi 🇦🇹 Vienna

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

🇦🇹Austria klausi 🇦🇹 Vienna

Looks good, thanks!

Uploading stable patch file for composer patches.

🇦🇹Austria klausi 🇦🇹 Vienna

Thanks, uploading stable patch file for composer patches.

🇦🇹Austria klausi 🇦🇹 Vienna

Uploading stable patch file for composer patches.

🇦🇹Austria klausi 🇦🇹 Vienna

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

🇦🇹Austria klausi 🇦🇹 Vienna

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

🇦🇹Austria klausi 🇦🇹 Vienna

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

🇦🇹Austria klausi 🇦🇹 Vienna
🇦🇹Austria klausi 🇦🇹 Vienna

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

🇦🇹Austria klausi 🇦🇹 Vienna

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.

🇦🇹Austria klausi 🇦🇹 Vienna

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

🇦🇹Austria klausi 🇦🇹 Vienna

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

🇦🇹Austria klausi 🇦🇹 Vienna

Thanks, this will be merged soon.

🇦🇹Austria klausi 🇦🇹 Vienna

@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.

🇦🇹Austria klausi 🇦🇹 Vienna

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.

🇦🇹Austria klausi 🇦🇹 Vienna

Thanks, uploading stable patch file for composer patches.

🇦🇹Austria klausi 🇦🇹 Vienna

Looks good to me, thanks!

Uploading stable patch file for composer patches.

🇦🇹Austria klausi 🇦🇹 Vienna

Looks good to me, thanks!

Uploading stable patch file for composer patches.

🇦🇹Austria klausi 🇦🇹 Vienna

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

Also uploading stable patch file for composer patches.

🇦🇹Austria klausi 🇦🇹 Vienna

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

🇦🇹Austria klausi 🇦🇹 Vienna

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

Also attaching stable patch file here for composer patches.

🇦🇹Austria klausi 🇦🇹 Vienna

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

🇦🇹Austria klausi 🇦🇹 Vienna

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

I think the tests are still missing here.

🇦🇹Austria klausi 🇦🇹 Vienna

Thanks, uploading stable patch file for composer patches.

🇦🇹Austria klausi 🇦🇹 Vienna

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.

🇦🇹Austria klausi 🇦🇹 Vienna

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

🇦🇹Austria klausi 🇦🇹 Vienna

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!

🇦🇹Austria klausi 🇦🇹 Vienna

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

🇦🇹Austria klausi 🇦🇹 Vienna

thanks, uploading static file for composer patches.

🇦🇹Austria klausi 🇦🇹 Vienna

Uploading static patch file for composer patches.

🇦🇹Austria klausi 🇦🇹 Vienna
🇦🇹Austria klausi 🇦🇹 Vienna

This will be merged soon.

🇦🇹Austria klausi 🇦🇹 Vienna
Production build 0.71.5 2024