Prevent empty block_content info fields from causing php deprecation notices when placing blocks with no label.

What are the steps to reproduce? We have had a lot of these issues providing defensive patches but we are more interested in knowing why it occurs.

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.

#4 mentions needing steps to reproduce.

I'm experiencing the same issue, this is the error stack:

Deprecated function: strpos(): Passing null to parameter #1 ($haystack) of type string is deprecated in Drupal\Component\Transliteration\PhpTransliteration->transliterate() (line 135 of core/lib/Drupal/Component/Transliteration/PhpTransliteration.php).
Drupal\Component\Transliteration\PhpTransliteration->transliterate(NULL, 'x-default', '_') (Line: 254)
Drupal\Core\Block\BlockBase->getMachineNameSuggestion() (Line: 390)
Drupal\block\BlockForm->getUniqueMachineName(Object) (Line: 137)
Drupal\block\BlockForm->form(Array, Object) (Line: 106)
Drupal\Core\Entity\EntityForm->buildForm(Array, Object)
call_user_func_array(Array, Array) (Line: 534)
Drupal\Core\Form\FormBuilder->retrieveForm('block_form', Object) (Line: 281)
Drupal\Core\Form\FormBuilder->buildForm(Object, Object) (Line: 48)
Drupal\Core\Entity\EntityFormBuilder->getForm(Object) (Line: 27)
Drupal\block\Controller\BlockAddController->blockAddConfigureForm('block_content:9252e0db-83d7-421b-b410-60cfa0629557', 'olivero')
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 580)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 163)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 74)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 58)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object, 1, 1) (Line: 686)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

When creating custom block types, you have the choice to hide the block description field that comes along with the 'Block Content' block, i.e., the 'Admin Label', this is being the case since D8, if I'm not mistaken. There are some use cases when the Drupal builder decides to hide description from the form (though it's not the best decision as that field definition is the one used to identify the entity through the UI; it acts as the entity title). When you choose to hide it, then create an entity of that block, and finally try to add it to the block layout, the error will be triggered.

The reason why is due to the `BlockForm` trying to generate a machine name suggestion. On `BlockPluginTrait::getMachineNameSuggestion`, we make the assumption that the `admin_label` definition (meaning the 'Block Description') is set, although we can hide it from the form when creating the custom block type (but when you decide to keep it, it is indeed required).

I have updated the instructions on how to replicate it.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Thanks for the steps to reproduce.

As a bug will also need a test case showing the issue.

Updated patch with test.

I tried replicating the issue on 10.1.x following the steps in the issue summary but was not able to trigger the error.

What additional steps are there?

I've tested it with 10.1.x and the issue still persists. I have updated the steps in the issue summary to be more clear.

See smustgrave's comment.

I tried replicating the issue on 10.1.x following the steps in the issue summary but was not able to trigger the error.

What additional steps are there?

I managed to reproduce this on 10.1.x. To get the error you need to open "Place block" form.

+ * @return string|null
+ * The suggested machine name or NULL if not applicable.

I would try to avoid nullable types wherever it is possible. They will bite us later with extra conditions. If block description is not available we can return empty string or block plugin ID.
Something like this.

$admin_label = $definition['admin_label'] ?: $definition['id'];

Also the test is too indirect. The bug belongs to BlockPluginTrait. No need to instantiate transliteration service to get it. The test should just verify output of \Drupal\Core\Block\BlockPluginTrait::getMachineNameSuggestion(), including the case when admin label is not available.

The bug belongs to BlockPluginTrait.

Actually it does not. There are other places where the "admin label" is always expected.
https://git.drupalcode.org/project/drupal/-/blob/10.1.1/core/lib/Drupal/...

Note that description field in block content entity is mandatory. So hiding it produces invalid block entities.

Given that I think the proper fix should ensure that blocks are never have empty admin label.

I agree on your feedback and assessment! I had already noticed the info field (the value used to set the admin_label value in BlockContentBlock block plugin) in the BlockEntity entity is required, I just took another route because I didn't want to add a fallback value into an entity. But I agree on that admin_label field shouldn't be empty in BlockContentBlock plugins.

I think the best route would be to add a fallback option into the Drupal\block_content\Plugin\Derivative\BlockContent::getDerivativeDefinitions() function, where the admin_label is set. If the BlockContent entity label is null, fallback to a generated admin label. The value could be built from the BlockEntity values such as the id and the entity bundle label. This way, we don't have to modify the \Drupal\Core\Block\BlockPluginTrait::getMachineNameSuggestion() and we don't mind whether the block label is hidden from the block entity.

Re #26. Adding fallback to Drupal\block_content\Plugin\Derivative\BlockContent::getDerivativeDefinitions() looks like the right approach.

Patches addressing feedback from #25 and #26.
See patches below - I attached the wrong ones here.

