Create a block which can render entity fields

It would be a nice addition to be able to render a fixed set of items from the field, for instance if we have a multiple value field. Is this something that we can consider implementing as well?

@johndevman, we already have an issue for that specific request here #1234624: Limit the number of fields to display.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
   @@ -0,0 +1,379 @@
   +   * The entity type id.
   

   Nit: s/id/ID

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
   @@ -0,0 +1,379 @@
   +    // Get the entity type and field name from the plugin id.
   

   Same.

3. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
   @@ -0,0 +1,379 @@
   +    list (, $entity_type_id, $field_name) = explode(':', $plugin_id);
   

   Nit: If you move the parent::__construct() call to the top of the method, you can explode $this->getDerivativeId(). Might be a little clearer. Also, can self::DERIVATIVE_SEPARATOR be used as the string on which to split?

4. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
   @@ -0,0 +1,379 @@
   +    /** @var \Drupal\Core\Field\FieldItemListInterface $field */
   +    $field = $entity->{$this->fieldName};
   

   Nit: No need for the type hint if you use $entity->get($this->fieldName).

5. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
   @@ -0,0 +1,379 @@
   +    $build['field'] = $field->view($display_settings);
   

   Why does $build['field'] need to be a thing? Can't we simply have $build = $field->view($display_settings)?

6. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
   @@ -0,0 +1,379 @@
   +    // Set the cache data appropriately.
   +    $build['#cache']['contexts'] = $this->getCacheContexts();
   +    $build['#cache']['tags'] = $this->getCacheTags();
   +    $build['#cache']['max-age'] = $this->getCacheMaxAge();
   

   I could be wrong, but isn't there a static method on \Drupal\Core\Cache that does this?

7. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
   @@ -0,0 +1,379 @@
   +      // Check that the entity in question has this field.
   +      if ($entity instanceof FieldableEntityInterface && $entity->hasField($this->fieldName)) {
   

   Is there no way to have the context system take care of this? Seems like, if we limit the available contexts to bundles which carry the field, this check is not necessary.

8. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
   @@ -0,0 +1,379 @@
   +          // Build a renderable array for the field.
   +          $build = $entity->get($this->fieldName)->view($this->configuration['formatter']);
   +          // If there are renderable children, return the field's access.
   +          if (Element::children($build)) {
   +            return $field_access;
   +          }
   

   I don't understand the benefit conferred by this potentially expensive operation. Why render it out to determine access?

9. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
   @@ -0,0 +1,379 @@
   +        'label' => 'above',
   

   Isn't there a constant we can use here?

10. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
    @@ -0,0 +1,379 @@
    +  /**
    +   * Render API callback: builds the formatter settings elements.
    +   */
    +  public function formatterSettingsProcessCallback(array &$element, FormStateInterface $form_state, array &$complete_form) {
    

    Supernit: It's a Form API callback, not Render API.

11. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
    @@ -0,0 +1,379 @@
    +  /**
    +   * Render API callback: gets the layout settings elements.
    +   */
    +  public static function formatterSettingsAjaxCallback(array $form, FormStateInterface $form_state) {
    

    Also technically a Form API callback.

12. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
    @@ -0,0 +1,379 @@
    +  /**
    +   * Gets the field definition.
    +   *
    +   * @return \Drupal\Core\Field\FieldDefinitionInterface
    +   */
    +  protected function getFieldDefinition() {
    

    Missing @return documentation.

13. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
    @@ -0,0 +1,379 @@
    +      $bundle = reset($field_map[$this->entityTypeId][$this->fieldName]['bundles']);
    

    What if the first available bundle doesn't match the context entity's bundle? This seems like it could have weird, unintended consequences.

14. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
    @@ -0,0 +1,379 @@
    +  /**
    +   * Gets the field storage definition.
    +   *
    +   * @return \Drupal\Core\Field\FieldStorageDefinitionInterface
    +   */
    +  protected function getFieldStorageDefinition() {
    

    Missing @return documentation.

15. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
    @@ -0,0 +1,379 @@
    +    // prevent $fieldStorageDefinition being erroneously set to $this.
    

    O_O

+++ b/core/lib/Drupal/Core/Field/Plugin/Deriver/FieldBlockDeriver.php
@@ -0,0 +1,124 @@
+        // Limit available blocks by bundles to which the field is attached.
+        $context_definition->addConstraint('Bundle', array_keys($field_info['bundles']));
+        $derivative['context'] = [
+          'entity' => $context_definition,
+        ];

Shouldn't this part do that limitation #7 @phenaproxima?

@johndevman: I was wondering if we could implement a context constraint (or reuse one, if one already exists) like "entity has field" or something, rather than do that $entity->hasField() call. The code you pointed out will ensure that the entity has the correct bundle, but the thing I pointed out in #9.7 is checking for the presence of the field.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
   @@ -0,0 +1,379 @@
   +    list (, $entity_type_id, $field_name) = explode(':', $plugin_id);
   

   should we use the Plugin::DERIVATIVE_SEPARATOR constant here. Also, should we use the third argument to explode to ensure we get only 3?

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
   @@ -0,0 +1,379 @@
   +    $build['#cache']['contexts'] = $this->getCacheContexts();
   +    $build['#cache']['tags'] = $this->getCacheTags();
   +    $build['#cache']['max-age'] = $this->getCacheMaxAge();
   

   $this->applyTo($build)? or (new CacheableMetadata())->addCacheableDependecy($this)->applyTo($build)?

3. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
   @@ -0,0 +1,379 @@
   +    // @todo Remove this manual cast after https://www.drupal.org/node/2635236
   ...
   +      $bundle = reset($field_map[$this->entityTypeId][$this->fieldName]['bundles']);
   +      $field_definitions = $this->entityFieldManager->getFieldDefinitions($this->entityTypeId, $bundle);
   

   Can't we set the bundle in the definition in the deriver, then use that here? The issue is that the same field can have different labels depending on the bundle so it will appear with the wrong label for the user? Yes there will be more definitions, but the context should still filter out the surplus ones.

4. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
   @@ -0,0 +1,379 @@
   +   * @return \Drupal\Core\Field\FieldDefinitionInterface
   ...
   +   * @return \Drupal\Core\Field\FieldStorageDefinitionInterface
   

   These need a comment

5. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
   @@ -0,0 +1,379 @@
   +    $this->fieldStorageDefinition = NULL;
   

   Check for a __sleep method on FieldStorageDefinitionBase perhaps

6. +++ b/core/lib/Drupal/Core/Field/Plugin/Deriver/FieldBlockDeriver.php
   @@ -0,0 +1,124 @@
   +        $derivative_id = $entity_type_id . ":" . $field_name;
   

   same here re constant

It would be a nice addition to be able to render a fixed set of items from the field, for instance if we have a multiple value field. Is this something that we can consider implementing as well?

I think views supports that so perhaps we can borrow from there

#13
1) Yes!

2) CacheableMetadata::createFromObject($this)->applyTo($build);

3) Did not address yet

4) Fixed

5) Did not address yet

6) Fixed

#14
Did not address yet

Also made some improvements to the access checking, and added the necessary changes for config schema

Working on this more, but I think it will be blocked by #2671964: ContextHandler cannot validate constraints
Also bumping to major since it hard blocks the Layout Builder work

+++ b/core/config/schema/core.entity.schema.yml
@@ -362,3 +366,8 @@ field.formatter.settings.entity_reference_label:
+block.settings.field_block:*:

This should be block.settings.field_block:*:*: because of the extra use of : to split the entity type ID and field name

#13.3
I've set the bundles in the definition, but left the reset() approach for now.
Opened #2931585: Consider providing a human-readable label for field storage definitions to discuss the label issue

#13.5
I can't reproduce the need for this, removing for now.

#14
Opened #2931586: Add support for specifying deltas or ranges of field values when adding a FieldBlock to discuss further.

