🇺🇸United States @tedbow

working on the fix. I think for now we should allow e2e test to opt of aggregation

@wim leers actually I have another idea that I push up in a MR to use `EntityConstraintViolationList`. See my comments on the as to why it is hard to test right now. If you think the MR is good idea though I could look at testing it.

🐛 Global AssetLibrary should render with its auto-saved state (if any) when rendered in the XB UI Active is fixed

🎉

Thanks everyone!

re #12 I created 🐛 Create loadAutoSave() helper function Active as follow-up for 2) and refactored for 1)

tedbow → created an issue.

2 e2e test fails, passed locally. PHPUnit tests passed

re #13.2 that seems to be known side effect of 📌 Compile Tailwind CSS globally for code components Active . It will be fixed by 🐛 Global AssetLibrary should render with its auto-saved state (if any) when rendered in the XB UI Active

Postponing on #3508937 because it will be easier to test after that

phpunit test fails

1. re #12

   Instead of enabling aggregation for XbPageVariantTest I enabled aggregation in \Drupal\Tests\experience_builder\Functional\FunctionalTestBase::setUp which is the base test that XbPageVariantTest extends. This way all our functional tests will run with CSS aggregation on not just XbPageVariantTest.

   I since found the only test that fails in this situation is \Drupal\Tests\experience_builder\Functional\AssetLibraryAttachmentTest because we are checking for CSS and JS file paths in the HTML, so I disabled aggregation just for this test

2. During manual testing I was finding weird behavior where the code component do not look like do in the editor preview. This affects user 1 also with aggregation off. I have tried testing on 0.x and still see the problem. so I am investigating that.

re #8

also enable CSS + JS aggregation for \Drupal\Tests\experience_builder\Functional\XbPageVariantTest

Since XbPageVariantTest extends FunctionalTestBase enabled it in FunctionalTestBase so we don't write functional tests that only work aggregation off. But if there is fail I can switch to XbPageVariantTest is needed.

Doing the manual testing now

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

see #3521819-12: Hovered preview for JS components in library not working anymore → for small MR that reverts a bit of 📌 Add a route for PATCHing both a config entity and its auto-saved version together Active that solve the problem on this issue

Since 📌 Compile Tailwind CSS globally for code components Active might take a while to fix I propose we just revert the small change ui/src/features/code-editor/dialogs/AddToComponentsDialog.tsx that was made in 📌 Add a route for PATCHing both a config entity and its auto-saved version together Active with todo to remove this in 📌 Compile Tailwind CSS globally for code components Active . This doesn't solve the problem I documented in 2) of the "Proposed solution" but that was an existing problem

@thoward216 nope I assume this should be able to be fixed here. Just related to the other problem. in manual testing of this MR I do see it fixing the problem of hover preview not being updated

Looks good to be. Needs /ui approval

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

It sounds like 📌 Compile Tailwind CSS globally for code components Active will solve this

Lets postpone on that and see if this is still an issue

crediting myself and hooroomoo

You can test this by following the directions in the summary and then requesting

1. http://exp-d-core.test/xb/api/v0/config/js_component/component_name This should have empty slots(and compiled_js)
2. /xb/api/v0/config/auto-save/js_component/component_name this should have slots filled

If you git checkout e72f0d0b538ef6abefae89f0a8f14409dfd3ec03^ which is the commit before 📌 Add a route for PATCHing both a config entity and its auto-saved version together Active

1. http://exp-d-core.test/xb/api/v0/config/js_component/component_name should have slots filled (and compiled_js)
2. /xb/api/v0/config/auto-save/js_component/component_name this should have slots filled

The difference is the client now never actually sends a request to /xb/api/v0/config/js_component/component_name with slots filled.

re #10

Let's start with a failing test. I'm assuming this is a back-end bug.

I don't think so. I think the root cause is the same as 🐛 Hovered preview for JS components in library not working Active .

