🇦🇺Australia @larowlan

wim leers → credited larowlan → .

https://www.drupal.org/project/nsw_feedback/releases/2.0.0-beta4 → done

Now that 📌 Move clientside assumptions about prop form data shape into a series of prop specific transforms Active is in I think we should explore using these for entity form fields as well.

I could also see a world where we had server-side transforms (these would be callbacks) so that we could sidestep issues like \Drupal\Core\Entity\Element\EntityAutocomplete::validateEntityAutocomplete and the like - this would hopefully mean we could decouple from the form system and not need to build and submit a form anymore.

Fixed in dfaf6b18

I confirmed #34 solved the issue so I reverted f3fd9d2f

#34 changed the behaviour in HEAD - previously if you sent a field you didn't have access it would generate an error.

Now the field is ignored.

I updated the test to reflect this and it now passes - that is 9592dd2a

Would be good to get a +1 from Ted/Wim/Lauri/Alex on the new behaviour

This should be green again and the 2 bugs are now fixed.

Thanks @bnjmnm, we should revert https://git.drupalcode.org/project/experience_builder/-/merge_requests/5... then as I did that as a temporary workaround

larowlan → created an issue.

This doesn't happen in head because we don't use the converter service, we use a custom convert method in preview controller

I vote we revert back to that and sort it separately

That's the style tag added in DummyPropsEditForm, it can go, it was something I tried to get the test passing

My gut feel is although it would be more elegant, it would be tight coupling.

larowlan → created an issue.

larowlan → created an issue.

larowlan → created an issue.

larowlan → created an issue.

There were 2 methods in ApiLayoutController that had '@todo make this a trait' pointing here.
I ended up making them a trait to share with ApiPreviewController, but I like your proposal on the issue to make it one controller.

Needed more of this in 📌 Support server side massage and validation of component prop form values Active

Can I suggest something other than propSources - we'll likely want to ship jsonSchema for block components - and currently this sits under field_data.

Or if we rename it to propSources, can we move the schema elsewhere?

Also blocked on 📌 OpenAPI spec insufficiently precise for `LayoutComponent` Active which will make the diff here smaller

Addressed #18 - it is much simpler than that now that 📌 Maintain a per-component set of prop expressions/sources Active is in, we just send the model. That's it. Rebased, then squashed to do that and then removed 📌 Include the preview HTML in the layout controller payload Active

Will start on the review feedback and the remaining tasks (including fixing tests).

Commit ace768f6 has the work here on top of 📌 Support server side massage and validation of component prop form values Active

When that is in, we can rebase --onto origin/0.x and drop d78ade44

Opened the source code of @swc/wasm-web and it has this

async function __wbg_init(module_or_path) {
    if (wasm !== undefined) return wasm;


    if (typeof module_or_path !== 'undefined') {
        if (Object.getPrototypeOf(module_or_path) === Object.prototype) {
            ({module_or_path} = module_or_path)
        } else {
            console.warn('using deprecated parameters for the initialization function; pass a single object instead')
        }
    }

    if (typeof module_or_path === 'undefined') {
        module_or_path = new URL('wasm_bg.wasm', import.meta.url);
    }
    const imports = __wbg_get_imports();

    if (typeof module_or_path === 'string' || (typeof Request === 'function' && module_or_path instanceof Request) || (typeof URL === 'function' && module_or_path instanceof URL)) {
        module_or_path = fetch(module_or_path);
    }

    __wbg_init_memory(imports);

    const { instance, module } = await __wbg_load(await module_or_path, imports);

    return __wbg_finalize_init(instance, module);
}

export { initSync };
export default __wbg_init;

so the default export takes a module_or_path

We're calling it with no arguments

But we should be able to pass in the path there and it will end up with

module_or_path = fetch(module_or_path);

I think we can use Rollup's replace plugin here (unless I don't understand the problem).

We can use __DRUPAL_XB_BASE_PATH__ in all of our code and in our viteconfig we can pass config to rollup plugin replace to replace that with a reference to a global, e.g. drupalSettings.xbBasePath that we ensure is always available on the page via #attached

Agree re 📌 [PP-2] Remove InputBehaviorsBlockSettingsForm and consolidate with input components form Active - added it

Yes I think this can land and I think some of the changes in 📌 Will passing the model props for a component via GET parameters hit limitations due to URL length Active highlight that we need to make sure the #parents are consistent - in fact they should be applied in the ComponenteInputForm rather than in each source plugin. That will move this closer I think - will try to take a stab at this before the end of the week

hooroomoo → credited larowlan → .

wim leers → credited larowlan → .

This is for when we fetch the component inputs form

x-post

I have an MR up on 📌 Will passing the model props for a component via GET parameters hit limitations due to URL length Active 🤞

Ok, added a hidden field with those values to ensure it works with AJAX.

