#Novice

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.

Hi, I've reviewed the changes in MR !100

The comment added above UPDATE_ACTION_DELETE helps clarify that the existing path_alias entity is actually updated, not deleted — which is great context for developers. It’s a small but important detail that wasn’t obvious from the label alone.

Everything looks good to me — the wording is clear, and it’s in the right place. Thanks for making it easier to understand!

Marking this as Reviewed & Tested by the Community.

Thanks for the contribution!

Hi, I am picking this Ticket up for Review

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

Committed e1e9732 and pushed to 11.x. Thanks!

nod_ → closed merge request !12749

Thanks, both of you, looks great! 🤩

Committed 5df135d and pushed to 11.x. Thanks!

nod_ → closed merge request !11745

Released 1.0.2 with this change.

I'll fix the deadline first so this simple, unrelated one becomes green. Thanks for the MR!

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 📌 Introduce and make use of NodePreview enum, to replace node related usage of DRUPAL_DISABLED, DRUPAL_OPTIONAL and DRUPAL_REQUIRED constants Active to start there.

I created a new MR for this: https://git.drupalcode.org/project/fluidui/-/merge_requests/40

I was able to hit the - and + buttons by using the keyboard.

Moving this to "Needs Review" just in case anyone wants to review this.

I'll merge it to the 8.x-1.x branch.

@mxr576 first, thanks a lot for this, I wish I know how to make Drupal Recipes, but I got caught up in extra work, and and never have the time.

Second, this MR addresses this, but it seems to be an issue with the pipeline (some failed jobs).

Fix seems ok:

I'll move it to "Needs Review" just in case anyone wants to have a look on this.

I changed the README.txt file to match the original developer.

By the way, this projects to replace the README.txt with a README.md and follow the template guidelines:

https://www.drupal.org/docs/develop/managing-a-drupalorg-theme-module-or-distribution-project/documenting-your-project/readmemd-template →

I might open a new issue for that

I imagine the bot is complaining there’s no return of void

The bot may be correct, MR is 1800 commits back

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

Thanks @jonathandealmeida!

I think this means we would need to completely change how the test would work. Based on the machine name, the whole point is that the role does not exist. However, if we're conditionally creating it, that's no longer the casein the situation under test. In other words, fixing it in this way would basically remove the existing test coverage for non-existent roles.

This test coverage was added originally in #2737719-71: EntityResource: Provide comprehensive test coverage: for every entity type, every format, every method → . Quoting that comment:

Apparently another fail sneaked in there. In all Block config entities. The root cause is that same problem that was discovered before (see #41):

    // All blocks can be viewed by the anonymous user by default. An interesting
    // side effect of this is that any anonymous user is also able to read the
    // corresponding block config entity via REST, even if an authentication
    // provider is configured for the block config entity REST resource! In
    // other words: Block entities do not distinguish between 'view' as in
    // "render on a page" and 'view' as in "read the configuration".

But #65 introduced additional assertions to ensure a 403 is returned before the necessary authorization is set up. The work-around I had in place didn't account for that. So, now I have changed the work-around to disallow access by any user until the necessary permission is granted. We still need to fix the root cause though.

Created an issue for that now: #2820315: Blocks' 'view' operation is interpreted as "render on page" instead of "read this config entity" → .

Adding those two issues to the related issues.

Based on the above, I don't think we can go ahead with the workaround of conditionally creating the role. Or, if we did, we would need to test the 4-3s for non-existent roles in some other way.

This is also definitely not novice anymore, so untagging.

To comply with the new ConfigExists constraint added in the user.schema.yml schema for the roles field of the user_role condition, it was necessary to ensure that the roles referenced in the tests actually exist as valid configuration. In the createEntity() method of the base test class BlockResourceTestBase, I added the conditional creation of the test role. This guarantees that the role exists in the configuration during test execution, satisfying the ConfigExists constraint.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.

It's not possible to run phpstan in this file

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.

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 e.g. Upgrade Status). This is one of the things we discussed before changing the policy, for sure.

Thanks!

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.

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. Attentive contrib maintainers would get two separate deprecations and update their modules twice, which is not what we want. 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!

xjm → closed merge request !12832

I read

</code> and confirmed that this is what it actually does. Since it's always good to check <em>why</em> something like this is wrong (especially to confirm that we have the correct replacement, I did:
<code>
git log -S "FullPageVariant"

