🇺🇸United States @tedbow

In 🌱 Research: Possible backend implementations of auto-save, drafts, and publishing Active one of the reasons we discussed for an having auto-save that was independent entity revisions was to allow having saving incomplete or invalid data.

It seems this would abandon that idea or at least limit to allowing empty slots. Is that right? I think because \Drupal\experience_builder\Controller\ApiLayoutController::buildPreviewRenderable already calls \Drupal\experience_builder\ClientDataToEntityConverter::convert() the "page data" information would already be validated and we can't now store incomplete or invalid Page data values in auto-save.

While looking into this I found 🐛 XB allows saving invalid date range field values Active , but I think this because date range validation for this only happens in the field widget

tedbow → created an issue.

I added a test on 3519992-only-convert-layout that passes locally but fails on 0.x

I created 🐛 ApiLayoutController::buildPreviewRenderable unnecessarily call \ClientDataToEntityConverter::converts causing errors Active to solve the smaller problem

Turns out it is little more complicated because the entity is also update during convert() so we don't just care about the return value

tedbow → created an issue.

I tried the steps to reproduce. #5 seemed to imply you could get the path access problem described by just

Grant the access administration pages permission to the anonymous user.

(from the steps to reproduce)

For me /xb/node/1/editor was accessible but UI didn't actually work because the client requests got 403s

Here is what I did

1. installed the standard profile
2. added all XB permissions to "content editor" role and 'access administration pages'
3. Removed the 2 "Path" permission from the "content editor" role
4. created node
5. went /xb/node/1/editor
6. the UI load without the "path" field
7. changed the title
8. click "publish all"

This works fine. I debugged \Drupal\experience_builder\ClientDataToEntityConverter::setEntityFields during this process and path is NOT sent back in $entity_form_fields.

I find way to get a similar problem.
You have to have 2 users. 1 user with field edit access on path and 1 without.

If the user with field edit access opens the XB editor and makes and changes a value for path will get in the auto-save.
Then if the user without field edit access opens the same node in XB then they will get an error.

but they get the on error on \Drupal\experience_builder\Controller\ApiLayoutController::get not post. This is because
\Drupal\experience_builder\Controller\ApiLayoutController::buildPreviewRenderable is called and that calls \Drupal\experience_builder\ClientDataToEntityConverter::convert which checks the field access to path because it is auto-save not because it was rendered in the field.

It would probably have the same problem on \Drupal\experience_builder\Controller\ApiLayoutController::post but I can't get it not error before that in this situation

We could try to get around this by passing a flag \Drupal\experience_builder\ClientDataToEntityConverter::convert like $no_field_edit_check. Because in the case of buildPreviewRenderable should we be checking edit access? At that point they are not editing they just need to see the preview. If another user has edited a field they don't have access then they should still effects in the preview(if any).

I think another problem this points out though is even if we figured a way around this. What happens if a user with field edit access first uses XB on node/1 and edits the path and then a user without field edit access edits the node in XB.

Right now calling \Drupal\experience_builder\Controller\ApiLayoutController::buildPreviewRenderable() with $updateAutoSave = true just calls sets entity_form_fields in the auto-save to what was on the form for the user. therefore it would wipe out any field values the user doesn't have access to edit

Ideally we would somehow merge the fields the user doesn't have access to
So if

1. user 1, has path edit access, user 2 does NOT have path edit access
2. user 1 uses XB with node 1 and sets path to /my-page
3. user 2 uses XB with node 1 and edits a different field
4. user 1 uses XB with node 1, should still see there /my-page change and user 2 field change
5. user 2 uses XB with node 1, should see the field 2 change
6. user 1 uses XB with node 1 clicks publish all, all changes should be saved(because they have complete access)

Right now set 3 would fail, user 2 could not use XB on node 1 after user 1 edited path.
If we got around that 4 would fail because user 2's use of XB would remove path from entity_form_fields

I think we could probably work on this now regardless of 🐛 [Meta] Selective Publishing and Reverting Active

I think 5 maybe a different issue.

In \Drupal\experience_builder\ClientDataToEntityConverter::setEntityFields() we try to filter out fields and only update the ones that were shown on the form.
In \Drupal\experience_builder\ClientDataToEntityConverter::checkPatchFieldAccess, which `setEntityFields` calls don't throw the AccessException if the user has view access and the field has not changed.

so from what you describe something in there is not working.

From the summary

