Ghent 🇧🇪🇪🇺
Account created on 26 December 2006, almost 19 years ago
  • Senior Principal Software Engineer — Drupal Acceleration Team at Acquia 
#

Merge Requests

More

Recent comments

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Did a first round of review, found a few concerns. 😇

Looking forward to 📌 Vet require-dev dependency justinrainbow/json-schema as a require dependency Active landing, which will be step #1 to simplify this MR!

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Added the dependency evaluation.

But basically: if it's good enough for Composer, surely it also is for us? 😅

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Discussed with @alexpott & @catch at DrupalCon.

I forgot the process, and e.g. 📌 Promote symfony/finder from dev-dependency to full dependency Fixed didn't document it either, so I think it's

composer require justinrainbow/json-schema "^5.2 || ^6.5.2" -W

So created an MR for that!

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Hi Wim, I tested multiple images on e90e348bfe94e21bd0fe798f63831188eac65903, but the issue still persists. The widget keeps the images while uploading multiple images, but it's only rendering the last uploaded image. Next time when opening the props on the component, only the last uploaded image is shown.

— @spfoos in Drupal Slack

I asked him to chime in with more detail, but reflecting his manual testing findings in the issue ahead of that 👍

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

#3: this subset of the issue has prior art: 🐛 DX & authoring experience: for SDC+code components, the example is treated as the default, may not be desirable Active . Let's descope that part from this issue 🙏

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

This came up again at DrupalCon Vienna — @pdureau talked to me about it, and raised it in 🐛 Compatibility of ComponentMetadataRequirementsChecker with SDC themes Active . He clarified his proposal at #3552069-3: Compatibility of ComponentMetadataRequirementsChecker with SDC themes :

Talked with Wim about "Required props must have examples".

This is related to 🐛 DX & authoring experience: for SDC+code components, the example is treated as the default, may not be desirable Active where, if my understanding is right, 2 definitions "default" where discussed:

  • as the value set by default before the component is rendered (so, in forms or in the builder when dropping a component)
  • as the value set by the component during rendering, when no value is provided

