Add sniff to check and fix the order of Use statements

I tested out using the slevomat sniff, but it was not as good the one by @cburschka → for several reasons

1. The sorting order is not obvious. It seems to sort using the final class name as prioity, so Drupal\B comes before Drupal\A\C, but this is not always the case
2. In my testing there were some very unpredictable and unexpected results, such as list of five use statements all passing, but just delete the first in the list and then the sniff reports that the remaining four are in the wrong order (why?)
3. It only reports the first line which is wrong. When you fix this manually you then find another one is wrong
4. Sometimes it offers a fixer (but not always). Then when you run the fixer it often fails to fix the problem, and says it can't be fixed

The preference on the coding standards issue was for a simple alphabetical ordering, so even if we accepted the problems 2-4 above, we still have problem 1 as a blocker. Hence I propose we use the sniff from cburschka on github

I have created PR https://github.com/pfrenssen/coder/pull/187 for this. There may be more work to do, as the tests are currently failing.

Thanks a lot for the explanation, I think we can be bold and go ahead with this sniff even if the coding standard was not officially accepted yet.

The most important question for me: are we 100% sure this ordering algorithm is the same as PHPStorm's? It would be super bad if this contradicts PHPStorm in any way. I'm not a PHPStorm user, so would appreciate another person to verify this.

Can anyone else double-check the sniff within PHPStorm?

I've been using SlevomatCodingStandard.Namespaces.AlphabeticallySortedUses for some time and I haven't really ran into the issues as mentioned in #4, except for the fact the sniff will only mention the first incorrectly sorted statement. Are you able to provide some examples? PHPStorm's sorting currently corresponds with the sorting used in Slevomat. The sniff provided by @cburschka also corresponds with the sorting used by PHPStorm, except for the logic when it comes to uppercase and lowercase characters. For example, PHPStorm will sort to the following:

use Drupal\car\CarInterface;
use Drupal\Core\Controller\ControllerBase;
use Drupal\door\DoorInterface;

Which will be marked as correct by Slevomat and marked as invalid on the custom sniff.

Interesting - I would prefer to use upstream code from Slevomat if we can, to avoid reinventing the wheel. We could even contribute there any improvements to the fixer we need.

@jonathan1055 what do you think?

This is an example of the test file I used, which passed with no errors

use Drupal\B;
use Drupal\Core\D;
use Drupal\Z\Z;
use Drupal\Core\File\D;
use Drupal\A\B;

which seemed odd. I then deleted the first row, so that the file had:

use Drupal\Core\D;
use Drupal\Z\Z;
use Drupal\Core\File\D;
use Drupal\A\B;  // <<-- this is reported as wrong

and the fourth line was reported as an error.

I did more testing and simplified the file. The following passes with no error:

use Drupal\B;
use Drupal\Core\D;
use Drupal\A\B;

but remove the first line and it fails:

use Drupal\Core\D;
use Drupal\A\B;  // <<-- this is reported as wrong

But now after @Arkener said that the rules are the same, I thought I must be doing something wrong, so I studied the examples more closely. I then realised that in these simple test files I was repeating the final class name in some places - B, D etc, which you would not do in real life. The slevomat sorting sniff simply ignores any subsequent line which has a repeated final class name and does not report that is in the wrong place. But then, when deleting the various rows as explained above, the ignored use statements are no longer repeats, so are not ignored, and thus they get reported. This is also probably the reason for the 'cannot fix' messages.

So it does look like we can use the slevomat sniff after all, as my test files did not represent real code examples and would have been invalid php.

Just checked further, and when valid aliases are added for the repeated classnames then the fixer can fix multiple incorrect lines.

I have removed the new sniff and added SlevomatCodingStandard.Namespaces.AlphabeticallySortedUses into the ruleset.xml. Interesting that the slevomat fixer has chnaged two other things that don't seem directly related to Use Statement order. Details on the pr https://github.com/pfrenssen/coder/pull/187#issuecomment-1458575411

Thanks Johnathan! It looks we can use the Slevomat sniff for the most part, but it has one bug with multiline use statements. Did you file an issue with Slevomat yet? Setting this to "needs work" in the meantime.

No I have not yet, as I wanted to do some more investigation to make sure I understood the bug (did not want to report it incorrectly). I thought there were two bugs, but now I think it is just one definite bug and one out-of-scope change which was adding to the confusion. When I have raised the issue I'll link to it here.

