Template suggestions for custom block view modes

Addressed feedback ready for review again.

From the thread the test should be updated. Should be verified before running.

@larowlan actually that is correct. I updated the variable but one is the block and the other the blockContent.

There are two MRs here, can we get clarification of which one is the correct one? I assume the one with the larger ID?

I will remove the other one.

larowlan → closed merge request !2646

Ah one is for 9.5, I closed that as this is a feature and 10.1+ only

Made the small tweak.

This looks RTBC in my book

#14 has not been addressed yet. I'm not sure if we should add new suggestions with the known bug because changing these can be quite painful.

A concrete recommendation for tackling this would be to implement the suggestions like this:

• 'block__block_content__view_mode__full'
• 'block__block_content__bundle__test_block'
• 'block__block_content__bundle__test_block__full'
• 'block__block_content__machine_name__machinename'
• 'block__block_content__machine_name__machinename__full'

Thoughts?

Ah sorry for missing that @lauriii - yes I agree avoiding the known collision now is the best approach

Going to take #61 as an FEFM review and remove the tag (please revert if not appropriate)

While I'm slightly off-topic for this particular thread, I just want to sing my praise for everyone's time and effort on this endeavor because otherwise I wouldn't have been able to complete the module I started to write over 1.5 years ago → and shelved in frustration, (while I otherwise waited for a Change to happen), after much time and effort myself. Thank you all from the bottom of my heart.

I'm using the patch from #34 but it looks like it's basically the same as the MR. I have a block type called mini-nav. Here are the theme suggestions I see:

<!-- FILE NAME SUGGESTIONS:
   * block--block-content--mini-nav--full.html.twig
   * block--block-content--mini-nav.html.twig
   * block--block-content--full.html.twig
   * block--block-content--2c9df2dd-821b-4147-9bd0-adf8ca05474d.html.twig
   * block--block-content.html.twig
   * block--block-content.html.twig
   x block.html.twig
-->

I think it's unexpected and undesirable that block--block-content--2c9df2dd-821b-4147-9bd0-adf8ca05474d.html.twig suggestion is not more powerful. Certainly the uuid is as specific as you can refer to something, right? I think a preferred list of suggestions would be:

<!-- FILE NAME SUGGESTIONS:
   * block--block-content--2c9df2dd-821b-4147-9bd0-adf8ca05474d--full.html.twig
   * block--block-content--2c9df2dd-821b-4147-9bd0-adf8ca05474d.html.twig
   * block--block-content--mini-nav--full.html.twig
   * block--block-content--mini-nav.html.twig
   * block--block-content--full.html.twig
   * block--block-content.html.twig
   x block.html.twig
-->

It seems like this needs to interact more gracefully with the existing theme suggestions provided by the block module. Maybe this should even be block_content_theme_suggestions_block_alter and do some splicing, along the lines of what happens in umami_theme_suggestions_block_alter:

function umami_theme_suggestions_block_alter(array &$suggestions, array $variables) {
  // Block suggestions for custom block bundles.
  if (isset($variables['elements']['content']['#block_content'])) {
    array_splice($suggestions, 1, 0, 'block__bundle__' . $variables['elements']['content']['#block_content']->bundle());
  }
}

It does the splice so that the new suggestion comes before the suggestion that features the uuid.

So looking at this to change the order think the code would have to be added to the block module now is that desired?
@laurii not sure I understand the conflict issue?

So we need to add an extra prefix to address #14 and I think #64 is valid too, the UUID specific one should trump any general suggestions

I think we need to pick one of the two to have a prefix for #14 - either view mode or bundle.

I think the bundle variation will be more common that the view mode, so suggest we prefix the view mode ones with `view_mode_$view_mode` and keep the bundle as is.

Switched to block alter and got

* block--sidebar--id--olivero-testthemes.html.twig
* block--sidebar.html.twig
* block--olivero-testthemes.html.twig
* block--block-content--90a1a6dd-3b88-45dc-9ed3-fe1aa04dcdd4.html.twig
* block--block-content--olivero-testthemes--full.html.twig
* block--block-content--olivero-testthemes.html.twig
* block--block-content--basic--full.html.twig
* block--block-content--basic.html.twig
* block--block-content--full.html.twig
* block--block-content.html.twig
x block.html.twig