This patch also cleans up the form portion of the code.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
   @@ -0,0 +1,398 @@
   +  public function blockForm($form, FormStateInterface $form_state) {
   ...
   +  public function formatterSettingsProcessCallback(array &$element, FormStateInterface $form_state, array &$complete_form) {
   ...
   +  protected function thirdPartySettingsForm(FormatterInterface $plugin, FieldDefinitionInterface $field_definition, array $form, FormStateInterface $form_state) {
   ...
   +  public static function formatterSettingsAjaxCallback(array $form, FormStateInterface $form_state) {
   ...
   +  protected function getFormatter(array $element, FormStateInterface $form_state, array $complete_form) {
   

   I think we need test coverage for these forms, they're not just simple FAPI forms, there are some complex ajax behaviours, and (see below) I think there may be subtle bugs

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
   @@ -0,0 +1,398 @@
   +      'settings' => $config['formatter']['settings'],
   +      'third_party_settings' => $config['formatter']['third_party_settings'],
   

   Correct me if I'm wrong, but isn't it possible that these might refer to a previous formatter? i.e. configuration is set, but then user changes the formatter, at which point the old configuration is reused.

For #19

I've rewritten ::getFormatter() to not rely on PHP's bad merging via +=, which is how the previous code protected against #19.2
This should be more clear that it uses EITHER the input OR the defaults.

Additionally, had to add a workaround for #2671964: ContextHandler cannot validate constraints.

Fixed the schema error I introduced.

Is the previous kernel test still needed? Not sure if it makes sense to have both.

Good idea, we use a similar trick in Layout Builder for custom regions.

Removing the other test, discussed it further and it is redundant.

Specifically for custom render field view can accept array of settings instead of view mode, see https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...

+++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
@@ -285,8 +285,7 @@ protected function thirdPartySettingsForm(FormatterInterface $plugin, FieldDefin
-        // @todo How do we determine the view mode?
-        'default',
+        '__field_block',

@@ -384,7 +383,7 @@ protected function getFormatter(array $parents, FormStateInterface $form_state)
     return $this->formatterManager->getInstance([
       'configuration' => $configuration,
       'field_definition' => $this->getFieldDefinition(),
-      'view_mode' => 'default',
+      'view_mode' => '__field_block',
       'prepare' => TRUE,
     ]);

Internals of entity view allows view mode as string and as array, so not sure this kind of default is good enough

IMO better to have fallback to default formatter & settings

@andypost This hook is invoked in one other place only, and is always a string.

IMO better to have fallback to default formatter & settings

We do delegate to the default formatter and settings. This is for third party settings only.

I like the idea of having blocks for fields but I wonder from UX perspective the change to the "Add Block" form will be very jarring.

without the patch:

Without the patch:

The problems I see are:

1. All the blocks that were previously available are not pushed down pass the scroll
2. There are tons of new blocks that could be confusing such as "Revision translation affected", "UUID", "Initial email". I could see how they could be useful but since these are set as not display configurable which Field UI picks up on why would show them here? There will a bunch of new fields that user has never been exposed to through the UI.
3. This will expose a bunch of base fields from contrib entities that were not never intended to be displayed or configured. It is not a security problem because that will be handled by field access.

The Password field is also exposed here but if you try to place it the form doesn't submit.
I see this error in the logs:

Notice: Undefined index: default_formatter in Drupal\Core\Field\Plugin\Block\FieldBlock->defaultConfiguration() (line 196 of /var/www/d8_ux/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php) #0

Error: Call to a member function settingsForm() on null in Drupal\Core\Field\Plugin\Block\FieldBlock->formatterSettingsProcessCallback() (line 253 of /var/www/d8_ux/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php) #0 [internal function]: Drupal\Core\Field\Plugin\Block\FieldBlock->formatterSettingsProcessCallback(Array, Object(Drupal\Core\Form\FormState), Array) #1 /var/www/d8_ux/core/lib/Drupal/Core/Form/FormBuilder.php(993):

Settings to "Needs work" for the password problem.

Re #23 / #24, is there any reason not to use \Drupal\Core\Entity\EntityDisplayBase::CUSTOM_MODE? It's pretty much what you're looking for, a view mode ID that should never collide with one that exists in configuration.

Password and UUID were a problem, and were caught by the test coverage.
They have no formatters!

This removes those from ever being available

I went one step further and set up a no_ui flag similar to \Drupal\Core\Field\Annotation\FieldType::$no_ui
This is used by those fields that can be rendered, but should not be exposed in the Block UI.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Deriver/FieldBlockDeriver.php
   @@ -82,9 +95,20 @@ public function getDerivativeDefinitions($base_plugin_definition) {
   +        $options = $this->formatterManager->getOptions($field_storage_definition->getType());
   +        if (empty($options)) {
   +          continue;
   +        }
   

   I think we should also skip base fields that are

   > hidden by default (in their view display definition) *and*
   > not view configurable

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Deriver/FieldBlockDeriver.php
   @@ -105,10 +129,18 @@ public function getDerivativeDefinitions($base_plugin_definition) {
   +            $no_ui = FALSE;
   ...
   +        $derivative['no_ui'] = $no_ui;
   

   Good idea

#31 needs to be done still

I could be missing these (only reviewed interdifffs), but is the no_ui flag missing tests?

This is basically https://www.drupal.org/project/fieldblock in core AFAICT? If so, we should ask @marcvangend to review.

Cursory review.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
   @@ -0,0 +1,360 @@
   +class FieldBlock extends BlockBase implements ContextAwarePluginInterface, ContainerFactoryPluginInterface {
   ...
   +   * Constructs a new EntityField.
   

   Nit: mismatch.

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
   @@ -0,0 +1,360 @@
   +    $access = $entity->access('view', $account, TRUE);
   +    $access->addCacheableDependency($entity);
   

   I applaud you thinking about cacheability, but the entity access check's access result should already have taken care of this.

3. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
   @@ -0,0 +1,360 @@
   +        $access = $access->andIf($field->access('view', $account, TRUE));
   

   👌

4. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Field/FieldBlockTest.php
   @@ -0,0 +1,107 @@
   +    $this->drupalGet('/admin');
   ...
   +    $block_url = 'admin/structure/block/add/field_block%3Auser%3Achanged/classy';
   

   Nit: Let's be consistent with leading slashes or not.

5. +++ b/core/tests/Drupal/FunctionalJavascriptTests/Core/Field/FieldBlockTest.php
   @@ -0,0 +1,107 @@
   +    $pattern = '//tr[.//td/div[text()=:title] and .//td[text()=:category] and .//td//a[contains(@href, :href)]]';
   

   Woah! Might be the most complex XPath expression in core :)

#31, perfect!

#35
1) I disagree with the "hidden by default" part. If it's view configurable, it should be available.

2) This now has test coverage in FieldBlockTest.

#36, not really. That has a very different flow of exposing the blocks. This is a port of the CTools EntityField block

#37
1)
...which is why this mismatch happened :)

2)
So it does

4)
+1

5)
It may be, but it is a copy/paste from BlockUiTest so it will be the second instance :)

