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?