Current theme condition plugin should provide an option to match all themes and introduce checkboxes

Hello, Chi :)

The #3 patch no longer applied cleanly, so I rerolled it.

Additionally, I think a more elegant way to introduce that "Any" option would be via #empty_option property on the select element. So I did in my patch.

I've adapted the patch code to be compatible with Drupal 9.2.x

Fixed typo that triggered the test fail and rephrased some sentences.

Ps.: This patch applies for 9.2.x, 9.3.x, and 9.4.x

Added test coverage for the '- Any -' option and also added period to all sentences to keep consistency.

Committed to the merge request the changes made by andregp

Tagging for framework input. If this is a path we want to go down.

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

Think it will help for steps to reproduce this bug.

With a UI change before/after screenshots should be added to the issue summary.

The current theme condition is not shown on blocks because of:
https://git.drupalcode.org/project/drupal/-/blob/10.1.x/core/modules/blo...

    // @todo Allow list of conditions to be configured in
    //   https://www.drupal.org/node/2284687.
    $visibility = $this->entity->getVisibility();
    $definitions = $this->manager->getFilteredDefinitions('block_ui', $form_state->getTemporaryValue('gathered_contexts'), ['block' => $this->entity]);
    foreach ($definitions as $condition_id => $definition) {
      // Don't display the current theme condition.
      if ($condition_id == 'current_theme') {
        continue;
      }

Should we recover this condition?

It was removed on https://www.drupal.org/project/drupal/issues/2339151

We are changing the plugin because of https://www.drupal.org/project/google_tag/issues/3263122, to be able to load the google tag on any theme, and recover this plugin on the module. Now is excluded because is not possible to select any theme.

Updated to apply to the last drupal version

Added patch also and diff between last commit and the current one, allowing blocks to use the current theme condition.

Added steps to test and captures as asked by @smustgrave

I will review the failed tests https://www.drupal.org/pift-ci-job/2710393

Fixed tests, ready to review

I think the test coverage is okay.

But found the workflow a little odd.
I did a Standard install
Went to block layout
Added a block under the Olivero tab but have it only appear on the claro theme.
Tagging for usability for that

Now if we had a global block layout tab then this setting would make sense I think.

Will leave in review for framework manager thoughts.

@smustgrave Do you think the options should be "current theme tab" or "any" in the case of block layout? instead of the list of all themes.
For the block layout, it have sense because it has theme tabs, for other cases, the list should have all themes

Could someone expand the issue summary to explain why is this setting needed? Drupal Block UI is built so that blocks are intended to be specific to a theme. If we want to change that, it would be helpful to understand why that's desired.

@lauriii You are totally right, I miss the original scope of the issue.

We use current theme condition on google_tag module, but we need the "- any -" option https://www.drupal.org/project/google_tag/issues/3263122 to have the ability to use google_tag on all themes or only on a theme.

https://www.drupal.org/project/google_tag/issues/3263122#comment-14460827
@solotandem:
Those condition plugins are excluded by this module precisely because of the core bug in condition plugin (issue and patch mentioned on project page). If you can get core to fix this, then we could allow them in this alter hook. At this point, if I were to allow that plugin, then people would file issues against this module that the plugin does not work when this module is installed. A vicious cycle to say the least.

For blocks the current_theme condition does not have any sense at all, I will remove the logic that I changed related with blocks and update the summary.

Add MR changes as patch

Seems like the block is the only drupal core entity that uses the "plugin.manager.condition", and the current theme is excluded in this case as explained in previous comments so in the example I will use the google_tag scenario.

Why a select list here when all the other variations of this use checkboxes?

1. +++ b/core/modules/system/src/Plugin/Condition/CurrentThemeCondition.php
   @@ -111,11 +112,18 @@ public function evaluate() {
   +      $summary = $this->isNegated() ?
   +        $this->t('The current theme is not @theme.', ['@theme' => $theme]) : $this->t('The current theme is @theme.', ['@theme' => $theme]);
   +    }
   +    else {
   +      $summary = $this->isNegated() ?
   +        $this->t('The current theme is not one of the enabled themes.') : $this->t('The current theme is one of the enabled themes.');
        }
    
   -    return $this->t('The current theme is @theme', ['@theme' => $this->configuration['theme']]);
   +    return $summary;
   

   we can return early here and avoid using else

2. +++ b/core/tests/Drupal/KernelTests/Core/Plugin/Condition/CurrentThemeConditionTest.php
   @@ -27,12 +27,20 @@ public function testCurrentTheme() {
   +    $this->assertEquals(new FormattableMarkup('The current theme is not one of the enabled themes.', ['@theme' => '']), $condition_any_negated->summary());
   

   This feels like a UX WTF?

   How can a current theme not be one of the enabled themes?

   So as such I think we probably want to prevent the negate option when 'Any' is selected

Nice work folks

#37 works for me on 10.2.2

Just ran into this when working on #3482723: Create an admin route condition plugin or add generic "Current theme" options for admin and default theme and I think we should rate this as major issue, because currently once you saved the conditions, this condition is always added and evaluated and user has no way to disable this rule! This leads to wrong and unexpected condition results!

So the DROWL team will push things forward here very soon, as it affects us in multiple projects. :)

One important addition: The form element should be checkboxes, not radio!
It makes no sense to force selecting only one theme!

Selecting no theme at all should always evaluate to TRUE!

I wonder how this ever made its way through core reviews. The condition neither has sufficient UX, nor works technically well... we should get this fixed ASAP!

Ok, I adjusted the code, so that we now use checkboxes instead of the select. As @anybody and @yoroy already mentioned, this makes much more sense UX wise.

The only problem is, that this change requires an update hook, as we can now select multiple themes instead of only one. But I can't seem to find a good way on how to update a condition configuration, as it usually just gets merged into the form it was used. @larowlan do you know of a good way on how to update all form configs using the condition? If not, we could alternatively leave the "theme" config in as a fallback (simply wrapping an array around it) and deprecate it. Although I am not super keen to do so...

Anyway, apart from the update hook this can go back to "Needs Review", note that I did a total rewrite, so this needs to get reviewed throughout again.

Thanks @grevil totally makes sense and code LGTM so far. Sadly, I have no clue, how an update hook for a condition plugin can be written to work globally. I'm also unaware of any existing examples, while I think they possibly exist.

Is there maybe any core maintainer or someone else experienced with that? We'd really like to finish this, for UX and bugfixing reasons, as currently it's kind of broken by design ;)