I have raised the issue on https://github.com/slevomat/coding-standard/issues/1547 but it immediately was closed as "wont fix". I think that's a bit harsh, and commented back that if multiple use statements are not supported then the fixer should not atttept to fix anything, rather than drop pieces of source code lines silently.

I have some ideas on how to fix slevomat, but if they are not willing then we can go back to the in-house version by @cburschka, which I already know how to fix to cater for multiline statements :-)

@klausi, two questions - when we run phpcs, either directly or from within the coder phpunit tests

1. is there any way for Coder to set the sequence of sniffs? for example a parameter with coder's ruleset.xml or something?
2. alternatively, is there a way to force a re-pass of all sniffs on a file if some fixes were made to that file?

The problem I have is that the only fix that will be accepted by Slevomet is to prevent fixing a file wrongly. They do not want to support multi-line use statement. This is fine, except that the Slevomat sniff currently runs before PSR2.Namespaces.UseDeclaration.MultipleDeclarations which splits the statements into singles. So if the slevomat sniff will only fix single use statements, it either has to be run after this PSR2 sniff, or all sniffs run again after the PSR2 has made fixes.

If neither of these are possible, then we cannot use the slevomat sniff to sort use statements with the current test files, because they have multi-line statements. I could submit the fix to stop Slevomat fixing the files incorrectly, and then it would just report the errors in the test files but not fix them. That might be OK, because a developer is likely to run phpcs fixer more than once, so would get the sorting fixes done automatically after the statements have been split into singles.

I think the PHPCBF fixer runs multiple iterations - so if you prevent the erroneous fixing by Slevomat then all should be fixed correctly in multiple passes.

And I don't think it is an issue that Slevomat reports the errors as duplicates of PSR2.Namespaces.UseDeclaration.MultipleDeclarations.

fwiw

The most important question for me: are we 100% sure this ordering algorithm is the same as PHPStorm's? It would be super bad if this contradicts PHPStorm in any way. I'm not a PHPStorm user, so would appreciate another person to verify this.

I'm going to say we've used SlevomatCodingStandard.Namespaces.AlphabeticallySortedUsesin private projects in addition to coder for years. I dont think I've ever seen a disagreement with how PHPStorm's formatter/'Optimize imports' behaves combined with the Slevomat sniff. Ive certainly hit the key combo manually many thousands of times.

@dpi Thanks for that, good to hear.

@klausi Yes I have verified locally that with my changes to the slevomat sniff we don't get the loss of the second parts of the multi-line statements, and also that all the test files do get fixed as hoped, due to multiple iterations of the fixer within one call to phpcbf. So that is good on both counts :-)

I have added test coverage to the slevomat change - the PR is https://github.com/slevomat/coding-standard/pull/1551

Is there a way to use this PR (or apply a patch from it) within Coder's github/workflows/testing.yml so that we can demonstrate that it works?

You could use composer patches in Coder's composer.json to apply your fix, similar as we use patches in Drupal projects.

Thanks Klausi for the hint. Yes I have done that now, and all the Coder tests pass as hoped.
https://github.com/pfrenssen/coder/pull/187

Yay, the Slevomat pull request was merged, I'm super happy for you @johnathan! Thanks for the cross project collaboration.

