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

Draft change record: https://www.drupal.org/node/3335460 →

Have requested a UX review on Slack. Time zones being what they are it probably won't be a realtime conversation...

Sorry for not reading any of the back scroll – leaving feedback purely on what I see in the screenshot in the issue summary. I think something we should probably clarify is whether selecting an option will make the block visible or invisible on the selected pages. Also, it's currently unclear what is the behavior when none of the options are selected.

Thanks @Lauriii.

We did have more help text in there earlier. I've added something similar back in:

Shows the block on pages with any matching response status. If nothing is checked, the block is shown on all pages. Other response statuses are not used.

This patch differs only by that change. I've updated the issue summary screenshot.

I have to point out that no other core block condition plugins bother to explain what happens if nothing is selected/entered. Let alone what "negate" means in that context.

Adding a patch compatible with Drupal 9.5.x. Patch #157 fails for Drupal 9.5.4.

Is it worth it adding 503 to the mix here? (to allow visibility conditions for blocks when in maintenance mode)

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.

Removing credit for #159 as it's expected for a patch to be tested before uploaded

The last patch doesn't pass commit checks, could you make sure to run ./core/scripts/dev/commit-code-check.sh before uploading a patch to make sure there are no issues with code formatting. see https://www.drupal.org/docs/develop/development-tools/running-core-devel... →

Reviewing #157

ResponseStatus
1. Return functions should be typehinted

Functionality everything appears to be working.
Added a block to only appear in 404
Going to /blah I see the block
Going to regular page I do not

Same for 403.

Question this was previously tested for usability in #72 what additional feature needs to be tested?

Question this was previously tested for usability in #72 what additional feature needs to be tested?

That previous usability review asked us to provide only 403/404 checkboxes and add a "negate" checkbox. The plug in was labelled "error pages" at that time.

We tried that, and it caused confusion over what was being negated, especially when no error page was checked.

We decided it is clearer and cleaner to remove the negation, include a "200 success" checkbox, and label the plugin "response status" instead.

We assume we need an additional usability review due to those changes. Do we?

Is it worth it adding 503 to the mix here? (to allow visibility conditions for blocks when in maintenance mode)

@klonos: what would the checkbox label be? Are there any other circumstances where Drupal/Symfony returns 503? Maybe this could be a followup issue?

So then in that case it will need UX review again. Will post to see if they can add to their next call.

Question on the 503, definitely think it would be a follow up that’s postponed on this ticket. But if you get a 503 then I don’t think the Drupal render system is called

what would the checkbox label be? Are there any other circumstances where Drupal/Symfony returns 503? Maybe this could be a followup issue?

@ John Pitcairn I haven't checked whether there were other cases where 503 is returned - I just happened to notice that in D7 in drupal_deliver_html_page() there are checks for 404, 403, and also for 503, and in the case of 503 it was only calling things relevant to maintenance status (it is adding a "'503 Service unavailable'" HTTP "Status" header as well).

Anyway, I thought that it might be relevant here, in case people want to add blocks that are shown during maintenance mode only. That's why I brought it up.

Can certainly be a follow-up issue if people think that it is not relevant/appropriate here or that it unnecessarily broadens the initial scope of the issue.

I think this might be blocked by 🐛 [regression] Language switcher block throws exception when no route is matched Fixed

Actually $exception = \Drupal::requestStack()->getCurrentRequest()->attributes->get('exception'); works with BigPipe so that's good.

#169 works for me (thanks everyone)

I think this feature makes a lot of sense, however it did not make it into 10.1, so untagging from release highlights. Would be great to get into 10.2 and then tag for 10.2 release highlights though :)

Will need a reroll for 11.x

It's been posted to the #ux channel few times also. Believe it's on a radar.

#169 tests pass for 11.x.

Brought this up at DrupalCon Pittsburgh and the usability tag can be removed as the bulk was done by @ckrina in #72.

So testing this out by placing a block to appear on each status.
Verified the block appears specifically on those codes and not on valid pages.

Some good excitement for this feature!

Re-roll Patch for 10.0.9

The last submitted patch, 169: 2245767-block-response-status-169.patch, failed testing. View results →

Still looks good to me.

Committed/pushed to 11.x and tagging for 10.2.0 release highlights.

The re-roll in #180 looks unnecessary so I committed #169.

Did my best with issue credit, but a lot of people and comments on the issue so apologies for any omissions.

Seems like this broke HEAD of 11.x, because the new test \Drupal\KernelTests\Core\Plugin\Condition\ResponseStatusTest uses the sequences in its ::setUp() ($this->installSchema('system', ['sequences']);).

This was deprecated in 📌 Deprecate Drupal\Core\Database\Connection::nextId() and the {sequences} table and schema Fixed .

Whoops. Reverted for now.

According to the change record https://www.drupal.org/node/3349345 → ,

Installing the table sequences with the method KernelTestBase::installSchema() is deprecated. The following code is no longer necessary and should be removed:

$this->installSchema('system', ['sequences']);

I made that change and the test passes locally.

The interdiff compares my patch to the one in #169, since @catch mentioned (#185) that he was ignoring the patch in #180.

I have not looked at the rest of the patch.

Since this was previously reviewed and only issue was the sequence think its safe to RTBC again

Committed badd417 and pushed to 11.x. Thanks!

Automatically closed - issue fixed for 2 weeks with no activity.

will this not be pushed to drupal10 ?

@matoeil at https://git.drupalcode.org/project/drupal/-/commit/badd417 you can see this becomes available in Drupal 10.2 (click "Tags containing this commit").