[meta] Deprecate DRUPAL_DISABLED, DRUPAL_OPTIONAL and DRUPAL_REQUIRED

Hm, I don't understand the summary. The example code snippet does not include DRUPAL_.

because those are the new constants. The current ones are DRUPAL_OPTIONAL and so on.

Basically, they are flags for *something* being disabled, optional or required. Currently used on Core for node and comment preview settings as well as the link title.

My point was that I think it is bad design to have such generic "super-constants" and each of those systems should define its own constants for this instead. NodeTypeInterface::PREVIEW_REQUIRED and so on.

+1

Just ran into this on #3428565: Implement lazy database creation for sessions 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 -- 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?

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.

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.

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

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.

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.

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

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 #3518987: Update NodeTypeInterface::get/setPreviewMode to use NodePreview enum, 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. Now that we have this overview, 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.

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

I don't see it that way, we're replacing constants that were abused 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 #3518988: Update CommentTestBase::setCommentPreview to use CommentPreview enum 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.

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, what benefit is there to recount work that's finished.

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.

Edit: here is the issue I referenced #3418232: [policy, no patch] relax patch size policies

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:
#3518987: Update NodeTypeInterface::get/setPreviewMode to use NodePreview enum
#3518988: Update CommentTestBase::setCommentPreview to use CommentPreview enum

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.

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.

I think this is ready again, hopefully the concerns in 48 are addressed now.
I pulled it down and confirmed there are no more uses or comments about these constants.

I confirmed the three enums values and cases align with the original constants.
The deprecations look right.

In this check I did the following for each file.

1. Check what the module is or what it is related to, comment, link, or node.
2. Confirm that the correct enum was loaded.
3. Confirm each replacement aligned with the correct constant and case.
4. Hit the checkbox to minimize the file and moved on to the next.

Updated the issue summary as requested as well.

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.

Was the bot actually wrong?

We probably want to keep the bot here so we can ensure this is up to date when committers get to it.

I reviewed everything that gitlab showed as changed since my review yesterday rebase seems good.

Was the bot actually wrong?

Yeah, it merged cleanly. I have seen it a handful of times.

One question on the MR about the removed version.

Part of me wondered whether this could have been a generic enum in core somewhere providing the same options, but node preview, comment preview, link title text does not really justify this at all, so agree it's better to just put them on where they're used.

@catch did a quick search of one of the constants and there are lots of usages in contrib so bumped to D13.

I confirmed all three deprecations were updated to 13!

I asked for the scope of this to be split up back in #48. One issue for each constant set, with the latter two postponed on the first.

This makes me sad:

-    $node_type->setPreviewMode(DRUPAL_REQUIRED);
+    $node_type->setPreviewMode(NodePreview::Required->value);

Once we have enums we shouldn't care about ->value, we want to just pass the constant around:

-    $node_type->setPreviewMode(DRUPAL_REQUIRED);
+    $node_type->setPreviewMode(NodePreview::Required);

The problem is that if if this lands, and people start using ->value, but we don't land the child issues in the same minor, then they have to update their code twice.

For me this is reason enough to split up the scope into separate issues where we can complete the conversion in smaller chunks but with a single API change.

RE 62, yes, which I hope we had addressed in 49, 50, 51 and 54.

RE 63, I do see your point. I think if we get this in earlier in the cycle we are more likely to get the follow up issues in before 11.3.

While touching the code twice should be something to consider, in this case I don't think that will happen.

The removal is in Drupal 13, so we don't have to hit 11.3 or even 12 for the replacement. There are some early adopters that remove deprecations right away, however, most contrib and custom code is only doing it when updating for the next major and this deprecation won't even show up for Drupal 12 if you're using upgrade status since it's targeting Drupal 13.

For the early adopters the first change is a simple one is adding a use statement for the correct constant and a find and replace for the deprecated constant so it's not a particularly tough deprecation.

Finally, I think one of the concerns raised above about splitting the issue up is that the actual api change issues are far more complex and adding the deprecation bit in is scope creep there.

@longwave and I are both release managers, so we do have discretion over issue scoping.

My recommendation is to create a meta, then pull out the smallest of the three subsystem sets from this MR, and do what @longwave is suggesting for it.

NW for that. Thanks!

We can move in that direction, but I would like some additional clarity on some things if possible.
Specifically it would help for future issues when scoping those out, there is a lot of cleanup work @acbramely and I (among others) have been working towards and understanding how to break them up to align with expectations would be great.

Here are my specific questions:
1. Isn't one of the major benefits of moving disruptive deprecations to 13 so that issues like this can be incremental?
My understanding is that people should not even get warned about this deprecation until 13 is the next major. (aside from the CR, but we could add a note that it's a 13 removal with planned work to tweak the api's so keep an eye on future changes), this means the timeline for us to get the improvement @longwave is concerned about is when people start looking at upgrading to 13, not 11.3.

If the above is correct, then I think @longwave's concern would be addressed, but I leave it to him to confirm my understanding here.

2. You raised a couple of different concerns in 48 that I believe we addressed in follow up comments (49, 50, 51 and 54), if those don't address the concerns you have it'd be helpful to know why, again to better align expectations in future issues.

That being said, I see this as having three sections, one for comment, one for node, and one for link. I know @acbramley is a maintainer of node so maybe that would make sense as the first one to break out.

Thanks @nicxvan.

Isn't one of the major benefits of moving disruptive deprecations to 13 so that issues like this can be incremental?
My understanding is that people should not even get warned about this deprecation until 13 is the next major. (aside from the CR, but we could add a note that it's a 13 removal with planned work to tweak the api's so keep an eye on future changes), this means the timeline for us to get the improvement @longwave is concerned about is when people start looking at upgrading to 13, not 11.3.

