🇦🇺Australia @larowlan

Thinking about this some more and I really think these need to be shown inline in the form, it feels like bad UX to accept a form submission (entity form) and store it in auto-save and then at some future state prevent publishing.

We already have a form-state slice with errors, it would be a matter of returning them from the BE and putting them into the slice, keyed by the name attribute on each <input name=""> (or select, textarea))

Went with option 3 - add additional temporary code with a todo to remove it in this issue.

This has implications for 🐛 Page status changes from "Published" to "Changed" even when no actual changes are made Active which I'm working on at the moment.
I have all the phpunit tests passing but are failing e2e on changes we made in 📌 Publish checks validation constraints, but not form validation Active

Do we think we should postpone 🐛 Page status changes from "Published" to "Changed" even when no actual changes are made Active on this? Or bundle this into that one?
Or add another workaround and reference it back here like the existing code does?

The specific issue in 🐛 Page status changes from "Published" to "Changed" even when no actual changes are made Active relates to form validation errors only (not layout/model/entity violation errors)

Committed to 11.x and backported to 11.2.x

Thanks folks.

I'm +1 for this, with one exception, I think entity-types should use ENTITY_TYPE_ID for DX reasons.

Dupe of ✨ Use constants for plugin IDs Active

larowlan → created an issue.

I think there's a chance we could store invalid data, so that feels like beta-blocker

larowlan → created an issue.

Yes I agree, I wrote something similar in 📌 Define how block settings should be presented in the UI Active yesterday - basically that we need to be calling ::blockValidate and ::blockSubmit from ::clientModelToInput in the block source plugin.

We can use this issue or that one to do that work

The code in question is parentElementInsideIframe = slotsMap[parentSlot.id].element;

The code in the MR is what I was expecting - this only happens if you write your own Source plugin and don't emit the comments.

Its failing tests so needs work

Committed to 11.x

Crediting adam who left a review in gitlab

Left some comments on the review, mostly nits, the one around null return from ::getCurrentRequest and addressing the local storage key are blocking the rest are minor

xjm → credited larowlan → .

Agree re tests, merge on green 👍

Do we have an existing issue to call ::blockValidate and ::blockSubmit from \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\BlockComponent::clientModelToInput ?

If so I think we could repurpose this to do that.

Should be down to just failures in ApiConfigAutoSaveControllersTest now, will continue next week

Addressed Wim and my review comments whilst its a public holiday in the US, this looks ready to go

This conflicted with 🐛 Page status changes from "Published" to "Changed" even when no actual changes are made Active but I had made the same change there and that's what it conflicted on - nice!

Left a question about using core's linkset endpoint instead of needing the contrib one

Just a couple of minor nits about the use of the verbose match for booleans

https://twig.symfony.com/play makes use of a php-wasm for client side rendering of twig files

This is something we could explore

This is very close, just a couple of minor adjustments needed

I think 🐛 Page status changes from "Published" to "Changed" even when no actual changes are made Active will resolve this, postponing

Agreed

\Drupal\workspaces\WorkspacePublisher::publish most definitely uses a transaction, We should be able to do something similar in our POST (publish) endpoint.

🐛 Page status changes from "Published" to "Changed" even when no actual changes are made Active effectively does this because the new code is just $entity->validate() then $entity->save() when we stop storing arbitrary values in autosave and instead store entities.

I've thought about this overnight and I'd be happy to join forces if the new features came with test coverage. If that's not feasible, then yeah let's keep them separate

🐛 Page status changes from "Published" to "Changed" even when no actual changes are made Active will fix this

♻️

Sorry, I wasn't clear, I'm not proposing any change to the discussed approach, just validating that the approach is viable. Will focus on 🌱 Milestone 1.0.0-beta1: Start creating non-throwaway sites Active

wim leers → credited larowlan → .

wim leers → credited larowlan → .

wim leers → credited larowlan → .

wim leers → credited larowlan → .

acbramley → credited larowlan → .

Salacious title update

larowlan → created an issue.

Ready for review again

This was causing cypress fails for me locally due to the time out.

MR improves performance by 80%

Tagging 2.0.0-beta2 thanks

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

larowlan → created an issue.

xjm → credited larowlan → .

Hey, I'm happy keeping them separate, core took a different approach (using modules instead of feature flags) so I think the feature set we have here is probably enough for now.

Thinking about this somewhat, the default rendering of Drupal is to HTML.
But that is a serialization format. JSON is another.

Perhaps if we reworked the renderer so that all of the processing aspects were part of 'normalizing' and then the render array to HTML was 'serializing' we could support serializing to JSON and other formats for render arrays.

These sound like the 2 steps you're talking about in #8 @fago