In my opinion:

  • the first definition belong to component definition (so JSON schema)
  • the second definition is covered in code (Twig's |default() filter, Javascript's nullish coalescing operator (??)

Doing that:

  • we don't misuse examples property
  • the "code" default value can depend of some UI logic inside the component template.
  • we allow the 2 default values to differ if it is expected by the design we are implementing (uncommon but already met use case)
  • at least with Twig, we can set a translatable "code" default value by |default("My taylor is rich"|t)

Discussed with @lauriii, and he said he'd provide guidance on how he wants this to work.

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

No more render crash:

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

The log entry in #9 showed that Evaluator was evaluating FieldObjectPropsExpressions for an optional prop in a way that it'd just get {key1: null, key2: null …}. That's neither meaningful nor helpful.

But the Evaluator did do its job correctly. The challenge is that SDC simply doesn't know how to handle that, which is also correct and reasonable. We could replace an optional prop that has an array of NULL values to just NULL, but that will trigger

Twig\Error\RuntimeError occurred during rendering of component d9904f4d-3b6d-43c2-8312-8360f26299d7 in Content template Article content items — Full content view (-): An exception has been thrown during the rendering of a template ("[canvas_test_sdc:image-optional-with-example-and-additional-prop/image] NULL value found, but an object is required.") in "canvas_test_sdc:image-optional-with-example-and-additional-prop" at line 1.

So instead, we need to make ::hydrateComponent() smart enough to cross-reference the resolved value against the expected prop shape, and simply omit it: https://git.drupalcode.org/project/canvas/-/merge_requests/242/diffs?com...

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

wim leers created an issue.

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

As of #8 and commit https://git.drupalcode.org/project/canvas/-/merge_requests/242/diffs?com..., the match appears in the UI.

But upon selecting it, this happens:

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

IMO it would make sense to provide it as an API there too.

That'd be a core issue. It's not Canvas' responsibility to do this on behalf of all entities in all of Drupal.

#27:

  1. 👍
  2. 👍, and thanks for creating that follow-up issue!
  3. Thanks!
  4. 🙏 Thanks!
  5. 🙏 Thanks!
  6. 🙏 Thanks!

I'll get on reviewing this 👍

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Thanks for creating this!

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

AFAICT we've got our first example of a slot-only ComponentSource: the Layout source, which will enable a Layout Builder-to-Canvas migration.

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Exciting start, @Berdir! 😄 Thanks so much 😊

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

#13++

It would also benefit Canvas, because https://git.drupalcode.org/project/canvas/-/blob/1.x/composer.json?ref_t... has this as a full dependency too, and in fact the SOLE non-core dependency:

    …
    "require": {
        "drupal/core": "^11.2",
        "justinrainbow/json-schema": "^6.3"
    },
    …
🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

🐛 Don't break other JSON Schema references Needs work is in, let's tackle this next!

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Whew! 👍

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

wim leers changed the visibility of the branch 3530351-x-allowed-schemes to hidden.

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Update for #4: but that didn't fix it. See #3551665-20: SDC with "image" + string prop: static image lost upon linking the string prop to a `DynamicPropSource` .

But woot, @bnjmnm the magical fixing fairy flew by and created an MR out of nowhere to fix it! ❤️🙏

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Nice to see this merged!

Now checking if this MR fixes that bug too! 🥳🤞

Update: it did not! I looked into this together with @spfoos at DrupalCon (… and I got pulled into other conversations.)

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Also, there are some weird stuff about JSON Schema management:

All of these are at best tangentially related to what this issue is about 😅 Still, responding to all:

  • Prop "XXX" is of type "object" without a $ref, which is not supported: the schema retrieved from $ref will be merged with the existing schema, so it is common to never have a type property alongside a $ref.

#3515074 will AFAICT implicitly fix it. See #3515074-12: Shape matching MUST work with the resolved equivalents of $refs .

  • Prop "XXX" is required, but does not have example value: this seems to be the role of the default property, no the examples property

🐛 DX & authoring experience: for SDC+code components, the example is treated as the default, may not be desirable Active

  • Component has no props schema: "Canvas always requires schema, even for theme components" which don't fit with the real world where many components have only slots.
  • ...

Please create a separate issue 🙏

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Per @pdureau at https://www.drupal.org/project/canvas/issues/3550169#comment-16305196:~:... ., this issue's MR should delete this bit:

      // A prop may not be of type "object" unless it has a $ref defined.
      if ($prop['type'][0] === 'object' && !isset($prop['$ref'])) {
        $messages[] = \sprintf('Prop "%s" is of type "object" without a $ref, which is not supported', $prop_name);
        continue;
      }
🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

wim leers made their first commit to this issue’s fork.

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

We need to sort this out before 1.0, because what @pdureau is arguing for would be a significant BC break.

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

@borisson_ pointed me to this issue — so, writing this, live from the Acquia booth 🤣🤓

  1. Fair point! I understand why you feel that for this particular super crucial piece of configuration it would be desirable to convert an exception triggered by a validation constraint violation to a deprecation error. However… does it really make sense to run a site with core.extension that is incorrect/nonsensical/lying? 😅 What if I have 50 modules listed and 49 of them don't actually exist on disk? Wouldn't you want to be warned while developing, to prevent it being deployed to production?

    Thanks to #2625212: Add ConfigSchemaChecker to development.services.yml , everybody who's developing Drupal sites using development.services.yml (many/most people), will already get exactly what you're advocating for: it uses LenientConfigSchemaChecker which warns about invalid config. 😊

    Note that this MR is not making core.extension:profile strict, only core.extension:module and core.extension:theme. And modules and themes should really exist on disk, which is all that ExtensionAvailableConstraintValidator is checking and its test coverage seems solid? 🤓

    (@borisson_ shared in-person the anecdote that in his Drupal dayjob, they NEVER uninstall modules and remove it from the codebase in a single deployment for clients. Because that'd make it impossible to run the uninstall hooks!)

    So: what scenario are you thinking of that could potentially break?

  2. The 6 points at the top of that issue you linked indeed cover it, and especially see #3405328: [meta] Make recipes safer to use in the real world by supporting config validation and rolling back a broken recipe which is linked from point 5. Perhaps also see https://wimleers.com/talk/config-validation-drupalcon-portland-2024
  3. Config validation never impacts config export. It does impact writes to config, so: saves/import/writes. But if we do the … softening I described in point 1, imports couldn't break either. OTOH … does it really make sense to allow invalid config imports or writes?
  4. See #1! :)
🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Dug into this with @spfoos at DrupalCon, turns out it's closely related to #3551665: #3551665-16: SDC with "image" + string prop: static image lost upon linking the string prop to a `DynamicPropSource` .

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Just reproduced & debugged 🐛 Multiple image props on SDC are not supported in Canvas Active with @spfoos, and turns out it's a different symptom and different set of steps to reproduce, but has the same root cause as #6: .

Now checking if this MR fixes that bug too! 🥳🤞

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Per discussion with @pdureau and @mherchel at DrupalCon Vienna.

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Thanks!

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

wim leers made their first commit to this issue’s fork.

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Jesse's investigating.

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Seems that the refreshing the browser as @effulgentsia wrote in #3 might be a red herring, because I can now reproduce it without that:

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

@lauriii uncovered what seems to be a UI bug that slipped through the cracks for several weeks: 🐛 Image lost when mapping a field Active .

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

The most closely related MRs are 📌 Follow-up for #3541037: Contextual panel flickers when linking prop to field Active (Oct 6) and 📌 Investigate why the XB UI seems to not apply client-side transforms Active (Oct 1). If I revert back to cabf3a01d53b64b98ec7e5829055b84421a2b7d3 (the commit before #3535024), I can still reproduce it. So this seems to be a bug that was not previously spotted in 🌱 [META] Content templates Active , not a recent regression.

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Looks like this is a front-end bug as @lauriii implied in (private Acquia) Slack.

Request #1

The first request to /canvas/api/v0/form/component-instance/content_template/node.article.full/1 contains this as the value for form_canvas_props in the request body:

{
  "source": {
    "image": {
      "sourceType": "static:field_item:entity_reference",
      "value": {
        "target_id": "1"
      },
      "expression": "ℹ︎entity_reference␟{src↝entity␜␜entity:media:image␝field_media_image␞␟src_with_alternate_widths,alt↝entity␜␜entity:media:image␝field_media_image␞␟alt,width↝entity␜␜entity:media:image␝field_media_image␞␟width,height↝entity␜␜entity:media:image␝field_media_image␞␟height}",
      "sourceTypeSettings": {
        "storage": {
          "target_type": "media"
        },
        "instance": {
          "handler": "default:media",
          "handler_settings": {
            "target_bundles": {
              "image": "image"
            }
          }
        }
      }
    },
    "heading": {
      "sourceType": "static:field_item:string",
      "expression": "ℹ︎string␟value"
    }
  },
  "resolved": {
    "image": {
      "src": "/sites/default/files/2025-10/Screenshot%202025-10-08%20at%204.34.29%20PM.png?alternateWidths=/sites/default/files/styles/canvas_parametrized_width--%7Bwidth%7D/public/2025-10/Screenshot%25202025-10-08%2520at%25204.34.29%2520PM.png.webp%3Fitok%3DdZtc8eX9",
      "alt": "asdf",
      "width": 2136,
      "height": 1004
    },
    "heading": "attempt #2"
  }
}

Request #2

The second request happens once a dynamic prop source is selected, and upon doing so, the client sends this form_canvas_props to the server:

{
  "source": {
    "heading": {
      "sourceType": "dynamic",
      "expression": "ℹ︎␜entity:node:article␝title␞␟value"
    },
    "image": {
      "required": false,
      "jsonSchema": {
        "title": "image",
        "type": "object",
        "required": [
          "src"
        ],
        "properties": {
          "src": {
            "title": "Image URL",
            "type": "string",
            "format": "uri-reference",
            "contentMediaType": "image/*",
            "x-allowed-schemes": [
              "http",
              "https"
            ]
          },
          "alt": {
            "title": "Alternative text",
            "type": "string"
          },
          "width": {
            "title": "Image width",
            "type": "integer"
          },
          "height": {
            "title": "Image height",
            "type": "integer"
          }
        }
      },
      "sourceType": "static:field_item:entity_reference",
      "expression": "ℹ︎entity_reference␟{src↝entity␜␜entity:media:image␝field_media_image␞␟src_with_alternate_widths,alt↝entity␜␜entity:media:image␝field_media_image␞␟alt,width↝entity␜␜entity:media:image␝field_media_image␞␟width,height↝entity␜␜entity:media:image␝field_media_image␞␟height}",
      "sourceTypeSettings": {
        "storage": {
          "target_type": "media"
        },
        "instance": {
          "handler": "default:media",
          "handler_settings": {
            "target_bundles": {
              "image": "image"
            }
          }
        }
      },
      "default_values": {
        "source": [],
        "resolved": {
          "src": "/modules/contrib/experience_builder/tests/modules/canvas_test_sdc/components/image-optional-with-example-and-additional-prop/gracie.jpg",
          "alt": "A good dog",
          "width": 601,
          "height": 402
        }
      }
    }
  },
  "resolved": {
    "heading": "yo"
  }
}

For the image SDC prop, it reverted back to literally what is provided for sdc.canvas_test_sdc.image-optional-with-example-and-additional-prop in /canvas/api/v0/config/component. It even includes data that never makes sense to be sent to the component instance form, but that is used purely by the UI/client to perform client-side validation:

<snip>
      "required": false,
      "jsonSchema": {
        "title": "image",
        "type": "object",
        "required": [
          "src"
        ],
        "properties": {
          "src": {
            "title": "Image URL",
            "type": "string",
            "format": "uri-reference",
            "contentMediaType": "image/*",
            "x-allowed-schemes": [
              "http",
              "https"
            ]
          },
          "alt": {
            "title": "Alternative text",
            "type": "string"
          },
          "width": {
            "title": "Image width",
            "type": "integer"
          },
          "height": {
            "title": "Image height",
            "type": "integer"
          }
        }
      },
<snip>
🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

This interaction sequence (using the tests/modules/canvas_test_sdc/components/image-optional-with-example-and-additional-prop SDC aka )

causes the client-side data model to effectively become corrupted, because it lost the source + resolved entries in model to go missing:

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

It still took me >10 attempts of various sequences to reproduce this. Just reproduced at last. Investigating.

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

I'd like to see some Functional Javascript tests, but that can wait pending changes to Canvas.

IDK what the process is y'all follow, but in Canvas, we tag it , until that follow-up exists. (And typically mark it instead of to avoid forgetting about it. But won't do that here, don't want to intervene how y'all run this 😅)

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

What's the exact request body that triggers this? Happy to adjust this in \Drupal\canvas\Entity\JavaScriptComponent::updateFromClientSide().

(Let's not make its config schema more loose. That would undermine its value in surfacing integrity problems, which is what is a very widespread problem in Drupal core. It's absolutely reasonable to expect the HTTP API to provide some intentional looseness though: I'm a big fan of https://en.wikipedia.org/wiki/Robustness_principle :))

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

After spending more hours than either of us really wanted pairing on this (and the blocking https://git.drupalcode.org/project/canvas/-/merge_requests/219 MR), we both think this is finally ready to go!

🌇

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

@penyaskito pointed out a weakness in my proposal: for content entities, my proposal assumes that all inputs are collapsed. Which is what 📌 Don't allow passing uncollapsed inputs if using default expression Active fixed and provided an update path for.

But then again, the XB → Canvas rename (~2 months after that) means that anybody out there realistically has been running with that all along.

But, it'd still be safer to bring back that update path. Even if untested. (Because we can't really test it anymore, since our reference is now not XB 0.7.x, but Canvas alpha1… unless we fake it of course.)

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Outline of how this update path can/should work:

  1. Update all Component config entities which have >=1 version with >=1 prop whose expression is either ℹ︎text_long␟processed or ℹ︎text␟processed. Why these 2 expressions? They're the ones specified/hardcoded in \Drupal\canvas\JsonSchemaInterpreter\JsonSchemaType::computeStorablePropShape(), which is what https://git.drupalcode.org/project/canvas/-/merge_requests/194 fixed.
  2. Crucially, while doing that, track: A) the IDs of the Component config entities that were updated, B) the updated Component versions, C) the new active (latest) version of each of those Components.
  3. Then, use that information to update:
  4. Test coverage