This is one of the points @catch, @longwave, and I discussed regarding the scoping of this issue, and our concern was that having an intermediate public API is non-ideal, especially with somewhat concerning DX and widespread contrib usage. Attentive contrib maintainers would get two separate deprecations and update their modules twice, which is not what we want. (Edit: This is the case even if it's between two minors.) It would be better to know the DX endpoint we're aiming for, rather than de-scoping that. All three of us had concerns about doing this in two steps.

Regarding point 2, in general, diffs of this size should be reserved for e.g. 1:1 replacements (not 3:9 replacements) because reviewers get fatigued and miss stuff when there are multiple patterns like this. This is really adding three new APIs rather than just one. Also, as you recognize in #67, these are API additions to different subsystems and therefore have different stakeholders who should probably be given an opportunity to review them.

Regarding:

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.

Yes, that is what I meant and the point stands. Deprecating constants in system.module is not, in itself, a goal that surpasses a properly used and designed API in importance. ;) In general, changes that regress developer experience should only be made in major or critical situations. This is not that.

Hope this helps. Thanks!

Thanks that helps!

Also I think my understanding was incorrect about whether notices would go out.

The logic to skip deprecations not in the next major is in upgrade status I think, not phpstan drupal so the phpstan job would likely flag these anyway.

I wonder if it is worth an issue to add a flag to phpstan drupal to ignore non next version deprecations since we're going to be getting a bunch of 13 removal deprecations now.

Regarding:

I wonder if it is worth an issue to add a flag to phpstan drupal to ignore non next version deprecations since we're going to be getting a bunch of 13 removal deprecations now.

I believe Gábor confirmed that this is already taken care of in our upgrade tooling (although I can't say for sure if it was for phpstan-drupal specifically or just in something like Upgrade Status). This is one of the things we discussed before changing the policy, for sure.

It's part of upgrade status, not phpstan https://drupal.slack.com/archives/C08QW79LJ94/p1749227536366059?thread_t...

Here is the code:
https://git.drupalcode.org/project/upgrade_status/-/blob/4.x/src/Depreca...

This means the bot will handle it, but the phpstan jobs in the pipeline won't as far as I can tell.

#3225792: Allow deprecation testing to distinguish between major versions when contrib modules run tests is the issue for ignoring deprecations depending on removed in version. For me the upgrade status support is enough because most contrib pipelines don't enable deprecations anyway, but it would definitely be nice if we could handle it in phpstan too.

I'm trying to make sense of the next steps, it seems incredibly wasteful to throw away the hours of work we've already done. Nevertheless, we need to find which subsystem is the smallest diffstat.

core/modules/comment is +61-34
core/modules/link is +60-34
core/modules/node is +45-22

But then there are other files that are touched:

link subsystem:
core/modules/field/tests/src/Kernel/Migrate/d7/MigrateFieldInstanceTest.php
core/modules/menu_link_content/src/Entity/MenuLinkContent.php
core/modules/shortcut/src/Entity/Shortcut.php
core/tests/Drupal/FunctionalTests/Routing/RouteCachingLanguageTest.php

comment subsystem:
core/modules/search/tests/src/Functional/SearchCommentTest.php

node subsystem:
core/modules/views/tests/src/Kernel/TestViewsTest.php
core/tests/Drupal/KernelTests/Config/Schema/MappingTest.php

core/modules/system/system.module - I guess we deprecate after we've made all the other changes

So node is clearly the smallest. I've opened #3538277: Add NodePreview enum and replace usage of DRUPAL_DISABLED, DRUPAL_OPTIONAL and DRUPAL_REQUIRED to start there.

We discussed this further in slack and the consensus seems to be:

1. Repurpose this issue into a META
2. Create 1 issue per module (link, comment, node) and in those issues also change the API that will use these enums. IIRC only Node has runtime code that'll change. Comment has a test method. Link has nothing (although we can tidy up some stuff).
3. Create a 5th issue to deprecate the constants, postponed on the 3 issues created in 2.

@mstrelan thanks for creating the Node issue. I don't think this work is thrown away though, we can just checkout the module path from this issue's branch and start from there.

Re #62 @xjm, I understand you asked for that and that is indeed where it landed (because I'm sick of arguing and it doesn't seem like something we can "win") but it's a bit of a shame that all the further comments after yours seem to have been not responded to.

Just saying "we are both release managers, so we do have discretion over issue scoping." feels quite patronising if I'm being honest.

Re #63 @longwave - it was always the intention to tidy that up in a follow up

In any case, let's go with a meta of 4 issues and try to finally get this over the line. I'd appreciate a quick turnaround on those issues once ready if possible. I am on leave today and tomorrow but will pick this up on Wednesday (my time).

Historically there's a lot of things that we have always intended to do in followups, but several times they have been forgotten or derailed and so don't happen in a reasonable timeframe. Sometimes that is okay, but in this case I think it's valid to do it differently.

Spent 6-7 hours on this today, and all 3 issues should be ready-ish.

#3538277: Add NodePreview enum and replace usage of DRUPAL_DISABLED, DRUPAL_OPTIONAL and DRUPAL_REQUIRED should be first, since all 3 issues use common code on the enum which should go into a trait.

I haven't moved it into a trait yet because I'm not sure where an enum trait that is used by multiple core modules should live.

@mstrelan, @acbramley, Great work here. Sorry for not being able to follow up and work on this more. (I only have 8 hours of paid time per week because I lost most of my sponsorship.)

All the child issues are done, I think this can be closed? RTBC just in case I'm missing something.

I think this can be marked as fixed, I took a pass at credit.