i saw the needs usability review tag, and we can put the issue on the shortlist for the meetings on friday. but only one question, i've tried to quickly test the MR on 11.x now, but i have a hard time to see any actual change in the ui. the proposed resolution section in the issue summary is not very clear, i then tried to follow the instructions in #32, but again i see nothing in the ui of a block configuration page on a block layout page enabling me to choose a theme (nothing theme related in the vertical tabs nor in the select for the region - and i ran an drush updatedb as well as a drush cr).

@rkoller thanks, yes that's because this condition plugin doesn't seem to be applied on blocks, if I remember correctly.
We ran into this in contrib modules that allow all condition plugins, e.g. https://www.drupal.org/project/posthog or https://www.drupal.org/project/context - unsure if there's also a place in core where it's shown.

@Grevil can you please update the IS accordingly by time, if I'm right?

@anybody I updated the IS just yesterday:

Only instance in core would be BlockForm, but there the condition is manually unset, so it needs to be manually set again before testing

I will add the info, that it needs to get manually set IN CODE, together with the line.

the update to the issue summary helped, i am able to reproduce and test now. thanks a lot for the updated instructions @grevil!

Does there need to be a BC layer for the old theme property? I think this might be especially tricky because I don't think there's anything in core that uses this condition plugin's configuration, so there's nothing to write update hooks. Contrib/custom will have to write to own update hooks as needed, most likely.

Ok, I will prepare everything to declare "theme" deprecated. Thanks for your feedback!

EDIT: I created a CR draft: https://www.drupal.org/node/3523553

Alright, that should be it!

We should be able to allow BC for the theme key in the plugin itself and throw a deprecation if it's set, and automatically translate it to themes at runtime?