basically before 📌 Add a route for PATCHing both a config entity and its auto-saved version together Active when clicked "add to components" it would send up the whole entity including compiled_js and slots and props. After #3519634 the client never actually makes a request to add the slots to the save config and probably the "layers" menu was always using the saved config and never using the auto-save. It just wasn't noticeable because you can't change slots after you add the component to the library.

This makes the problem in 🐛 JS component slots don't appear in the preview canvas until published Active worse because since it no longer showed the saved config compiled js for hovering and since auto-save doesn't work either there is no preview on hover

I chatted with @hooroomoo about this. Updating the summary with more details

re the 1st step in the summary

Create a new code component and make a JS change

I don't think it makes difference if you make a JS change or not the hover will not work

Looking into this

Needs work for https://git.drupalcode.org/project/experience_builder/-/merge_requests/9...

Since 🐛 Regression after #3500386: import map scope mismatch when auto-saved code component's JS sends a 307 Active just landed another merge request I test this again because it does relate to auto-save. Both situations I described in #20 still work

Tested locally and the 1 e2e test that failed passed

Investigating.

@mayur-sose, small point but the video shows the steps in slightly different order than the summary

From the summary

1. Ensure xb_dev_js_blocks as this provide block overrides which you can't make from the UI yet
2. Open XB and open code editor for one of the overrides (e.g., branding)
3. Make changes to the override
4. Drag the dynamic component being overridden to the page
5. Observe that the modified overrides are not applied
6. Publish the project, and then note that the override changes finally appear

In the video, step 4, dragging the component on, was done before step 2 and 3, editing the override.

In any case both orders should work.

I tested both orders and they worked for me. I have attached screen recordings.

It very well could be that something in the last 18 days since @mayur-sose posted his screen recording that some other commit fixed this.

@mayur-sose can you test this again with both editing the override and then dragging it on, as the summary says and dragging it on and then editing it.

Thanks

Marking back as RTBC since I just changed to variable names for clarity. When tests are green I will merge

To be practical, I am going to implement my last MR comment then mark it back it as RTBC, since it is only naming comment changes.

Merged. I already add the test coverage I mentioned

setting to RTBC based on f.mazeikis MR approval

Going to mark as fixed as 🐛 Click publish shortly after making a change can lead to error Active should make it better.

If we still want to improve the messaging further, lets just reopen this issue

Ok. Let's get this and then figure how we can make it better

The problem is actually in JavascriptComponent

See my comment on MR about test coverage

tedbow → created an issue.

This is still marked as "Needs design" but I think it would be ok to merge the existing MR and then improve. The updated message would at least give the user something they can do that I think should resolve the problem. We can then improve

think is good. Thanks @meghasharma. Hopefully e2e test will pass otherwise because it is component-operations.cy.js is probably random fail

Updated the summary to indicate that I think we would already check field access and we should have test proving this

Assigning to @larowlan for review

Updated the summary

re #24 and #25 I ran the failing tests locally on 0.x and the MR branch.

publish-validation.cy.js consistently passes for me locally on both branches

component-operations.cy.js consistently passes for me on 0.x and consistently fails on the MR branch 😢. I haven't looked into why yet

@hooroomoo have you run the tests locally? Would be good to double check if someone else gets the same result

wim leers → credited tedbow → .

Manually testing this fixes the problem. I only test with adding global CSS and it also fixes the similar problem in 0.x

Not sure if there is a better way to solve this but it seems logical that the .draft libraries where the CSS and JS files point to URL that dynamically send back the contents should not be aggregated.

