Preview entire page not just content area

Full page without toolbar etc. The XB Content area slot works like it should. IDK if we should do more within this issue scope or just get this in so it's easier to try out different page editing approaches.

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

Looks great!

E2E tests pass, which means this does work.

The only test that fails is \Drupal\Tests\experience_builder\Kernel\ApiPreviewControllerTest, and that fails because it directly tests the controller, which now no longer returns a JsonResponse, but a render array. The transformation to a JSON response object now happens in that new main content renderer 👍

The new base test class that 📌 Adding or editing a Page brings user into Experience Builder not entity form Active adds introduces the infra that would allow ApiPreviewControllerTest to largely remain unchanged.

I suggest cherry-picking that base class into this MR — whichever one lands first introduces the infra, the other MR will be able to become simpler 👍

This blocks 📌 Add support for global regions Active .

Seems fine to switch the response to HTML. I know error handling was one of the concerns mentioned in Slack, but JS error checking/reporting is still captured and displayed properly in the try/catch and the preview itself has Drupal message regions which makes it extra capable at error reporting.

FYI, I opened 📌 Improve server-side error handling Active to improve the error handling. I don't think we should render the error message in the rendered preview but instead there should be a specific interface in XB that handles them. It sounds like the JSON to HTML change might make that more challenging.

longwave → changed the visibility of the branch 3489302-preview-entire-page to hidden.

I would like to propose that we reconsider switching to HTML as the response format and stick to JSON.

As @lauriii mentioned in #11 📌 Preview entire page not just content area Active , error messages shouldn't appear as part of the preview. This is in line with our aspiration to keep the rendered preview as intact as we can, so it truly represents what end users will see.

In order to handle errors outside of the preview, we do need them in a JSON response. @longwave's suggestion to set the responseHandler option to content-type for the endpoint generated by RTK Query would solve this elegantly: the response would be parsed according to the Content-Type header from the response, so it's possible to mix HTML and JSON responses.

While that is certainly a solution, I'm concerned that RTK Query's abstraction hides the fact that we would be creating an unusually complex client requirement. Our implementation happens to solve it easily through an abstraction, and while I don't pretend to foresee that there will be other clients consuming this API endpoint, it's probably a good idea not to rule that out, even just for the sake of designing a better API.

I think it's overall a better API design to keep a consistent response format.

What is the problem that we're trying to solve with switching to HTML response? Debugging was mentioned on Slack. What would be the exact scenario?

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

Other people on this issue are making compelling arguments to go back to the JSON response. I switched it out yesterday because I thought it effectively preserved the error handling but it looks like that isn't the case. The motivation wasn't that strong, tbh and not opposed to switching back.

Let's revert to using JSON responses.

#13 + #15: could an alternative be to:

1. always send a HTML response
2. … but in case of an error due to an invalid component tree (typically in the inputs, so props for an SDC-sourced Component, settings for a Block-sourced Component), make the status code of the response 422: Unprocessable Content (and probably an empty response body, but this is less important)
3. … and in case of plain failure (e.g. incorrect logic in a block plugin, partially failed code deployment …), make the status code of the response 500.
4. update the client to verify the response status code is 200, and for anything else, provide a fallback UX as @lauriii described in 📌 Improve server-side error handling Active