Due to the change hiding non-configurable fields, had to switch from `created` to a true field.

I agree. And putting it on the Annotation is a little too much of an API.
Went with 'block_ui_hidden'.

Patch looks good to me I have a couple of questions:

1. +++ b/core/config/schema/core.entity.schema.yml
   @@ -55,29 +55,7 @@ core.entity_view_display.*.*.*:
   -        type: mapping
   -        label: 'Field formatter'
   -        mapping:
   -          type:
   -            type: string
   -            label: 'Format type machine name'
   -          weight:
   -            type: integer
   -            label: 'Weight'
   -          region:
   -            type: string
   -            label: 'Region'
   -          label:
   -             type: string
   -             label: 'Label setting machine name'
   -          settings:
   -            type: field.formatter.settings.[%parent.type]
   -            label: 'Settings'
   -          third_party_settings:
   -             type: sequence
   -             label: 'Third party settings'
   -             sequence:
   -               type: field.formatter.third_party.[%key]
   +        type: field_formatter.entity_view_display
   

   Shouldn't we at least clear the cache in update hook for this change?

2. Are we planning to provide the upgrade path for \Drupal\ctools_block\Plugin\Block\EntityField in ctools?

And putting it on the Annotation is a little too much of an API.

If it's an @internal thing, then let's prefix with _ to discourage anybody from relying on it.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
   @@ -0,0 +1,360 @@
   +   * Render API callback: builds the formatter settings elements.
   ...
   +      // Store the array parents for our element so that we can use it retrieve
   +      // the formatter settings in our AJAX callback.
   

   Render API/AJAX callback, let's use the same terminology in both places.

