Block visibility node type not working on preview/revision routes

The patch is correct, very nice find @ljcarnieri! Although I don't envy you. This would have taken me ~1day to debug....

In any case: we will need tests for this.

The same thing will happen if a route contains {node_revision}, but not {node}, so maybe it makes sense to include that as well.

I hit this issue when constructing a site where I'm using two "main content" blocks and conditionally hiding them in certain scenarios:

* one main content block for all node types in a full-width region to produce and style edge-to-edge content and
* another main content block inside a region that is constrained to a max-width of 960px, for all non-node pages like user pages, login page, etc.

Well, as per the bug here, the node preview were not triggering the node context thus rendering the node preview content inside the constrained region and completely throwing off the display. Needless to say, the patch in #1 elegantly solved this. I'm so glad I found this issue, because I, like tstoeckler, would have taken all day to figure out how to solve this.

I guess that adding the node_revision piece to the patch would be easy enough to do, but I wouldn't want to write that on pure speculation unless we prove that that is a real issue with a test. I'm not sure at all how to go about writing a test to cater to block plugins with context-based visibility settings, much less the node_revisions, so sheepishly have to say a "thanks" and "me too" here.

Many karma points and beers owed to ljcarnieri for tracking this one down.

The patch makes sense but the lines are getting very long. I think it would be more readable if we'd split it into two nested conditions, one for node and one for node_preview.

I don't *think* this is an issue for revisions because the route parameters for revisions do include 'node', it is only the node preview routes that do not have a node parameter.

From core/modules/node/node.routing.yml:

   31:  entity.node.preview:
   32:   path: '/node/preview/{node_preview}/{view_mode_id}'

   43:  entity.node.version_history:
   44:   path: '/node/{node}/revisions'

   54:  entity.node.revision:
   55:   path: '/node/{node}/revisions/{node_revision}/view'

   63:  node.revision_revert_confirm:
   64:   path: '/node/{node}/revisions/{node_revision}/revert'

Yes, it is an issue, because node is just the ID and doesn't get upcasted.

Adding a related issue > https://www.drupal.org/project/drupal/issues/2942054

The solution in #7 displayed some of my blocks on the preview page (before there were none), but not all of the blocks that appear on the page itself.

Hi , I am working on a project version-8.5.1 where we need multiple blocks on home page and blocks were not appearing on preview. So i searched and found this patch which is already done by ljcarnieri so i applied this patch and found some spacing error, then i decided to recreate this patch. Now this patch is working fine and all blocks is appearing on preview.

Hi , Now the patch is updated and its working fine.

#18 Review and tested.

Still need a test here, sorry

Hi larowlan, Can you mentioned that what's the test case where it is failed if you are sure.

For me, it's working. Needs someone to test it.

It needs an automated test written to cover the change,

Fixed wrong indentation in #18.

Working and tested.

As per #23 we need an automated test.

In order to commit a bug fix we need an automated to test to prove that we've fixed the bug and ensure that we don't break it again in the future. For more information about writing tests in Drupal 8 see the following links:

1. https://www.drupal.org/docs/8/testing
2. https://api.drupal.org/api/drupal/core%21core.api.php/group/testing/8.5.x

As per #15 this does show some but not all blocks for me.

Looking at the twig debug it seems drupal uses different templates when previewing.

When viewing the page normally we see this:

<!-- FILE NAME SUGGESTIONS:
   x page--homepage.html.twig
   * page--front.html.twig
   * page--node--1632.html.twig
   * page--node--%.html.twig
   * page--node.html.twig
   * page.html.twig
-->

But during preview mode we get this:

<!-- FILE NAME SUGGESTIONS:
   * page--node--preview--e52a41c5-97f4-4bc0-8f00-d474e329d55b--full.html.twig
   * page--node--preview--e52a41c5-97f4-4bc0-8f00-d474e329d55b.html.twig
   * page--node--preview.html.twig
   * page--node.html.twig
   x page.html.twig
-->

Due to this it seems the wrong template is shown. Does anyone have an idea of how to get around this?

That problem has nothing to do with block visibility.

What you are describing is by design, those page templates are path-based, and relying on that is in most cases actually a pretty bad idea, you shouldn't do that. There is an issue to remove those default suggestions but it didn't get into Drupal 8.

If your problem must be solved in the page template, implement your theme suggestions hook and consider the preview as well, or find a solution that doesn't require a different page template (which might actually be better for performance and cacheability as drupal will still do some work to prepare the blocks that you then might not display, e.g. in a special region.

#28 thanks for the informative response. I will take a look at your suggestions

I experienced a similar issue.

Have a page CT setup with a header image. The header image is rendered through a "ctools entity view" block with a "header" view mode for that CT.
This block would not show in preview mode.

After applying the patch from #24 the issue was fixed.

