Each class using DRUPAL_* constants from system.module should define its own constants

Just ran into this on 📌 Implement lazy database creation for sessions Active trying to remove system module from a test. +1 and tagging novice because this should be a straightforward change.

Updating issue summary.

Opened a MR:
https://git.drupalcode.org/project/drupal/-/merge_requests/8118

Opened a draft CR:
https://www.drupal.org/node/3448089 →

needs re-run failed test as it known as random-failure

@andypost

I thought it was random failure at first, but then found an issue for it: https://www.drupal.org/project/drupal/issues/3448036 🐛 InstallerTranslationExistingFileTest fails on 11.x branch Active -- There's an MR already, so hopefully it will get fixed soon. I'll rebase on top of that when it's merged.

Gone back n forth but since it was a constant used through out can we add simple test coverage that if you use any of these deprecated constants you get a warning.

@smustgrave

> Gone back n forth but since it was a constant used through out can we add simple test coverage that if you use any of these deprecated constants you get a warning.

I couldn't find a way to trigger a runtime warning for constants. The Drupal docs have suggestions for other similar cases but not for constants:

https://www.drupal.org/about/core/policies/core-change-policies/drupal-d... →

Also looked into core's various tests but couldn't find anything that would fit here. Do you have an example?

Thanks for working on this, it would be great to have this completed.

I had a very brief look at the MR and see that it needs to be updated to meet the Drupal Coding Standards for constants → . I didn't review anything else in the MR.

Should we be using enums instead?

acbramley → changed the visibility of the branch 11.x to hidden.

can we add simple test coverage that if you use any of these deprecated constants you get a warning

I don't think this is possible, at least I can't think of how to trigger a warning.

I had a very brief look at the MR and see that it needs to be updated to meet the Drupal Coding Standards for constants

Is this relating to this part of the standards: "Module-defined constant names should also be prefixed by an uppercase spelling of the module that defines them."? Because IMO that should not apply to constants on classes. Adding the module prefix would just duplicate what's already provided by the classname, e.g NodeTypeInterface::NODE_PREVIEW_OPTIONAL. Perhaps the docs need an update?

I've merged 11.x into the branch and updated a few more spots that were missed or added since the last merge.

#30 is right. When this issue has been created there was no enum support or, at least, we’ve still supported old PHP versions. I think this should be rescoped to deprecate and convert some, if not all, to enums

The CR also needs to adapted

Will swap to enums

Now using enums, I imagine the naming might be contentious here 😁

Non-exhaustive review. Added some thoughts to the MR. I tend to find enums most useful when you can pass them around to functions rather than just relying on them for their backed values. In an ideal would you could use the enum for field config as well rather than the int value.

Thanks for the thorough review @mstrelan - all comments have been actioned.

Maybe related ✨ [PP-1] Enum cases stored in config Postponed

Had a couple of questions.
I went through very carefully and confirmed all of the replacements align.