I made suggestions to handle translating the value to "themes" in the form and deleting in the submit, but I forget about translation at runtime. I'm not sure how this can be done, since I'm not sure how to know which property/subproperty in a config entity would correspond to this. And similarly outside of the form submit, how can the property be identified in a config save?

Separately, it might make sense to create a follow up to add constraints to the config schema for the new property, reusing the ExtensionAvailable work in #3432353: Add validation constraints to core.extension once that's in.

Left a review

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.

Ok adjusted everything accordingly! For some reason the KernelTests time out. I had a similiar issue in another core issue. Should be hopefully fixed with the next rebase.

Thanks @grevil - the broken test persists, no idea why. I just rebased it with latest core. Maybe look into other core MR's and check differences first?

@anybody! Nono, this is a success! The tests now actually show failures! Before it simply timed out after 1 hour.

All green now, finally! Ready for final review!

Whao, GREAT @grevil, tests are also green now!

I'll review this very soon. Unclear if this still Needs usability review? I think it's 10000x better than before, which really was misleading and bad ux :)

Totally fine and expected UX-wise from my perspective.

Thank you!

Code-wise LGTM and works as expected, has tests, what else do we want :)

Finally I just left some comment questions asking if we should add a consistent @deprecated comment everywhere "theme" is still present in code, so that it can be easily and consistently removed in D12 or would that just bloat the code?

Settings this RTBC anywhere for the maintainers etc. to look into this and decide.

Thank you all!

PS: I'd also still consider this a major UX bug, just like @chi in #3. From my perspective, the condition was not really usable.

Given we don't know where this might be used, and those places will need to write an upgrade path, I think the deprecation should be for removal in Drupal 13.

Alright, that should be it!

• I added further comments inside the tests for the deprecated "theme" key, so once it is not supported anymore, the reason for the failing test can be found quite easily.
• I updated the deprecation messages to note about the removal in Drupal 13 (instead of 12).
• I updated the change record to reflect this.

Please review!

Since we haven't heard anything from @rkoller since #55, I guess we can remove the "Needs usability review" tag?

The only place this is used is in block, and there it is unset, because it is completely unusable in its current state.

Nice, thank you Grevil! Thanks for also adding the other deprecation notices.

@catch is that fine?

Trying for a better title.

There is a failing test, Current theme condition plugin should provide an option to match all themes and introduce checkboxes

Sorry, forgot to message the expected deprecation messages inside the tests. Should be green now!

Yes, all green again.

Yes

Usability review

We discussed this issue at #3552911: Drupal Usability Meeting 2025-10-24. That issue will have a link to a recording of the meeting.

For the record, the attendees at the usability meeting were @rkoller, @simohell, and @the_g_bomb

At first apologies, I remember we've tried to review this issue back in #3522326: Drupal Usability Meeting 2025-05-09 but had issues reproducing and getting to the gist of the problem. We agreed on asking in here for clarification. But somehow no one of us has actually commented on the issue back then. :( Sorry again. :(

About two weeks ago I've noticed in my inbox that I got referenced in comment #70 by @grevil. I am "a bit" swamped in recent weeks so just got to revisiting the issue in todays meeting. We were able to navigate around the previous stumbling block and were able to review it. A few observations:

• The description is referring to a boolean value which is kind of taxing for less technical users and should probably be avoided . And in combination with the label of the negate checkbox it makes things hard to comprehend. the cognitive load is rather high.
• In general the negate checkbox is a taxing pattern. We have raised our concerns in related issues in the context of visibility plugins using that negate checkbox before. From a clarity perspective the radio button pattern used for the pages plugin is the preferable choice. Update: I found the related issue and the link to our review: #2986958-41: Add support for negating user role condition for block visibility... and i wanted to also point to the comment by @berdir on the same issue #2986958-44: Add support for negating user role condition for block visibility raising a concern about that approach from a technical perspective (also the reason why i wrote "IF" in the recommendation section below - not sure if his concern applies to this issue as well).
• In general the preferable reading/processing order on those conditional plugins would be first what you want to do (aka the negate checkbox or the radio button pattern) and after that "what you want to apply it to"
• the tab is missing a menu link summary other tabs like pages, response status, roles, and content type have.

