Stop hijacking the twig theme engine to render the edit form, move to a custom serialization format

Created on 12 August 2024, 4 months ago

Overview

Route experience_builder.component_props_form uses a form \Drupal\experience_builder\Form\ComponentPropsForm
This form mixes form API and React components via the semi-decoupled theme engine.
This theme engine:

  • Hijacks the default twig theme engine
  • Has hardcoded special treatment of theme suggestions that end in _xbxb
  • Uses json in twig to map Form API properties to JSX data-types

Proposed resolution

At its heart, the return from this form is a serialized representation of the form.
Here's a snippet of the return from the semi-decoupled theme engine:

<template hyperscriptify>
  <drupal-form--xbxb
    attributes="{&quot;class&quot;:[&quot;component-props-form&quot;],&quot;data-drupal-selector&quot;:&quot;component-props-form&quot;,&quot;action&quot;:&quot;form_action_p_pvdeGsVG5zNF_XLGPTvYSKCf43t8qZYSwcfZl2uzM&quot;,&quot;method&quot;:&quot;post&quot;,&quot;id&quot;:&quot;component-props-form&quot;,&quot;accept-charset&quot;:&quot;UTF-8&quot;}">
    <drupal-html-fragment slot="children">
      <div class="field--type-string field--name-heading field--widget-string-textfield js-form-wrapper form-wrapper"
           data-drupal-selector="edit-xb-component-props-static-static-card1ab-heading-wrapper"
           id="edit-xb-component-props-static-static-card1ab-heading-wrapper">
        <drupal-form-element--xbxb attributes="{}" label-display="&quot;before&quot;" title-display="&quot;before&quot;"
                                   type="&quot;textfield&quot;"
                                   name="&quot;xb_component_props[static-static-card1ab][heading][0][value]&quot;">
          <drupal-html-fragment slot="children">
            <drupal-input--xbxb
              attributes="{&quot;class&quot;:[&quot;js-text-full&quot;,&quot;text-full&quot;,&quot;form-text&quot;,&quot;form-element&quot;,&quot;form-element--type-text&quot;,&quot;form-element--api-textfield&quot;],&quot;data-drupal-selector&quot;:&quot;edit-xb-component-props-static-static-card1ab-heading-0-value&quot;,&quot;type&quot;:&quot;text&quot;,&quot;id&quot;:&quot;edit-xb-component-props-static-static-card1ab-heading-0-value&quot;,&quot;name&quot;:&quot;xb_component_props[static-static-card1ab][heading][0][value]&quot;,&quot;value&quot;:&quot;hello, world!&quot;,&quot;size&quot;:60,&quot;maxlength&quot;:255,&quot;placeholder&quot;:&quot;&quot;}"
              children="&quot;&quot;" render-children="&quot;&quot;"></drupal-input--xbxb>
          </drupal-html-fragment>
          <drupal-html-fragment slot="label"><label for="edit-xb-component-props-static-static-card1ab-heading-0-value"
                                                    class="form-item__label">heading</label></drupal-html-fragment>
          <drupal-html-fragment slot="renderChildren">
            <drupal-input--xbxb
              attributes="{&quot;class&quot;:[&quot;js-text-full&quot;,&quot;text-full&quot;,&quot;form-text&quot;,&quot;form-element&quot;,&quot;form-element--type-text&quot;,&quot;form-element--api-textfield&quot;],&quot;data-drupal-selector&quot;:&quot;edit-xb-component-props-static-static-card1ab-heading-0-value&quot;,&quot;type&quot;:&quot;text&quot;,&quot;id&quot;:&quot;edit-xb-component-props-static-static-card1ab-heading-0-value&quot;,&quot;name&quot;:&quot;xb_component_props[static-static-card1ab][heading][0][value]&quot;,&quot;value&quot;:&quot;hello, world!&quot;,&quot;size&quot;:60,&quot;maxlength&quot;:255,&quot;placeholder&quot;:&quot;&quot;}"
              children="&quot;&quot;" render-children="&quot;&quot;"></drupal-input--xbxb>
          </drupal-html-fragment>
        </drupal-form-element--xbxb>

      </div>

But we already have a way to serialize things, and that's the serialization module.

