ID attribute is missing from blocks rendered by BlockFieldFormatter

Looks good.
Thanks!

Hm, only saw this now with the release. The non-existence of the id was more or less by design, because unlike block config entities, there is no unique ID here. Are you sure that placing the same block plugin more than once on the page isn't going to result in duplicate HTML ID's?

I also get that modules need an ID, although I think that is a bug in those modules. There's also page manager, panels, layout builder and so on that likely also aren't adding ID's or not all of them.

And other modules more or less assume that if there is an ID, it is the ID of a block config entity, e.g. \block_class_preprocess_block(). So either that's pointless overhead of trying to load a non-existing config entity or worse, there actually is a block config entity with that name and it might apply classes or do something else based on that.

Reopening this to discuss this, I'll try to do my own tests when I don't get feedback.

@berdir, I agree, this is a little bit of a risky change. With blocks rendered by BlockViewBuilder, if there's an '#id' property set it maps to a block config in the system. With this change, the #id no longer ties back to a real entity. You could argue that modules should be checking both to see if '#id' is present and that the id exists in the system, but a lot of modules/themes aren't doing that second additional check. With this change you're getting theme suggestions that didn't exist before or worse, a wsod if a module/theme is depending on the #id property mapping back to a real entity.

If this is just about HTML ids, could we set that id instead of the '#id' property itself to maintain the old flow? i.e, replace

      $elements[$delta] = [
        '#theme' => 'block',
        '#attributes' => [],
        '#configuration' => $block_instance->getConfiguration(),
        '#plugin_id' => $block_instance->getPluginId(),
        '#base_plugin_id' => $base_id,
        '#derivative_plugin_id' => $block_instance->getDerivativeId(),
        '#id' => $block_instance->getMachineNameSuggestion(),
        '#pre_render' => [
          [$this, 'preRender'],
        ],
        '#block' => $block_instance,
      ];

with

      $elements[$delta] = [
        '#theme' => 'block',
        '#attributes' => [
          'id' => Html::getUniqueId('block-' . $block_instance->getMachineNameSuggestion()),
       ],
        '#configuration' => $block_instance->getConfiguration(),
        '#plugin_id' => $block_instance->getPluginId(),
        '#base_plugin_id' => $base_id,
        '#derivative_plugin_id' => $block_instance->getDerivativeId(),
        '#pre_render' => [
          [$this, 'preRender'],
        ],
        '#block' => $block_instance,
      ];

I'm not sure if #attributes works here, depends on what the relevant preprocess functions and the template itself is doing exactly I suppose. That said, we could possible do a #block_field_id if not.

Then again, not setting an #id might also not actually fix the problems this attempts to fix, depending on where and when those modules need an ID.

@Berdir #attributes is what I used before I discovered the feature in the development version so it worked in my environment at least.

A unique HTML identifier alone would be enough I think so the approach in #13 would be fine.

> Sorry Berdir - template_preprocess_block() already ensures to only add a unique ID, so the '#id' is just the base suggestion to use.

It's not as simple as that as a bug in one of our projects just showed.

the template might make it unique, but it doesn't change #id, it just copies it into the attributes. Any other preprocess function still sees the original ID. And because this uses the ID suggestion that the block UI also uses, if you do for example put a block of a menu into content that already exists as a block too, there is a high chance that the block ID does match that block and it will for example add classes from the actual block.

I remain convinced that block_field should _not_ add this ID. We're not the only system using blocks. If you use those things that aren't working with layout builder or page manager/panels then it won't work either.

If we want to add an ID to the markup then we could set that directly on #attributes or use our own #block_field_id as discussed above. It should IMHO also use a more technical ID based on the plugin ID and not the UI label. For now, I'm going to open a follow-up issue with a patch to revert this.

Yeah, I created #3223741: Revert adding #id to block render array and posted a revert patch as a quickfix for my project, but I'm open to patches that ensure an HTML ID is set somehow.

@james.williams feel free to join us over at #3223741: Revert adding #id to block render array.
@berdir should we carry something over from #13? I'm also unsure about possible side-effects setting the id attribute. (Let's discuss further in the follow-up issue)