So in summary we would have the following recommendations:

1. add the menu link summary for consistency reasons.
2. IF it would be technical possible try to use a radio button component instead of the negate checkbox. The two labels for the radio buttons could simply be Show for the selected themes and Hide for the selected themes and move the radio button before the list of checkboxes for the available themes. The description would become obsolete cuz the radio button labels would be self explanatory and clear from our perspective. In case a radio button would be impossible for this plugin or that step should be moved to a follow up then replace the description Select the themes this condition should evaluate TRUE on. with Show for the selected themes for now

Thank you, @rkoller! Back to "Needs work" to implement the recommended usability adjustments.

The description is referring to a boolean value which is kind of taxing for less technical users and should probably be avoided . And in combination with the label of the negate checkbox it makes things hard to comprehend. the cognitive load is rather high.

Adjusted the description accordingly. It now uses a similiar description to the "language" condition plugin.

add the menu link summary for consistency reasons.

This one is trickier than I thought. I have no idea, where this is defined in the condition plugin. I looked at other plugins but still no idea...
Interestingly enough, the "Vocabulary" condition has the same problem:

EDIT: Ok, this is WILD, the summary is defined inside "core/modules/block/js/block.js"... that should definitely NOT be the case, since this logic should be part of the condition plugin and NOT bound to the block module. Also, worth for another follow-up issue

In general the preferable reading/processing order on those conditional plugins would be first what you want to do (aka the negate checkbox or the radio button pattern) and after that "what you want to apply it to"

This should be fixed in a general issue for all conditional plugins, since ALL of the use the same pattern:
Applied first
Negate second

you simply can not see it, since Blockform disables negation for all other condition plugins in line 253-265 of "BlockForm":

    // Disable negation for specific conditions.
    $disable_negation = [
      'entity_bundle:node',
      'language',
      'response_status',
      'user_role',
    ];
    foreach ($disable_negation as $condition) {
      if (isset($form[$condition])) {
        $form[$condition]['negate']['#type'] = 'value';
        $form[$condition]['negate']['#value'] = $form[$condition]['negate']['#default_value'];
      }
    }

IF it would be technical possible try to use a radio button component instead of the negate checkbox.

Same here, we should create a central follow-up issue for this as well. Since the "negate" behavior is baked in code through "ConditionPluginBase" "isNegated()" method:

  /**
   * {@inheritdoc}
   */
  public function isNegated() {
    return !empty($this->configuration['negate']);
  }

This is not covered in the MR / Patches of [#14640540]. Therefore, I'd leave it as is for now.

I hope this is ok as is. Please review.

Ok, edited my other comment. Should be all fine now.

Thank you for the final improvements @grevil!

I agree that the checkbox vs. radio change should better be done everywhere consistently in a separate issue and not further block this. Of course, I totally agree with the UX improvement.

Thank you, RTBC! @rkoller and @quietone would you have a final look if all is fine for you now?

I left a suggestion on the MR in regard the wording. keeping the string in line with the one for the language plugin is a good call. i wasnt aware that you are able to unhide that plugin as well.

in regard of the summary, yep we noticed the missing summary on the vocabulary tab as well. we would open a followup issue for that in case no one else beats us to it. but for the current theme we just thought if it would be easy to add then do the adding in the scope of this issue for the sake of consistency, otherwise also as a follow up.

in regard of the order and the radio buttons i agree. that is what i tried to express. in contrast to the summary those two bits introduce sort of "new" approaches so it would hold back the issue.

Thanks for the rapid feedback @rkoller! I went ahead and fixed it, also fixed the return type issue. Please check if both is fine now, if tests are green.

Will you create the related / follow-up issues?

@grevil: You forgot to fix the tests accordingly. Please do so, I gave some examples... still WIP / NW

@anybody, why have you removed the PluralTranslatableMarkup return type?

The "response_status" condition plugin has the exact same return type.

Tests were failing here, see my commit message. Maybe you looked into a different core version or something...? After removing, all was fine again.

Ok, I am now using the "starterkit_theme" instead of the "test_theme", since its label is wrapped in strong tags for some reason.

Also adjusted the summary logic slightly, to support the old deprecated "theme" config key.

Please review once again!

Thanks for fixing the tests with the new texts! :)