Yes these can be tampered, but so could the URL so 🤷

Maybe we need an anti-tamper hash?

Yep, doesn't work with AJAX forms because the form needs to be rebuilt and that requires the params - and they are no longer in the body of the request because now we're POSTing the Drupal form

That was far too easy

Had a shot at using PATCH for this, but not sure what impact it will have on subsequent AJAX requests.

So we have an answer to 📌 Will passing the model props for a component via GET parameters hit limitations due to URL length Active then - in our failing cypress tests on this issue.

{
                      "status": "PARSING_ERROR",
                      "originalStatus": 414,
                      "data": "<!DOCTYPE HTML PUBLIC \"-//IETF//DTD HTML 2.0//EN\">\n<html><head>\n<title>414 Request-URI Too Long</title>\n</head><body>\n<h1>Request-URI Too Long</h1>\n<p>The requested URL's length exceeds the capacity\nlimit for this server.<br />\n</p>\n</body></html>\n",
                      "error": "SyntaxError: Unexpected token '<', \"<!DOCTYPE \"... is not valid JSON"
                    }

So I think this is blocked on 📌 Will passing the model props for a component via GET parameters hit limitations due to URL length Active and we need to make form fetching use a POST request.

The intent is partially performance but also it gets us closer to 📌 META: Conflict free concurrent editing Active

We need to do 📌 Support server side massage and validation of component prop form values Active first because it introduces the previewSlice

larowlan → created an issue.

In 📌 Support server side massage and validation of component prop form values Active I've managed to do away with ::findTargetForProps once the user selects something from the media library

However I've had to retain it for the use-case when the user adds a new image component because the default value is invalid

This reinforces my comment above

The root of the issue here is that we map the image property to an entity reference field of type media, but set a default value that is a hard-coded URL - suggesting that in fact this could/should be a URI field. But we want to use the media library to edit the values. So we're in a state where the component has a default value that doesn't match the data shape (field type) we've specified

I think until we have a way to reconcile those two we're at an impasse. Either we're storing a media reference OR we're storing a URL.

And perhaps that points to us needing a custom field type - a compound field that is an ER with additional columns/properties for src/alt/width/height.

I think we also have a disconnect in our components API with what we store in the config entity

Here's what's in the components api for the default value for the image component - this is taken from the example in the component.yml file.

But here's whats in the config entity:

uuid: 0202d62a-193a-4129-ae3a-b7c8f742f743
langcode: en
status: true
dependencies:
  module:
    - media_library
label: Image
id: sdc.experience_builder.image
source: sdc
category: Other
settings:
  plugin_id: 'experience_builder:image'
  prop_field_definitions:
    image:
      field_type: entity_reference
      field_storage_settings:
        target_type: media
      field_instance_settings:
        handler: 'default:media'
        handler_settings:
          target_bundles:
            image: image
      field_widget: media_library_widget
      default_value: {  } # 👈️👈️👈️
      expression: 'ℹ︎entity_reference␟{src↝entity␜␜entity:media:image␝field_media_image␞␟entity␜␜entity:file␝uri␞␟url,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}'

And this is because \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase::getClientSideInfo special cases the a prop with $ref 'json-schema-definitions://experience_builder.module/image'

And I also think that reveals a possible solution for us here as follows:

• We already have \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase::getPropsForComponentPlugin which is called from updateConfigEntity and createConfigEntity.
• Currently in that code we're bailing out if the prop shape maps to an entity-reference field BUT perhaps we shouldn't - perhaps we should add support in there for looking at the default image referenced in the $ref and importing it into a media entity AND then setting a default value that includes the target ID
• Remove the special casing of 'json-schema-definitions://experience_builder.module/image' in \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\GeneratedFieldExplicitInputUxComponentSourceBase::getClientSideInfo
• This also allows us to manage config dependencies, as we can make a reference from the component config entity to the media entity which will mean \Drupal\Core\Config\ConfigEvents::IMPORT_MISSING_CONTENT gets fired on a config import
• We then make use of an additional key in the source plugin settings for SDC/Code components to store a base64 encoded version of any default images for deployment sake and add an event listener for \Drupal\Core\Config\ConfigEvents::IMPORT_MISSING_CONTENT to take care of creating the missing media entity in a deployment based on these settings
• Then our default value would include target_id and this would get us closer to removing findTargetForProps too

If this seems like a reasonable idea I can do a spike on it.

Most of this seems to have todo's in the code so it might be time to pay down these debts

Came here to post a reference to 📌 Update XB's `image` SDC to comply with best practices, and document those best practices Needs review and then realised Wim already had 😀

I think this needs 📌 Support server side massage and validation of component prop form values Active

I've started on this in 📌 Support server side massage and validation of component prop form values Active - so postponing

I _think_ the blocker is in to at least do a spike here