Thoughts?

Also updated suggestion, that address #14?

Looks like we have a valid test fail

Also need test coverage but what do you think of the order?

Not sure about the ones with the theme name being more specific - or is that the block ID?

@larowlan:

Not sure about the ones with the theme name being more specific - or is that the block ID?

Yes, looking at the code olivero-testthemes must be the block ID. You can see that at block--sidebar--id--olivero-testthemes.html.twig

So we should come back to the order discussion. As I agree the example might be confusing, it would be best to write down the expected outcome into the issue summary and discuss that (if needed), based on the current example, but with more clear ID's and an explanation perhaps?

@smustgrave would you do that?

This should be ready for review. Updated issue summary too.

This looks good to me.

I like the extra specificity of the `_view_` and `_view_type_` so we don't get collisions, but it would be good for a FEFM to formerly approve that pattern, so tagging as such.

I'll ping @lauriii as he's already looked at this before

The issue summary explains clearly what is being done. Although, the remaining task says that there is a remaining task of 'Agree of suggestions'. Has that been done?

I then read the comments. I do see discussion of order, in particular #64. So, that seems to be done. I also see that @larowlan and @lauriii have been reviewing this.

I then read the MR and have commented there.

I read the CR and made a few changes to make the before/after clearer. The version numbers in the CR need to be updated. Also, the examples need to be checked and possibly updated. They are they same one when it was created in Aug 2022.

Just a few things, so setting back to NW.

To committers: I updated the credit, the ones unchecked need to be reviewed.

Brought this up in #needs-review-queue-initative and @lauriii said he already reviewed and can remove framework tag.

Have addressed the feedback so back to NR.

Everything looks to be good to go now here

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. 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.

I think it's safe to remark this myself. Only fix was https://git.drupalcode.org/project/drupal/-/merge_requests/3094/diffs?co... where machinename became machine_name.

Added some questions to the MR.

Feedback addressed.

Added a small nit on the MR otherwise this looks RTBC, all threads have been resolved and code is looking good.

I've updated the CR versions, but not 100% sure if they are accurate.

acbramley → changed the visibility of the branch 11.x to hidden.

+1

Thoroughly reviewed and tested; the proposed solution effectively resolves theme suggestions for custom block view modes. Endorsed for community acceptance.

While MR isn't merged, I'm attaching a patch based on the latest MR that can be applied to Drupal 10.2.x.

Hiding patches to reduce confusion.

FYI - you can download an MR diff locally and use that in your composer workflow instead of uploading to an issue. e.g https://git.drupalcode.org/project/drupal/-/merge_requests/3094.diff

@alt.dev
I try to apply patch but getting below message.

Skipped patch 'core/modules/block_content/block_content.module'.
Skipped patch 'core/modules/block_content/tests/src/Kernel/BlockTemplateSuggestionsTest.php'.
Skipped patch 'core/modules/block_content/tests/src/Kernel/BlockTemplateSuggestionsTest.php'.
Skipped patch 'core/modules/block_content/tests/src/Kernel/BlockTemplateSuggestionsTest.php'.
Skipped patch 'core/modules/block_content/tests/src/Kernel/BlockTemplateSuggestionsTest.php'.
Skipped patch 'core/modules/block_content/block_content.module'.
Skipped patch 'core/modules/block_content/block_content.module'.
Skipped patch 'core/modules/block_content/block_content.module'.
Skipped patch 'core/modules/block_content/tests/src/Kernel/BlockTemplateSuggestionsTest.php'.
Skipped patch 'core/modules/block_content/tests/src/Kernel/BlockTemplateSuggestionsTest.php'.
Skipped patch 'core/modules/block_content/tests/src/Kernel/BlockTemplateSuggestionsTest.php'.
Skipped patch 'core/modules/block_content/tests/src/Kernel/BlockTemplateSuggestionsTest.php'.
Skipped patch 'core/modules/block_content/block_content.module'.
Skipped patch 'core/modules/block_content/block_content.module'.

nod_ → closed merge request !3094

Updated CR with suggestion names and fancy emojis

Committed b050dbf and pushed to 11.x. Thanks!

Automatically closed - issue fixed for 2 weeks with no activity.