We already have these concepts for entities, but not for render arrays (which with 📌 Slowly, very slowly start OOPifying the render system Needs review start to move towards objects anyway).

This was the same approach I proposed in 📌 Stop taking over the twig theme engine for the whole site, just to render the edit form, move to a custom serialization format Active

Ideally it would be good to do a spike on a BC loading layer for Paragraphs/Layout builder to triple check we've got a clean run at that in the future with the current data model

xjm → credited larowlan → .

Thanks, will cut another release

Discussed with Adam and we agreed getting XB to listen to default content pre import event and making sure it has generated components feels like a good approach to make sure this race condition doesn't happen

Poked at this after chatting with Adam

There were two issues:

1) missing component versions - I fixed this and pushed
2) the recipe runner installs modules before themes

I put a debugger on the drupal_flush_all_caches call in experience_builder_install and stepped into \Drupal\experience_builder\Plugin\ComponentPluginManager::setCachedDefinitions

At that point the mercury theme doesn't exist - so no component config entities get created for the SDC

But when we get to the theme install step, on 11.2 that yields a cache clear of the plugin definitions _but_ we're in a ::isSyncing TRUE state, and hence the logic in \Drupal\experience_builder\Plugin\ComponentPluginManager::setCachedDefinitions is skipped

I think that means we might need to ship default component config entities, which is the opposite of what I told Adam in a call earlier.

But then I tried adding them, and the issue is that the plugins don't exist at that point, because the clearing of the cache in the theme installer doesn't do discovery, it just clears the cache.

So I then added this to experience_builder.install

function experience_builder_themes_install(): void {
  \Drupal::service(ComponentPluginManager::class)->getDefinitions();
}

But that didn't seem to help either.

I've pushed up the config, hopefully this helps Adam pinpoint where the issue lies.

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

We need to add an index then to avoid the `using filesort` - thanks @tgauges

Just one minor comment on the MR, I think its ok, but would be good for a second opinion

Hi @abhinesh could you open an MR for that? thanks!

Per \Drupal\datetime\Plugin\Field\FieldType\DateTimeFieldItemList::processDefaultValue there is no value key, you pass default_date_type of relative and

\Drupal\datetime\Plugin\Field\FieldType\DateTimeFieldItemList::processDefaultValue handles is

Add mentioned in the other issue, handleDefaultValue transforms this

Date type relative accepts the value, see the handleDefaultValue method

xjm → credited larowlan → .

Discussed this in a call with @effulgentsia, particularly the need for the dynamic nature rather than static image styles.

The desire for this is coming from feedback collated by Lauri that configuring this correctly is difficult and arduous.

I pointed out that some of our sites have 100+ image styles and @penyaskito pointed out that Drupal CMS has 55. I was trying to illustrate that having a lot of image styles is common. Alex rightly pointed out that this illustrates the pain point - which I agree. We have cli tooling to get around but this is a attempting a simpler approach.

I think the only addition needed for this is to instead of instantiating it as an ImageStyle to instantiate a subclass (e.g., DynamicImageStyle), where the subclass overrides getPathToken() from hashing $this->id() to instead hashing just the part of $this->id() before --

that sounds like a good idea, this class could also override ::flush and deal with the GC/cleanup. I wonder if we could explore storing third-party settings on the image style rather than relying on magic xb-- naming. This could even hold the dynamic widths so that in theory we could have an extension point. Doesn't need to happen now, but I think trying to do this in a way that could have a path back to core that solves the N image styles proliferation issue should be a long term goal.

So +1 for going down the subclass approach.

Closing this for now until we actually work on LB support it might not be needed

I'm personally still in favour of this because

a) I want to see \Drupal\experience_builder\Controller\ClientServerConversionTrait gone. It's use in Patterns, PageRegions etc feels wrong
b) I want to see the static methods on that trait returned to non static methods on the service.
c) I don't want the controllers to have to work with the raw data, I want the request object to have upcast objects automatically

Adding beta-blocker tag as this blocks 📌 [PP-1] Autosave data should store converted server-side representation of model Postponed and that itself has the tag

Actually, I think the new approach in 🐛 Page status changes from "Published" to "Changed" even when no actual changes are made Active makes this redundant. Because conversion happens immediately, so the data stored in the auto save store cannot get out of date w.r.t the source plugin/version.

Postponing on that.

Updating the issue title to reflect the approach.

In light of #10 and #11 I still agree with myself in #8 that this approach feels wrong.

We should validate the user has access at the time the autosave store is written to, and then check that new permission (only) at publish time.