Using Drupal core 8.7.3.

I just want to point out that patches #24 and #1 are functionally the same. The only difference appears to be that #24 is formatted differently somehow to adjust two lines that have zero impact and no changes — namely, the $value = $node; line. However, the real logic changes are the same between the two patches:

Patch #1

+    if (($route_object = $this->routeMatch->getRouteObject()) && ($route_contexts = $route_object->getOption('parameters')) && (isset($route_contexts['node']) || isset($route_contexts['node_preview']))) {
+      if (($node = $this->routeMatch->getParameter('node')) || ($node = $this->routeMatch->getParameter('node_preview'))) {

Patch #24

+    if (($route_object = $this->routeMatch->getRouteObject()) && ($route_contexts = $route_object->getOption('parameters')) && (isset($route_contexts['node']) || isset($route_contexts['node_preview']))) {
+      if (($node = $this->routeMatch->getParameter('node')) || ($node = $this->routeMatch->getParameter('node_preview'))) {

Also, we still need automated tests here to get this into core.

1. Thanks for the tests.

2.

+++ b/core/modules/node/src/ContextProvider/NodeRouteContext.php
@@ -42,8 +42,11 @@ public function getRuntimeContexts(array $unqualified_context_ids) {
+    if (($route_object = $this->routeMatch->getRouteObject()) &&
+      ($route_contexts = $route_object->getOption('parameters')) &&
+      (isset($route_contexts['node']) || isset($route_contexts['node_preview']))) {
+      if (($node = $this->routeMatch->getParameter('node')) ||
+        ($node = $this->routeMatch->getParameter('node_preview'))) {
         $value = $node;

I think the standard is to put the closing parenthesis and opening bracket of multi-line if statements on a separate line below the last condition in the statement.

    if (
      ($route_object = $this->routeMatch->getRouteObject()) &&
      ($route_contexts = $route_object->getOption('parameters')) &&
      (isset($route_contexts['node']) || isset($route_contexts['node_preview']))
    ) {

3.

      if (
        ($node = $this->routeMatch->getParameter('node')) ||
        ($node = $this->routeMatch->getParameter('node_preview'))
      ) {

is the second nested if statement with the route match even required if we already have the first if statement that checks for those same route context variables?

Not able to apply #37 patch.
Getting error
Checking patch core/modules/node/src/ContextProvider/NodeRouteContext.php...
Checking patch core/modules/node/tests/src/Functional/PagePreviewTest.php...
error: while searching for:
'edit own page content',
'create page content',
'administer menu',
]);
$this->drupalLogin($web_user);

error: patch failed: core/modules/node/tests/src/Functional/PagePreviewTest.php:63
error: core/modules/node/tests/src/Functional/PagePreviewTest.php: patch does not apply

+++ b/core/modules/node/src/ContextProvider/NodeRouteContext.php
@@ -42,8 +42,11 @@ public function getRuntimeContexts(array $unqualified_context_ids) {
     $context_definition = EntityContextDefinition::create('node')->setRequired(FALSE);
     $value = NULL;
-    if (($route_object = $this->routeMatch->getRouteObject()) && ($route_contexts = $route_object->getOption('parameters')) && isset($route_contexts['node'])) {
-      if ($node = $this->routeMatch->getParameter('node')) {
+    if (($route_object = $this->routeMatch->getRouteObject()) &&
+      ($route_contexts = $route_object->getOption('parameters')) &&
+      (isset($route_contexts['node']) || isset($route_contexts['node_preview']))) {
+      if (($node = $this->routeMatch->getParameter('node')) ||
+        ($node = $this->routeMatch->getParameter('node_preview'))) {
         $value = $node;
       }
     }

It was hard to read before and more conditions makes it even more complex.

We already have the $value thing with multiple conditions so we can easily add this in a separate condition instead. I'd recommend to format it like this:

    if (($route_object = $this->routeMatch->getRouteObject())) {
      $route_contexts = $route_object->getOption('parameters');
      if (isset($route_contexts['node']) && $node = $this->routeMatch->getParameter('node')) {
        $value = $node;
      }
      elseif (isset($route_contexts['node_preview']) && $node = $this->routeMatch->getParameter('node_preview')) {
        $value = $node;
      }
    }

As far as I see, there's no need to put $route_contexts into a condition as well as we only check it with isset() anyway, and so we have two separate conditions for node and node_preview.

Thanks to @Berdir & @jwilson3 for reviewing the patch.

Yay tests. My suggestions breaks the existing test coverage for the node add form. We no longer go into the elseif for the routename check. We could just move that elseif below also inside the if because it doesn't really seem likely that there is a route name and no route object.

yay, 47 passed tests, however it looks like a reroll of that patch is required for 8.8.x
I was unable to patch 8.8.x with the 9.1.x patch from #47

9.0 and 8.9 yes, but likely only after it has been committed to 9.1. 8.8 only gets security updates and won't receive bugfixes anymore.

Also looks like the logic went back to the previous complicated if, not quite what I was suggesting. Don't want to hold this up just for my opinion, but I think the logic there is really complicated to read. And we have a related bug for revision pages, so that would make it even more complicated and we should IMHO try a pattern like I suggested.

here's a reroll for 8.8.x

Re-rolled for 8.9.x

the patch 51(52 oops) applies cleanly to 8.9.x and 9.0.x

fwiw(to avoid .orig files) an explicit reroll for 9.0.x

+++ b/core/modules/node/tests/src/Functional/PagePreviewTest.php
@@ -187,6 +190,18 @@ public function testPagePreview() {
+    $this->drupalPostForm("admin/structure/block/add/system_powered_by_block/$theme", $block_edit, t('Save block'));

BTW, the t() call is unnecessary here,

$this->drupalPostForm("admin/structure/block/add/system_powered_by_block/$theme", $block_edit, 'Save block');

Removed t() call from #53 as mentioned in #56, please review.

Solving failed test case, kindly review a patch.

Verified and tested by applying the patch #59.It looks good to me.Can be moved to RTBC.

Steps to test-
1. Go to admin site.
2. Go to admin/structure/block/block-content.
3. Create a custom block.
4. Go to admin/structure/block/list/bartik.
5. Go to Content type and click on Place block , add the created block.
6. Go to admin/content.
7. Add a Basic Page content type.
8. View the created Basic Page content type.
9. Go to node/1/edit.
10. Edit the created Basic Page content type.
11. Click on Preview
12. Verify

Content type view -

Before Patch Teaser -

Before Patch FullView -

After Patch Teaser -

After Patch FullView -

+++ b/core/modules/node/src/ContextProvider/NodeRouteContext.php
@@ -42,8 +42,14 @@ public function getRuntimeContexts(array $unqualified_context_ids) {
+    if (
+      ($route_object = $this->routeMatch->getRouteObject()) &&
+      ($route_contexts = $route_object->getOption('parameters')) &&
+      (isset($route_contexts['node']) || isset($route_contexts['node_preview']))
+    ) {
+      if (($node = $this->routeMatch->getParameter('node')) ||
+        ($node = $this->routeMatch->getParameter('node_preview'))
+      ) {

The first if has a line break after the opening paren. The second nest if does not. We should at least be consistent in the formatting.

Fixed Formatting as mentioned in #63.

1. +++ b/core/modules/node/tests/src/Functional/PagePreviewTest.php
   @@ -63,6 +65,7 @@ protected function setUp(): void {
          'create page content',
          'administer menu',
   +      'administer blocks'
        ]);
   

   should have a trailing comma.

2. +++ b/core/modules/node/tests/src/Functional/PagePreviewTest.php
   @@ -486,7 +504,7 @@ public function testSimultaneousPreview() {
    
        $edit = [$title_key => 'New page title'];
        $this->drupalPostForm('node/' . $node->id() . '/edit', $edit, t('Preview'));
   -    $this->assertText($edit[$title_key]);
   +    $this->assertRaw($edit[$title_key]);
    
   

   there's something weird going on with this, I was wondering why we have to make this seemingly unrelated change. comparing the verbose output of the preview page in this test with and without the patch, I can see that the page title vanishes with the patch.

   First I thought that there's something really weird going on, but it's quite simple. The test now enabled the block module, so the page title isn't displayed anymore unless we actually place the block. So add $this->drupalPlaceBlock('page_title'); in the setUp() and all the assertText() -> assertRaw() changes can be reverted, making this much simpler to review.

3. I think that my suggestion in #46 to do this would be much more readable and avoids those formatting problems. I tested it and it works fine.

New patch attached. No interdiff because I was messing around too much and was too lazy to re-apply my changes from the start, but I think it would be bigger than the patch now is anyway.

Ok, I think we can do even better with the test. Instead of adding block to the preview test, I think it makes more sense to test preview in the existing node block integration test that was failing before on the node add form.

The only thing we need to do there is grant the admin user permission to edit/preview the node and then access the preview page. Fewer side effects on existing tests, simpler patch.

Re-Rolled Berdir patch 66 for 8.8.8 , 8.9.x, 9.0.x

easier reroll this time

@joseph.olstad: Lets wait with the backports until this is RTBC, doing that so often makes this a bit confusing.

Per discussion in #2730631: Upcast node and node_revision parameters of node revision routes, the same problem exist on node revision pages. This patch also fixes that, and unlike the issue over there, it actually does that by supporting the node_revision parameter, so we provide the IMHO correct revision explicitly instead of falling back to the default revision of the given node.

Adding that was simple, adding test coverage for the node type condition as well, but making sure that we pass in the correct revision was a little bit more work, as we need a new block plugin for that that displays enough information for us to work with it. But implementing this check first and testing that should ensure that we don't introduce any regressions then on that other issue. This could be argued to be a different problem and out of scope, but fixing it together makes sense to me as it's basically the same place in the code and doing it separately would conflict on pretty much the whole fix.

Doing it before that other issue is fixed also means the code is a bit more ugly, but I decided against doing DI, because we can just remove that again in the other issue and just use getParameter('node_preview').

Also updating the issue summary and title to reflect the extra changes

This looks good to me. Let's add the new test only patch as well.

I didn't include a new test only patch because that would have failed the same way as before, before even getting to the new assertions :)

Thanks for the review.

1. +++ b/core/modules/node/src/ContextProvider/NodeRouteContext.php
   @@ -42,15 +42,25 @@ public function getRuntimeContexts(array $unqualified_context_ids) {
   +        $value = \Drupal::entityTypeManager()->getStorage('node')->loadRevision($revision_id);
   

   Any reason not to pass this dependency through the constructor?

2. +++ b/core/modules/node/src/ContextProvider/NodeRouteContext.php
   @@ -42,15 +42,25 @@ public function getRuntimeContexts(array $unqualified_context_ids) {
   +        $value = Node::create(['type' => $node_type->id()]);
   
   +++ b/core/modules/node/tests/src/Functional/NodeBlockFunctionalTest.php
   @@ -183,7 +191,33 @@ public function testRecentNodeBlock() {
   +    $this->assertSession()->pageTextContains('Displaying node #' . $node1->id() . ', revision #' . $node1_revision_1->getRevisionId() . ': ' . $node1_revision_1->label());
   

   I think it is more clear here to have the expected label/title static as in the other assertions.

Thank you all for the effort put into these.

Just a heads up though, anyone in my position, temporarily nailed to D8.9.1 for the moment, and wanting to apply the patch from #68, the D891-preview_block-2890758-67.patch would better be named D89x-preview_block-2890758-67.patch. The 8.9.x branch has moved on a little since 8.9.1 and the patch doesn't apply to 8.9.1 any more.

This does not alter the RTBC status. It's just a note for those applying the patch to get access now rather than waiting. :)

New ckeditor random fail, seen that once before as well and clearly not related:

1) Drupal\Tests\quickedit\FunctionalJavascript\QuickEditIntegrationTest::testCustomBlock
Field block_content/1/body/en/full did not match its expectation selector (.cke_editable_inline), actual HTML: The name "llama" was adopted by European settlers from native Peruvians.
Failed asserting that 'The name "llama" was adopted by European settlers from native Peruvians.' is true.

#72.1: Yes, the @todo above, it would introduce a dependency that the other issue would then remove again. Hopefully once this is in, the other issue becomes easier to resolve and we can remove that again and just use the upcasted node revision parameter.

Committed 1979928 and pushed to 9.1.x. Thanks!

Going to discuss with other committers before backporting. I want to think over and BC implications - especially around situations where you’d prefer the node from the route over what is specified but the node_revision param - I've been trying to work out if that is ever the case. I discussed this a bit with @Berdir who pointed out

so on HEAD, such an author block, or any block with node type visibility is not visible at all, this makes it visible. I hope that only has positive effects on sites :slightly_smiling_face:

this issue has 47 followers, the node revision one 100+, so I think it is fair to say that a lot of people would like it to work somehow, but it doesn't answer the default vs current revision

should be safe to jam this into 9.0.x also

thanks!

ReRoll for 8.8 adding fix for revisions. Copied from the D9 patch.

Re-roll for 8.9 adding fix for revisions, copied from 8.8.x patch from #80

@catch and I discussed this and agreed to leave as 9.1.x only since it could potentially be a surprise for someone.

Adding a release note snippet.

Adding the CR link to the release note.

Hi,

The same thing happening. if I restrict the block with Page restriction. For example, if I want to show block only in /node/11 page. I add Page restrictions with the only show to the page /node/11 the same block not showing in the node revisions page in the path /node/11/revisions/12006/view

That's expected, if you want to include revision pages then use /node/11* to have it on sub-pages as well.

Yeah, understood. Can't we apply the same kind of logic to the Path restrictions too?
If there is a solution like that, then I don't need to manually edit all those blocks in different pages.

I don't think that makes, that's not the same thing, you don't always want sub-paths included in something, that's why wildcard exists. If you have a lot of blocks like that you might want to consider handling that differently. Among other things, this can grow to be a serious performance issue. Drupal has to load *every* placed block and check its access and visibility rules, if you have a lot of blocks, that is a lot of work.

I'll just paste this here: https://www.drupal.org/project/block_visibility_groups