Unassigning myself since it I am not working on it. I see a couple tests that have @covers \experience_builder_library_info_build() we should probably up them to assert the preprocess change(if that is indeed the correct fix

For #16.2 I think we figured out that it is NOT an issue with the current MR.

which makes sense because in 0.x we would be deleting the auto-saved entry of the component if you just sent the status, while in 0.x we would just updating the status in the auto-save entry

@hooroomoo can you confirm?

Investigated #16.1 with @hooroomoo , created a new issue 🐛 Auto-saved Javascript Components CSS changes do not work with CSS aggregation Active ,

going to give it a try

tedbow → created an issue.

Investigating

Looks good!

Looks good!

Yep. will look into the test fail

code looks good. I also manually tested with and without the fix. I saw the problem and the it is fixed!

the phpunit tests for some memory issue, so rerunning

Looks good! will merge on green tests

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

@balintbrews I have pushed a new MR that tries to nest the imports like you mentioned

I am not getting this error

Warning: Array to string conversion in
Drupal\Core Render Htm|ResponseAttachmentsProcessor->process-Htm|HeadLink() (line 416 of core/lib/Drupal/Core/Render/Htm/Respon-seAttachmentsProcessor.php).

$attached['html_head'][] = [$element, 'html_head_link:' . $rel . ':' . $href];

because $href is my list of imports not sure why this happening yet

@wim leers yes the intention was to handle the other things in the another MR. will update issue summary title tomorrow

But … there's no field-level access checking in this MR

See #14 I think it is bit more complicated, that is why I want to get the route level access done first. also I think we should get 🐛 ApiLayoutController::buildPreviewRenderable unnecessarily call \ClientDataToEntityConverter::converts causing errors Active before we handle the other field level access because I think that bug makes it more complicated to test

Ok. ready for review again. There still isn't test coverage for the 3 new public methods added to JavaScriptComponent. They are test indirectly. so we could add the tests in a follow-up

Created 🐛 Click publish shortly after making a change can lead to error Active as idea how we might make this situation in #9 less likely

Needs review for the general idea. Then we could clean it up or just scrap it

tedbow → created an issue.

@lauriii thanks. Yeah that is the case that I could reproduce manually but not all the time.

Ok. looks good to me

@lauriii since you made this issue did you run into these messages while using XB or just reviewing?

If you ran into it using XB what circumstances where they under? Locally where there was only one person using XB or where there might have been more?

. I was noting this is something new in #11 not looking to block it. Though we could still add the summary that it is something new and worth the risk to make Package Manager and auto-updates usable on shared hosting

Hostinger currently makes composer 1 and composer 2 available. Composer 1 is composer and Composer 2 is composer2

Did you have to do which composer2 to actually find the path?

So I had to download the phar file and point the project to there to work.

It sounds like if you have to do this already is updating settings.php such a big deal.

Trying to think of admin who has all the skills get composer at a path and/or figure out the current path, where updating settings.php is such a big deal, especially since we get tell you abouting updating settings.php in the status check message

I am not against this just trying figure out the profile of the person we are helping with this

Setting to Needs Review for the current merge request https://git.drupalcode.org/project/experience_builder/-/merge_requests/929

This only does the route level entity access but it seems like this would easy to get in and then we can take on the more complicated parts

Does drupal core have any other cases where you can set the path of executable from the UI? Just wondering because I guess you could do ton of damage if you could set this whatever you want

Also the summary doesn't mention rsync

🤞🏻 tests will pass

test failing

wim leers → credited tedbow → .

I think we could do 1) under Proposed Resolution, Update experience_builder.api.layout.post to use _entity_access, now. It wouldn't be delayed by my other comments or 🐛 ApiLayoutController::buildPreviewRenderable unnecessarily call \ClientDataToEntityConverter::converts causing errors Active

See MR comment

@phenaproxima thanks for this. I think it is great improvement!

@catch thanks for the detailed review.

I still see a lot of variables that start with `$stage*` or are just `$stage` . and function names like `\Drupal\Tests\package_manager\Kernel\PackageManagerKernelTestBase::createStage`

Since this issue just deals with class names and the follow-up 📌 Update all comments and documentation in Package Manager to refer to "sandboxes" and related terminology Postponed do we want to expand this issue update variable and function names or create a follow-up for that too?

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