(For point 3: I described in detail in #3485878-7: Component previews should NEVER be able to render PHP errors, should fall back to a meaningful error instead → how literally any error at any point during the preview rendering could be guaranteed to be caught to ensure a great UX in XB.)

Since @balintbrews had a strong argument in favor of JSON responses, I'm especially curious what he thinks about #17. 😇

We should improve the error handling so that errors that happen during API requests, never end up in the preview.

— @lauriii at #3485878-6: Component previews should NEVER be able to render PHP errors, should fall back to a meaningful error instead →

👆 This is why I'm not sure we even need any error details for previews specifically, unlike what @balintbrews describes in #13.

#19 📌 Preview entire page not just content area Active :

That quote from @lauriii is what I also expressed in #13 📌 Preview entire page not just content area Active .

My interpretation is that we don't intend to display errors as part of the generated preview, but that doesn't mean we don't intend to display errors in the XB app somewhere when generating the preview goes wrong. Likely in a toast, as mentioned in 📌 Improve server-side error handling Active .

The question is how granular we would like to be with these errors. If not so much, i.e. we're happy with two different kind of generic errors, then #17 📌 Preview entire page not just content area Active works! I guess many other scenarios can be expressed with HTTP codes, so this can even scale to more errors.

I will say, I don't get why we are putting so much emphasis on this when have a system worked out for JSON responses with error handling, already implemented, used by many other endpoints. I'm probably failing to understand some benefit of this potential change. 😊

The only real argument is that the response is a full HTML page and nothing more, and HTTP already allows us to return this trivially by setting the Content-Type header.

We can still return error details in JSON if we need to. One thing I'm not sure how to handle yet: if there are warnings during preview, but not errors that prevent rendering, where and how should they be surfaced?

Warnings should also be displayed outside the preview. We could render them in a toast or a similar component.

In which case if we can have both HTML response *and* warnings, we need to represent it via JSON.

It looks like the XB preview is now showing global regions but the actual main content is slightly different between the real site and XB. Is there still an additional step needed to render the node content using the display mode? FWIW, if this is difficult to do, it would be fine to handle this in a follow-up issue.

XB:

Actual page:

#26:

That's because this MR is not rendering the main content block \Drupal\system\Plugin\Block\SystemMainBlock. This MR pretends the component tree for the current Node or Page is the main content, and then lets that "main content" be decorated by blocks — i.e. it still uses BlockPageVariant.

This is good enough for the Page entity introduced by 🌱 [Research] Landing page integration Active , because \Drupal\experience_builder\Entity\PageViewBuilder::alterBuild() only renders the component tree.

For Node (and other content entities, and actually when editing the PageTemplate and looking at an arbitrary route), we need to reuse the original route (for editing a content entity that'd be its $content_entity->toUrl('canonical') route/link template) but replace/override/intercept the route parameter with one that reflects the values in the XB UI of the XB field (the component tree) and all other fields.

• For content entities being edited through XB that means calling the controller with an EntityInterface object with all field values dynamically overridden compared to the saved (published) values. (For Node: \Drupal\node\Controller\NodeViewController::view() with a NodeInterface object.)
• For arbitrary routes viewed in XB (to edit the regions for the active theme's PageTemplate — see 📌 Add support for global regions Active ), we don't need to do anything: it'd just be pass-through.

I think updating ApiPreviewController for that is indeed out-of-scope here. I think ApiPreviewController must use the previewed route's controller, and override canonical content entity routes' received entity object is an accurate albeit long issue title. 🤓 😄

The changes this MR makes are simple and focused. #26 is out-of-scope here, and #28 provides an outline of how that could work. Let's create a new issue for that, with #28 as the initial implementation proposal.

This MR allows the PageTemplate functionality that 📌 Introduce an XB `PageTemplate` config entity Active introduced to become visible in previews in the XB UI! 🚀

Why?

As soon as a PageTemplate config entity is created for the default theme, as of this MR, the preview will use the component trees stored in the PageTemplate config entity, because ApiPreviewController no longer calls BareHtmlPageRenderer. Instead, it returns a render array ("the main content for the controller"), and lets the default Drupal page rendering process handle wrapping that main content render array into a full page. By default, that is BlockPageVariant. And once a PageTemplate config entity exists, it'll be \Drupal\experience_builder\Plugin\DisplayVariant\PageTemplateDisplayVariant.

If you're interested in how that works, see \Drupal\Tests\experience_builder\Functional\PageTemplateVariantTest::test().

The next step is to actually start creating/populating a PageTemplate config entity. That's what 📌 Add support for global regions Active is for, which this blocks.

Thank you @wim leers! Based on #28, it totally makes sense to do it in a different issue.

Turns out #28 pre-emptively answered most of what @longwave raised in #3489899-8: [PP-1] Add support for global regions → ! 😃

Hm … perhaps the E2E test failures are legitimate? 🤔 Last full green CI pipeline was Nov 26. 📌 Adapt E2E tests to work with auto-save Active landed Nov 27. I bet that's where the root cause lies, somewhere… 🙈

The UI + OpenAPI changes are minimal and @jessebaker and @wim leers have been active here so going ahead and merging this.

This copied \Drupal\Tests\experience_builder\Kernel\ExperienceBuilderTestBase and broke any test using it when dependencies are installed due to how \Drupal\Tests\experience_builder\TestSite\XBTestSetup operates.

@mglaman what's broken exactly? Wim asked for the test base to be copied over in #6.

Because you cannot add dependencies to be installed on the base test. The test this issue added runs the site setup install script, which manually installs modules. It assumes a non-kernel test environment.

I suggest cherry-picking that base class into this MR — whichever one lands first introduces the infra, the other MR will be able to become simpler 👍

— me in #6

Of course, that assumed that there would not be any changes made to the base class, and changes were made. So it's a bit more complicated now.

This MR updated ApiPreviewControllerTest like so:

- class ApiPreviewControllerTest extends KernelTestBase {
+ final class ApiPreviewControllerTest extends ExperienceBuilderTestBase {

… but the

    (new XBTestSetup())->setup();

line was already present in the test, @mglaman.

I do agree that is a bit atypical (calling another class' :setup() method from a test's ::setUp()), but it doesn't seem impossible for 📌 Adding or editing a Page brings user into Experience Builder not entity form Active to overcome? 🤔

I have to completely rewrite ApiPreviewControllerTest due to its usage of XBTestSetup. Unless we skip defining dependencies in the test base class, which is completely useless. Perhaps it should have all gone into a trait as I was wondering in my MR.

#40++ — I had the same thought 😅 Will get that done.

Did #40 in https://git.drupalcode.org/project/experience_builder/-/merge_requests/4... 👍

Created the missing follow-up: 📌 [later phase] ApiPreviewController must use the previewed route's controller, and override canonical content entity routes' received entity object Postponed . Based on @lauriii in #26 and yours truly in #28.

Automatically closed - issue fixed for 2 weeks with no activity.