Add sniff to check and fix the order of Use statements

Thanks dpi. It might make sense to re-use that instead of us writing our own in Coder.

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.

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 without an alias. 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 in one run.

I have removed the new sniff and added SlevomatCodingStandard.Namespaces.AlphabeticallySortedUses into the ruleset.xml.

The slevomat fixer requires the same changes to the .inc.fixed and good.php files as the custom sniff, but interesting that it has also changed two other things that don't seem directly related to the ordering of Use Statements.

Details on the pr https://github.com/pfrenssen/coder/pull/187#issuecomment-1458575411

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 statements split into singles on the first run, then the sorting would be done on a subsequent run.

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

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

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 the next version of 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.

Yes, will do.

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

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.

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

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.