All the feedback has been addressed so setting it to RTBC as @EclipseGc suggested in #41.

+++ b/core/modules/system/system.install
@@ -2056,3 +2056,10 @@ function system_update_8403() {
+function system_update_8500() {

This can be a post-update hook as well.

Just for the record upgrade path doesn't make sense because we are using update hook just to clear the cache.

Do we not need http://cgit.drupalcode.org/ctools/commit/?h=8.x-3.x&id=73a75efd630cef31f... anymore? I think we should credit all the contributors of \Drupal\ctools_block\Plugin\Block\EntityField here. Pasting the names here:

dsnopek
EclipseGc
phenaproxima
tim.plunkett
Xano
TravisCarden
Tim Bozeman
DamienMcKenna
neerajsingh
gaurav.kapoor
KarlShea
hctom
mroycroft

Sorry, found some more nits.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
   @@ -0,0 +1,360 @@
   +    $build = $this->getEntity()->get($this->fieldName)->view($display_settings);
   +    CacheableMetadata::createFromObject($this)->applyTo($build);
   ...
   +          $build = $field->view($this->configuration['formatter']);
   

   We should statically cache the $build IMO because building it twice doesn't make sense. We should also add cache metadata to blockAccess $build array.

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
   @@ -0,0 +1,360 @@
   +          if (!Element::children($build)) {
   

   Can we use isVisibleElement here instead of children?

I <3 this patch. I found a few nitpicks, but nothing serious. Although my review was relatively shallow :)

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
   @@ -0,0 +1,360 @@
   +    // Get the entity type and field name from the plugin id.
   

   Nit: 'id' should be 'ID'.

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
   @@ -0,0 +1,360 @@
   +        $access = $access->andIf(AccessResult::forbidden());
   

   Nit: You could simply return $access->andIf(AccessResult::forbidden()), then remove the else block and "flatten" the code a bit, for readability.

3. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
   @@ -0,0 +1,360 @@
   +          $build = $field->view($this->configuration['formatter']);
   

   This should probably use $this->getConfiguration() to ensure that defaults are merged in and whatnot.

4. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
   @@ -0,0 +1,360 @@
   +      '#options' => [
   +        'above' => $this->t('Above'),
   +        'inline' => $this->t('Inline'),
   +        'hidden' => '- ' . $this->t('Hidden') . ' -',
   +        'visually_hidden' => '- ' . $this->t('Visually Hidden') . ' -',
   +      ],
   

   \Drupal\field_ui\Form\EntityViewDisplayEditForm has a protected getFieldLabelOptions() method, which this is directly replicating. Any chance we can move that protected method into a public static method somewhere else in core and re-use it, rather than copy and paste? If not here, maybe in a follow-up?

1. --- a/core/config/schema/core.entity.schema.yml
   +++ b/core/config/schema/core.entity.schema.yml
   

   These changes seem out of scope? Why can't or shouldn't this live in a different issue/patch?

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
   @@ -0,0 +1,360 @@
   +  protected function blockAccess(AccountInterface $account) {
   +    $entity = $this->getEntity();
   +
   +    // First consult the entity.
   +    $access = $entity->access('view', $account, TRUE);
   +    if ($access->isAllowed()) {
   +      // Check that the entity in question has this field.
   +      if (!$entity instanceof FieldableEntityInterface || !$entity->hasField($this->fieldName)) {
   +        $access = $access->andIf(AccessResult::forbidden());
   +      }
   +      else {
   +        // Check field access.
   +        $field = $entity->get($this->fieldName);
   +        $access = $access->andIf($field->access('view', $account, TRUE));
   +        if ($access->isAllowed()) {
   +          // Build a renderable array for the field.
   +          $build = $field->view($this->configuration['formatter']);
   +          // If there are no renderable children, deny access.
   +          if (!Element::children($build)) {
   +            $access = $access->andIf(AccessResult::forbidden());
   +          }
   +        }
   +      }
   +    }
   +    return $access;
   +  }
   

   I think unit tests to assert correct behavior for both common cases and edge cases would be prudent.

   If this patches goes in as-is, there's a significant risk for security regressions in edge cases IMHO.

#48
Addressed

#50
1)
Statically caching this makes me nervous. I'm not sure it's necessary?
2) getVisibleChildren, but yes

