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

Created on 11 February 2017, over 8 years ago
Updated 18 March 2024, over 1 year ago

Problem/Motivation

Split from #2807785: Move global constants from *.module files into interfaces β†’ , comment #64, suggested by @Berdir.

DRUPAL_DISABLED, DRUPAL_OPTIONAL, and DRUPAL_REQUIRED are too generic to be meaningful.

I can only see 3 uses of them, node preview, comment preview and link title. Maybe each should define its own constants?

Proposed resolution

Add separate constants to node preview, comment preview, and link title, and deprecate the system.module DRUPAL_ constants in favor of the replacements.

Remaining tasks

User interface changes

API changes

Data model changes

πŸ“Œ Task
Status

Active

Version

11.0 πŸ”₯

Component
BaseΒ  β†’

Last updated about 8 hours ago

Created by

πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

Live updates comments and jobs are added and updated live.
  • Novice

    It would make a good project for someone who is new to the Drupal contribution process. It's preferred over Newbie.

Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

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

    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.

  • Status changed to Needs work about 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States joshmiller Indianapolis, Indiana, USA
  • πŸ‡ΊπŸ‡ΈUnited States joshmiller Indianapolis, Indiana, USA

    Updating issue summary.

  • Status changed to Active about 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States joshmiller Indianapolis, Indiana, USA
  • First commit to issue fork.
  • Pipeline finished with Failed
    about 1 year ago
    Total: 200s
    #176224
  • Pipeline finished with Failed
    about 1 year ago
    Total: 222s
    #176230
  • Status changed to Needs review about 1 year ago
  • πŸ‡«πŸ‡·France andypost

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

  • Pipeline finished with Failed
    about 1 year ago
    Total: 1940s
    #176231
  • Pipeline finished with Canceled
    about 1 year ago
    Total: 33s
    #176246
  • πŸ‡΅πŸ‡ͺPeru diegoe Lima, PerΓΊ

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

  • Pipeline finished with Success
    about 1 year ago
    Total: 663s
    #176690
  • Status changed to Needs work about 1 year ago
  • πŸ‡ΊπŸ‡ΈUnited States 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.

  • πŸ‡΅πŸ‡ͺPeru diegoe Lima, PerΓΊ

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

  • Pipeline finished with Success
    about 1 year ago
    Total: 1592s
    #205239
  • πŸ‡³πŸ‡ΏNew Zealand quietone

    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.

  • πŸ‡¦πŸ‡ΊAustralia mstrelan

    Should we be using enums instead?

  • πŸ‡¦πŸ‡ΊAustralia acbramley

    acbramley β†’ changed the visibility of the branch 11.x to hidden.

  • πŸ‡¦πŸ‡ΊAustralia acbramley

    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.

  • Pipeline finished with Success
    3 months ago
    Total: 551s
    #470815
  • πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

    #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

  • πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

    The CR also needs to adapted

  • πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄
  • πŸ‡¦πŸ‡ΊAustralia acbramley

    Will swap to enums

  • Pipeline finished with Failed
    3 months ago
    Total: 242s
    #472520
  • Pipeline finished with Failed
    3 months ago
    Total: 214s
    #472534
  • Pipeline finished with Failed
    3 months ago
    Total: 604s
    #472604
  • Pipeline finished with Success
    3 months ago
    Total: 1521s
    #472617
  • πŸ‡¦πŸ‡ΊAustralia acbramley

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

  • πŸ‡¦πŸ‡ΊAustralia mstrelan

    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.

  • Pipeline finished with Success
    3 months ago
    Total: 909s
    #472669
  • Pipeline finished with Success
    3 months ago
    Total: 1490s
    #472739
  • πŸ‡¦πŸ‡ΊAustralia acbramley

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

  • πŸ‡·πŸ‡΄Romania claudiu.cristea Arad πŸ‡·πŸ‡΄

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

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

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

  • Pipeline finished with Canceled
    2 months ago
    Total: 95s
    #479714
  • Pipeline finished with Success
    2 months ago
    #479715
  • The Needs Review Queue Bot β†’ tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide β†’ to find step-by-step guides for working with issues.

  • Pipeline finished with Failed
    about 2 months ago
    Total: 280s
    #495361
  • Pipeline finished with Success
    about 2 months ago
    Total: 513s
    #495372
  • πŸ‡ΊπŸ‡ΈUnited States nicxvan

    All of my questions have been answered. All of the replacements line up and the new landing places make sense to me!

  • The Needs Review Queue Bot β†’ tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

    This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

    Consult the Drupal Contributor Guide β†’ to find step-by-step guides for working with issues.

  • Pipeline finished with Success
    about 1 month ago
    Total: 1841s
    #506182
  • Assigned to acbramley
  • Status changed to RTBC 4 days ago
  • πŸ‡ΊπŸ‡ΈUnited States xjm

    This issue makes a lot more sense now than it did in 2017. :) Nice work!

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

    I have some concerns with the scope of this issue, BTW. For example, is it still contstants from system.module? There's quite a lof of different specifc APIs being changed in this issue, so this is of of those change sets where it might actually be better to split it up on the basis of the subsystem. There are 300 total lines changed in the diff and it's adding new API in a variety of different context, each of which has to be evaluated on its own merit.

    At a minimum, it would he helpful for the IS to include the list of constants and the choices we're making about them. For example, DRUPAL_DISABLED is being replaced by several contextually-specific enums of what's being enabled in the given context.

    I also had concern's similar to @berdir's above (although his phrasing is much funnier). Is the idea that there will be any number of followup issues like πŸ“Œ Update NodeTypeInterface::get/setPreviewMode to use NodePreview enum Postponed , and that is first example filed? If not, they should have their own meta, rather than this issue as a parent.

    After reading that recommendation, I'm recommending splitting this out into some child issues. Let's pick one of the juicier subsystems that already has topics filed -- either node, or comment -- and follow through with what the DX will look like there once we're able to do the necessary deprecations and conversions.

  • πŸ‡¦πŸ‡ΊAustralia acbramley

    it's adding new API in a variety of different context

    I don't see it that way, we're replacing constants that were reused by a bunch of different modules with enums specific to those modules. The API doesn't change (yet) since we're not passing around enums (yet), those will be the follow-ups you mention.

    Splitting it by subsystem will mean 4 issues (1 per subsystem and then 1 to deprecate the constants) and then the issues to make use of the enums.

    We've already got the other issue to use the enum πŸ“Œ Update CommentTestBase::setCommentPreview to use CommentPreview enum Postponed I don't think there are any more to do so a meta seems a bit overkill. 300 lines is pretty close to the previous 200 line recommended limit (iirc?) and these changes are IMO not difficult to parse since they are all so similar. Splitting this up seems like a bunch of busy work that will stall this work out. It's also already had thorough review by 2 other community members.

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

    I strongly agree with @acbramley in this case.

    It is 3 constants being replaced.

    Each replacement in modules is self contained and easily reviewable using gitlab tooling.

    You can isolate each and check them off once they are reviewed.

    Further the work is done, it would be riskier to split this up in my opinion.

    There is an issue I can't find at the moment talking about adjusting scoping guidelines that I think this is an excellent candidate for.

    That being said, we need to update the deprecation version.

  • πŸ‡¦πŸ‡ΊAustralia acbramley

    Answering some more specifics in #48

    For example, is it still contstants from system.module?

    Yes, only 3 constants are being deprecated from system.module

    There's quite a lof of different specifc APIs being changed in this issue

    There are 300 total lines changed in the diff and it's adding new API in a variety of different context

    No APIs are changing except the deprecation of the constants. We are adding 3 enums and using them in existing APIs instead of the constants.

    I also had concern's similar to @berdir's above

    His comment refers specifically to readability, that is not the goal of this change. The goal is to deprecate globally defined constants in system.module. The improved readability will come in the follow-up issues that change API to use the new enums properly.

    So far we have:
    πŸ“Œ Update NodeTypeInterface::get/setPreviewMode to use NodePreview enum Postponed
    πŸ“Œ Update CommentTestBase::setCommentPreview to use CommentPreview enum Postponed

    We could add another for things like LinkItem::generateSampleValue to use a match and tryFrom instead of a switch on $field_definition->getSetting('title')

    If not, they should have their own meta, rather than this issue as a parent.

    We could have a meta, but it's only going to be a handful of issues.

    After reading that recommendation, I'm recommending splitting this out into some child issues

    I disagree, this is going to make it take much longer to get these constants deprecated. I'm not even really sure what the recommendation is? If I'm reading between the lines correctly that would be coupling the addition of the enum with a usage of it in an existing API (e.g what the issues linked above would be doing), that seems like scope creep.

    The goal of this issue is to deprecate global constants that are both too generic, and used by too many things and allows us to remove things like require_once 'core/modules/system/system.module'; in a test case.

Production build 0.71.5 2024