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

Command icon 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

klausi created an issue. See original summary.

klausi’s picture

Issue summary: View changes
Status: Active » Needs review

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

claudiu.cristea made their first commit to this issue’s fork.

bramdriesen’s picture

Would also be quite disruptive in contrib space where on could be extending off a class or using an enum that is badly named.

klausi’s picture

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

bramdriesen’s picture

Extending or using a badly named enum or class will not throw an error. This sniff only checks the declaration of the enum.

Correct, but fixing them would break things

klausi’s picture

Title: Enforce UpperCamel naming for class, enum, interface, trait + forbid upper case acronyms » Forbid upper case acronyms in class, enum, interface, trait names
Issue summary: View changes
klausi’s picture

Issue summary: View changes

We can fix class declarations in a backwards compatible way:

class NewGoodName {
// ... all the code of the old class is now here.
}

/**
 * @deprecated in 1.5 and is removed from 2.0. Use NewGoodName instead.
 */
class OldBADName extends NewGoodName {}

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:

/**
 * 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;
}
kristiaanvandeneynde’s picture

Class 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

arkener’s picture

As 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.NoUpperAcronyms result by employing the available PHPCS ignore implementations.

klausi’s picture

Issue summary: View changes
Status: Needs review » Needs work

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

travis-bradbury’s picture

Right now, cases are required to contain a lower-case letter. That breaks on cases with a letter and number.

enum FiscalQuarter {
  case Q1;
  case Q2;
  case Q3;
  case Q4;
}

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:

  // Upper case parts are allowed for now.
  case FourJSONCase = 4;
  case FiveAndAHorseCorrect = 5;

"AHorse" is UpperCamel, and sequential uppercase like "JSON" are allowed.

klausi’s picture

Thanks 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!

kristiaanvandeneynde’s picture

but that would break two other cases that are allowed now:

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

klausi’s picture

Coder 8.3.28 was released with the fix for comment #12.

claudiu.cristea’s picture

Well, 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

kristiaanvandeneynde’s picture

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

klausi’s picture

Issue summary: View changes

Fixing link in issue summary.