You'll see that many of these things were actually built for \Drupal\Tests\canvas\Kernel\Plugin\Canvas\ComponentSource\ComponentInputsEvolutionTest, which was part of 📌 Version component prop definitions for SDC and Code components Active .

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

https://git.drupalcode.org/project/canvas/-/merge_requests/219 landed, which means the update path for https://git.drupalcode.org/project/canvas/-/merge_requests/205 is in!

That leaves only the actual DX bits, which @f.mazeikis is working on 👍

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

The second case:

props:
  type: object
  properties:
    images:
      title: Images
      type: array
      items:
        $ref: json-schema-definitions://canvas.module/image
        type: object

does work on the server side, because tests/modules/canvas_test_sdc/components/image-gallery/image-gallery.component.yml u ses that exact pattern.

What doesn't work yet, is the widget side for it, for that we have 🐛 Editing support for type: array Active .

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Thanks, @hooroomoo in #97!

📌 Require content templates to have at least one dynamic property source Active landed!

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Ah, I found now in private chat that #24.1 was skipped because:

@tedbow: but the issue summary doesn’t actually say whether the goal is to expose this in the UI. it just mentions handcrafting templates
@effulgentsia: No UI for this issue.

Which surprises me, but I guess it does mean Drupal CMS is unblocked 🚀

