Refactor block visibility to use condition plugins

Here's the plan, as I see it:

• Give block plugins the ability to have conditions (a ConditionBag on BlockBase)
• Hardcode the UI to always use exactly one instance of each condition
• Determine a mechanism for gathering the contexts of a block (possibly borrow the evented approached in PageManager)
• Use #2277981: Provide a service for handling context-aware plugins to manage the context
• Borrow ContextAwarePluginAssignmentTrait to manage the context assignments (needs issue)
• ???
• Design a UI that helps us remove the hardcoded conditions

1. +++ b/core/modules/block/src/BlockBase.php
   @@ -154,7 +169,22 @@ public function buildConfigurationForm(array $form, array &$form_state) {
   -    $period = array(0, 60, 180, 300, 600, 900, 1800, 2700, 3600, 10800, 21600, 32400, 43200, 86400);
   +    $period = array(
   +      0,
   
   @@ -227,7 +257,8 @@ public function validateConfigurationForm(array &$form, array &$form_state) {
   -  public function blockValidate($form, &$form_state) {}
   +  public function blockValidate($form, &$form_state) {
   +  }
   
   @@ -251,7 +282,8 @@ public function submitConfigurationForm(array &$form, array &$form_state) {
   -  public function blockSubmit($form, &$form_state) {}
   +  public function blockSubmit($form, &$form_state) {
   +  }
   
   @@ -318,7 +350,7 @@ public function getCacheBin() {
   -    return (int)$this->configuration['cache']['max_age'];
   +    return (int) $this->configuration['cache']['max_age'];
   

   Out of scope (a bunch of other things too)

2. +++ b/core/modules/block/src/BlockBase.php
   @@ -332,4 +364,55 @@ public function isCacheable() {
   +        'request_path',
   +        'node_type',
   +        'user_role',
   +        'language',
   

   This works for now, but we should definitely put an @todo above this.

The easy part of the saving is just changing BlockBase::getConfiguration to be return array('visibility' => $this->getConditions()->getConfiguration()) + $this->configuration;

Pushing this forward a bit.

This now depends on #2277981: Provide a service for handling context-aware plugins.
Attaching a version without it as well.

It seems to be working, but I need to start in on converting the tests to use the new visibility (the old stuff is alongside it for now).

Also tracking this for page_manager since I'm moving code from there to here (ConditionAccessResolverTrait and ConditionPluginBag)

Fix schemas, default config, the language condition, fixed the hardcoded route context to work only for nodes.

Still includes ContextHandler.
Pretty much everything should be working now, except the vertical tabs are broken.
Also some bizarre serialization problem.

Fix node type for node/add/% page, fix migration path, clean up tests looking for visibility, use defaultConfiguration consistently.

19b includes a change to ConditionPluginBag that tries to work around the ConfigInstallAllTest fail.

The ContextHandler issue went in, so this is a bit smaller.

Okay, this should pass now!
Remaining tasks:
- Fix vertical tabs
- Decide if this will go in as-is with the hardcoded conditions
- Chop this up into smaller issues where possible.

First sub issue: #2283929: Clean up condition plugins to have schema support and to allow them to be optional

Second sub issue: #2283969: Add a protected BlockBase::blockAccess() to contain plugin-specific logic

Rerolled with a couple tweaks, and also a version for when the subissues are committed.

Woot, was able to both fix the vertical tabs and remove the moduleExists() hacks.

Fixed the test fails, and finally switched the hardcoded contexts for injected events.

Okay, both blockers are in, here it is!

There are no unresolved @todos, there are just two pointing to #2284687: Redesign UI for better management of block visibility.

Judging by the other major tasks in the queue, this is equally major.

Leaving assigned for rerolls and responding to feedback, but this is ready for review.

Read through the whole patch, installed and tried the functionality as well. Works nice.

Found only a few things.

1. +++ b/core/lib/Drupal/Core/Condition/ConditionAccessResolverTrait.php
   @@ -0,0 +1,52 @@
   +      catch (PluginException $e) {
   +        // If a condition is missing context, consider that a fail.
   +        $pass = FALSE;
   +      }
   

   In case a context is required and therefore it is an error state, shouldn't this exception bubble up? Otherwise it will just resolve to false, which is a regular state and we do not get notified about the error state.

2. +++ b/core/modules/block/src/BlockBase.php
   @@ -212,6 +287,18 @@ public function buildConfigurationForm(array $form, array &$form_state) {
   +    foreach ($this->getVisibilityConditions() as $condition_id => $condition) {
   

   Minor UI thing: the Negate checkbox for pages visibility comes below the textarea, while for others it is the very first form element. So let's make it uniform :)

3. +++ b/core/modules/block/tests/src/BlockBaseTest.php
   @@ -38,19 +37,78 @@ public function testGetMachineNameSuggestion() {
   +    $plugin_manager = $this->getmock('drupal\core\executable\executablemanagerinterface');
   ...
   +          'id' => 'language',
   

   Shouldn't be get*M*ock?

To make this RTBC someone else should take a look as well as I do not feel confident enough for that as most of this stuff is still new for me. Therefore keeping as "needs review"

This needs UX review. From the screenshot 'Negate the condition' is painful.

Colloquial meaning of negate is to nullify something. If you don't know its usage within CS then you'd think it was disabling the specific condition rather than reversing it.

It's great to see we are finally tackling this problem, its long overdue - and I am also very happy to see it in the core queue early on.

I am not really sure, what I should do with this issue. It states that the UI is to be discussed in a new issue. I think the best way moving forward is to

1. Open the UI issue for discussion, and update the issue summary - so designers can start throwing ideas.
2. Minimize the UX regressions, introduced by this patch.
3. Raise the criticality of the UI issue, as without it we will need a good complete solution.

Frankly I'd like to avoid the strategy we took with the Blocks UI that basically left us with UX cleanup for over 6 months (and some parts are still broken). The best way to tackle that is to reduce the regressions we introduce in the initial patch and having a clear path to fixing the whole UX. As Tim rightfully points out, this list of conditions can grow exponentially and we should have a usable pattern for it.

Sorry, for being so hard - but we are now in release mode anything that's added should be ready for release.

To fix the current hardcoded conditions, we should aim to be as close as possible to the original design - without losing the new functionality.

The labels for each group should not include any lingo or terms that are not familiar to the site builder audience. I suggest we move back to the original as much as possible, which has been usability tested a lot:

• Node bundle should be "Content types"
• Request path should be "Pages"
• User role should be "Roles"

And where possible we should bring back the vertical tab descriptions.

We should remove the text "If you select no roles, the condition will evaluate to TRUE for all users.". That should be obvious.

We should change Negate the condition to something more sensible. Honestly, I don't think we should be showing the option - since it's terribly confusing. Thats better reserved for the complete redesign of the conditions UI. Not something thats easily fixed by just text.

I had comments written up pretty much in line with Bojhan, so just saying +100 to those. Additionally, did we also loose the friendly summaries that made us not needing to tab into each tab to see what is going on?

"Negate this condition" for the path/pages condition replaces the old toggle of "Shows this block on every page except the listed pages." vs "Shows this block on only the listed pages."

So we at least need that there.

I restored the detail description JS, and added an override for the condition labels *within the block form*.

We should remove the text "If you select no roles, the condition will evaluate to TRUE for all users.".

This is in the condition plugin itself. We can open a general issue for improving their UX.

I also bumped #2284687: Redesign UI for better management of block visibility to major.

Here's an updated screenshot of the new (old) names and the JS back in action.

Now its content types vs. node types. Why is the condition plugin UI even using developer facing terminology like node type / bundle and TRUE in the first place?

No idea why conditions are that way. NodeType is relatively very old. And it alternates between "node types" and "node bundles" itself. Fixing here is in scope, messing with the conditions themselves is not.

Removing the user roles description per @Bojhan's request, and fixing the negate positioning per @blueminds.

Ugh what a silly test. Why did I ever write that.

1. +++ b/core/lib/Drupal/Core/Condition/ConditionAccessResolverTrait.php
   @@ -0,0 +1,52 @@
   +    // If no conditions passed and one condition was required, deny access.
   

   I think this comment is wrong. The logic looks right, but the comment doesn't match what we're doing here.

2. +++ b/core/modules/block/src/Event/BlockContextEvent.php
   @@ -0,0 +1,39 @@
   +
   +/**
   + * @file
   + * Contains \Drupal\block\Event\BlockContextEvent.
   + */
   +
   +namespace Drupal\block\Event;
   +
   +use Drupal\block\BlockPluginInterface;
   +use Symfony\Component\EventDispatcher\Event;
   +
   +/**
   + * Wraps a block in order for event subscribers to add context.
   + *
   + * @see \Drupal\block\Event\BlockEvents::BLOCK_CONTEXT
   + */
   +class BlockContextEvent extends Event {
   +
   +  /**
   +   * @var \Drupal\block\BlockPluginInterface
   +   */
   +  protected $block;
   +
   +  /**
   +   * @param \Drupal\block\BlockPluginInterface $block
   +   */
   +  public function __construct(BlockPluginInterface $block) {
   +    $this->block = $block;
   +  }
   +
   +  /**
   +   * @return \Drupal\block\BlockPluginInterface
   +   */
   +  public function getBlock() {
   +    return $this->block;
   +  }
   +
   +}
   

   As mentioned in channel, I question the need to pass the block around. We should just collect an array (or bag or something) of contexts and pass the whole group off to the plugin bag of conditions in order to do the appropriate mapping.

3. +++ b/core/modules/block/src/Event/BlockEvents.php
   @@ -0,0 +1,23 @@
   +  const BLOCK_CONTEXT = 'block.block_context';
   

   This is actually used for condition contexts on blocks, not the block context itself.

4. +++ b/core/modules/block/src/EventSubscriber/NodeRouteContext.php
   @@ -0,0 +1,71 @@
   +    elseif ($request->attributes->get(RouteObjectInterface::ROUTE_NAME) == 'node.add') {
   +      $node_type = $request->attributes->get('node_type');
   +      $context = new Context(array('type' => 'entity:node'));
   +      $context->setContextValue(Node::create(array('type' => $node_type->id())));
   +      $event->getBlock()->addContext('node', $context);
   +    }
   

   OOOOH DAYUM! Yes please. :-)

In general, I approve of what I see here. I question some of the decisions here, but none of them are worth blocking this over and can be sorted out later as we get to play with this in other situations.

Eclipse

1) I think the word "required" threw you off here. Switched to "needed" and clarified.
2&3) Fixed, I reworked it so that the plugin bag is responsible.

I also included an interdiff of changes I made independently:

• Use a base class for context subscribers
• Don't typehint AccountProxy if not needed

The only blocker is resolving the "negate" language, which @Bojhan said he'd do tonight/today.

Started with this review yesterday, a few things might have been pointed out by others already, for example the == and trick.

1. +++ b/core/lib/Drupal/Core/Condition/ConditionAccessResolverTrait.php
   @@ -0,0 +1,52 @@
   +   * @param string $condition_logic
   +   *   The logic used to compute access, either 'and' or 'or'.
   

   Would it make sense to have a constant for and/or? not sure, but passing around arbitrary strings is always a bit fragile... Ignore if we're already doing it like this elsewhere (didn't found any mention of $condition_logic in core yet, though)

2. +++ b/core/lib/Drupal/Core/Condition/ConditionAccessResolverTrait.php
   @@ -0,0 +1,52 @@
   +      catch (PluginException $e) {
   +        // If a condition is missing context, consider that a fail.
   +        $pass = FALSE;
   +      }
   

   Should we check for the context explicitly? Can we? Relying on exceptions for this seems bad because it's an expected behavior that we (I assume) can check in advance and throwing exceptions is slow because that has to generate the backtrace and so on.

3. +++ b/core/lib/Drupal/Core/Condition/ConditionAccessResolverTrait.php
   @@ -0,0 +1,52 @@
   +
   +    // If no conditions passed and one condition was required, deny access.
   +    return $condition_logic == 'and';
   

   Had a hard time understanding this, especially as the comment doesn't really make sense to me.

   We're dealing with both 'and' and 'or' here, so for 'and', we return TRUE because we would have returned FALSE if one did not match and for 'or', we would have returned TRUE already if one did match.

   Maybe we can comment it more like that?

4. +++ b/core/modules/block/block.module
   @@ -391,10 +375,11 @@ function template_preprocess_block(&$variables) {
      foreach (entity_load_multiple('block') as $block) {
   -    $visibility = $block->get('visibility');
   -    if (isset($visibility['roles']['roles'][$role->id()])) {
   -      unset($visibility['roles']['roles'][$role->id()]);
   -      $block->set('visibility', $visibility);
   +    /** @var $block \Drupal\block\BlockInterface */
   

   There's Block::loadMultiple() now that is supposed to avoid the @var, but unfortunately, PhpStorm doesn't understand static[], so it doesn't work yet..

5. +++ b/core/modules/block/block.services.yml
   @@ -6,3 +6,18 @@ services:
   +  block.current_language_context:
   +    class: Drupal\block\EventSubscriber\CurrentLanguageContext
   +    arguments: ['@language_manager']
   +    tags:
   +      - { name: 'event_subscriber' }
   

   Shouldn't it be language module that provides this context and the condition/context for it?

6. +++ b/core/modules/block/block.services.yml
   @@ -6,3 +6,18 @@ services:
   +  block.node_route_context:
   +    class: Drupal\block\EventSubscriber\NodeRouteContext
   +    arguments: ['@request_stack']
   +    tags:
   +      - { name: 'event_subscriber' }
   

   Same here, doesn't this add a dependency on node.module to block.module? afaik node right now checks block access/visibility?

7. +++ b/core/modules/block/src/BlockBase.php
   @@ -236,6 +323,15 @@ public function validateConfigurationForm(array &$form, array &$form_state) {
   +    foreach ($this->getVisibilityConditions() as $condition_id => $condition) {
   +      // Allow the condition to validate the form.
   +      $condition_values = array(
   +        'values' => &$form_state['values']['visibility'][$condition_id],
   +      );
   +      $condition->validateConfigurationForm($form, $condition_values);
   

   That's pretty scary but I've noticed this before, I guess we're already doing something like that for the block plugins themself?

   $form_state there isn't actually form state, and if they use it to store something else for example it wouldn't work...

"Click to negate" seems hard to understand to me. I added an idea to #2284687: Redesign UI for better management of block visibility that might work around having to use that wording.

Lets move that discussion over here, in #2284687: Redesign UI for better management of block visibility we should be discussing the more advanced and complete condition UI.

I would take roy's suggestion and do some final tweaking to the wording. I suggest something like this:

Show for the selected content types:
Show for the selected roles:
Show for the listed pages:

The spacing should mimic what we do in Views for options that are directly related to each other (visual nesting on the horizontal space).

Ok, some quick responses to other comments here.

1.) Tim I don't think your clarifications on that comment that both berdir and I mentioned clarified it much.

2.) Berdir, the CurrentLanguageContext & NodeRouteContext services are specific to block module's implementation (as is the current user service). I completely understand your comment here, but this is just block module's specific methodology for doing this and is not generic to context resolution in the grand scheme. In that light, I think having it in block module is perfectly legitimate.

3.) Yoroy & Bojhan, keep in mind this needs to be a string we can generically generate over and over and over again for any condition plugin ever created. I previously suggested something like "Invert" or something along these lines. I understand how "negate" is perhaps suboptimal in terms of communicating what this does, but we also can't get too wordy here because we have to support generating that string. It's not hard coded.

Kris

#45.1
I borrowed this from EntityQuery/Views, I don't think its a problem.

#45.2
I don't think its reasonable for us to introspect the plugin to ask it what context[s] it expects, and there is a chance that conditions throw other exceptions.

#45.3
I rewrote this comment in the last patch, is that enough?

#45.4
Womp womp.

#45.5
Similar to block_user_role_delete, I agree with @EclipseGc that this can stay in block.module, at least for now.

#45.6
This is a pattern across all places PluginFormInterface is used...

Let me see what I can do about #47, I agree this should be solved here.

Patch incoming.

#48.2. Yeah, it's basically the question if block should depend on node or node on block ;) node.module is optional now (as is block). So if you run that context without node.module enabled, it will simply never find a route that has {node} in it.. a bit strange, but not something we have to be worried about I guess.

Btw, cross-referencing #2204363: [sechole] Returning TRUE from hook_entity_access()/hook_ENTITYTYPE_access() must not bypass EntityAccessController::checkAccess(), If I understand this correctly then this issue will solve the "critical security issue" part of that issue as we will no longer (mis-)use entity access for block visibility.

Okay, I found the original code for the access condition stuff in ctools_access(), and borrowed the comment directly.

I also was able to switch to the radio buttons.

Nice, I like the pragmatic approach this patch took.

@EclipseGc expressed some concerns about confusion stemming from using negate with content types, since it could lead to confusing scenarios.
So, I've hidden negate on everything but Pages, since that maps exactly 1:1 with HEAD.

See you over in #2284687: Redesign UI for better management of block visibility for expanding/improving the UI, and thanks to @Bojhan and @EclipseGc for talking through this on IRC.

   * @return \Drupal\Core\Condition\ConditionInterface
   */
  public function &get($instance_id) 

Why do we need to return a reference to an object? Either the return value is incorrect or the reference is. Do we expect the caller to use this to set the plugin? Cos then the return should be amended with |NULL . If not... then it shouldn't be a reference.

Negating node types is a welcome feature addition.

I like where this is going. Much more secure; this way.

@chx, that's in the base PluginBag class itself, I'm just overriding it for the typehint change.

All my left over concerns are fairly mild and look like something that can be iterated upon in contrib. I'm ++

RTBC

Eclipse

Reroll around #2256679: Use config schema to determine which config entity properties to export regardless of whether they are public or protected, back to RTBC

Oh right, that's a result of the above issue. Reroll.

Suggested commit message:

Issue #2278541 by tim.plunkett, kim.pepper, Bojhan, EclipseGc: Refactor block visibility to use condition plugins.

@kim.pepper and @tim.plunkett nice work on this.

I just have a minor concern, if #1932810: Add entity bundles condition plugin for entities with bundles is a 9.x feature(which will also removing the existing code with new and more generic api) then why this issue is not a feature. This is not an API clean up because it is replacing the existing code with new generic api which means it is an API change. IMHO we shouldn't be doing this right now at this point and move it to 9.x.
And if we are still doing this then please explain why #1932810 is not happening.

Wonderful :)

I can only find nitpicks, won't un-RTBC.

1. +++ b/core/lib/Drupal/Core/Condition/ConditionAccessResolverTrait.php
   @@ -0,0 +1,53 @@
   +   * @return bool
   +   *   Whether these conditions grant or deny access.
   

   It feels like these should return AccessInterface::(ALLOW|DENY|KILL)?

2. +++ b/core/modules/block/block.module
   @@ -12,22 +12,6 @@
   - * Shows this block on every page except the listed pages.
   - */
   -const BLOCK_VISIBILITY_NOTLISTED = 0;
   -
   -/**
   - * Shows this block on only the listed pages.
   - */
   -const BLOCK_VISIBILITY_LISTED = 1;
   -
   -/**
   - * Shows this block if the associated PHP code returns TRUE.
   - */
   -const BLOCK_VISIBILITY_PHP = 2;
   -
   -
   -/**
   

   \o/

3. +++ b/core/modules/block/src/BlockBase.php
   @@ -236,6 +340,15 @@ public function validateConfigurationForm(array &$form, array &$form_state) {
   +      $condition->validateConfigurationForm($form, $condition_values);
   +
   +    }
   

   Extraneous newline.

4. +++ b/core/modules/block/src/BlockPluginInterface.php
   @@ -140,4 +141,35 @@ public function blockSubmit($form, &$form_state);
   +   * @return \Drupal\Core\Condition\ConditionInterface[]|\Drupal\Core\Condition\ConditionPluginBag
   +   *   An array of configured condition plugins.
   

   s/array/array or bag/

5. +++ b/core/modules/block/tests/src/BlockBaseTest.php
   @@ -38,19 +37,78 @@ public function testGetMachineNameSuggestion() {
   +  public function testCondtionsBagInitialisation() {
   

   s/Condtions/Conditions/

@jibran The EntityBundle issue has very little obvious benefit. #2281659: Create a vocabulary condition and NodeType are relatively different, and just from a UX perspective it doesn't help to use derivatives.
Regardless, if that issue went in, we'd have yet another unused condition in core.

This issue proves that conditions and context actually work, and fixes a security hole. So that's why its major.

#63.1
No, since there is no third state here, we explicitly want TRUE/FALSE. By the constant names in AccessInterface, it would seem we wanted ALLOW/DENY, but that's TRUE/NULL, and would have zero use for FALSE.

#63.5
More importantly, I misspelled "initialization"! ;)

For the record I 100% agree with Tim on 63.1 and still consider this RTBC

Eclipse

Thank you @tim.plunkett for the explanation.

and fixes a security hole.

Ok then I think it is fine.

#63.1: Ok. I find it bizarre to use booleans in one case of access checking and the standardized ALLOW/DENY/KILL elsewhere. Consistency is more important than the implementation detail that this would happen to not use DENY, only ALLOW (TRUE) and KILL (FALSE). But I'm no expert in this system, so if you feel strongly about this, that's fine with me, but doesn't remove my concern.

Overall, this patch is wonderful though, so +1 RTBC :)

Wim,

This is not an access checker. Could it be used as such? of course it could, but this is a condition system designed to build up boolean returns. Both PageManager and Rules leverage this sort of approach, so having a non-boolean 3rd option with which to contend would greatly complicate the system.

To my point about checking access, yes this could be used for that purpose, but should have an adapter layer around the condition response(s) in order to return in a way consistent with Drupal core. This has no bearing on a block wanting to know if a node is of a particular type. I can only answer TRUE/FALSE to that question. What the system asking that question does with the answer is a completely separate thing, though in the case of "visibility" this continues to be a boolean question. It either does display or it doesn't. Another option is unnecessary. There is no need to "differentiate between deny/kill because the system acknowledges different potential approaches, both and/xor-ing conditions together as well as (in this example) locking to Drupal 7 style defacto "and".

I hope that basic explanation put your mind at ease with what we're doing here. TL;DR: We're attempting to replicate what D7 does currently but with our plugins, and that (like all other intended use cases for conditions) is a simple boolean operation.

Eclipse

Added suggested commit message to summary

I spent a good half hour looking through this patch with Tim. Lots of things to love here. Overall this is a great improvement, which both adds better security and actually exercises the condition/context APIs in core, and also provides a solid foundation from which modules like Page Manager and Rules can build. It is, however, a HUGE change, very late in the cycle, and opens the door to other massive changes. :\ But in talking to Tim, this is the main key foundational piece ("once this goes in, my changes to page_manager are 10 files changed, 44 insertions(+), 225 deletions(-), plus a whole extra set of functionality to play with" which is pretty impressive), and further, since it's "merely" tying two existing APIs together that are already in core, qualifies under "API completion," in my view.

Thanks all for the thorough reviews on this, as well as the UX work. Wasn't really anything for me to nitpick, except possibly the location of some of the block visibility conditions, but that's a minor point. Change notice looks good!

Committed and pushed to 8.x. Thanks, all!

Thanks! Change notice is here https://drupal.org/node/2287827

tag

#68: your explanation makes a lot of sense — thanks! However, then I'd argue that ConditionAccessResolverTrait is misnamed. Because it leaves very little doubt it's about access, its @return documentation even says Whether these conditions grant or deny access., and the code comments in the function body have similar descriptions. What you're saying makes me think it should be called just ConditionResolverTrait?

I'll leave you and Tim to debate that, but you may be correct.

Eclipse

+++ b/core/modules/block/src/BlockAccessController.php
@@ -64,65 +30,8 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
-      if (empty($visibility['language']['langcodes'][\Drupal::languageManager()->getCurrentLanguage($visibility['language']['language_type'])->id])) {

+++ b/core/modules/block/src/BlockForm.php
@@ -94,122 +80,6 @@ public function form(array $form, array &$form_state) {
-      // If there are multiple configurable language types, let the user pick
-      // which one should be applied to this visibility setting. This way users
-      // can limit blocks by interface language or content language for example.
-      $info = $this->languageManager->getDefinedLanguageTypesInfo();
-      $language_type_options = array();
-      foreach ($language_types as $type_key) {
-        $language_type_options[$type_key] = $info[$type_key]['name'];
-      }
-      $form['visibility']['language']['language_type'] = array(
-        '#type' => 'radios',
-        '#title' => $this->t('Language type'),
-        '#options' => $language_type_options,
-        '#default_value' => !empty($visibility['language']['language_type']) ? $visibility['language']['language_type'] : reset($language_types),
-        '#access' => count($language_type_options) > 1,
-      );

While working on #2183983: Find hidden configuration schema issues I noticed there is a language type setting on blocks that is somehow not valid. That made me think.... Wait a minute, blocks do have a language type setting. Well, not anymore.... Looks like this refactoring ignored that blocks had that setting and got rid of them on the way. It IS true that there is/was no adequate test to prove the language types worked, but core/modules/block/src/Tests/BlockLanguageTest.php still tests the interface language type (which causes the strict config fail).

It would be great if we'd not remove features like this and instead add it back. Opened #2339151: Conditions / context system does not allow for multiple configurable contexts, eg. language types.