Revert case sensitive use-statement sniff

Created on 23 October 2024, about 2 months ago

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

Revert the case sensitive sort, so that is the default.
Allow project to opt in to case sensitive sort

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

πŸ“Œ Task
Status

Active

Version

8.3

Component

Coder Sniffer

Created by

πŸ‡³πŸ‡ΏNew Zealand quietone

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @quietone
  • πŸ‡³πŸ‡΄Norway eiriksm Norway

    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.

  • πŸ‡¬πŸ‡§United Kingdom jonathan1055

    [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.

  • πŸ‡©πŸ‡ͺGermany jurgenhaas Gottmadingen

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

  • πŸ‡¬πŸ‡§United Kingdom jonathan1055

    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 πŸ“Œ Coding standards for "use" statements RTBC is fully agreed and completed.

  • πŸ‡ΊπŸ‡ΈUnited States cmlara

    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

  • πŸ‡¬πŸ‡§United Kingdom jonathan1055

    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.

  • πŸ‡¬πŸ‡§United Kingdom catch

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

  • πŸ‡¬πŸ‡§United Kingdom jonathan1055

    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. The longer the sniff stays in the more grief we are causing for developers, until the standard has been agreed.
    Thanks.

  • πŸ‡ΊπŸ‡ΈUnited States cmlara

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

  • πŸ‡¬πŸ‡§United Kingdom jonathan1055

    @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
  • πŸ‡ΊπŸ‡ΈUnited States cmlara

    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 is the only individual to have pushed commits 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.

  • πŸ‡³πŸ‡±Netherlands arkener

    Agreed, we should remove the sniff and revisit once πŸ“Œ Coding standards for "use" statements RTBC 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!

  • πŸ‡¬πŸ‡§United Kingdom jonathan1055

    Thank you @arkener, that's great.

  • πŸ‡¦πŸ‡ΉAustria klausi πŸ‡¦πŸ‡Ή Vienna

    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.

  • πŸ‡¦πŸ‡ΉAustria klausi πŸ‡¦πŸ‡Ή Vienna
  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.71.5 2024