Additionally 🐛 Page status changes from "Published" to "Changed" even when no actual changes are made Active is moving to remove AutosaveManager::save(EntityInterface $entity, array $arbitrary_data) and replace it with AutosaveManager::saveEntity(EntityInterface $entity) such that you have to convert the entity before you can store it in the autosave and you can only store the entity and nothing else. That will mean the updated_entity_form_fields we're using here will be removed. It will also bring us closer to a workspaces backed auto-save because internally to AutosaveManager::saveEntity(EntityInterface $entity) we could almost immediately switch the internals of that method to write a revision to a workspace if $entity was a revisionable content entity.

I think on #8 I could live with this checking entity update access, but the field level access should only be checked at the time of autosave write, not at publish time.

Thinking about an analogy to how Drupal works now.

If I have field level permissions for the node edit form and have two roles as follows:

* Super secret field editor, can edit the values in a secret field
* Publisher, can move any content from needs review to published

If a user of role Super secret field editor makes an edit to a secret field and sets it to needs review, the Publisher can still move it from needs review to published. In that operation all they are changing is the moderation state and that is covered by permissions in content moderation module around transitions they have access to (provided they have edit access to the node). At the time of publishing, each of the individual fields aren't checked, only the transition permission. The individual field permissions are checked at the time the draft revision is created.

I think we should be doing the same thing here.

Cheers, thanks

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

Re #15 this test actually provides additional test coverage for when you update another component in the preview.

Because when that happens the source plugin's ::clientModelToInput method is called and that changes the stored values.

For example, when a stored component that previously used a generated source plugin (Sdc, JS) is stored in autosave it has a source and resolved key, these come from it's ::inputToClientModel method.

When we call ::patch we rebuild the layout and that calls ::inputToClientModel on a different component, but is passed the old client model from autosave.

So we still need this test, as it covers that scenario

Ah, then that is a bug.

Fairly sure that will result in mismatched entity field definition warnings in status reports unless we add an update hook

I've started a new branch named 3529622-convert-before-autosave that changes the signatures on autosave to only take the entity, i.e. you have to convert before you ask the autosave manager to save it.
I've also done some work to make hashing of the entity determinate and have APiLayoutControllerPostTest::post passing. I expect there will be a lot of failure and I will check in with @tedbow when I start tomorrow to get a handover to pick it back up again. No pressure to work on this @tedbow

Because its Ted's weekend I'll try and push this forward a bit

Rebased this off 0.x

We do still need it because when $component->getComponentSource() is using the fallback as the last version, the shape of the input data server side is different to that on for the default generated components.

Updated the logic to allow for the new versioned properties and got the test passing again.

The steps to reproduce would be:

* Create a new menu
* Place an XB version of that menu's block in an unsaved page
* Delete the menu
* Try to render the component preview

But having said that, with the introduction of component versions, it might not be an issue.

I'll do some digging.

xjm → credited larowlan → .

xjm → credited larowlan → .

balintbrews → credited larowlan → .

griffynh → credited larowlan → .

Implemented #31 (With some modifications, pseudo code etc) and updated the tests.
Removed the separate table and services and updated the usage in FieldTypeUninstallValidator and associated tests.

This is green and ready for review again.

@catch let me know that the longer term plan is to wait for PHP 8.4 and property hooks rather than dedicated properties. Would be good to document that somewhere

Thanks for crediting me, but I'm going to remove it because I got the same error 4 hours earlier and didn't do anything about it. If adam could have 2 credits instead that'd be great 😀

xjm → credited larowlan → .

Ok, addressed the review feedback, this is ready for another review.

benjifisher → credited larowlan → .

Took a look at this and it wasn't what I was expecting but agree it is a logical first step towards object oriented form/render array.

It see some comments in the code around longer-term plans for D12/D13 of deprecating arrays and using the element plugin instances everywhere.

In light of that I think it would be good to have a parent meta created with some sibling issues for the next steps.

The longer term things I would expect we would move towards would be replacing the @property doc block comments with real properties and using $storage for random overflow (people have gotten used to dumping random things in #somekey, this would give us language level type-checking and possibly memory improvements

Aside from that I think the best course of action is to get this into 11.x for 11.3.x early and unblock the follow up work, but let's articulate what those next steps are first.

Thanks folks.

Thanks will look at review feedback and #11 later today

This comment https://git.drupalcode.org/project/experience_builder/-/merge_requests/1... confirms we're doing it here 👌

Thanks, makes sense - merged

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

Thanks, this all looks non-controversial - merged

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

I wonder if we can track why we're posting to the layout endpoint - the preview comes back from the get controller now so we shouldn't be making any post requests _until_ we change something.

Ah, that's because the form state gets set from input behaviours in the FE which does things like mapping booleans from 0/1 to true/false
Tricky 🤔

Thanks, should we tackle #31 here or do we have a separate issue for that? Assuming here is fine.