It looks like this was removed back in 2014 in [#]. That issue includes e.g.:

diff --git a/core/lib/Drupal/Core/Display/Annotation/DisplayVariant.php b/core/lib/Drupal/Core/Display/Annotation/DisplayVariant.php
index 3c05cf3b85e..3bf74920f3c 100644
--- a/core/lib/Drupal/Core/Display/Annotation/DisplayVariant.php
+++ b/core/lib/Drupal/Core/Display/Annotation/DisplayVariant.php
@@ -25,8 +25,9 @@
  *
  * Plugin namespace: Plugin\DisplayVariant
  *
- * For a working example, see
- * \Drupal\block\Plugin\DisplayVariant\FullPageVariant
+ * For working examples, see
+ * - \Drupal\Core\Render\Plugin\DisplayVariant\SimplePageVariant
+ * - \Drupal\block\Plugin\DisplayVariant\BlockPageVariant
  *

...and git even detects it as a rewritten file:

 .../{FullPageVariant.php => BlockPageVariant.php}  |  12 +-

Finally, I grepped to confirm that there were no other stale references to FullPageVariant:

[ayrton:drupal | Fri 14:33:16] $ grep -r "FullPageVariant" *
core/lib/Drupal/Core/Display/PageVariantInterface.php: * For example, the \Drupal\block\Plugin\DisplayVariant\FullPageVariant page

Committed to 11.x, and cherry-picked to 11.2.x as a patch-safe documentation bug fix. Thanks!

My previous concern about this being quite long for a theme description remains. I don't know that it's the place of the theme info file to provide links to the theming documentation? I think we could drop the last bit with the link. The updated proposal could also get a quick usability review at the weekly usability meeting. To that end, a screenshot of the description in context alongside other themes' descriptions would be helpful.

Also, this still has not had subsystem maintainer feedback. However, given that the maintainer for this subsystem was out for the past eight weeks and is currently discussing their maintainership, we can escalate this to the FEFMs.

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.

NW for that. Thanks!

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

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.

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.

Further, 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 deprecate right away, most contrib and custom code is only doing it when updating for the next major and this deprecation won't even show up for 12 since it's targeting 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.

general projects don’t get packaged and a license file added automatically.

TIL :)

Could you create an MR with this?

Thanks for making this great looking recipe!

Thanks! :)

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, ideally we want to just pass the constant around? ie.

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

I asked for the scope of this to be split up back in #48.

I had a coding standards nitpick, otherwise this looks great.

Merge request created to update the remaining comments to the new method added in https://www.drupal.org/project/drupal/issues/3533049 📌 Bulk convert the remaining hooks to OOP Active .

I confirmed all three deprecations were updated to 13!

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

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

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

Fixed, and all the checks are green: https://git.drupalcode.org/project/gutenberg_ai_tools/-/merge_requests/13

I'll merge it to the 1.0.x branch now.

Seems straight forward.

The MR resolves the issue for me. Thanks.

Hm this looked pretty simple to me. ECA indeed already has an integration into Gin like I explained. For some reason I can't tell anymore, we resetted the $routes passed to the alter hook, which is the root cause of this issue. It will just remove the previously defined routes which breaks the bpmn UI with Gin.

hydra → changed the visibility of the branch 3526208-buttons-in-certain to hidden.

I found the BlockPageVariant as an alternative to mention

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.

I can confirm that the feature branch fixes the issue - first call was on current 11.x branch - the second on the feature branch

But there is still an open thread in the MR https://git.drupalcode.org/project/drupal/-/merge_requests/11651#note_49...

The above patch is wrong. The correct one with tests.

catch → closed merge request !12282

Committed/pushed to 11.x, thanks!

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

I think this description makes sense, looks good to me.

patch for 11.2 without tests

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.

maneesha binish → changed the visibility of the branch 2873447-rest-image-style to hidden.

The solution was updating outdated (and deprecated) code. I've rebased and squashed the MR with the new fix.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.

Think the latest changes kinda read weird "Stark should not be typically used as a base theme,"

What if we just keep it simple with "Stark should not be used as a base theme, instead build a custom theme using the Theming Guide → as a reference."

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

Hi thanks for the contribution. This looks good to me so far.

The high priority task would be to

- Get Tests run in CI. Right now the test are getting executed with wrong configuration. The test fail are setup issues.

Lower priority tasks

- Fix warnings in various linter services.

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

Refactored ImageFieldNormalizer to address code review feedback.
Changing status to Need Review.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.

Updating issue summary.

I've moved this to "Needs Review" just in case you decide to go ahead with the MR.

Can we cleanup the summary some since we aren't hiding anymore.