Update ApiLayoutController::post() to perform field-level access checking; but ONLY check field edit access for fields that are actually being modified (this ensures that 4xx responses continue to be thrown when trying to bypass access, but would not trigger a 4xx for the inaccessible path field in the example in

So if understand this we are basically already trying to do this. It may be some problem specific to the path field. Wed had to put in special logic already for changed

I will debug and try to see what is happening with the path field

re #9 it is not only use the draft version of the libraries and JS files if there is an auto-save for the dependency. Hope @wim leers can confirm that since he worked on 🐛 Regression after #3500386: import map scope mismatch when auto-saved code component's JS sends a 307 Active

I think I need to update this issue based on 🐛 Regression after #3500386: import map scope mismatch when auto-saved code component's JS sends a 307 Active because I think introduced the same problem for dependencies. Shouldn't be too hard

penyaskito → credited tedbow → .

Started but probably still rough. Need to self review more

#5 sounds good to me

wim leers → credited tedbow → .

@hooroomoo Pointed out that if we don't send imported_js_components back to the client certain Component client requests will fail.

I would suggest we just send it back since it should not be the client's responsibility to know that `imported_js_components` is stored differently on the back end

✨ Store and calculate dependencies in `JavaScriptComponent` Active is in!

Thanks all!

Tests were failing. I think I have fixed them

If so, I think this issue is good to go

The test should be passing now. @wim leers if they are not feel free to assign it back to me

tests failing

balintbrews → credited tedbow → .

re #11 so are talking about disabling "Publish all changes"? If so are we planning on working on that before 🐛 [Meta] Selective Publishing and Reverting Active .

It seems like we shouldn't do the work of disabling "Publish all changes" before this because at that point the user may some changes they can't publish but if they don't select them then they would not get an error. We could choose to disable the ability to select entities they won't have permission to publish or just let them select whatever and then get an error(less ideal UX probably less work)

Anyways it would be not have to do this work over again

I think it is ready for review

tedbow → created an issue.

By updating enforced config dependencies directly we don't actually need to update the config schema

wim leers → credited tedbow → .

@wim leers would it possible to use the enforced dependencies system for this directly instead of adding js_dependencies to the entity schema?

I have added an MR that does this but I am not sure if this is how the dependencies property of config entities is supposed to be used.

If the general idea would work I can clean it up and add more testing. If it won't I can add the schema changes.

Don't need a full review just an idea if this idea is workable or not

balintbrews → credited tedbow → .

balintbrews → credited tedbow → .

One thing I don't understand is because `\Drupal\experience_builder\EventSubscriber\RenderEventsSubscriber::getSubscribedEvents` and `\Drupal\block\EventSubscriber\BlockPageDisplayVariantSubscriber::getSubscribedEvents` both subscribe to `RenderEvents::SELECT_PAGE_DISPLAY_VARIANT` but don't set a priority what guarantees that `RenderEventsSubscriber` will 2nd ? If BlockPageDisplayVariantSubscriber ran 2nd wouldn't it just overwrite RenderEventsSubscriber settings?

re #47 I am going to merge since @larowlan gave a +1

I have test coverage that proves the auto-saved version is used in the layout preview but not elsewhere.

I am not sure if the best place for the test coverage but it works

working on this

updating summary to mention xb_dev_js_blocks as I think you need that because you can't make overrides from the UI yet

re #18, I chatted with @wim leers and he is fine with this in a follow-up. I created 📌 Doc and otherwise make clearer how the 'html' preview element is generated Active

tedbow → created an issue.

@wim leers re #15 and #16

Could we actually do that in another issue? I think doing it in another issue would allow us to rename a class or 2 to make the relationship more clear, if needed. Whereas if we just do it here I think we might just do quick job and keep the whole flow a bit hard to understand

To write a test I really wanted to understand how this all works

Just noting that I just now realize how the html previewing works. I wasn't involved in these issue but wondering if we should document better how these classes relate to each other

1. PreviewEnvelopeViewSubscriber
2. PreviewEnvelope
3. XBPreviewRenderer
4. XbPageVariant
5. RenderEventsSubscriber
6. ApiLayoutController

I will re-read the class docs again and see if they are clear but I think they might be clear only because I have actually done some research to see how all these classes fit together.

I think each one this classes have good `@see` links to 1 or 2 of the other classes but I haven't yet found a place where it is clear how on the parts fit together.

Assigning to myself to add tests. I looked at this last week but confused about the number of places/ways I could test this.

Hopefully a couple days break will have given me clarity 🤞🏻

@Wim Leers this looks good to me. I updated test expectations to get tests to pass. The expectations look correct to me but you probably should review.

Just fixed 1 link in the summary

Thanks, reading through

Gave this a start, I put a couple todo's in where I was user how to get the preview context.

Manually testing it, it does work to pick up the autosaved Code component changes

I can work on it tomorrow but if someone wants to take it over before then that works too

working on this

Thanks all!

2 e2e tests but re-tested and those have now passed

Ok. all e2e tests passed beside media-library, reran and media-library just passed🎉

Re @wim leers approval I think this good since I merged 0.x. Last phpunit job is running. Will merge when/if it passes

Not currently working on this and it is not urgent.

Looks like @wim leers just set this back to Needs Work for the ES Lint fail which @jessebaker fixed, thanks!

Since our `experience_builder.info.yml` was already requiring "^10.4 || ^11.1" and we going through the trouble to remove logic specifically Drupal 10, let just require 11.1.2 because we have special logic for that version since it came 2 months and we are still in alpha stage

Thanks!

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

Reviewing

Reviewing

tedbow → created an issue.

I chatted with @laurii and we agreed that for "Remaining questions 1" option 2 is best.

this means the client will only has to send the information for the changes it wants to publish.

This will mean even if there "Review X changes" list does not have the most recent entries or isn't aware that some entities might have been deleted that is ok because we poll the server every 10 seconds. But we will also create a child issue here to ensure that "Review X changes" list request has been made in the last 10 seconds. This way if gets bunch of network errors and their "Review X changes" is very old they will a new successful request before they can publish. This will ensure they should have nearly up-to-date list

setting to Needs Review. I think I have addressed the feedback but the pipeline is failing because of 403 errors in UI and Composer jobs. I think it is related to cloudflare issues https://www.cloudflarestatus.com/incidents/gshczn1wxh74

Will try to re-run the job in a bit

@wim leers @larowlan thanks for the reviews, will address

tedbow → created an issue.

I am not sure why yet but when manually test the default values for radio buttons don't show up

Steps:

1. Add 3 fields to article, 1 integer list, using radio buttons, 1 string list using radio buttons, 1 text field

   make sure all 3 fields have default values

2. Add an article, all 3 default value should be set in the form
3. open the article in XB
4. The text field default value is correct but no values are selected for the radio buttons
5. Select values for the radio buttons, and change the value of the text field
6. reload the page
7. The value for the text field retains the updated values, the radio buttons are not selected again
8. Select values for the radio buttons(make sure not to use default), a
9. publish the node
10. The radio button fields are not correct, in case they completely different from the default and what I selected

I am going to test 0.x to see if some of these problems exist

Before we can start doing the back-end changes for this, will to decide on the 1 current item in "Remaining questions"

This is really a product question so assigning to Lauri

My vote would be option 3. We require the client to list of all entity keys regardless of whether they will be published but all have to match the auto-save has for changes that will be published.

This seems like a good balance of making sure the user is aware changes but also not stopping publishing of some items if another item is actively being edited in XB

tedbow → created an issue.

wim leers → credited tedbow → .

I think 📌 Media Library dialogs triggered from page data do not have buttons yet Active caused 🐛 "There are no users matching "/" whe publishing a node Active . Think we need to postpone on that because it changes the entity fields and will always show a change after publish

Looks like 📌 Media Library dialogs triggered from page data do not have buttons yet Active caused this.

Reminder to self, did this also reset the "sticky" property?

I am not sure why but manually testing at previous commits I figured out this issue introduced 🐛 "There are no users matching "/" whe publishing a node Active . It might also be resetting sticky

Some suggestions but think it looks pretty good!

tedbow → created an issue.

whoops, forgot this needs tests

based on @larowlan approval

Or, to put it another way, if you're updating a package you already have, minimum-stability should be ignored. It should only matter if you're adding something you don't already have.

I don't think this exactly true if you minimum-stability: beta you have an existing page that is at 1.0.0-beta1 you should not be able to update to 2.0.0-alpha1 because even though it is an update it is going from below at minimum-stability to below it

Obviously we need tests because nothing is failing now because of this. Also should be stable blocker

I wonder if we want a more complex e2e test where we say start with no media item select load a media item and then remove it again. This should return to the start state and not show a change but I wonder if it actually would

tedbow → created an issue.

I just noticed the comments at the end of \Drupal\experience_builder\Controller\ClientServerConversionTrait::convertClientToServer are wrong

// Update the entity, validate and save.
    // Note: constructing ComponentTreeStructure from `layout` and
    // ComponentInputs from `model` also included validation. But that
    // included only structural validation, not semantical validation.
    // @see \Drupal\experience_builder\Plugin\Field\FieldType\ComponentTreeItem
    // @see \Drupal\experience_builder\Plugin\Validation\Constraint\ValidComponentTreeConstraintValidator
    // @see \Drupal\experience_builder\Plugin\Validation\Constraint\ComponentTreeMeetsRequirementsConstraintValidator
    return [
      'tree' => json_encode($tree, JSON_UNESCAPED_UNICODE | JSON_FORCE_OBJECT | JSON_THROW_ON_ERROR),
      'inputs' => json_encode($inputs, JSON_UNESCAPED_UNICODE | JSON_THROW_ON_ERROR | JSON_FORCE_OBJECT),
    ];

re "Update the entity, validate and save." we don't do any of this here any more, probably did from where ever this was refactored from .

Don't know if I should make a new issue or fix or this is going away in the current issue

probably needs test to prove we now have the entity context. this should result in the client receiving entity context when there are errors