Problem/Motivation
Since 2013 acronyms are not allowed in all upper case form in class names https://www.drupal.org/docs/develop/standards/php/php-coding-standards#s... . This is currently not checked in Coder
The checking is done by the Drupal.NamingConventions.ValidClassName sniff.
If we make this checking more strict then we will get many naming fails in Drupal 11 core, see https://github.com/pfrenssen/coder/actions/runs/12619249748/job/35163551208 . For example:
- ConfigCRUDTest
- FTPExtension
- PDOConnection
- NodeTranslationUITest
Class names are case insensitive in PHP: https://www.php.net/manual/en/language.oop5.basic.php#122293
So we can rename the casing of classes without breaking existing code.
We cannot write a PHPCS fixer for this as it cannot update all references to a class. But maybe there are other tools like Rector that can do the renaming.
Another workaround is possible by telling PHPCS to ignore the class declaration line:
/**
* Doc block is here and an ignore directive is ok.
*/
// phpcs:ignore Drupal.NamingConventions.ValidClassName
enum PUROSELY_WRONG_BUT_OK: int {
case One = 1;
case Two = 2;
}
Steps to reproduce
Write bad class names.
Proposed resolution
- I thought about changing the error codes to give developers more fine grained control for silencing. Changing error codes would be a small API break and I think there is not much to gain. Since classes are in separate files the error can be silenced on the file path level in phpcs.xml.dist. The error can also be silenced in a comment with phpcs:ignore before the class.
Test examples:
class CorrectClassName {}
class CorrectClassWithAReallyLongName {}
class IncorrectJSONClassName {}
class INCORRECT_CLASS_NAME {}
class INCORRECTCLASSNAME {}
class incorrectLowercaseClassName {}
interface CorrectInterfaceName {}
interface CorrectInterfaceWithAReallyLongName {}
interface IncorrectJSONInterfaceName {}
interface INCORRECT_INTERFACE_NAME {}
interface INCORRECTINTERFACENAME {}
interface incorrectLowercaseInterfaceName {}
trait CorrectTraitName {}
trait CorrectTraitWithAReallyLongName {}
trait IncorrectJSONTraitName {}
trait INCORRECT_TRAIT_NAME {}
trait INCORRECTTRAITNAME {}
trait incorrectLowercaseTraitName {}
enum CorrectEnumName {}
enum CorrectEnumWithAReallyLongName {}
enum IncorrectJSONEnumName {}
enum INCORRECT_ENUM_NAME {}
enum INCORRECTENUMNAME {}
enum incorrectLowercaseEnumName {}
Remaining tasks
Discuss if this is what we want, then finish https://github.com/pfrenssen/coder/pull/255
API changes
None
Issue fork coder-3497433
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
klausiOh my, this will be quite disruptive, lots of names failing in core: https://github.com/pfrenssen/coder/actions/runs/12619249748/job/35163551208
Please review! https://github.com/pfrenssen/coder/pull/255
Once we have agreement that we want to move forward with this I will make ignore rules for core testing in Coder.
Comment #4
bramdriesenWould also be quite disruptive in contrib space where on could be extending off a class or using an enum that is badly named.
Comment #5
klausiExtending or using a badly named enum or class will not throw an error. This sniff only checks the declaration of the enum.
I will split this issue again to do uncontroversial change first: check enums and traits for the same criteria as we do with classes as interfaces right now. Then we can continue discussing UpperCamel enforcement with acronyms here and if we want it.
Comment #6
bramdriesenCorrect, but fixing them would break things
Comment #7
klausiMerged #3497580: Enforce UpperCamel naming for enum and trait for the simple stuff.
Comment #8
klausiWe can fix class declarations in a backwards compatible way:
This is not bullet-proof as the old class name could be hard-coded in special cases, but this is the usual way we deprecate code in Drupal core.
Another workaround is possible by telling PHPCS to ignore the class declaration line:
Comment #9
kristiaanvandeneyndeClass names are case insensitive in PHP, so can't we simply change them in core?
See: https://www.php.net/manual/en/language.oop5.basic.php#122293
Comment #10
arkener commentedAs Kristiaan noted, fixing incorrectly cased class names should be straightforward, given that class names are not case-sensitive. For other scenarios, such as the use of underscores, one of the backward compatibility approaches mentioned in #8 should be used. In the end, this change simply enforces an existing rule. If necessary, developers can always ignore the
Drupal.NamingConventions.ValidClassName.NoUpperAcronymsresult by employing the available PHPCS ignore implementations.Comment #11
klausiAh great, then this will be no compatibility break at all. Just a lot of file and class renaming.
We cannot write a PHPCS fixer for this as it cannot update all references to a class. But maybe there are other tools like Rector that can do this task.
Nest step:
* Ignore Drupal core fails in Coder testing.
Comment #12
travis-bradbury commentedRight now, cases are required to contain a lower-case letter. That breaks on cases with a letter and number.
Those are UpperCamel. The sniff could look for multiple upper case characters in a row, but that would break two other cases that are allowed now:
"AHorse" is UpperCamel, and sequential uppercase like "JSON" are allowed.
Comment #13
klausiThanks a lot Travis, that is a good point. It belongs a little bit more to the initial checking in #3497580: Enforce UpperCamel naming for enum and trait . I pushed https://github.com/pfrenssen/coder/pull/259 for that one!
Comment #14
kristiaanvandeneyndeAs far as I can tell FourJSONCase is not allowed, it would be FourJsonCase.
And I don't think we should account for the word 'a', I have yet to see a class named ADog rather than Dog in all my years as a programmer. If you run into trouble with the sniffer on the very rare occasion that you might want to use 'a', then you should probably just add a sniffer ignore comment before that.
Comment #15
klausiCoder 8.3.28 was released with the fix for comment #12.
Comment #16
claudiu.cristeaWell, a class name built from "a dog" might not be correct as ADog but what about "eGovernment"? How should I build a class containing this word? EGovernment will not be allowed, in the same time Egovernment looks bad
Comment #17
kristiaanvandeneyndeWe already have Email, even though technically that could also be EMail, which is not allowed. I'm not too concerned with seeing classes such as Ebook, Email, Egovernment, etc. over EBook, EMail and EGovernment.
Comment #18
klausiFixing link in issue summary.