Problem/Motivation

It turns out that enforcing sorting is made difficult because of the behavior of IDEs.

For example, PHPStorm has a 'sort lines' action which will sort text lines alphabetically. case sensitve. PHPStorm will also automatically add 'use' statements but in the case the sort is not case sensitive alphabetical. So this becomes a burden. for project.

Steps to reproduce

Proposed resolution

Remove the use-by order rule
Allow project to opt in to the order-by rule, with or without case sensitive sort

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Comments

quietone created an issue. See original summary.

eiriksm’s picture

And what would be the actual consequences for projects that has already changed the sort order to case sensitive. Would it have to be reverted? I guess I could figure it out somehow on my own, but currently on my phone and just wondering.

jonathan1055’s picture

[For those not on Slack, here is what I said on that thread]
I think it would be better to remove the entire Coder rule, not just the latest change which added case-sensitivity. Then when the standard is agreed, we can write the sniffs that implement exactly what we want. If we leave in place the sniff which does a plain sort, we'll be creating phpcs failures again for those projects which have just committed changes to satisfy case-sensitivity. We will create more annoyance than good, until the standard is fully settled, so best to remove the Coder rule entirely.

jurgenhaas’s picture

@jonathan1055 I like that idea but also wonder if the sort mode couldn't be configurable on a per-project basis? But I don't know Coder and PHPCS well enough, so I might be completely off track here.

jonathan1055’s picture

Yes the case-sensitivity can be configured per project by adding the following to the project's phpcs.xml file

<rule ref="SlevomatCodingStandard.Namespaces.AlphabeticallySortedUses">
  <properties>
    <property name="caseSensitive" value="false"/>
  </properties>
</rule>

The latest Coder commit added property name="caseSensitive" value="true" to the default Drupal coding standard config, so the lines above effectively revert that, but leave the original non-case-sensitive sort in place. This would be OK, but it means that
(a) the project maintainer has to do this change when they have already made a change to sort the statements case-sensitively,
(b) not all projects have their own phpcs.xml if they are OK with following all Drupal default standards. So we'd be forcing a project to add a phpcs.xml file just in the interim period, before the full standard is agreed, then the file may not be needed by the project when the new Coder sniffs are implemented. We'd then be forcing the maintainer to again make a change by deleting the phpcs.xml file, or removing that caseSensitve override.
(c) The is no guarantee that the final coding standard and Coder implementation of it will use the SlevomatCodingStandard sniff. There were certainly a few bugs in the phpcbf fixer for it it when we first worked on adding that standard. It might be that we write our own sniffs in Coder. Then that projects own phpcs.xml file would be referring to a sniff that is not used, making more confusion.

All of these changes just introduce annoyance and unnecessary work, so I think the best thing to do at the moment is remove the rule entirely until the new standard is tested and written up and Coding Standards issue #1624564: Coding standards for "use" statements is fully agreed and completed.

cmlara’s picture

Title: Revert case sensitive use-statement sniff » Remove use-statement order sniff
Issue summary: View changes

I agree, given there is no official standard at the moment and the final standard is still very much undecided removing the sniff completely appears to be the best choice.

Updating IS/Title to reflect new proposal

jonathan1055’s picture

Status: Active » Needs review

I have created a GitHub PR for this change
https://github.com/pfrenssen/coder/pull/240

The issue summary says "Allow project to opt in to the order-by rule, with or without case sensitive sort" but this is not a thing that Coder can do. It just needs to be documented (somewhere) that the line
<rule ref="SlevomatCodingStandard.Namespaces.AlphabeticallySortedUses"> can be added to the project's phpcs.xml file if they want to opt in.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Agreed, better to remove this then enable it once there's a proper standard.

jonathan1055’s picture

Hi @klausi,
The priority of this is set to 'major' and it has been RTBC for a week. Any chance you could commit it and make new Coder release, please? The longer the sniff stays in the more grief we are causing for developers, until the standard has been agreed.
Thanks.

cmlara’s picture

Anyone heard anything from @klausi? Should we ping on Slack?

jonathan1055’s picture

@klausi is in the #coder channel (I added him :-) but the other maintainers are not in that channel. I don't think it is fair to always ping one person if there are other maintainers too, we could add them there as that would be good, but I do not know which of these are active in Coder now:

  • klausi
  • pfrenssen
  • arkener
  • douggreen
  • mikejw
  • solotandem
  • stella
  • sun
cmlara’s picture

I don't think it is fair to always ping one person if there are other maintainers too, w

Generally fair, I will note klausi has made 100% of the releases since 8.x-3.2 in April 2019.

GitLab Activity report indicates klausi pushed all but one of the changes into the repo in the past 2 years
https://git.drupalcode.org/project/coder/activity

I believe we can call this (at least the D.O. side of the project) a 'single active maintainer' project.

Edit: I missed one push a year ago by @pfrenssen

  • jonathan1055 authored fd98546c on 8.3.x
    fix(AlphabeticallySortedUses): Remove use statement order rule (#3483028...
arkener’s picture

Status: Reviewed & tested by the community » Fixed

Agreed, we should remove the sniff and revisit once #1624564: Coding standards for "use" statements is approved. I've merged the PR into 3.x, and I'll plan to create a new release this weekend, assuming no unexpected issues pop up. Thanks for the work on this!

jonathan1055’s picture

Thank you @arkener, that's great.

klausi’s picture

Sorry, was busy with other things. Thanks a lot for stepping up Arkener, love the Drupal community!

I will make a release now and explain in the release notes how people can add a snippet to their phpcs.xml file to keep sorted use statements. It is very useful in teams to better avoid git merge conflicts.

klausi’s picture

Status: Fixed » Closed (fixed)

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