Problem/Motivation

Part of #2851705: [meta] Deprecate DRUPAL_DISABLED, DRUPAL_OPTIONAL and DRUPAL_REQUIRED - we want to deprecate DRUPAL_DISABLED, DRUPAL_OPTIONAL and DRUPAL_REQUIRED. Doing this in a single issue is too hard to review and we want to change the API as we go.

This ticket is for the Node module.

Proposed resolution

- Add NodePreview enum
- Replace usages of constants with new enum
- Add deprecated $returnAsInt param to NodeTypeInterface::getPreviewMode and update return type to int|NodePreview
- Update usages in core of NodeTypeInterface::getPreviewMode to expect NodePreview enum
- Allow int|NodePreview in NodeTypeInterface::setPreviewMode
- Update usages in core of NodeTypeInterface::setPreviewMode to use the NodePreview enum

API changes

NodeTypeInterface::getPreviewMode and NodeTypeInterface::setPreviewMode now interact with the NodePreview enum

Release notes snippet

N/A

Issue fork drupal-3538277

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:

About issue forks

Comments

mstrelan created an issue. See original summary.

acbramley’s picture

acbramley commented
Title: Introduce and make use of NodePreview enum, to replace node related usage of DRUPAL_DISABLED, DRUPAL_OPTIONAL and DRUPAL_REQUIRED constants » Add NodePreview enum and replace usage of DRUPAL_DISABLED, DRUPAL_OPTIONAL and DRUPAL_REQUIRED
Issue summary: View changes
acbramley’s picture

acbramley commented
Parent issue: » #2851705: [meta] Deprecate DRUPAL_DISABLED, DRUPAL_OPTIONAL and DRUPAL_REQUIRED

acbramley opened merge request !12864

acbramley’s picture

acbramley commented
Status: Active » Needs review
mstrelan’s picture

mstrelan commented

Added a couple comments on the MR.

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

catch’s picture

catch
he/him
Primary language English
commented

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 #3244848: Remove comment and node preview ever happens we wouldn't have enough uses to justify a trait.

acbramley’s picture

acbramley commented

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.

acbramley’s picture

acbramley commented

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

dww’s picture

dww
we/he/they
commented
Status: Needs review » Needs work

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

acbramley’s picture

acbramley commented
Status: Needs work » Needs review

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

nicxvan’s picture

nicxvan commented
Status: Needs review » Reviewed & tested by the community

I think this is ready.

All deprecations line up.
I commented on 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.

All other changes seem relevant and scoped to node.

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

dww’s picture

dww
we/he/they
commented

Saving credit for reviews.

dww’s picture

dww
we/he/they
commented

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

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

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.

acbramley’s picture

acbramley commented
Status: Needs work » Reviewed & tested by the community

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.

mstrelan’s picture

mstrelan commented

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

nicxvan’s picture

nicxvan commented

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

  • catch committed f4ee26f0 on 11.x 
    Issue #3538277 by acbramley, mstrelan, dww, catch, nicxvan: Add...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

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

acbramley’s picture

acbramley commented

Thanks @catch, I've published the CR

Status: Fixed » Closed (fixed)

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