Allow blocks to be configured to show/hide on 200/403/404 response pages

Here is a test.

Probably it's better to say, 'Show block on error pages' rather than 'exception pages'. 'Exception' is quite esoteric.

+++ b/core/modules/block/lib/Drupal/block/BlockAccessController.php
@@ -81,6 +81,11 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
+    if (isset($visibility['path']['exception']) && empty($visibility['path']['exception']) && \Drupal::request()->attributes->has('exception')) {
+      return FALSE;
+    }

If the access is cached, it is just cached statically, so this is fine. Can we please inject the request stack instead?

The patch works great for me.

Reroll after #2245783: Regression: Help blocks display on 403/404 page and a small fix to the checkbox. I think we need a new test because the other issue added a similar test.

1. +++ b/core/modules/block/lib/Drupal/block/BlockAccessController.php
   @@ -81,6 +93,11 @@ protected function checkAccess(EntityInterface $entity, $operation, $langcode, A
   +    if (isset($visibility['path']['error_pages']) && empty($visibility['path']['error_pages']) && $this->requestStack->getCurrentRequest()->attributes->has('exception')) {
   

   Our ExceptionController does not set the 'exception' attribute. Should we check for '_exception_statuscode' here or set the attribute in ExceptionController?

2. +++ b/core/modules/system/lib/Drupal/system/Plugin/Block/SystemHelpBlock.php
   @@ -91,11 +91,6 @@ public function access(AccountInterface $account) {
   -    // Do not show on a 403 or 404 page.
   -    if ($request->attributes->has('exception')) {
   -      return '';
   -    }
   -
   
   +++ b/core/profiles/standard/config/install/block.block.bartik_help.yml
   @@ -13,6 +13,7 @@ visibility:
   +    error_pages: false
   
   +++ b/core/profiles/standard/config/install/block.block.seven_help.yml
   @@ -13,6 +13,7 @@ visibility:
   +    error_pages: false
   

   Per #2245783: Regression: Help blocks display on 403/404 page I think we should not change this, but always hide the help block on 403/404.

Our ExceptionController does not set the 'exception' attribute. Should we check for '_exception_statuscode' here or set the attribute in ExceptionController?

But doens't the exception listener?

But doens't the exception listener?

You are right it does, but that request (with exception attribute) is not used if you set error pages at admin/config/system/site-information.

Here is a new test and the exception attribute. Couldn't find any usages for _exception_statuscode - do we need it?

Thanks, filed #2251249: Make exception and _exception_statuscode available on all exception pages..

What do you think about #11.2? The help block does not really work well on error pages and "could potentially cause an information disclosure vulnerability" (#2245783-9: Regression: Help blocks display on 403/404 page).

Patch no longer applies.

Hello.
I've created new patch for this issue (tested on Drupal 8.5.6). It resolves the issue for me. But it does not contain changes for help block (it will be hidden on error pages in any case).
Hoping it will be useful for someone.

Updated version of patch #24 with fixed tests affected by this change.

Updated version of patch #24 with fixed tests affected by this change.

I tried the patch in #26, but it does not work in the following case:

I have a node (content type: basic page) that I use as a custom 403 (access denied) page.

I set this as the custom 403 page on: /admin/config/system/site-information. The path is /node/21, and the alias is /error/403.

On the block admin page, I added the user login block to the content region. I added the restriction "show only on certain pages":

/error/*

/view/*

/node/21

I have a view that requires "add content type ABC permission" to access. The path to the view is /view/abc. The anonymous user does NOT have this permission.

So, as an anonymous user, I go to /view/abc and am shown the content of /node/21. BUT, the user login block is not shown.

If I go to the block administration screen and remove the "show only on certain pages" restriction, then I see the block as the anonymous user on /view/abc, but I don't want to show the login block on every single page.

Since this patch adds a checkbox "Show block on error pages", I assume that if I have the error page path set as an allowed path and I have checked the "Show block on error pages" checkbox, then I should see the block, but I don't.

I can confirm that patch #26 works as designed in combination of custom error page set via Basic site settings and rabbit hole module.

I thought this would solve an issue I was having, but this turned out not to do what I would expect and isn't really useful in my case.

I would expect that I can explicitly choose to show a block on error pages, regardless of other visibility settings. If other visibility settings are taken into account, then there seems to be no way to only show a block on the 404 page, but nowhere else.

I tried making the block visible on the node I'm using as a 404 page, and then checking the box to display the block on error pages, but it seems that the path filter is overriding the "show on error pages" checkbox. Is this only able to be used on blocks that are visible by default on the site?

It almost seems like we need a path filter such as "[404]" or "[error]" or something to be able to match any path that would return that error, so that it can be combined with other path-based filters.

> I would expect that I can explicitly choose to show a block on error pages, regardless of other visibility settings. If other visibility settings are taken into account, then there seems to be no way to only show a block on the 404 page, but nowhere else.

This is what i expected too from the IS.

> Add a visibility setting to blocks that shows them on 403/404 pages.

Looking into the code, this adds an additional restriction on the path condition. We should either
* a) extend this path condition addition to errorpages/ordinarypages/both
* b) make this a separate condition plugin

I think a separate condition plugin is better, I have the code for that in a project in working on.
If you want I can provide that code for this patch.

Hi there! Thank you all for the work done here so far.
I agree that a separate condition plugin would be better. It would totally improve site builders options.
@jidrone would you share your code with us, please?

@jidrone #32:
> If you want I can provide that code for this patch.

I'd appreciate and review that.

Is this code similar to the code of https://git.drupalcode.org/project/block_in_page_not_found?

Okay I added a new condition in core/modules/system/src/Plugin/Condition/StatusCode.php. And I basically made it follow all similar conditions. You can select status code 200, 403 and 404. Or select none to have this block shown on all pages. In a first attempt I tried to make this work with only 403 and 404 being selectable but this would have made this condition plugin leave the common pattern. I think having 200, 403 and 404 as options makes more sense in this place and maybe even works better together with the other condition plugins.

We still need tests though. I'm still quite inexperienced with tests. But with someone giving me some hints (where to add them exactly or what to look at as samples or similar use cases) I'm absolutely willing to look into it as well.

Ui, seems there's a bit more to do. $ drush cim is failing now due to status_code not being a valid plugin ID. Need to look if this comes from Drupal or from Drush.

The import failed due to the following reasons:
Unexpected error during import with operation create for block.block.loginr
edirectbutton: The "status_code" plugin does not exist. Valid plu
gin IDs for Drupal\Core\Condition\ConditionManager are: entity_bundle:crop,
entity_bundle:node, entity_bundle:redirect, entity_bundle:taxonomy_term, e
ntity_bundle:menu_link_content, entity_bundle:paragraph, language, node_typ
e, current_theme, request_path, user_role

False alarm. Probably just the cache. But love to hear your opinions.

Great stuff. The interface looks super intuitive. Code-wise it looks quite straightforward. Did not test though.

It would be nice to add an option like we have for the page paths.

Show for the listed codes
Hide for the listed codes

Because if you use the option, not "path/test" path, you can also wanted it to not show on the 404 page and you cannot predict the path. Just a suggestion.

Thanks for the feedback.

I think this show/hide option for the listed status codes doesn't make much sense for this one condition plugin. There are only three codes. If you don't want to have a block shown on the 404 page, you'd select 200 and 403, keeping 404 unselected.

Hiding a block on all three status codes wouldn't make much sense as well. Why would you place it on the page then at all? And even if this would be necessary for whatever reason you'd instead simply create a new region naming it "Disabled" for example which never gets printed anywhere and place this block in this region.

This condition plugin should enable you to

• Show a block everywhere except 403 and 404
• Show a block everywhere except 403 and 404 and use the other conditions as well (roles, pages, language)
• Show a block only on a 403 code
• Show a block only on a 403 code and use the the other conditions as well (roles, pages, language)
• Show a block only on a 404 code
• Show a block only on a 404 code and use the other conditions as well (roles, pages, language)
• Show a block only on a 403 and 404 code
• Show a block only on a 403 and 404 code and use the other conditions as well (roles, pages, language)

Probably some test cases in there.

I'm currently using the provided patch on a site to have a "Login and redirect back here" button placed on 403 status code and path restricted to "/node/*" and "/admin/content". Editors following a link to https://example.com/node/1/edit or https://example.com/admin/content while not being logged in will see this block. But it won't appear on just /admin or /admin/config 403 pages for example. Works fine so far.

I haven't tested it yet with a specific node defined as 403/404 page. But I'd say it should work there as well.

#41-43:
#42 is correct, "not 404" is equivalent to "200, 403"

If you need a more complex condition, use [Block Visibility Groups](https://www.drupal.org/project/block_visibility_groups).

HTH!

Used #36 on a site recently with a specific node targeted instead of just the status code and it worked well. Really saved the day too as we needed a specific block on the 404 page.

I'm using #36 to try and only show breadcrumbs on non-error pages.

I've checked the '200' box to only show the breadcrumbs on HTTP OK pages. Now my breadcrumbs are gone from all pages, not just from my 404 page.

I'm going to check out https://www.drupal.org/project/response_code_condition in the interim.

I haven't done a full review of the patch, but used it and hit an error. The condition is expecting that a request can be only interrupted by HttpExceptionInterface exceptions, but that's not the case. For example see Drupal\Core\Form\EnforcedResponseException.
Here is the fixed patch to handle this. It would be good to have a deeper look into whether a better handling of this is necessary.

Thanks a lot for the review. Included feedback from #48 and #49.

I'm still completely new to core tests. I don't even know how to run core tests. But I suppose we are going for core/modules/block/tests/src/Unit/Plugin/migrate/process/BlockVisibilityTest.php right?

Or core/modules/block/tests/src/Functional/BlockTest.php? Or both?

I can confirm #47.

$status = $this->requestStack->getCurrentRequest()->attributes->get('exception'); returns NULL on 200ers since there's no exception. We need to get the status code differently. I will have look into the mentioned module.

The module mentioned in #47 leverges the same code as we and says it only works for 4xx response codes.

The Response Code Condition module has the same problem. It currently only works with error codes, but not with 200. See ResponseCodeCondition.php#L103-105.

I'm wondering myself now if it's safe to assume that if there's no exception the status code must be 200. I reworked the last patch that way now and it seems to work just fine when testing it manually. But again, I'm unsure about the assumption.

/**
 * {@inheritdoc}
 */
public function evaluate() {
  $config = $this->configuration['status_codes'];
  if (empty($config) && !$this->isNegated()) {
    return TRUE;
  }
  $codes = array_combine($config, $config);
  if (!$this->requestStack->getCurrentRequest()->attributes->has('exception') && isset($codes['200'])) {
    return TRUE;
  }
  $exception = $this->requestStack->getCurrentRequest()->attributes->get('exception');
  return ($exception instanceof HttpExceptionInterface && isset($codes[$exception->getStatusCode()]));
}

I also created an associative array to work with. I think that's much faster than in_array().

> I'm wondering myself now if it's safe to assume that if there's no exception the status code must be 200.

I think not academically, but for our purposes, we can say that the absence of a HttpException means a 200 response.
We could have a redirect (301/303) response, but these don't show blocks.

> I also created an associative array to work with. I think that's much faster than in_array().

i did not look into the code, but the the impact usually is neglebible so i would not sacrifice readability for it.

I got some enlightening input under a related post on Drupal Answers from @4k4.

The method \Drupal::requestStack()->getCurrentRequest()->attributes->get('exception')->getStatusCode() actually is there to differentiate between a 403 or 404 error and serve the corresponding error page (from Drupal not from the server). The absence of that exception doesn't tell that the status code is 200. It just tells you are not on an error page.

This finally brought me to the conclusion that the patch I added to introduce a status code condition simply hasn't the right naming yet. This condition must not be there to differentiate between status codes. It must differentiate between being on of the two error pages (403, 404) or on a page with no errors. So this condition should be named: "ErrorPage.php", label: "Error Pages". The rest can probably stay the same. Just some renaming.

The options should be like @4k4 suggests:

* no-error pages
* 403 page
* 404 page

Link /admin/config/system/site-information and explain the visibility is for these configurable error pages and the options in this context would then be "403 page" "404 page" and "no error page"

Rerolled against 9.1.x and changed to ErrorPage.php, label: "Error Pages" per #58

Assigning myself for testing

Hi, I have applied the patch in #60 in Drupal instance 9.1.x and it worked!!
sharing screenshoots for reference.

Steps that I followed to test:
1. Applied patch
2. Go to base-url/admin/structure/block
3. Configure any block, you will find an extra column there "Error Pages"
4. Click on "Error pages", will display some error code options, choose any of them and save the block
5. Visit Access denied page, if you choose 403 option on error pages while configuring the block
6. you will that block on Access denied page.

Note : there is also a small checkbox on "Error pages" option while configuring the block that is "Negate the condition ", if you checked this enable, it will perform in opposite direction.

Shared screenshoots manner :

1. Block configuring page before applying patch
2. Block configuring page, after applying patch
3. Access denied page screenshot without configuring search block, you see search block there
4. Access denied page screenshot after configuring search block with negate condition, which means you will not see search block there

It is RTBC from my side.

Thanks jyotimishra123!

This still needs test coverage.

I think probably a usability review is also in order. You can get usability feedback in the #ux channel in Drupal Slack.

Thanks!

I posted the issue in the #ux channel on Slack.

We reviewed this last week during the weekly UX call and we agreed on suggesting to move it back to a separate tab with the title Error pages because it's easier to define separate conditions. Also, removing the 200 option (so 403 and 404 are indeed error pages) and it's easier to understand to anybody not used to that terminology and it's clear that "Pages" and "Error Pages" are different things.
Also, another suggestion is to add the English word for each error, like "Page Not Found (404)".

This patch addresses #72.

1. move it back to a separate tab with the title Error pages

   Each condition plugin already gets a separate tab. It's pictured in the second screenshot of #65. So there's nothing to do here.

2. removing the 200 option

   Done. Also removed some 200-specific logic from evaluate().

3. add the English word for each error

   Done.

I've been thinking about how extensible this is. Can a custom module add another status code?

The patch here adds a validation function, which only accepts status codes in the 4xx range.

With that constraint in place, a custom module can do something as simple as this:

/**
 * Implements hook_form_alter().
 */
function mymodule_form_alter(&$form, \Drupal\Core\Form\FormStateInterface $form_state, $form_id) {
  if ($form_id = 'block_form') {
    $form['visibility']['error_page']['status_codes']['#options'][451] = 'Unavailable for Legal Reasons (451)';
  }
}

Also, an updated screenshot of the block form.

The "negate the condition" checkbox hasn't been discussed yet. All of the visibility plugins get this from ConditionPluginBase but BlockForm hides it for most of them.

In this case though, the negate feature is VERY useful. A key use case of this condition will be to keep error pages clutter-free.

Should we change the checkbox to a pair of radio buttons, like the Pages (path) condition? I've left it alone, for now.

(Aside: what's the reason for hiding the negate checkbox on the role, language, and content type conditions? I went looking for the history of that, but couldn't find the issue.)

Coding standards fixes. The testbots complained about #75.

Regarding the "Negate" I think it's ok to show it because it gives the site builders more flexibility, also there are issues like this #2986958: Add support for negating user role condition for block visibility where other developers are claiming that removed functionality for other conditions.

We should keep the checkboxes because it is possible to add more error codes, then using checkboxes allows creating customized conditions.

For those that need it, I did a reroll for drupal 8.9 of the last patch in #77

Regarding the "negate" condition, I'd definitely want that available.

This makes it far easier do things like to use a node as a 403/404 page, while you also want to limit the page title block visibility based on content type, and based on negated /node/* paths. The 403/404 negate condition allows setting up multiple title blocks to do this. Without it, you wind up with two title blocks on 403/404, or no title block when the user views that actual node. This has been a thorn in my side for years.

#80, #77, #75 not working in 9.3.x.dev
#60 Patch working in drupal-9.1.x sharing screenshot ....

The patch in #77 works for me on Drupal 9.2.7.

I added the core Search form block to the content area, and it is displaying correctly when 403, 404, or both, are selected under "Error Pages".

One small nit: "Error Pages" should be "Error pages" (small 'p') as per Drupal conventions.

Rerolled the patch in #77, thanks!

Here's a first pass at test coverage. It's a Kernel test that has lots of test cases. Maybe we'll want to add BlockTest::testBlockVisibility() too, but this is a start at least.

However, in writing these tests, I discovered I was very confused about the expected behavior when the response code is 200. Should this condition always execute as TRUE if the response code is 200?

I'm also confused about the case where "negate" is checked but neither of the 4xx boxes is checked. If the response is a 403 then I would expect the condition to execute as TRUE. But if I uncheck the negate box and get a 403, then the condition will still be TRUE because it's unconfigured and essentially ignored. It's so weird to me that there would be a scenario where "negate" does not create the opposite result.

Can someone clarify these scenarios for me? It would be great if someone could evaluate each of the test cases in the new data provider.

I still think the UX needs some improvements. I think this was clearer back when there was a 200 checkbox and no negate checkbox. At the very least, there needs to be some clarification for what this condition does when the response is 200.

I always do that with data providers for some reason...

This one should run. It's going to fail though on the two cases where negating does not result in the opposite value when the condition is executed.

So now we have some test cases to work against!

The ones that failed in #86 are cases where checking "negate" does not change the result of the condition. Which is weird to me.

This new condition also needs schema. That can be added to system.schema.yml and will end up looking very much like condition.plugin.request_path which is near the bottom of that file.

Maybe this needs to be a bit more user-friendly, with a #states element that hides the negate checkbox unless a 403/404 condition is checked, and corresponding validation, ie "You have not selected anything to negate". Maybe that's a separate core issue for block visibility plugins?

That aside, to my mind, negate checked with no other conditions is pure boolean, saying something like NOT(IN([ ])), ie show the block on any response (when the response is not empty() if we want to be pedantic). Implement that as a fallback, but it's a bit silly, and hardly beginner-friendly. It shouldn't be a possible UI selection.

We are only providing 2 status codes as checkboxes, corresponding to what core config considers "error pages", so all status codes in the description for negate is misleading (503 doesn't even hit Drupal for example) and should say all error codes above or such. Let's not assume site builders are clueless about other HTML error codes.

The boolean logic isn't the problem. The labelling/descriptions is the problem.

For anyone moving on from using #55 previously you might run into the same issue I did where the status_code plug-in is missing.

My fix was to temporarily put the file back at core/modules/system/src/Plugin/Condition/StatusCode.php, find all blocks using this setting and resave them, then export the config and remove the file again.

I'm not sure if I'll need to take similar steps yet in my deployment, but it at least got me past the error locally.

Thanks, @tim.plunkett. I removed that isNegated check and now all the test cases pass.

I also added schema, which I pointed out as missing in #88. I went with string as the type. Even though they are numeric strings, they're not really numbers because we don't do math with them.

In addition to the updated patch, I have updated the Remaining Steps in the IS. To me it looks like there are at least two important things remaining:

1. Review the test cases and decide whether they match the expected behavior.
2. Review UX of the configuration form

Regarding #1, I'm still confused about what is supposed to happen on 200 pages and I'm not positive the tests accurately reflect the expected behavior.

Regarding #2, part of the reason I'm confused about #1 is that the configuration form is not clear enough (to me at least).

Another contrib module for this, https://www.drupal.org/project/http_client_error_status

This patch is more powerful, and seems to be through code review and working. Would be great to have in core!

Nice to see progress here.

Starting a review.
The issue summary is out of date. It should state that this patch is adding a new tab to the configure block screens, not just adding a setting. Also, the screenshot does not match the latest patch. This needs up-to-date before and after screenshots in the Issue summary. Adding tag.

This is changing the UI, adding Usability tag.

Questions were asked in #93 that need to be addressed.

I skimmed the code and this jumped out at me.

+++ b/core/tests/Drupal/KernelTests/Core/Plugin/Condition/ErrorPageTest.php
@@ -0,0 +1,172 @@
+      'Case 1' => [

It would be a lot easier to understand if the name explained what this test case is for? Or, maybe in the doc block explain the cases? As is it is hard to figure out if a test base is missing.

Updated issue summary.

I bump also the version to 10.1.x, because this is the version for new features.

The code is now more typehinted and the test data yielded with inline comments.

Updated after screenshot, based on 10.1.x code.

#93 works good on Drupal 9.4.8, so I guess it is RTBC

#102 applied successfully on 10.1.x-dev

#102 looks good

Mostly looks great, but wasn't sure about these two points:

1. +++ b/core/modules/system/src/Plugin/Condition/ErrorPage.php
   @@ -0,0 +1,131 @@
   +    // Status codes must be in the 4xx range.
   +    $valid_status_codes = array_filter(
   +      $form_state->getValue('status_codes'),
   +      function ($k) {
   +        return filter_var($k, FILTER_VALIDATE_INT, [
   +          'options' => [
   +            'min_range' => 400,
   +            'max_range' => 499,
   +          ],
   +        ]);
   +      }
   

   Why are we validating a range between 400 and 499 when the options are hard-coded to 404 and 403 - also checkboxes should enforce that these are the only possible values sent to the form anyway?

2. +++ b/core/modules/system/src/Plugin/Condition/ErrorPage.php
   @@ -0,0 +1,131 @@
   +    $codes = implode(', ', $result);
   +    if (!empty($this->configuration['negate'])) {
   +      return $this->formatPlural($count, 'Do not return true on the following status code: @codes', 'Do not return true on the following status codes: @codes', ['@codes' => $codes]);
   +    }
   +    return $this->formatPlural($count, 'Return true on the following status code: @codes', 'Return true on the following status codes: @codes', ['@codes' => $codes]);
   +  }
   

   Is 'return true' the language we use elsewhere? Can't we use something more colloquial/closer to what actually happens like 'show' or 'allow'?

Also this should have a change record.

I agree that validation is not needed for this condition, so I did the following changes.

• Added the constants to avoid repeating the same numbers in the class.
• Changed the summary to be more descriptive.
• Removed validation.

Interdiff tool didn't want to work for me today, so I couldn't provide interdiff now, sorry.

#108 works fine, thanks

Applied patch #102 successfully on drupal version 10.1.x-dev
Adding screenshot for the reference.
Thank You

Quick re-roll for 9.5.x

The last patch is not working as expected, for example, if you leave the error pages tab empty (don't check any options) and save the block, the block will not appears at all, the problem is because the array_filter function on submitConfigurationForm is removed in patch #75 and added to validateConfigurationForm function, then the validateConfigurationForm removed in patch #108, what causes empty options to be saved them in the configuration, and the evaluate function will not return true.

This patch solves the issue by re-adding array_filter on submitConfigurationForm.
This patch is for Drupal 9.5.x

This patch is for 10.1.x

Re rolling #112 for 10.1.x

Thank you @ameymudras

Verified and tested patch #114 on 10.1.x-dev. Patch applied successfully.

Test Steps:
1. Goto Structure > Block Layout
2. Configure any block
3. Check "Error Pages" tab exist with checkboxes for status code (403, 404) and Negate condition
4. Check by default, this is true for blocks
5. Place "User Login" block in content region and do not check any options under "Error Pages" tab
6. Visit "access denied" and "page not found" pages and check "User login" block renders
7. Now, edit "User Login" block > Select "Negate condition" checkbox under Error pages tab
8. Visit "access denied" and "page not found" pages and check "User login" block does not render
9. Now, edit "User Login" block > Select only "Access denied (403)" checkbox under Error pages tab
10. Visit "access denied" and "page not found" pages and check "User login" block renders only on "access denied" pages and not on "page not found" pages.

Test Results:
1. By default, Error pages with status code 403 and 404 are enabled for all blocks
2. When no checkbox option is selected under Error page tab for "User login" block > "User login" block appears on 403 and 404 pages
3. When only "Negate condition" checkbox option is selected under Error page tab for "User login" block > "User login" block does not appear on 403 and 404 pages
4. When only "Access denied (403)" checkbox option is selected under Error page tab for "user login" block > "User login" block appears only on 403 pages and not on 404 page.

Before Patch:

After Patch:

When no status code is selected > "User login" block renders on 403 and 404 pages by default:

When only 403 selected > "User login" block appears on 403 only and not on 404 page:

Moving to RTBC

Not strictly relevant to this issue but just a bit of history/archaeology - we used to hide blocks completely on 404 pages: #232037: block_list() renders all blocks, even on 404, then we showed them again in #423992: Remove show_blocks page variable. In both cases it was all or nothing, adding it to block visibility never came up as far as I know but possibly would have saved us some flip-flopping 10-15 years ago.

1. This looks really close we have some blocks UI work to do and we need to add some WebDrvierTestBase testing of the how the summary stuff works (see below).

2. +++ b/core/modules/system/config/schema/system.schema.yml
   @@ -350,6 +350,13 @@ condition.plugin.request_path:
   +    status_codes:
   +      type: sequence
   +      sequence:
   +        type: string
   

   Along with the change to just save the array keys we should also change this to

       status_codes:
         type: sequence
         sequence:
           type: integer
   

   Then we have nice config with no duplication.

3. +++ b/core/modules/system/src/Plugin/Condition/ErrorPage.php
   @@ -0,0 +1,122 @@
   +      '#description' => $this->t('Select status codes to enforce. If none are selected, all status codes will be allowed.'),
   

   The whole select none to display on all being the same as selecting both 403 and 404 is very very confusing from a user experience perspective. In order to make this as usable as it is for the other options here in the blocks UI (i.e. roles and content type) we need to fix blocks.js as per below BUT also we're adding a further complexity by adding a negate - in order to allow us to see this block should not appear on a page. I think we need to revisit how the javascript function checkboxesSummary() in block.js works if a negate option is present.

4. +++ b/core/modules/system/src/Plugin/Condition/ErrorPage.php
   @@ -0,0 +1,122 @@
   +    $this->configuration['status_codes'] = array_filter($form_state->getValue('status_codes'));
   

   $this->configuration['status_codes'] = array_keys(array_filter($form_state->getValue('status_codes')));
   Will make nicer config along with the change above to the schema.

5. We to make a change to blocks.js in order to display a summary as the summary added to the PHP code is not actually used in the block UI.
   

   diff --git a/core/modules/block/js/block.js b/core/modules/block/js/block.js
   index a642ce363e6..2d4195843c8 100644
   --- a/core/modules/block/js/block.js
   +++ b/core/modules/block/js/block.js
   @@ -46,7 +46,7 @@
          }
    
          $(
   -        '[data-drupal-selector="edit-visibility-node-type"], [data-drupal-selector="edit-visibility-entity-bundlenode"], [data-drupal-selector="edit-visibility-language"], [data-drupal-selector="edit-visibility-user-role"]',
   +        '[data-drupal-selector="edit-visibility-node-type"], [data-drupal-selector="edit-visibility-entity-bundlenode"], [data-drupal-selector="edit-visibility-language"], [data-drupal-selector="edit-visibility-user-role"], [data-drupal-selector="edit-visibility-error-page"]',
          ).drupalSetSummary(checkboxesSummary);
    
          $(
   

The blocks.js changes actually was present in patches around #108 but got removed (probably due to a mistake) in #111.

It's important we have a test that actually saves a block plugin with this configured because without that we have no test coverage of the configuration schema.

The whole select none to display on all being the same as selecting both 403 and 404 is very very confusing from a user experience perspective.

It's just silly here IMO. There are only 2 options. They are not dynamic like content types or roles so there will never be more, which is the only reason you'd need negation and select-none-means-all.

Why do we try to force a consistent selection UI for both a simple non dynamic set and a more complex dynamic one when it results in far worse UX in the simple case?

Two checkboxes (checked by default?), no negation, less confusing help text?

🎯👆🏻

#121 let's suppose we only have those two checkboxes and see what use cases are covered and how:

• display a block on all pages (error and non-error) - ???
• display a block only on all error pages - check both checkboxes
• display a block only on all non-error pages - uncheck both checkboxes
• display a block only on 404 or 403 - only check the appropriate checkbox
• display a block on all non-error pages AND 404 or 403 (bot not both) - ???

We could have a "200" checkbox too but we couldn't call this filter "Error pages". If you really want to remove the negate condition (which is quite common in a lot of other conditions), we could rename the entire condition set to "Response status" and provide "200 - Normal response", "404 - Page not found" and "403 - Forbidden access" choices.

I'm okay with the existing implementation but I'm no usability expert. I only hope this won't need another 8 years to get into Core.

My bad, I was neglecting those use cases.

display a block on all pages (error and non-error) - ???

So this one needs nothing selected = "no condition". It's the labelling and description that cause confusion by mixing "show", "enforce" and "allow" without good explanation. We could simplify and clarify the labelling and descriptions, using the "restrict" terminology that is already used in Pages, Roles and Content type vertical tab summaries (for Claro).

display a block on all non-error pages AND 404 or 403 (bot not both) - ???

And this one needs the negation, ie not 403 or not 404 for the above. I've seen non-technical users really struggle with this concept when implemented as a single checkbox. I think this should use a pair of radios explicitly stating what happens, the same as the Pages condition. The default should be "hide", which is not ambiguous with a default of nothing checked. Users have to explicity choose "show" and leave nothing checked to generate an ambiguous state, and that's when the checkbox description really needs to describe what will happen.

Finally, if the vertical tab label is "Error pages", I don't think we should be labelling the selection UI as "Status code".

So, something like:

Error pages
[ ] Access denied (403)
[ ] Page not found (404)
If no error pages are selected, no restrictions will be applied.

( ) Show for the selected error pages
(•) Hide for the selected error pages

If no error pages are selected, no restrictions will be applied.

Hmm. Unfortunately that isn't quite how block visibility conditions and negate work. Nothing selected = no restrictions are applied, but negate applies as if both are selected.

I keep thinking that this is not user friendly because status 200 results are entirely implicit. If someone does not have a basic understanding of how this condition works, they will have quite a hard time to figure out what to do.

Is suggest the following:

Response status (for both the vertical tab and this label)
[ ] Success (200)
[ ] Access denied (403)
[ ] Page not found (404)
Other response statuses are not caught by Drupal.

( ) Show for the selected response statuses
(•) Hide for the selected response statuses

This way, the default status as shown above is the current behavior of blocks (shown on every pages) and it's explicit for all use cases I listed in #123.

What do you think about it?

Unfortunately thats not how negation works. The problem is that selecting none and negating (hide) is the same as selecting all and negating. There are examples of this all over core and contrib, but it seldom produces a good UX.

So nothing selected and "hide for selected" hides for all. That's very confusing and very difficult to explain with non-dynamic labels and descriptions.

Maybe #states could be used here to hide the negation options until a selection is made, and we unset the negation if nothing is selected.

So hide for all becomes an explicit choice (check both and negate), with show for all remaining the default.

I should have time to try that today.

This patch:

• Changes the label to "Error pages" for consistency
• Changes the description to explain what happens if nothing is selected
• Makes the negation use "show" and "hide" radios instead of a checkbox
• Hides the negation radios if no error page is selected
• Resets the negation to "show" (on submit) if no error page is selected

The last point is inconsistent with how other conditions with negation behave when nothing is selected (ie poor UX). But it does help remove a major source of confusion.

It does not address the other points from #118. Setting to needs review for testing.

I think this may surface a bug in #states where deselecting one checkbox from a checked pair with an OR condition triggers a change when it shouldn't, but only after an initial load in that state. Not sure if there is an existing issue for that.

Addressing the use cases from #123:

display a block on all pages (error and non-error)
Default configuration, nothing checked

display a block only on all error pages
Check 404 and 403, plus show on selected error pages

display a block only on all non-error pages
Check 404 and 403, plus hide on selected error pages

display a block only on 404 or 403
Check 404 or 403, plus show on selected error pages

display a block on all non-error pages AND 404 or 403 (but not both)
Check 403 or 404 (but not both), plus hide on selected error pages

Back to needs work for the points from #118. I may have time to look at those over the next day.

I think this may surface a bug in #states where deselecting one checkbox from a checked pair with an OR condition triggers a change when it shouldn't, but only after an initial load in that state. Not sure if there is an existing issue for that.

This can be worked around by using "invisible" as the negate element state rather than "visible", with a simple AND condition for the checkbox states. Good. Nope.

@alexpott, from #118

I think we need to revisit how the javascript function checkboxesSummary() in block.js works if a negate option is present.

I think we can get away with checking whether a condition is set OR negated, and the summary saying either of:

• Not restricted
• Restricted for certain error pages

Which at least is a correct indication of whether restrictions are applied or not. Note it's restricted "for" not "to".

The only other example of this in standard profile for checkboxes is the Vocabulary condition, and that sidesteps the issue by not providing a summary at all. The Pages condition summary is wrong if its textarea is empty and it is negated, but a minor change as above would help. Roles and Content type conditions are not negatable.

Beyond that, I think fixing the poor UX of other negatable conditions is beyond the scope of this issue, but definitely warrants an issue of its own. We could get this committed and use the work done here as a possible way forward.

Made changes as per the #118.
Cannot address 118.3 .
Please review.

Forgot to upload patch.

My mistake forgot to upload patch.

Thanks! I will attempt to address #118 point 3 as I described above. Tomorrow.

We will also need a test as per #120.

I think #126 is a great idea because then we don't need to deal with negation at all. And it can work like the role / content selectors. We don't need to fix any JS or anything. Like the forma can be:

Response status (for both the vertical tab and this label)
[ ] Success (200)
[ ] Access denied (403)
[ ] Page not found (404)
Other response statuses are not caught by Drupal.

If none are selected it's not restricted. If 200 is selected then the blocks only appear on 200s... etc. This will cover all use-cases. If you don't want the block to appear on 403 then select 404 and 200.

OK, will revisit the patch shortly.

Note UX review earlier in the issue wanted the 200 removed, which is perhaps how we have ended up where we are...

This patch:

• Renames the plugin id to response_status
• Renames the plugin class to ResponseStatus.php
• Changes the labels to "Response status"
• Adds a Success (200) option
• Changes the description text to "Other response statuses are not used."
• Removes the negate option
• Cleans up the duplicated code that disables negation in BlockForm.php
• Removes the tests for negation
• Adds test coverage for all combinations of 200, 403, 404.

Do we need to run this past the UX team with our reasons for not following their advice?

Updated the IS and added a new after screenshot.

It's important we have a test that actually saves a block plugin with this configured because without that we have no test coverage of the configuration schema.

@alexpott: Where/how are existing block visibility condition schema tested via block save? Is it just BlockTest::testBlockVisibility() etc, and should we add a test there?

1. Thank's for pointing out the UX review. I think we need to discuss this with them because not including the 200 and adding the negate definitely led us to worse UX problems. Hopefully with the renaming to response status we've overcome the initial objections.

2. +++ b/core/modules/system/src/Plugin/Condition/ResponseStatus.php
   @@ -0,0 +1,138 @@
   +  /**
   +   * Success status code.
   +   */
   +  const SUCCESS = 200;
   +
   +  /**
   +   * Not Found error code.
   +   */
   +  const NOT_FOUND = 404;
   +
   +  /**
   +   * Access denied error code.
   +   */
   +  const ACCESS_DENIED = 403;
   ...
   +      self::SUCCESS => $this->t('Success (@error_code)', ['@error_code' => self::SUCCESS]),
   +      self::ACCESS_DENIED => $this->t('Access denied (@error_code)', ['@error_code' => self::ACCESS_DENIED]),
   +      self::NOT_FOUND => $this->t('Page not found (@error_code)', ['@error_code' => self::NOT_FOUND]),
   ...
   +    $status_codes = [self::SUCCESS, self::ACCESS_DENIED, self::NOT_FOUND];
   

   Let's use the constants from \Symfony\Component\HttpFoundation\Response eg. Response::HTTP_OK

   Then we don't need to define our own and we're using what Drupal uses under the hood.

3. +++ b/core/modules/system/src/Plugin/Condition/ResponseStatus.php
   @@ -0,0 +1,138 @@
   +    $config = $this->configuration['status_codes'];
   

   Let's call $config $allowed_codes (or something similar) instead. $config does not mean much.

4. +++ b/core/modules/system/src/Plugin/Condition/ResponseStatus.php
   @@ -0,0 +1,138 @@
   +    $codes = array_combine($config, $config);
   ...
   +      return ($exception instanceof HttpExceptionInterface && isset($codes[$exception->getStatusCode()]));
   ...
   +    return isset($codes[self::SUCCESS]);
   

   Why do this? With an array this size I think in_array(Response::HTTP_OK, $allowed_codes, TRUE) instead of isset() is fine.

5. Re testing - yes adding to testBlockVisibility would be great - also because we don't have access to the actual response it'd be great to have a functional test. The kernel test is making assumptions about when the exception request attribute is set that might not be true.

+++ b/core/modules/system/src/Plugin/Condition/ResponseStatus.php
@@ -0,0 +1,138 @@
+      self::SUCCESS => $this->t('Success (@error_code)', ['@error_code' => self::SUCCESS]),
+      self::ACCESS_DENIED => $this->t('Access denied (@error_code)', ['@error_code' => self::ACCESS_DENIED]),
+      self::NOT_FOUND => $this->t('Page not found (@error_code)', ['@error_code' => self::NOT_FOUND]),

@status_code - not @error_code

OK. I may not have any time for a few days, if anyone else wants to jump on it.

This patch addresses #143 points 2-4. The relevant Symfony constants are used in the plugin and tests, and all handling/comparison of those is changed to expect integers.

Setting to needs review for tests (passing locally). Assigning while I work on a test as per point 5.

This patch:

• Adds a 200 response condition to BlockTest::testBlockVisibility()
• Tests that the condition is still set after block save
• Tests the block is not displayed on a 403 response
• Tests the block is not displayed on a 404 response
• Adjusts the string translation variable name as per #144

Unassigning. Still to do:

• Draft a change record (?)
• Discuss with the UX team as per #143 point 1.

Tests the block is not displayed on a 403 response

This should test the block does appear on a 403, so we are testing a real exception response. Will revisit.