That does mean this issue should definitely be open again though, for A) tests, B) docs, C) updating our shape matching logic.

I think A + B could largely be handled by @phenaproxima. I am probably the only one who can do C.

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Thanks for that test coverage! 😊

But … why was this merged?!?! It was clearly not ready. I spent a few mins hacking something together sprinkled with many @todos, @phenaproxima added minimal test coverage, and … that's it.

  1. Even
        // @todo if $content_entity_type_id has a `canonical` URL template, also offer `host-entity-url:absolute:canonical` as a choice. Something like:
        // $suggestions[] = (new HostEntityUrlPropSource())->asChoice();
    

    was merged as-is. 😅 Meaning no human can use it!

  2. And there's many more @todos beyond that.
  3. Doc updates are missing throughout.
  4. \Drupal\Tests\canvas\Kernel\Config\ContentTemplateValidationTest::setUp() should've been updated to use this, to prove it works end-to-end
  5. \Drupal\Tests\canvas\Kernel\Config\PatternValidationTest::testInvalidComponentTree() should've gotten a test case that tries to use it, to prove you can't use it in Pattern
  6. Same for PageRegion

This was rushed in. 👎

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

For #20, see these MR threads:

… which points to Support rendering a fallback if a child fails to render Active , which has been opened against core and would make this simpler.

