Preview entire page not just content area

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 #3487075: Adding or editing a Page brings user into Experience Builder not entity form 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 #3489899: Add support for global regions.

FYI, I opened #3485878: Server-rendered component instances should NEVER result in a user-facing error, should fall back to a meaningful error instead (+ log) 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.

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

As @lauriii mentioned in #11, 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?

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 #3485878: Server-rendered component instances should NEVER result in a user-facing error, should fall back to a meaningful error instead (+ log)

(For point 3: I described in detail in #3485878-7: Server-rendered component instances should NEVER result in a user-facing error, should fall back to a meaningful error instead (+ log) 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: Server-rendered component instances should NEVER result in a user-facing error, should fall back to a meaningful error instead (+ log)

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

#19:

That quote from @lauriii is what I also expressed in #13.

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 #3485878: Server-rendered component instances should NEVER result in a user-facing error, should fall back to a meaningful error instead (+ log).

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 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. So to me this is the more semantically correct thing to do.

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 from completing, 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 #3482259: Landing page integration: new content entity type for unstructured content, 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 #3489899: Add support for global regions), 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 #3478537: Introduce an XB `PageTemplate` config entity 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 #3489899: Add support for global regions 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: Add support for global regions! 😃

Hm … perhaps the E2E test failures are legitimate? 🤔 Last full green CI pipeline was Nov 26. #3481736: Adapt E2E tests to work with auto-save 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 #3487075: Adding or editing a Page brings user into Experience Builder not entity form 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: #3491701: [later phase] ApiLayoutController must use the previewed route's controller, and override canonical content entity routes' received entity object. Based on @lauriii in #26 and yours truly in #28.