Access metadata is not propagated properly

I have tested this patch together with https://www.drupal.org/project/block_field/issues/3114056
Fix and tests looks good!
Just 2 questions:
- Why we didn't include this fix also in these changes (https://www.drupal.org/project/block_field/issues/3114056) issue for make testing simpler.
- little question regarding this part:

+++ b/tests/src/Kernel/BlockFieldFormatterTest.php
@@ -0,0 +1,102 @@
+    // Use Classy theme for testing markup output.
+    \Drupal::service('theme_installer')->install(['classy']);
+    $this->config('system.theme')->set('default', 'classy')->save();
+    $this->installEntitySchema('entity_test');
+    // Grant the 'view test entity' permission.
+    $this->installConfig(['user']);
+    Role::load(RoleInterface::ANONYMOUS_ID)
+      ->grantPermission('view test entity')
+      ->save();
+
+    // The label formatter rendering generates links, so build the router.
+    $this->container->get('router.builder')->rebuild();

Are we really need this part of setUp()?

+++ b/tests/modules/block_field_access_test/src/Plugin/Block/BlockFieldAccessTestBlock.php
@@ -0,0 +1,36 @@
+namespace Drupal\block_field_access_test\Plugin\Block;

Maybe it is simpler to use the existing test module 'block_field_test' and implement the plugin in the existing one.

Changes looks good.
But first should be merged this issue: https://www.drupal.org/project/block_field/issues/3068281

1. +++ b/tests/src/Functional/BlockFieldTest.php
   @@ -29,6 +30,13 @@ class BlockFieldTest extends BrowserTestBase {
   +
   

   Nit: remove empty line.

2. +++ b/tests/src/Functional/BlockFieldTest.php
   @@ -70,6 +79,15 @@ class BlockFieldTest extends BrowserTestBase {
   +    $element = $this->getSession()->getPage()->find('css', $selector);
   

   $element is not used.

3. +++ b/tests/src/Functional/BlockFieldTest.php
   @@ -70,6 +79,15 @@ class BlockFieldTest extends BrowserTestBase {
   +    // Check that a referenced block that is no longer available is not visible.
   

   The comment could be improved, the block is still available, it's just unpublished.

4. +++ b/tests/src/Functional/BlockFieldTest.php
   @@ -114,6 +132,26 @@ class BlockFieldTest extends BrowserTestBase {
   +    // When the blocks is not available anonymous users can't see it.
   

   Again, let's talk about visibility/published and not availability.

5. +++ b/tests/src/Functional/BlockFieldTest.php
   @@ -114,6 +132,26 @@ class BlockFieldTest extends BrowserTestBase {
   +    // enabling it and checking if it is available for anonymous users.
   

   Same here.

6. +++ b/tests/src/Kernel/BlockFieldFormatterTest.php
   @@ -0,0 +1,112 @@
   +  protected $entityType = 'entity_test';
   

   I don't think we should use class properties for these. For example, in the test method you end up creating a local variable for $fieldName, which outlines how we don't need these. Let's just use the strings around.

7. +++ b/tests/modules/block_field_test/src/Plugin/Block/BlockFieldAccessTestBlock.php
   @@ -0,0 +1,65 @@
   +      '#markup' => '<div> Custom access tag. </div>>',
   

   Nit: double > at the end.

8. +++ b/tests/modules/block_field_test/src/Plugin/Block/BlockFieldAccessTestBlock.php
   @@ -0,0 +1,65 @@
   +    return AccessResult::allowedif($this->configuration['access'])->addCacheTags(['custom_tag']);
   

   AllowedIf (capital I)

9. +++ b/tests/src/Kernel/BlockFieldFormatterTest.php
   @@ -0,0 +1,112 @@
   +      $this->assertEqual($build['#cache']['tags'], ['custom_tag']);
   

   assertEquals()

10. +++ b/tests/src/Kernel/BlockFieldFormatterTest.php
    @@ -0,0 +1,112 @@
    +      $this->assertEqual($build['#cache']['tags'], ['custom_tag']);
    

    assertEquals()

11. +++ b/tests/src/Kernel/BlockFieldFormatterTest.php
    @@ -0,0 +1,112 @@
    +    }
    

    Nit: empty line after function closing bracket.

12. +++ b/tests/src/Kernel/BlockFieldFormatterTest.php
    @@ -0,0 +1,112 @@
    +
    

    Nit: remove empty line.

13. +++ b/tests/src/Kernel/BlockFieldFormatterTest.php
    @@ -0,0 +1,112 @@
    +      $renderer->renderRoot($build);
    

    To make sure that our block is working well, let's assert that the rendered markup contains or doesn't contain the label.
    In order to render the block properly, make sure to include the block module in the list of modules above.
    Also, add an extra message in the loops assertions to understand on which formatter they would be failing.

We could have used the block_content module to make this test, but I have noticed that the block label formatter would return only one cache tag (block_content:) while the other formatter would return also the list cache tag. We can keep our custom block for ease of test.

Since the tests cannot be green unless #3068281: BlockFieldLabelFormatter CacheableMetadata::createFromRenderArray points to incorrect array is merged first, add here a patch that includes the other one too, so we see the tests are green.

+++ b/tests/src/Kernel/BlockFieldFormatterTest.php
@@ -0,0 +1,102 @@
+  protected $fieldName = 'field_test';

Let's remove also this property, like the others.
There are also some double empty lines, let's clear those and for me we are good to go.

1. +++ b/tests/src/Kernel/BlockFieldFormatterTest.php
   @@ -0,0 +1,91 @@
   +   */
   

   We can use @inheritdoc here.

2. +++ b/tests/src/Kernel/BlockFieldFormatterTest.php
   @@ -0,0 +1,91 @@
   +  protected function setUp() {
   

   Missing @inheritdoc.

3. +++ b/tests/src/Kernel/BlockFieldFormatterTest.php
   @@ -0,0 +1,91 @@
   +   * Assert access tags are propagated.
   

   Let's use "Tests that block access cache metadata is propagated".
   Also the method name, let's use "testBlockAccessCacheMetadata" or similar.

4. +++ b/tests/src/Kernel/BlockFieldFormatterTest.php
   @@ -0,0 +1,91 @@
   +      // Assert block contents are properly rendered.
   

   We are asserting that they are not rendered here.

5. +++ b/tests/src/Functional/BlockFieldTest.php
   @@ -70,6 +78,14 @@ class BlockFieldTest extends BrowserTestBase {
   +
   

   Double empty line.

Good for me! Moving to RTBC.

This will need a an update after the Drupal 9 patch has been committed.

Patch re-rolled.

Patch #21 is wrong.

Attaching new one.

Looks good to me, great test coverage. However, it already needs another reroll due to the coding standard changes I think.

This also contained the fix for #3068281: BlockFieldLabelFormatter CacheableMetadata::createFromRenderArray points to incorrect array but I think without test coverage for that unlike the other issue, so I committed that one, another thing to check when rerolling.

Attached a new patch.

Latest uploaded re-roll is missing some of the original test coverage.

Missed the coding standards fixes.

Patch looks good to me!

Fixed in dev branch.