There's some comments in the MR from @mstrelan that need addressing - thanks for working on this

The root of the issue here is that we map the image property to an entity reference field of type media, but set a default value that is a hard-coded URL - suggesting that in fact this could/should be a URI field. But we want to use the media library to edit the values. So we're in a state where the component has a default value that doesn't match the data shape (field type) we've specified. We work around this with ::findTargetForProps but this is a temporary workaround (And commented as such).

In HEAD we try to reconcile this by at least ensuring that a media entity exists with the given default image URL - but as discussed in this issue, that isn't working in all cases.

We're trying to resolve this on multiple fronts.

- 📌 Support server side massage and validation of component prop form values Active
- 📌 Split model values into resolved and raw Active

However those are both very tricky. I'll have a think about what we can do in the meantime

Thanks - is there any way we can present this with a checkboxes instead of a text field?

I was hoping to store this in a base field definition override rather than a separate config object - to sidestep needing to add a schema and an update hook.

What was the reasoning for that not being suitable?

If you look at our docs for menu links → - that override should be possible via a base field definition override - https://www.previousnext.com.au/blog/overriding-base-field-labels-and-de...

I think there are a few more curly bits here:

• Element validate callbacks - e.g entity autocomplete elements change the value set on a field in a validation callback - we have no way to do this yet without submitting the Drupal form structure via the form submitter - 📌 Support server side massage and validation of component prop form values Active is probably where we'd handle that
• Value callbacks - e.g. date time element takes the incoming values and converts them into something else - I think transforms have resolved this, but we need to confirm - like in 📌 Support server side massage and validation of component prop form values Active as well
• Things where the shape of what we store is different to the resolved props in the model and we can't resolve them client-side - e.g. media references - we store target_id but resolve to src/alt/width/height - 📌 Support server side massage and validation of component prop form values Active , 📌 Split model values into resolved and raw Active and 📌 Maintain a per-component set of prop expressions/sources Active will hopefully resolve that

So it would be good to keep this open in the meantime, but postponed on those.

hooroomoo → credited larowlan → .

balintbrews → credited larowlan → .

benjifisher → credited larowlan → .

Wim asked me to write up my thinking about how we will achieve this.

• At present we send entity_form_fields to Drupal using the HTML input name as the keys
• However when we return 422 errors from Drupal, the validation errors are keyed using entity-api property paths - e.g. `title.0.value`. These don't always align with what the name attribute is on the HTML input element (or select, or textarea).
• When 📌 Move clientside assumptions about prop form data shape into a series of prop specific transforms Active is done we have transforms to map from widget structures to an object structure. So we should be able to do something similar for the page data form - but instead of using it to map input values to prop names, we need to be able to use it to generate a map from HTML name attributes to property paths. e.g an error on some_enum_field.0.value might map to <select name="some_enum">
• Once we have this we can also do away with the flat structure of entity values and start sending them in a fashion closer to how we send JSON with JSON:API. This allows us to do away with the \parse_str we are using in a few places to convert the flat structure back into a nested object.

larowlan → created an issue.

larowlan → created an issue.

Confirming I cannot reproduce this on the autosave code components branch

https://www.drupal.org/project/hal/releases/2.0.4 →

Thanks, will cut a new release

larowlan → made their first commit to this issue’s fork.

We're working to remove this feature in favour of project browser

To bring it back in the meantime, I think you need to set $settings['allow_authorize_operations'] to TRUE in your settings.php

quietone → credited larowlan → .

There is an api to update the cache in onQueryStarted to avoid this https://redux-toolkit.js.org/rtk-query/usage/manual-cache-updates

Pausing this to work on 📌 Move clientside assumptions about prop form data shape into a series of prop specific transforms Active

Remaining tasks:

• PHPUnit coverage for \Drupal\experience_builder\Controller\ApiPreviewController::updateComponent
• Modify \Drupal\experience_builder\PropSource\StaticPropSource::massageFormValuesTemporaryRemoveThisExclamationExclamationExclamation to mimic form submission aspects of \Drupal\experience_builder\ClientDataToEntityConverter::setEntityFields and \Drupal\Core\Entity\Entity\EntityFormDisplay::extractFormValues - the e2e fails are because form values for things like datetime need to make use of valueCallbacks on render elements as well as massageFormValues. We're already doing massageFormValues and are hacking our way around #element_validate callbacks (which can also set the widget value) - but we need to bite the bullet and make use of the form submitter because of things like value callbacks on element plugins. Hopefully there's some logic in \Drupal\experience_builder\ClientDataToEntityConverter::setEntityFields that we can break out into a helper service or similar. Then the pieces of \Drupal\Core\Entity\Entity\EntityFormDisplay::extractFormValues could probably go into component source specific plugins that make use of widgets

We're fighting a lot of flexibility and inconsistency between widgets and entity data already, but element validate and value callbacks make things harder still.