I propose we should:

  • Finish πŸ› Component props edit form route should use a custom wrapper format Closed: duplicate so that we have a unique wrapper format we can target to trigger the serialization
  • Add custom serializers to support converting form API's data structure to a new serialization format. We could name this format something like hyperscript_template or xb_template (the name doesn't really matter).
  • These serializers can make use of the renderer for aspects they do no know about (things we currently don't handle with _xbxb theme suggestions) but can correctly map to the JSON contents of the twig templates currently used by the semi decoupled theme engine.

User interface changes

πŸ“Œ Task
Status

Active

Component

Data model

Created by

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @larowlan
  • Assigned to effulgentsia
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    So rather than me theorizing, I'd rather have somebody with more distance to the XB code base, but deep insight into the https://www.drupal.org/project/jsx β†’ codebase, to respond here. That person would be @effulgentsia.

    P.S.: the term "hijacking" has a very negative connotation. I don't think that is necessary.

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    I realise it's a strong word, I'm trying to convey that the current approach is not suitable for production sites

    I've toned it down

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
  • πŸ‡ΊπŸ‡ΈUnited States effulgentsia

    I agree that experience_builder_system_info_alter() is too heavy handed in terms of overriding all themes to use the semi_coupled engine, and that this will need refactoring prior to XB being suitable for production sites.

    we already have a way to serialize things, and that's the serialization module

    Do we have any precedent for using the serialization module to serialize render arrays? I think of the serialization module as there to serialize models, but render arrays are views (not in the Views module sense, but in the MVC sense). And serializing render arrays to HTML is exactly what the render and theme systems do.

    Conceptually, the *--xbxb.html.twig files function exactly like theme hook suggestions. They override the base template and render different HTML than what the base template renders. In this case, that "different HTML" is an HTML custom element.

    I agree that it's weird that the *--xbxb.html.twig files contain JSON instead of Twig. I think there's ways that that can be refactored. For example, the JSON could be moved to a different filename, like *--xbxb.spec.json, and the semi_coupled engine could be refactored to a Twig function, so that the *--xbxb.html.twig file could become regular Twig files that just do something like:

    {{ renderSemiCoupledCustomElment(_variables, 'NAME.spec.json') }}
    

    That would then also remove the need for experience_builder_system_info_alter().

    Not sure if the above is the best DX though, and there might be better ideas for how to bridge XB's theme hook suggestions to rendering the desired custom element, but I don't see how or why the serialization module belongs in the mix for this. Maybe I'm wrong though and am just underestimating the scope or utility of the serialization module?

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    Symfony setializer defines two interfaces, NormalizerInterface and SerializerInterface. The first one is for turning objects into arrays, the second one is for turning arrays into strings. Eg there's an XmlSerializer, a JsonSerializer. We'd just have a XB template setializer for turning render arrays into a string in our bespoke manner

  • πŸ‡ΊπŸ‡ΈUnited States effulgentsia

    But no other module that defines theme hook suggestions does that. E.g., `field_ui.module` adds `form_element__new_storage_type` to its `hook_theme()`; it doesn't define a new serializer. How much of the theme system would we need to duplicate into an XB template serializer?

  • πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

    I think with some refactoring of theme manager (eg adding methods as extension points) , we could subclass it and not need to duplicate much
    But it would require a spike to identify those extension points

    I also think storing the link between a react component and a form element plugin should happen via form element plugin definitions. The setializer can read that to work out what should be formatted for hyperscriptify and what should not

    This means in the future we can allow modules to opt in their custom elements. The hard coded mapping in the JS could be replaced with a mapping of lazy imports we could share via Drupal settings

    This requires changes to vite config for the UI and the use of import maps, but we would need to do that anyway to support UI extension

    I can work on a spike on this when I'm back

  • πŸ‡¬πŸ‡§United Kingdom longwave UK

    Just been digging into the semi-coupled theme engine a bit, and I think that if theme engines were tagged services, and could be selected by their supported file extension, instead of using hook-style function names, then you could hopefully just decorate the Twig engine in order to extend it?

  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
  • πŸ‡ΊπŸ‡ΈUnited States effulgentsia

    By the way, if I understand the desired UX correctly, we're wanting to render the forms in the right-hand sidebar using a set theme, neither the site's front-end theme, nor the admin theme. If this is correct, then even if we did no refactoring to the semicoupled theme engine, we could just use it as it was initially designed for: as the theme engine of the theme that we use for rendering the forms in the right-hand sidebar, so we wouldn't need to hack-alter the engine of themes we don't control.

    That said, I'm not opposed to refactorings that decouple it from being a theme engine, so that it could be more cleanly integrated with any theme.

  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    If this is correct, then even if we did no refactoring to the semicoupled theme engine, we could just use it as it was initially designed for: as the theme engine of the theme that we use for rendering the forms in the right-hand sidebar, so we wouldn't need to hack-alter the engine of themes we don't control.

    It's not entirely correct.

    @lauriii has indeed indicated that XB needs its own look and feel.

    • In Drupal, "look and feel" is defined in Drupal Themes.
    • But for XB, the intent is to do that in React, using the semi-coupled theme engine. IOW: just like a Drupal Theme would impose its decisions wrt markup/class names/…, in XB those decisions should happen in *.tsx files.
    • To make that possible, the starting point must be reliable β€” just like it must be for Drupal Themes. "stable starting point" implies no theme interacts with markup before XB's React code. Which is why πŸ› Changing default theme can break HTML forms generated by XB API routes: always negotiate the Stark theme Fixed made it so that we always negotiate the "Stark" Drupal Theme, which actually means "No Drupal Theme": it's only the raw render arrays/default templates that Drupal Modules provide.
  • πŸ‡ΊπŸ‡ΈUnited States effulgentsia

    we always negotiate the "Stark" Drupal Theme, which actually means "No Drupal Theme"

    I don't get that. If we always negotiate the Stark theme, why can't we instead create an xb_stark theme that's just like Stark (whether a fork or subtheme) but that sets semicoupled as its theme engine, and always negotiate that theme? That would allows us to remove experience_builder_system_info_alter() which is the part that's "taking over the twig theme engine for the whole site" that this issue's title is referring to.

  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    πŸ€”

    Ohhhhhhhh! πŸ€“πŸ˜

    TIL that's possible! 🀯 Quoting core/modules/system/tests/themes/test_theme_nyan_cat_engine/test_theme_nyan_cat_engine.info.yml:

    name: 'Test theme for Nyan Cat engine'
    type: theme
    description: 'Theme for testing the theme system with the Nyan Cat theme engine'
    version: VERSION
    engine: nyan_cat
    base theme: false
    

    … built by a certain "lauri" 9 years ago at #2574717: Remove PHPTemplate, and add test coverage for multiple theme engine support β†’ … during DrupalCon Barcelona … 2015! πŸ˜„

    I think this should be the next thing for @deepakkm to tackle now that πŸ“Œ Lift all methods out of `SdcController` into separate invokable services-as-controllers Needs work is 99% done.

  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    Captured #13 into a concrete proposed solution. @effulgentsia, can you review/confirm, and then assign back to @deepakkm if you do agree? πŸ™

  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
  • πŸ‡ΊπŸ‡ΈUnited States effulgentsia

    I agree with #14, but that wouldn't fully address @larowlan's original goals in this issue. He may still want to pursue the "custom serialization format" part, which I disagree with, but who knows, maybe he'll come up with an MR that convinces me. Therefore, I think we should open a child issue to do just the part that's in #13 and what's minimally required for that, and leave this issue as potentially still having @larowlan's more ambitious goals.

  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    Fair! Done: πŸ“Œ Introduce the `xb_stark` theme that uses the Semi-Coupled theme engine, removes the need for taking over the Twig theme engine for the whole site Active .

    Let's wait for #3478287 to land, and then ask @larowlan to re-assess 😊

    Restored original issue summary.

  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
  • Issue was unassigned.
  • Status changed to Postponed 2 months ago
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
  • πŸ‡ΊπŸ‡ΈUnited States effulgentsia

    With πŸ“Œ Introduce the `xb_stark` theme that uses the Semi-Coupled theme engine, removes the need for taking over the Twig theme engine for the whole site Active in, we're no longer "taking over the Twig theme engine for the whole site", so retitling the issue to what I understand @larowlan's proposal to be. However, per #5, I still don't really see why we would want to circumvent the theme system when rendering to HTML, and I think trying to do so would lead to a lot of difficulties when the form has a mix of some elements that have XB suggestion overrides and some that don't interspersed through the tree (e.g., either can be a child of the other).

  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    Agreed with @effulgentsia. Perhaps this is the more appropriate issue status πŸ˜‡

Production build 0.71.5 2024