Odd grouping behaviour with `ordered_imports`

[msbit]

Bug report

Description

With a PHP file that has manually separated import groupings, ordered_imports seems to act a little weird, effectively maintaining the count of each element in the groupings but ordering the imports alphabetically. My expectation is that it would (or would be configurable to) order each of the groupings independent of the others.

Runtime version

PHP CS Fixer 3.42.0 Three Keys by Fabien Potencier and Dariusz Ruminski.
PHP runtime: 8.0.30

Used command

$ php-cs-fixer fix foo.php -vvv
PHP CS Fixer 3.42.0 Three Keys by Fabien Potencier and Dariusz Ruminski.
PHP runtime: 8.0.30
Loaded config default from "/private/var/folders/4r/wj429mzd6fn32n08kql83zsw0000gn/T/tmp.cJpT7Vq8/Parrot-Refactoring-Kata/PHP/.php-cs-fixer.php".
Using cache file ".php-cs-fixer.cache".
F                                                                                                                                                                                                                                1 / 1 (100%)
Legend: .-no changes, F-fixed, S-skipped (cached or empty file), I-invalid file syntax (file ignored), E-error
   1) foo.php (ordered_imports)

Fixed 1 of 1 files in 0.004 seconds, 14.000 MB memory used

Configuration file

<?php

return (new PhpCsFixer\Config())
    ->setRules([
        'ordered_imports' => true,
    ]);

Code snippet that reproduces the problem

Input (and expected output)

<?php

namespace Parrot\Tests;

use PHPUnit\Framework\TestCase;

use Parrot\Parrot;
use Parrot\ParrotTypeEnum;

class ParrotTest extends TestCase {}

Output

<?php

namespace Parrot\Tests;

use Parrot\Parrot;

use Parrot\ParrotTypeEnum;
use PHPUnit\Framework\TestCase;

class ParrotTest extends TestCase {}

Diff

diff --git a/PHP/foo.php b/PHP/foo.php
index 90e48f6..e7aaaaf 100644
--- a/PHP/foo.php
+++ b/PHP/foo.php
@@ -2,9 +2,9 @@

 namespace Parrot\Tests;

-use PHPUnit\Framework\TestCase;
-
 use Parrot\Parrot;
+
 use Parrot\ParrotTypeEnum;
+use PHPUnit\Framework\TestCase;

 class ParrotTest extends TestCase {}

[Wirone]

I believe it was introduced in #7023 and I pointed it out, but it was merged anyway as it fixed other issues 😅. I agree that this is not an expected behaviour and I would rather see 2 strategies: group everything and sort all imports, or order imports separately in each group. What do you think?

[msbit]

@Wirone

What do you think?

I'll restate my understanding of the two strategies, hopefully that matches with what you're proposing.

From a more order representing start:

<?php

use ZZZ\AAA;
use MMM\BBB;
use AAA\CCC;

use ZZZ\DDD;
use SSS\EEE;
use MMM\FFF;
use GGG\GGG;
use AAA\HHH;

use ZZZ\III;
use UUU\JJJ;
use QQQ\KKK;
use MMM\LLL;
use III\MMM;
use EEE\NNN;
use AAA\OOO;

class Foo {}

I would imagine the first strategy would render the following:

<?php

use AAA\CCC;
use AAA\HHH;
use AAA\OOO;
use EEE\NNN;
use GGG\GGG;
use III\MMM;
use MMM\BBB;
use MMM\FFF;
use MMM\LLL;
use QQQ\KKK;
use SSS\EEE;
use UUU\JJJ;
use ZZZ\AAA;
use ZZZ\DDD;
use ZZZ\III;

class Foo {}

and the second strategy the following:

<?php

use AAA\CCC;
use MMM\BBB;
use ZZZ\AAA;

use AAA\HHH;
use GGG\GGG;
use MMM\FFF;
use SSS\EEE;
use ZZZ\DDD;

use AAA\OOO;
use EEE\NNN;
use III\MMM;
use MMM\LLL;
use QQQ\KKK;
use UUU\JJJ;
use ZZZ\III;

class Foo {}

Both make sense and either one could be the default; for my specific use case the second would be the output I would like to see, but I can see that in some messier codebases it would be useful to be able to quickly enforce a single sane output (the first).

[github-actions[bot]]

Since this issue has not had any activity within the last 90 days, I have marked it as stale.

The purpose of this action is to enforce backlog review once in a while. This is mostly for maintainers and helps with keeping repository in good condition, because stale issues and PRs can accumulate over time and make it harder for others to find relevant information. It is also possible that some changes has been made to the repo already, and issue or PR became outdated, but wasn't closed for some reason. This action helps with periodic review and closing of such stale items in automated way.

You may let maintainers handle this or verify current relevancy by yourself, to help with re-triage. Any activity will remove stale label so it won't be automatically closed at this point.

I will close it if no further activity occurs within the next 30 days.

[github-actions[bot]]

Since this issue has not had any activity within the last 90 days, I have marked it as stale.

The purpose of this action is to enforce backlog review once in a while. This is mostly for maintainers and helps with keeping repository in good condition, because stale issues and PRs can accumulate over time and make it harder for others to find relevant information. It is also possible that some changes has been made to the repo already, and issue or PR became outdated, but wasn't closed for some reason. This action helps with periodic review and closing of such stale items in automated way.

You may let maintainers handle this or verify current relevancy by yourself, to help with re-triage. Any activity will remove stale label so it won't be automatically closed at this point.

I will close it if no further activity occurs within the next 30 days.