Increase priority of the ordered_interfaces fixer

[erickskrauch]

TL;DR;

I need to increase the priority of the ordered_interfaces fixer in order to allow placing another fixer between it and the ordered_class_elements fixer.

Details

I'm working on the fixer, that will sort methods in the same order, as they are defined in parent classes/interfaces. Details. The fixer is implemented, but there is a problem: it sorts methods based on the order of class extensions: extends first, then implements. ordered_interfaces might change the order of those extensions and it will affect my fixer, so I want to run my fixer after ordered_interfaces. ordered_interfaces doesn't have the getPriority() method thus it has priority 0.

This would be fine if I didn't also want to leave an opportunity for the fixer ordered_class_elements to execute AFTER my fixer. The ordered_class_elements has priority 65, which gives me an invariant: my fixer should have priority higher than 65, but lower than 0.

So I ask you to increase the priority of the ordered_interfaces to 75 or something to leave room to place my fixer in between.

[Wirone]

I think we need a more experienced voice here, but as far as I know from many discussions here, the idea behind Fixer is that it has only processed file's context and the requirement is that the result is deterministic. I don't see how your fixer fits this 🤔.

In order to be able to sort methods the way they were defined in the parent class/interface, you would need to know that file's content, which couples Fixer with a runtime, because parent class/interface may be defined in the vendor directory. Currently you can run Fixer as a PHAR file or using a Docker image, and running analysis does not require vendors to be installed. There also can be magic autoloaders involved in the app to work, so Fixer should provide an API to configure it, which IMHO is a no-go (or maybe you want to require it in your fixer's constructor?).

Regardless of technical limitations, I believe such rule simply does not make sense. It does not matter how methods are ordered, the only thing that matters is that all required stuff is implemented (and this is not Fixer's job to check it). Such a rule could lead to a situation when you have constant changes in a file only because parent classes' / interfaces' authors changed the order of their methods. I also don't see how it would work when multiple interfaces are implemented by a class - I would rather see methods sorted alphabetically, than have them grouped by source. Not to mention traits, from which methods could be imported with an aliased name and fulfil the contract 🤷‍♂️. It's really, really complex stuff involving many little details and probably it's not something for single fixer.

I don't want to discourage you, but IMHO you should drop this idea 😉.

[erickskrauch]

I have already did that :) It uses reflection, which isn't idiomatic for PHP-CS-Fixer, but it does the job. I'm left to solve the problem with the order in which the fixers are applied so that the result of a run doesn't change after consecutive runs.

[Wirone]

Yeah, I saw the implementation, and it is vulnerable to everything I wrote. The result of the fixer is undeterministic because it depends on reflection+autoloading (the runtime), and external sources which may change the order of the methods causing different results. You do you, but for me it does not make sense to order methods like that (I don't see any value in it).

When it comes to changing the priority, I don't know if there's something preventing us from doing it, but I would like to know @keradus' opinion.

[erickskrauch]

I believe such rule simply does not make sense

That's why it is a custom fixer :)

You're absolutely correct about the fact, that it depends on the order of parents and causes a lot of changes when the author changes something. But that's the point of this fixer.

On my projects, this fixer makes sense since we have interfaces and many implementations of them. When methods are placed in different order during implementation, it sure doesn't make the review difficult, but it doesn't speed it up either. Also, this fixer helps to group implementations of different interfaces if there are several of them (even if implicitly, I plan another fixer for explicit grouping).

I agree that it's all just a matter of preferences. But you won't argue that the whole code style is a matter of preferences, will you? :)

---

On a higher level, I think the next major version of PHP-CS-Fixer should abolish the getPriority() method and replace it with something like runAfter() and runBefore(), which would solve any such problems with standard and custom fixers.

[Wirone]

I agree that it's all just a matter of preferences. But you won't argue that the whole code style is a matter of preferences, will you? :)

Of course, without a doubt 🙂.

On a higher level, I think the next major version of PHP-CS-Fixer should abolish the getPriority() method and replace it with something like runAfter() and runBefore(), which would solve any such problems with standard and custom fixers.

I agree, this is something I pointed out (e.g. here and here) already. It shouldn't be needed to modify priorities in several fixers just to put a new one between them.

Anyway, we need to wait for the lead developer 😉.

[keradus]

I believe @Wirone shared the context pretty well.

Contextless - as stated. Your Fixer violates that and for your custom needs it may be OK, but core repo may struggle to support that.

ordered_class_elements - you suggest a rule that would be in same responsibility to existing rule, but with different sorting algo. I would suggest to not create new rule with priority dependention but replace/extend existing rule.

runAfter - as discussed. PRs welcome.
Yet, my fixer should have priority higher than 65, but lower than 0 - regardless if this relation between built-in fixers or custom fixers is based on abstract numbers or linked via runAfter, it won't solve the problem (you cannot declare graph that A runs after B, B runs after C, C runs after A and expect to have it somehow resolved.

Changing the prio of rule - It's always risky and I am very careful here, because we assume there is no priority dependency , but there may simply be no declared, already discovered dependency. Having priority changed with no tests covering the need for that change will lead to someone coming in x months and changing it to another arbitrary number, effectively breaking the dependency order for you.

[keradus]

Closing due to #7761 (comment)