Next steps: wait for the next Slevomat release and then set that as minimum version for Coder (could be problematic for older PHP versions, since we allow 7.x and 8.x versions, but let's see).

Thanks for your support @klausi, it's nice to have had my first contribution to Slevomat accepted and committed.

Regarding the versions, I have just tested coder on another github branch, getting the latest dev-master of Slevomat coding standards and removing the pacthing of it in composer.json. It ran green and passed on all Coder tests except the first one, at PHP7.1

slevomat/coding-standard dev-master requires php ^7.2 || ^8.0 -> your php version (7.1.33) does not satisfy that requirement

I would say that Coder is aimed at developers, who are most likely running the newer versions of PHP. Coder is not really a module that is used on legacy sites, with old php. Any development on there would be for emergency fixes of old systems, and I don't think it would be a problem if Coder could not be installed on PHP7.1.

So maybe it is time we considered setting the minimum to PHP7.2? Just a thought, to start the discussion off.

I have also checked the Slevomat 7 series, the latest is 7.2.1 but support for PHP7.1 was dropped in 7.1.0. So, if they were to back-port my fix to Slevomat coding-standard 7* then the next version of Coder could have minimum php increased to PHP7.2 and specify the two new minimum slevomat versions 7.2.2 and 8.10.1. If they do not back-port to 7* then we'd have to only allow slevomat 8 and not 7 any more. I guess either of these might also mean a step up in version from Coder 8.3.18 to 8.4.

PHP 7.1 support ended 3 years ago, so I would be fine dropping PHP 7.1 support from Coder. Then we can bump Slevomat to 8.x in our composer file.

It is not perfectly nice to drop older PHP versions without making a new major release, but on the other hand 3 years is long enough.

Pull request looks good, so now we are just waiting for a new Slevomat release.

Slevomat Coding standard was released, so we are unblocked here.

Next steps: Drop PHP 7.1 support in Coder and require Slevomat coding-standard at least 8.11.0. I think we can do it within the scope of this issue and require PHP 7.2 minimum.

@jonathan1055 do you have time to update your pull request with that?

Yes, will do.

Updated PR to required slevomat ^8.11 and dropped support for PHP7.1

Merged, thanks!

Will escalate the coding standards issue now so that it gets priority.

Automatically closed - issue fixed for 2 weeks with no activity.

I observed yesterday that PHPStorm's "sort lines" feature sorts with a different order than is described in comment 6 above.

If I enter into a class the following case insensitively sorted lines, as SlevomatCodingStandard.Namespaces.AlphabeticallySortedUses wants it.

use Drupal\car\CarInterface;
use Drupal\Core\Controller\ControllerBase;
use Drupal\door\DoorInterface;

then use "Edit > Sort lines" in the IDE, it orders like this (case sensitive), Co precedes ca:

use Drupal\Core\Controller\ControllerBase;
use Drupal\car\CarInterface;
use Drupal\door\DoorInterface;

This may be a PHPStorm inconsistency where "Edit > Sort lines" doesn't sort by the same rules as the sorting from "Code > Optimize Imports". Solution seems to be to use the latter, which I hadn't discovered until now!

Optimise imports is the one. Its also present in Prefs -> Tools -> Actions on save, but you'd probably only use that on private projects.

Sort lines is just for sorting each line alphabetically unrelated to imports without any intelligence of context, good for reordering array/symbols.

The updated coding standard does not mention anything about case
https://www.drupal.org/docs/develop/coding-standards/namespaces#order →

The sniff is case-insensitive so the following is the fixed order:

use Drupal\car;
use drupal\Core;
use Drupal\Dare;
use Drupal\door;

I think I will add a sentence about this, just to be really clear.

Could this be made a warning instead of an error?

Hi Dave,
The current thinking, I believe, is that there is not a lot of point in having 'warnings' for standards. I know there are lots already, but if a standard is agreed then it should be enforced (by default). Developers are already confused enough with DrupalPractice sniffs which suggest things but do not enforce them.

If a maintainer does not want to change the code in a project you can ignore the entire rule in your phpcs.xml file.

Jonathan

We distinguish in our CI runs for all of our enterprise consumer sites (who admittedly aren't as well synced in the Drupal community and its standards), that warnings do not fail phpcs runs (using phpcs --runtime-set ignore_warnings_on_exit true), but errors do still return a failure exit code. This feels more like a warning than something that *needs* to be fixed.

Maybe. But the agreed standard says 'must'.
https://www.drupal.org/docs/develop/coding-standards/namespaces#order →
I don't know the background of the standards policy on error vs warning, and whether 'must' is expected to generate an error whereas 'should' will only generate a warning. I expect there are cases of both ways and all combos in the Coder sniffs.

I think you will need to raise a new issue on the coding standards queue if you want to get the message down-graded from error to warning.

Since we are just inheriting this sniff from Slevomat we cannot directly change the severity from error to warning.

Maybe there is a way in PHPCS config to redefine a particular error as warning? Did not look into that, and I'm not convinced we should do this in the canonical Coder ruleset configuration.

But maybe you can research that and use that for your client projects?

Agreed that we shouldn't redefine this to a warning, as the coding standard dictates this as a "MUST".

The following phpcs.xml syntax can be used to redefine the severity for a specific rule when needed:

<rule ref="SlevomatCodingStandard.Namespaces.AlphabeticallySortedUses">
  <type>warning</type>
</rule>