I hit this on a fresh install and can confirm the file is created, but the media entity is not, which would indicate the media type doesn't exist when experience_builder.install is running - likely because we're now installing modules in bulk 📌 Add the ability to install multiple modules and only do a single container rebuild to ModuleInstaller Active (I'm running 11.x HEAD)

larowlan → created an issue.

mglaman → credited larowlan → .

kristen pol → credited larowlan → .

wim leers → credited larowlan → .

wim leers → credited larowlan → .

no worries mate, I'm GMT+10, so we almost line up - albeit a day out. @dww is also in your part of the woods and it seems like we've been able to overlap a fair bit TZ wise in the past.

Related issue

larowlan → created an issue. See original summary → .

Sorry, when I say 'visit update.php' I mean visit `/update.php` in your browser and run the updates. If these won't run check https://www.drupal.org/docs/updating-drupal/troubleshooting-database-upd... → for help

You can't delete a theme without uninstalling it, you might need to uninstall it first from /admin/appearance/uninstall

Do you have existing forums/replies? If so I don't think reinstalling is an option.

But you can recreate adding the field, but that will require a few manual steps. I'm happy to help with that but it might be best to ping me on slack and we can jump in a call - see drupal.org/slack for details on how to join

wim leers → credited larowlan → .

surabhi-gokte → credited larowlan → .

larowlan → created an issue.

larowlan → created an issue.

Hi folks @larowlan chiming in here as a subsystem maintainer.

Wondering if there's a simpler solution that is some javascript in the redirected page that does this:

const query = new URLSearchParams(window.location.search);
query.delete('check_logged_in');
const queryString = query.toString();
let url = window.location.pathname;
if (queryString.length) {
  url = `${url}?${queryString}`;
}
history.replaceState({}, "", url) 

Then when the user presses back they don't land on the URL with check_logged_in

This is much less maintenance overhead and feels much more reasonable than adding additional redirects every each login

You can test this yourself on https://drupal.org/?check_logged_in=1
Paste the code above into the console.
Then click on any link
Then press back.
It's as though you never visited https://drupal.org/?check_logged_in=1

This feature of the web platform is designed entirely for this problem, so we should leverage it in my book.

I'm changing the category here to a feature request. The check_logged_in behaviour is working the way it was intended.

I think we can improve it with the above approach and keep everyone happy.

Thanks, both of those make sense. I forgot about in place editing 👍️

FWIW

@larowlan did implement the "auto-create upon save" functionality, just not in ::postSave(), but in the new JavascriptComponentStorage

I did it in the storage handler because we have DI in storage handlers, in Entity::postSave we have to use \Drupal. We use this pattern fairly regularly for client projects and I think it is cleaner.

I'm very keen to keep going on 🐛 Some components cannot be used in XB because UI prevents SDC props being named `name` Active so we can kill off ::findTargetForProps which is where this issue is coming from - but it sounds like we need to do something else in the meantime as a band-aid

4 is not related to the issue you're seeing here - but it does indicate you haven't run database updates (which is what 1 is saying). Can you run `drush updb` or visit update.php to run those updates. One should add an index to the reported forum table and that should resolve 1 and 4.

For 3, if you're not using bartik you can uninstall it from admin/appearance.

Did you migrate your site from Drupal 7? When forum module is installed it adds a term reference field named 'taxonomy_forums' - you don't seem to have that field.

xb-prop-start- is not actually possible

My understanding is that this was added to support a future state where we didn't need a round trip to the server to update the preview.

With code components, we don't do any server side rendering so I don't think we need to worry about props - we will already have client side updates right?

In the permissions page under Node what do you see?
Can you screenshot the manage fields page for your forum topic content type?

Thanks

larowlan → created an issue.

Opened an MR to replace the file if it exists, that should smooth things until we get a chance to work on 🐛 Some components cannot be used in XB because UI prevents SDC props being named `name` Active

All of this is temporary code until we can delete ::findTargetForProps which is what 🐛 Some components cannot be used in XB because UI prevents SDC props being named `name` Active and its children are for.

I think in the meantime using FileExists:Replace will smooth the issue Lauri experienced

Can you put a breakpoint in the install hook and advise where it exits?

This looks like a really good cleanup to me and I think makes sense on it's own - the CR mentions calling drupal_static_reset with a given key

Can we emit a deprecation error for that key inside drupal_static_reset?

My thinking around the client side/server preview comes down to import maps. If we need an import maps it needs to be in an IFrame. But if it's already solved, that's awesome

I think there's also a world where we use jspm in the editor to build an import map but then have Drupal fetch the resultant files, validate their resource integrity and serve them locally from inside the assets:// stream wrapper (aka sites/default/files/js or equivalent).

Could even be lazy like core's asset optimisation so that we only fetch it when it is requested