The exception that is being thrown for the malformed-image SDC is a \Twig\Error\RuntimeError, with the message

An exception has been thrown during the rendering of a template (""src" is an invalid render array key. Value should be an array but got a string.") in "canvas_broken_sdcs:malformed-image" at line 2.

👆 That's just another exception that should be caught by RenderSafeComponentContainer, but somehow isn't.

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

I am baffled how @mglaman can run into this but not a single other person, in three months (which is when 📌 Populate data to drupalSettings to enable Dynamic Code Components Active introduced this).

It has to be something related to the dataDependencies in a code component on the site, but I'm not sure how to track that down.

Any chance you have code components that do NOT have dataDependencies set yet? See the update path in \Drupal\canvas\CanvasConfigUpdater::updateJavaScriptComponent(), which is called automatically by \Drupal\canvas\Entity\JavaScriptComponent::preSave().

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Thanks! And … nice! 🤩

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

I think we should gather a list of things we think that should NEVER be available to map.

I think a clear example is all revision information, because thats' basically internal information. Or perhaps "author-only" information, if you will.

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

I was wrong in #17 and #19. I should've called it a day before, but did not 🫣

That was a non-conditional breakpoint on the "Bubbling failed" assertion, which is how I fooled myself.

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Looks like Canvas' (formerly "Experience Builder") work-around does not work in all scenarios: 🐛 Error 500: Bubbling failed. Active .

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

New day, continuing this.

Grepping the issue that introduced the RenderSafeComponentContainer ( 📌 Improve server-side error handling Active ) for "bubbl" yields several matches. Seems to confirm my hunch.

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Elegant and eloquent conclusion! 😊👏

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Had a few more mins today, and this stack trace hints that #17 is onto something (I can't fully explain it yet), because if I go down one level in the stack trace:

… it seems to somehow be happening not for the crashing SDC, but for another component! 🤪🕵️

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

This issue basically about the one more way a component can crash, and #3548922: A component appearing in the list of components to instantiate that is broken during development should convey that will more explicitly convey the brokenness in the UI 👍

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

I think #6 is great and we should go with it. If/when anyone wants to design it differently than that, we can open a follow up for further refinement.

💯

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

I suspect the actual root cause might be the doubly-nested ::executeInRenderContext():

If we'd fix that, then the actual exception would correctly bubble, and we wouldn't need this work-around. I think. TBD. Will continue debugging later, now need to go pick up Jack from daycare.

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

