Drupal Current theme condition plugin should provide an option to select all themes

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?

We are changing the plugin because of https://www.drupal.org/project/google_tag/issues/3263122 ✨ Recovery the current theme condition plugin Active , 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" 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 ✨ Recovery the current theme condition plugin Active to have the ability to use google_tag on all themes or only on a theme, 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 ✨ Create an admin route condition plugin or add generic "Current theme" options for admin and default theme Active 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?!

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!

grevil → changed the visibility of the branch 2783897-drupal-current-theme to hidden.

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. 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 🐛 Drupal Current theme condition plugin should provide an option to select all themes Needs review , 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 first, 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 📌 [PP-1] core.extension should be validatable Postponed once that's in.