@rkoller: All fine now?

i havent had the chance to test another time. but looking at the commits all the changes since i last looked were about code related issues and tests. so i most likely havent anything else to add, and in regard to your question in #82 i have not enough time and focus at the moment. currently completing a course which is heavy on assignment. i have currently dialed back everything in regard to contributions, the friday's ux meeting is the sole thing i do in recent weeks. i wouldnt be sad, rather thankful, if anyone else beats me to opening those follow ups or otherwise i will do that after that course is over at the end of november. and in case someone already opened the followup i will catchup and add to those then there.

Since @lauriii asked for more details on why this was needed in 2023, tagging for product manager review to make sure that question is sufficiently answered.

Thanks @catch! Yes would be great to finally get this fixed and finished. It was simply broken by design all the time... but rarely used I think, so that was barely visible, at least in core.

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.

Rebased

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.

@grevil: We have failing test, could you have a look again?

When will the product manager have a look at this presumably?

I removed the expectation of an deprecation error entirely, as there were a lot of new unrelated deprecation errors and IMO it doesn't make sense to check for 10+ deprecation errors inside a test where only 1 is relevant for the test in place.

@grevil agreed!

All green again!

Please post before / after screenshots to help evaluate what is being changed on the UI.

Added them to the issue summary.

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.

@godotislate do you have the required permissions to set the GitLab comments resolved that are resolved?
Are we able to finally get this fixed?

This is still entirely blocking the usability of the theme condition.
Thank you!

Minor test coverage issue in https://git.drupalcode.org/project/drupal/-/merge_requests/4331#note_747105.
IMHO we should remove #[IgnoreDeprecations] and create a specific test for the old way, which would be the only one tagged with #[IgnoreDeprecations] and should use $this->expectDeprecation

@penyaskito, great idea! I split the test accordingly, rebased the issue branch to current main and set the deprecation version to 12.0.0 (both in the deprecation message and the change record). The last step was done, because current main should be on par with Drupal 12 I think?!

When everything is green, we should definitly start merging this, as the theme condition is currently unusable otherwise.

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

@penyaskito can I put the status back to RTBC? I really don't want to delay this any further...

Ah sry, tests fail.

All green again! I only adjusted the tests, so this should be good to go again!

Thanks for the fix @grevil! Back to RTBC! Let's get this one fixed!

It looks like we need a follow to properly implement \Drupal\Component\Plugin\DependentPluginInterface::calculateDependencies() for this plugin as it's not adding the theme as a dependency.

Adjusted accordingly.

It looks like we need a follow to properly implement \Drupal\Component\Plugin\DependentPluginInterface::calculateDependencies() for this plugin as it's not adding the theme as a dependency.

I'll create a follow-up issue, once this is merged. Thanks for the review @alexpott! Should "hopefully" be good to go now!

Created the follow-up issue here: #3590591: Implement "DependentPluginInterface::calculateDependencies()" for the "CurrentThemeCondition" .

All green again, LGTM. Thanks @grevil! Thanks also for the follow-up!

While we've got BC and a trigger_error deprecation, what happens to existing config that uses the old storage format? Site builders generally do not see deprecation messages, and even if they do see 'The "theme" configuration key is deprecated in drupal:12.0.0 and will be removed in drupal:13.0.0. Use "themes" instead.' neither the message nor the change record tell them what to actually do to fix this problem. I think they just have to re-save any affected config? But if they don't, then this will silently break in Drupal 13 when we remove the BC layer.

We could have an update hook that saves all affected block config for example - but note also that the condition API could be used elsewhere in other config entities, and I'm not sure how to solve this.

We could have an update hook that saves all affected block config for example

Blocks don't actually use the condition. It's removed from the block form and there are no settings for it saved to block configuration AFAICT. I believe there is no use of the current_theme condition plugin in core other than tests.