feat: Introduce `OrderedAttributesFixer`
Sorts attributes:
<?php
+#[Bar(3)]
+#[Corge(a: 'test')]
#[Foo]
-#[Bar(3)]
#[Qux(new Bar(5))]
-#[Corge(a: 'test')]
class Sample1 {}
#[
+ Bar(3),
+ Corge(a: 'test'),
Foo,
- Bar(3),
Qux(new Bar(5)),
- Corge(a: 'test'),
]
class Sample2 {}
Inspired by https://github.com/rectorphp/rector-src/pull/3838
coverage: 96.122% (+0.009%) from 96.113% when pulling b9ca5bbca408b62741a7be4b4a62c6909467989b on HypeMC:ordered-attributes-fixer into bb387e163ac5823adbe4c6a630867b0ce264312e on PHP-CS-Fixer:master.
@HypeMC I've rebased your branch to keep PR up to date and it seems like it requires some attention - we recently introduced auto-review test that enforces tests' naming convention, can you align the code here? Sorry for the delay, I plan to revisit this PR soon because it's a really good feature worth merging 🙂.
@Wirone No problem, done.
@Wirone I've rebased the PR now that #7909 was merged. I've also extracted the logic to determine attribute FQCNs into a separate class for reusability. If you don't think this is useful, I'll just revert the commit.
@HypeMC I'll remove last commit because FQCN resolver seems to be incomplete, as it does not resolve properly names when multiple namespaces are used:
Also, it should be FullyQualifiedNameResolver because:
- Abbreviations should be treated as words (
Fqcn, notFQCN) in order to be able to convert camelCase to snake_case and keep the meaning (with current name it would beclass_f_q_c_n_resolver) -
CinFQCNstands forClass, soClassFqcnResolverhas double "class" - it can be fully qualified name of interface too, so I would avoid using
classin resolver's name completely
I don't want to block this PR longer, so let's extract such resolver separately - SRP also applies to PRs 🙂.
@HypeMC I did some refactoring to shorten the feedback cycle. It's close to what I would consider ready for merge, I want to try refactor one more thing but I had some problem and won't finish it now. Anyway, one concern: with this implementation (my refactor does not matter here) namespace analysis will be performed for every attribute (so the same work will be done multiple times within the same namespace), which is not good from performance perspective. However, it's only for none sorting algorithm, so maybe not that critical 😅.
I've renamed strategy to custom as this reflects the intent better. none strategy is... when you don't use this rule 😉.
@Wirone No problem, thank you for the reviews :smile:
Wdyt about adding a new syntax for custom order '*' ?
Allowing something like
['sort_algorithm' => 'custom', 'order' => ['A\\B\\Qux', 'A\\B\\Bar', '*', 'A\\B\\Corge']].
Which means "Qux first, then Bar, then everything then Corge" ?
@VincentLanglet looks interesting. Basically you would like to enforce what's at the top and what's at the bottom, and the rest in the middle in the exact same order as they currently are?
@VincentLanglet looks interesting. Basically you would like to enforce what's at the top and what's at the bottom, and the rest in the middle in the exact same order as they currently are?
Yes, in the same way, enforcing just the bottom or the top would be possible with
['sort_algorithm' => 'custom', 'order' => ['*', 'A\\B\\Corge']]
or
['sort_algorithm' => 'custom', 'order' => ['A\\B\\Qux', 'A\\B\\Bar', '*']]
I dunno if people will come up also asking for something like
['sort_algorithm' => 'custom', 'order' => ['A\\B\\*', '*']]
ie: all the attributes in the namespace A\B then the others. This would avoid having to list every attributes.
Do you want a feature request @Wirone ?
@VincentLanglet Yes, please 🙂. You can link your comment in the issue form (instead of discussion).
Done https://github.com/PHP-CS-Fixer/PHP-CS-Fixer/issues/7993