Introduce and make use of NodePreview enum, to replace node related usage of DRUPAL_DISABLED, DRUPAL_OPTIONAL and DRUPAL_REQUIRED constants

Added a couple comments on the MR.

FWIW 📌 Remove comment and node preview Active , but that's not likely to get in quickly, so let's continue here.

A trait for shared logic in the enums was mentioned in the meta issue. A bit of a cop out - but could we just add that to Drupal\Core\Utility.

But also if 📌 Remove comment and node preview Active ever happens we wouldn't have enough uses to justify a trait.

Trait added, I think the logic is good enough to keep around even if comment/node previews were removed because the Link enum will still use it and it may be handy for other things in the future.

@mstrelan linked me to ✨ Add support for PHP 8.1 Backed Enums in Select, Checkboxes, Radio elements Needs work which could use this trait as well.

Thanks for working on this! Opened a bunch of MR threads. Mostly very nit-picky bikesheds. A couple things of substance.

Mostly addressed, left stuff I don't agree with or have no strong feelings about.

I think this is ready.

All deprecations line up.
The open threads I agree should remain as is.

There is one bc break that is commented that will want confirmation it is ok before commit.

This includes a helpful trait in utility.

Al other changes seem relevant and scoped to node.

I read the CR as well and it seems clear to me.

Saving credit for reviews.

There are 2 legit unresolved MR threads. I want to take this out of RTBC, since it’d be better to resolve those before a core committer looks at it. But we probably need a framework manager or release manager to decide on the BC break problem and how to handle the API change to NodeTypeInterface::getPreviewMode(). 😅 So I’m leaving the RTBC, but won’t formally tag it for any specific sign-off (yet).

Thanks/apologies,
-Derek

Replied on the MR, we can use the process in https://www.drupal.org/node/3376455 → to add the argument to the interface method. I think that's reasonable to change in Drupal 12.

Thanks @mstrelan, I don't think the bikeshedding of the trait name should hold this back from RTBC. We have 2 +1s for the current name and one against.

Test fail (twice) seems unrelated. This is the code on the status report page:

// Ensure the status is not a warning if APCu size is greater than or equal
// to the recommended size.
if (preg_match('/^Enabled \((.*)\)$/', $elements[0]->getText(), $matches)) {
if (Bytes::toNumber($matches[1]) >= 1024 * 1024 * 32) {
$this->assertFalse($elements[0]->find('xpath', '../../summary')->hasClass('system-status-report__status-icon--warning'));
}
}

And this is the browser output https://issue.pages.drupalcode.org/-/drupal-3538277/-/jobs/6109542/artif...

Needs a rebase, that test has been skipped on HEAD.

If we have lots of enum traits we want to group, we could move them to Drupal\Core\Utility\Enum or similar. I wasn't sure about the trait initially but it looks tidy now that it exists. Interface param addition looks right now too.

Have to say while the original issue was borderline for me on MR size, the scope of doing the parameter changes one by one feels much better - this was a much nicer MR to review and it should be easier to tie up the loose ends in Drupal 12/13 and for contrib modules to copy the pattern from.

Committed/pushed to 11.x, thanks!

catch → closed merge request !12864

Thanks @catch, I've published the CR

Automatically closed - issue fixed for 2 weeks with no activity.