#51
1) Fixed
2) Many returns vs one is a stylistic choice, but I think in this case you are right about the readability.
3) Switched to using ::build() directly
4) Opened #2933924: Move \Drupal\field_ui\Form\EntityViewDisplayEditForm::getFieldLabelOptions() to a trait

#52
1) The first 2 hunks could be a hard blocker for this issue, but the final hunk is specifically for this addition and requires the other part. I'm not sure it would be better to have this split?

2) Okay, working on that now.

Thanks, for addressing the feedback.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
   @@ -0,0 +1,366 @@
   +  public function build() {
   ...
   +    $build = $this->build();
   

   The problem is build method is called twice on a single page request. I think we can avoid that by just store it statically.

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
   @@ -0,0 +1,366 @@
   +    if (!Element::getVisibleChildren($build)) {
   

   What if hook_block_build_alter adds a visible child to the element?

hook_block_build_alter is block.module, this is all internal to the block system.
hook_block_build_alter wouldn't run when blocks are placed via another mechanism, like Panels or Layout Builder.

I fully intended to write a pure unit test, but there's way too much in TypedData to mock.

1. +++ b/core/tests/Drupal/KernelTests/Core/Field/FieldBlockTest.php
   @@ -0,0 +1,200 @@
   +  public function testBlockAccessEntityNotAllowed($expected, $entity_access) {
   

   Tests entity access.

2. +++ b/core/tests/Drupal/KernelTests/Core/Field/FieldBlockTest.php
   @@ -0,0 +1,200 @@
   +    $data['entity_forbidden'] = [
   

   FYI: these labels can contain spaces :) They're meant to be human-readable test case descriptions.

3. +++ b/core/tests/Drupal/KernelTests/Core/Field/FieldBlockTest.php
   @@ -0,0 +1,200 @@
   +  public function testBlockAccessEntityAllowedNotFieldable() {
   

   Tests unfieldable entity.

4. +++ b/core/tests/Drupal/KernelTests/Core/Field/FieldBlockTest.php
   @@ -0,0 +1,200 @@
   +  public function testBlockAccessEntityAllowedNoField() {
   

   Tests fieldable entity without a particular field.

5. +++ b/core/tests/Drupal/KernelTests/Core/Field/FieldBlockTest.php
   @@ -0,0 +1,200 @@
   +  public function testBlockAccessEntityAllowedFieldNotAllowed($expected, $field_access) {
   

   Tests field access.

6. +++ b/core/tests/Drupal/KernelTests/Core/Field/FieldBlockTest.php
   @@ -0,0 +1,200 @@
   +  public function testBlockAccessEntityAllowedFieldAllowed($expected, $build) {
   

   Tests populated vs empty build.

   BUT: this one has a name that doesn't match what it's testing.

Just that one nit at the very end, other than that, it looks like this is testing all cases. I didn't review this in detail though.

hook_block_build_alter wouldn't run when blocks are placed via another mechanism, like Panels or Layout Builder.

This assumption is not correct. Please see #2559533: Support hook_block_build_alter() and hook_block_view_alter() for panels. For layout builder in \Drupal\layout_builder\SectionComponent::toRenderArray we are already have '#theme' => 'block', which is defined in block.module so layout_builder already dependent on block hence it should call hook_block_build_alter while rendering the blocks.

1. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
   @@ -0,0 +1,379 @@
   +    // If there are no visible children, deny access.
   +    if (!Element::getVisibleChildren($build)) {
   +      return $access->andIf(AccessResult::forbidden());
   +    }
   

   Why would the output of the build affect access? What if a field used a lazy builder, would that be "not visible" at this point?

2. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
   @@ -0,0 +1,379 @@
   +      $bundle = reset($this->getPluginDefinition()['bundles']);
   

   Is there a case where users would want to use this block for a specific bundle that isn't the first bundle returned by reset here? Would different field definitions affect the form or build at all?

   Edit - This was already brought up in #9.13 and #13.3, but I'm not sure if it was addressed.

3. +++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
   @@ -0,0 +1,379 @@
   +      // Next check the raw user input.
   +      $configuration = NestedArray::getValue($form_state->getUserInput(), $parents);
   

   Why is using the raw user input required at this point? Some AJAX weirdness?

4. +++ b/core/lib/Drupal/Core/Field/Plugin/Deriver/FieldBlockDeriver.php
   @@ -0,0 +1,167 @@
   +        $derivative['_block_ui_hidden'] = $block_ui_hidden;
   

   Why not continue at this point and not add the derivative to $this->derivatives, if block_ui_hidden is set?

I also manually tested the patch by placing the "User picture" in a theme region and verifying that it renders correctly. Nice job!

One functionality note - I think that users may expect to be able to use this block to place any entity field in any theme region, only allowing it to be displayed if the appropriate context is available. For example, you may want to display a node's author information in the sidebar region of a theme, which is not possible with this patch as only user fields are available for placement at /admin/structure/block. Field Layout and Layout Builder address this use case, but it's important to note that without those modules the functionality is basically limited to user fields.

#60.1

Upon further testing, that check for build is an attempt to avoid #953034: [meta] Themes improperly check renderable arrays when determining visibility, which is a longstanding bug that affects all render arrays, not just this.
I don't think that this is our responsibility. Instead, I propose that we check to see if the field has value. In fact, this even matches with the renamed test methods @Wim Leers suggested (testBlockAccessEntityAllowedFieldHasValue)

This also means we can remove the static caching of the built block, which is a huge relief.

#60.2
As mentioned in #17, I opened #2931585: Consider providing a human-readable label for field storage definitions as a means for solving this.

#60.3
This is needed because of AJAX, yes. See also
\Drupal\image\Plugin\Field\FieldWidget\ImageWidget::validateRequiredFields()
\Drupal\menu_ui\MenuForm::submitOverviewForm()
\Drupal\file\Element\ManagedFile::valueCallback()

+++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
@@ -173,10 +160,8 @@ protected function blockAccess(AccountInterface $account) {
-    // Build a renderable array for the field.
-    $build = $this->build();
-    // If there are no visible children, deny access.
-    if (!Element::getVisibleChildren($build)) {

I think we should add a todo for #953034: [meta] Themes improperly check renderable arrays when determining visibility. Other than that it is RTBC.

Are you proposing that we stop checking for field values once that issue is in?
I'm not sure we'd want to remove that check.

No, we need the empty value check. I thought we'd need an empty rendered string check but that's always checked in the template(or not) so no we don't need @todo.

+++ b/core/lib/Drupal/Core/Field/Plugin/Block/FieldBlock.php
@@ -0,0 +1,364 @@
+class FieldBlock extends BlockBase implements ContextAwarePluginInterface, ContainerFactoryPluginInterface {

I think we should mark this as @internal until layout builder is beta.

Or move it to layout builder module.

Putting it straight into core signals its ok to use now, but we might need more iterations.

Putting it straight into core signals its ok to use now, but we might need more iterations.

We have been using it via ctools for a while now. I think this can be a public api now.

I think we should mark this as @internal until layout builder is beta.

Or move it to layout builder module.

Putting it straight into core signals its ok to use now, but we might need more iterations.

+1 for this. IMHO it should be in Layout Builder and marked internal. As long as the plugin ID remains stable, it's trivial to un-internal it and move it elsewhere into core.

I think this is overly cautious, but it's close enough to 8.5.0 that it's probably for the best.

Can we get an update to https://www.drupal.org/node/2924128?

Committed as b66af73 and pushed to 8.5.x.

Thanks

@tim.plunkett made the change record updates at https://www.drupal.org/node/2924128/revisions/view/10721865/10778707

Very nice to see this one fixed.

I second that woohoo!

I think we started on the CTools implementation during DC Barcelona - it's so cool to see this in core :-)

Thanks everyone!