I attached the wrong patches above (#28). My apologies. Re-submitting correct patches.

So, I've spent a few hours digging into this, and it is kind of interesting. I changed the title because this doesn't use strpos anymore, it triggers the same notice on str_contains now. Ultimately this patch would fix the issue listed here, and while I have some nits over the patch and tests which I'll get to, I'm not fully convinced this is the right approach. While this gets closer to the root issue than just handling nulls in transliteration or in the machine name suggestions, the bigger issue is that we use the baseFieldDefinitions of content entities to set the label to be required in the entity form, yet allow it to be configured (i.e. hidden on the form) and then treat it in code like it's actually required. To compare and contrast block_content entities with nodes and terms, all three set the form field for the label as required (node:title, term:name, block_content:info) and yet allow the form display to be configurable such that the field can be removed from the form. The difference between blocks and terms and nodes is that terms and nodes define their own storage_schema handlers that set the title and name fields in the database to be not nullable, and if you go through this process with either (hide the node title or term name on the entity form), you actually end up with a sql exception along the lines of `Drupal\Core\Entity\EntityStorageException: SQLSTATE[23000]: Integrity constraint violation: 1048 Column 'name' cannot be null: INSERT INTO "taxonomy_term_field_data"`. See #2358537: Remove 'not null' constraint from node title field/column.('Title' field present in node entity should not be mandatory.)

Ultimately it feels like display configurable base fields that are required should be restricted from being hidden in the form display UI. But then we need to go entity by entity and decide how required the labels actually are, and I feel like for block content entities, maybe not so much, and it seems like a fairly common use case to want to hide the info field in the block_content entity form. If we want to make that field not required, and we don't want to compute a dummy label at the content entity level, then computing a default at the plugin level seems reasonable. I'll note here that this does change the administrative experience when placing blocks, as blocks that would previously have had an empty label in the selection form will now have <block type> <id> as a label.

1. +++ b/core/modules/block_content/src/Plugin/Derivative/BlockContent.php
   @@ -49,7 +50,7 @@ public function getDerivativeDefinitions($base_plugin_definition) {
   -      $this->derivatives[$block_content->uuid()]['admin_label'] = $block_content->label();
   

   Went back and forth on this in my head. I'm not a huge fan of using $block_content->type directly here to get the bundle config entity, but we do it elsewhere in core, and more methodical ways of getting it (i.e. \Drupal::entityTypeManager()->getStorage('block_content_type')->load($block_content->bundle())->label() or $block_content->get($block_content->getEntityType()->getKey('bundle'))->referencedEntities[0]->label() seem overly cumbersome here. I also considered whether we actually needed the human readable bundle label, or if we could just use the bundle machine name, or even just falling back to the uuid, but this string is going to end up on the administrative interface in the list of options when you click 'Place Block', so this seems appropriate That said, I don't think we need a separate method here just to do a simple string concatenation that we only need to execute in one place. So, I think I would just go with

   $this->derivatives[$block_content->uuid()]['admin_label'] = $block_content->label()
     ?? $block_content->type->entity->label() . ' ' . $block_content->id();
   

   or

   $this->derivatives[$block_content->uuid()]['admin_label'] = $block_content->label()
     ?? sprintf('%s %s', $block_content->type->entity->label(), $block_content->id());
   

2. +++ b/core/modules/block_content/tests/src/Kernel/BlockContentDeriverTest.php
   @@ -62,4 +92,72 @@ public function testReusableBlocksOnlyAreDerived() {
   +    $this->blockContentStorage->expects($this->any())
   

   So, because we are creating actual block content entities and saving them here, we really don't need to mock the block content storage, we can use the real one. i.e. $definitions = (new DerivativeBlockContent(\Drupal::entityTypeManager()->getStorage('block_content')))->getDerivativeDefinitions($this->baseDefinition);
   Also, inside the foreach($block_content_entities as $entity) loop, we can do $plugin = \Drupal::service('plugin.manager.block')->createInstance('block_content:' . $entity->uuid()); and get the actual plugin. We could just check the derived definition there with $plugin->getPluginDefinition() and we can also directly call $plugin->getMachineNameSuggestion() and do some assertions on that which ultimately would trigger the original deprecation notice that started this issue in the first place, and would make for a better 'proof of bug and fix' test.

3. +++ b/core/modules/block_content/tests/src/Kernel/BlockContentDeriverTest.php
   @@ -62,4 +92,72 @@ public function testReusableBlocksOnlyAreDerived() {
   +  public function testAdminLabelFallbackFunction() {
   

   Really, this should probably be a unit test that passes in a mocked BlockContent object, or just get rid of it if we are going to put the concatenation inline instead of making a new method.

4. +++ b/core/modules/block_content/tests/src/Kernel/BlockContentDeriverTest.php
   @@ -62,4 +92,72 @@ public function testReusableBlocksOnlyAreDerived() {
   +  protected function getMockedBlockContentEntities() {
   

   nit: We aren't creating mocked block content entities here, we are creating and saving actual entities, so something like getTestBlockContentEntities() would be a better name.

@mikelutz thanks for the thorough review and feedback!
I agree with your assessment; that's one of the reasons I took the first approach - I think the bigger issue is that of being able to hide required fields, but because I noticed other entities allow that behavior, I decided to go on this approach.

As per your review: 1) yeah, I noticed doing it the more methodical way is too cumbersome, that's why I decided to do it that way. I actually did think whether to create a function or make the fallback code inline, I even thought of making it static in case this could be used on other places, but being honest I don't see it being used in other places, so I agree on making it inline. 2)3)4): I agree on these ones, thank you!

I've applied the recommended changes on the 3340159-block-content-deprecation-notices branch on the issue fork.

Can the MR be updated for 11.x please

Done! I created a new branch with the 11.x base following the recommended guidelines, but I can change the base of the original MR to 11.x if needed.

Thanks!

Verified the bug following the steps from the issue summary

Create or edit a custom block type
Go to Manage Form Display
Hide the 'Block Description' field
Go to `/block/add` and choose the custom block type you just edited/created
Fill the required fields and then press `Save`
Place the newly created block to layout (admin/structure/block/library/olivero)
You can also trigger it when adding an existing custom block entity through the Block Layout UI

Applying MR fixed the problem.

And seems a deep review was done in #30

I'm triaging RTBC issues.

I read the issue summary and the comments. I think the proposed resolution needs more detail to explain how this is being fixed. And after the review by @mikelutz in #30, which resulted in code changes, I don't see that anyone has down another code review. Did I miss it?

I looked at the MR, and left some comments. That is not a thorough code review. There are some things that need to change so setting to NW.

Also changing component.

I am using the 10.1 patch from #29 and it's working well. Thanks @robert-arias!

Thanks for the feedback, @quietone. I addressed your points, and added a more detailed proposed solution we came after some reviews.

Changing it to Needs Review.

Hiding old branches and patches, review to follow. We also need an IS update describing the latest solution and why this is happening.

In my opinion, this sounds like a bit of an edge case issue. Should we really handle this in core? There are plenty of contrib/custom solutions for automatically pre-filling an entity's label (https://www.drupal.org/project/auto_entitylabel comes to mind).

As @mikelutz states in #30 - this is more of a mismatch between DB schema and base field definitions (required flag) than handling an empty label in a plugin definition.

The only time I've needed an empty admin label for a block content entity is in layout_builder context, which doesn't run into this issue since inline blocks don't create derivatives.

Sure, this is the latest update:

If you do so, and create a Block Content entity and then place it into the block layout, a deprecated notice will be thrown. The error is this one:

Deprecated function: str_contains(): Passing null to parameter #1 ($haystack) of type string is deprecated in Drupal\Component\Transliteration\PhpTransliteration->transliterate() (line 135 of /app/web/core/lib/Drupal/Component/Transliteration/PhpTransliteration.php)

This is error is triggered due to the fact that in code, when creating a block, the Block Content Block definition tries to add into the Block Description the block content entity label (i.e., the Block Description field). That plugin definition will be later used to suggest the machine name for the Block Content Block, which will throw the deprecated notice due to the fact of that definition being empty, as the field was never filled because it's hidden.

The proposed resolution is to add a fallback value in the Block Content Block derivative discovery, adding into the admin_label definition said fallback value. This fallback value is composed by the Block Content type and the entity ID. This way we avoid the deprecation errors and also handling cases where the entity label is empty.

I will also add this update into the summary.

And for your assessment, I disagree. I don't think this is an edge case, mainly by the fact that cores allows to hide the field without complaining, plus the lack of storage schema handlers that would make the info field required in the database as well. I think the discussion on whether this mismatch should be addressed through making the info field non-nullable at the database level is a fair discussion, but it's out of scope for this ticket in my opinion; and a similar discussion has started in #2358537: Remove 'not null' constraint from node title field/column.('Title' field present in node entity should not be mandatory.).

Ultimately, I think cases where the block content info field is to be hidden, computing a default value at the plugin level seems reasonable.

Had a chat with @larowlan about this and we are happy to go ahead with the fallback in the form of block_content:123

I agree with @quietone that the deprecation testing is odd, we should be able to simply call the function that triggers the error. If it produces a deprecation error the test would fail.

I will work on these changes today.

Added a related issue with Shortcuts.

Pushed simplifications to the test and rebased against 11.x

Tossed up between the fallback, but I think it would be useful to keep the bundle's label (although what happens when that is empty?)

Back to NR

For the shortcut issue we ended up setting a default to the database and an update hook.

Not sure the correct approach but this fix does address the problem at hand. I think the label of bundle: ID is perfect. I was a little torn because most people might not know the block ID but if you're in the block layout I think it's a fair assumption you know what you're doing and can find out the ID.

LGTM

Committed b0cef94 and pushed to 10.2.x. Thanks!
Committed ed13e94 and pushed to 10.3.x. Thanks!
Committed 7cf9fb4 and pushed to 11.x. Thanks!

Backported to 10.2.x as a non-disruptive bug fix.