wim leers changed the visibility of the branch 3546249-500-bubbling-failed to hidden.

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

As said on the MR, I think 🐛 Deleting or renaming SDC prop will break xb pages using old version of the component Active 's !196 should land first. We just had a productive discussion about it, and it should be able to land shortly 👍

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Per discussion with @effulgentsia, downgrading the urgency.

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

It would be generic as it would work with any external JS bundles provided.

Note that Canvas' code components are currently restricted to (p)react, but they won't remain that.

So… it has a similar ambition!

Except that I think you are looking to have those JS components live in code (and git), and not in config entities. Is that right?

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

In the meantime, people can manually edit and re-save their existing content that uses HTML props that are getting double escaped.

No, they can't.

They'd need to recreate the component instances.

Only once 📌 [later phase] When the field type for a PropShape changes, the Content Creator must be able to upgrade Postponed is done, what you wrote will be true. So … to be on the safe side, restoring the tag.

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

so it's a judgment call of which set of weak arguments is stronger.

That's a good assessment.

But perhaps Page Title is a relatively unique exception where we can just expand this issue to include that as well: i.e., dynamic or host-entity-url or page title? But then I wonder how many more things this will end up needing to expand to, and ultimately what's the benefit that's gained from figuring out and implementing that list of things to check for.

While factually correct, I think this too is a weak argument. Because: who goes and creates pages/nodes/articles/products/… on their website to then only list the title of each?

I think this is a bad idea, because it undermines what ContentTemplates are supposed to be about: presenting structured data using components.

Decision

But clearly it's 2 vs 1:

  • @lauriii: con
  • @effulgentsia: con
  • @wim leers: pro

So, I'll just give up. So: here's an MR that removes the @todos pointing here.

Future

In case we reconsider this in the future: I think conceptual integrity is very important. Many products lack it. I think this decision undermines Canvas' conceptual integrity.

Over a year ago, I argued that an MVP feature for the Content Templates UI should be to visualize which fields have not yet had their data mapped. This is what I drew ~1.5 year ago:

If we had something like that, I would not be as concerned. The Site Builder would be unescapably aware of which structured data exists on the content entity type+bundle, and which of the fields had at least some of its data mapped vs not.

In the absence of such a UI (which I believe is still critical to add later), this issue would guarantee that at least one bit of structured data would've been mapped.

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Merging per @jessebaker in chat:

The front end is almost a one liner. I’m happy for this to be merged without a FE code review but I’m more worried about the potential UX implications that are discussed on the MR and how we want to go about following that up

— and tagging this , to capture those concerns in a follow-up issue

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

I'd prefer to actually have this test SDC in our codebase, so I'll look into the failing PHPUnit tests.

Fixed!

Closed the other MR (~210) in favor of the one with the test SDC. Having that test SDC will be handy in any future work we do (to quickly be able to test this). And arguably for a potential future e2e test, if @jessebaker deems that wise.

Works beautifully:

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

The last crash was due to a bug in the test SDC's Twig.

@mherchel, next time please provide a minimum SDC that reproduces the bug 🙏😊

Managed to find the solution in the screencast you shared, at the 23 second mark: https://youtu.be/6GcFdxP0PnM?si=tNN7oNefoi_47cqo&t=23

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

I'd prefer to actually have this test SDC in our codebase, so I'll look into the failing PHPUnit tests.

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Thanks for confirming, @balintbrews! Merged in upstream, merging shortly…

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Right, because of the changes to composer.json.

Will bypass that and merge for you :)

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

Merge away as soon as this is green — https://git.drupalcode.org/project/canvas/-/commit/90fd4cba4c0faedc4d10d... unfortunately conflicted 😅

(Approved!)

🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺
🇧🇪Belgium wim leers Ghent 🇧🇪🇪🇺

A crucial piece of metadata was missing that would've prevented building this UX: tracking of which props are required 🐛 Deleting or renaming SDC prop will break xb pages using old version of the component Active 's first MR (!205) fixed that. 👍

That means this issue is now able to:

  • Compare requiredness of each prop
  • Compare field type, storage settings, instance settings, widget and default value of each prop
  • Compare the set of slot definitions of each prop (which slots exist, their order and the example values for each)

Which should be enough to build a first iteration

Production build 0.71.5 2024