When building out an example side by side component for experience builder in #3446722: Introduce an example set of representative SDC components; transition from "component list" to "component tree" I encountered an error about missing definitions for slots I was trying to populate in an embedded component. My attempt at embedding the two_column component in the side by side throws the error:

Twig\Error\SyntaxError: An exception has been thrown during the compilation of a template ("We found an unexpected slot that is not declared: [column_one, column_two]. Declare them in "side_by_side.component.yml"."). in Twig\Environment->compileSource() (line 2 of experience_builder_example_components/components/compound/side_by_side/side_by_side.twig). 

<div{{ attributes.addClass("bg-"~background) }}>
   ........

  {% embed 'experience_builder_example_components:two_column' %}
    {% block column_one %}
      {% if media_position == "left" %}
        {{ media }}
      {% else %}
        {{ comp_content }}
      {% endif %}
    {% endblock %}

    {% block column_two %}
      {% if media_position == "left" %}
        {{ comp_content }}
      {% else %}
        {{ media }}
      {% endif %}
    {% endblock %}
  {% endembed %}
</div>

This is unexpected because the slots column_one and column_two are defined correctly for the two_column component which means they shouldn't need to be defined in the side_by_side component.

To reproduce this you can install the experience_builder_example_components submodule that is introduced in the MR for #3446722: Introduce an example set of representative SDC components; transition from "component list" to "component tree". The module automatically places the side_by_side component on the front page of the site which will error out due to this slots issue

CommentFileSizeAuthor
#22 3446933-nr-bot_4e5op5tf.txt90 bytesneeds-review-queue-bot
#21 3446933--21--embeds-in-slots.patch16.87 KBe0ipso
#17 3446933-nr-bot_pgfg7jx_.txt90 bytesneeds-review-queue-bot
#15 3446933--9--embeds-in-slots.patch14.12 KBe0ipso

Issue fork drupal-3446933

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

ctrlADel created an issue. See original summary.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

I am able to reproduce this.

We will also need to consider extend, no just embed.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Issue tags: +Experience Builder

e0ipso opened merge request !8234

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Status: Active » Needs review

Test only patch is failing with the expected error: https://git.drupalcode.org/issue/drupal-3446933/-/jobs/1730262#L45

Twig\Error\SyntaxError: An exception has been thrown during the compilation of a template ("We found an unexpected slot that is not declared: [the_slot]. Declare them in "my-banner.component.yml".").

The pipeline with the fix is green.

This is ready to review.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Status: Needs review » Reviewed & tested by the community

I can reproduce this issue with the Storybook module integration, the patch fixes it, it includes tests, and test-only run is failing.
Code looks good.

The only thing I could suggest is a test with two elements nested instead of one, to check that those are traversed from bottom to top, but that might actually be testing twig itself and not our code.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

#2 says we need to consider extend and not just embed but we only add test coverage for embed. Have we considered extend? Should we add a test for it?

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Status: Needs work » Reviewed & tested by the community

@alexpott while working on an example with extends I realized that the trick to forward block contents will not work on extended templates. This is not related to SDC, but a Twig limitation.

{# extends-example.twig #}
{% extends 'sdc_test:just-a-slot' %}
{% set slot_value %}
  {% block nested_slot %}{% endblock %}
{% endset %}

{% block the_slot %}Foo {{ slot_value }}{% endblock %}

In the example above there is no scope for extend-example.twig to grab the contents of the block nested_slot.

Since we are extending another template it does not make sense to introduce a new slot/block, you just use the blocks defined by the parent instead. Therefore I think now, that I was wrong in #2 to suggest that we need to support extends.

Back to RTBC based on the above.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Status: Reviewed & tested by the community » Needs review

Please ignore my comment in #8 🤦. I was conflating two different issues here.

To recap this issue proves that we can:

  1. Have embed within embeds.
  2. Have embed within extends.
  3. Have embed within embeds and forward blocks from the parent to the children.
e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented

If you are curious about my confusion in #8, I was trying to write a test for 4, before I wrote one for 2:

4. Have embed within extends and forward blocks from the parent to the children.

However, that is not valid Twig. And that is what a I was realizing in #8.

The commit above adds test coverage for extends, and we should be good now.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Parent issue: » #3450586: [META] Back-end Kanban issue tracker
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Can the MR be updated for 11.x vs 11.0.x please

Also can issue summary be updated using the standard template, wasn't entirely sure what the solution is without having to look at the code.

quietone’s picture

quietone commented
Version: 11.0.x-dev » 11.x-dev

Fixes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies. Also, 10.2 is in security mode now.

alemadlei’s picture

alemadlei
Primary language Spanish
commented

Merge requests works for me on D11.0.5

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
StatusFileSize
new 3446933--9--embeds-in-slots.patch14.12 KB

Adding a patch for the MR as of #9, so we can add it to composer patches.

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
Status: Needs work » Needs review
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3446933-nr-bot_pgfg7jx_.txt90 bytes

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 necessarily 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.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented

Rebased

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Status: Needs work » Reviewed & tested by the community

I rebased with cherry-pick + fixed the test components slots adding title. Trivial changes so I feel eligible to get this back to RTBC.

smustgrave’s picture

smustgrave commented
Related issues: +#3464719: Twig embed leads to recursion and InvalidComponentException

Closed https://www.drupal.org/project/drupal/issues/3464719 as a duplicate

e0ipso’s picture

e0ipso
Primary language Catalan
Location Can Picafort
commented
StatusFileSize
new 3446933--21--embeds-in-slots.patch16.87 KB

Uploaded a patch file for composer.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3446933-nr-bot_4e5op5tf.txt90 bytes

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 necessarily 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.

chesn0k made their first commit to this issue’s fork.

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

daniel.pernold’s picture

daniel.pernold commented

Patches or MRs do not apply on Drupal 10.6.10.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾 Seville 💃 Andalusia, UTC+2 🇪🇺
commented
Status: Needs work » Needs review

Rebased with main.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs issue summary update

Can we cleanup the IS some?