Refactor ApiLayoutController into 2 sub-class to support Content Templates

The MR !1439 Draft is based on a PoC by Ted and not all of it applies.
To the best of my knowledge, Ted worked with the assumption that left sidebar menu would list Drupal display modes grouped by entity types (currently only Nodes) and bundles. The Controller Ted worked on assumed that Template entity would be loaded based on combination of Entity Type (Node for now), it's bundle and view mode. For Drupal view modes with no existing Template entity a GET request would create one and place it into AutoSave. This is handled here: ApiTemplateLayoutController:: getContentTemplate().

Currently the UX design is being updated and we will have dedicated UI elements for creating Template entities.
These are not final designs, but they show the current work in progress:

According to latest designs, the "Template list" in the sidebar on the left will contain actual Template entities and their IDs and UX elements for creating and deleting Templates.
This means that "finding matching Template entity by entity_type + bundle + view_mode" is no longer required. We also won't be displaying Drupal view modes in UI, only actual Template entities. Functionality of "create Template entity during GET request if it doesn't exist" is also no longer required.

After a discussion with @wim.leers we decided that there are no more reasons left to split ApiLayoutContorller. Instead this issue should focus on updating existing ApiLayoutContorller code to support Template entities.

Creating, listing and deleting of Template entities via HTTP requests can be done in a separate issue using existing ApiConfigControllers infrastructure.

@f.mazeikis re #7 . If we create a saved, not just auto-saved, ContentTemplate entity in the UI as soon as the user wants to start working on the template won't that mean that the template will take effect immediately outside of XB? It seems that `\Drupal\experience_builder\EntityHandlers\ContentTemplateAwareViewBuilder::buildComponents` will just use the template if it exists. So as soon as we create the actual ContentTemplate config entity it would be an empty template but I think it would be used to render any entity for that bundle, meaning thy would should as blank.

Of course we could add some sort status to have the template not take effect until publishing but then we would need to set this when publishing in `\Drupal\experience_builder\Controller\ApiAutoSaveController::post` like we do for `StagedConfigUpdate` entities

if ($entity instanceof StagedConfigUpdate) {
          $entity->applyUpdateOnSave();
        }

If we are going to do this at some point we might want to create an interface for config entities that should enabled on publish or we are going keep having more entity type specific logic in ApiAutoSaveController::post

But also I am not really sure why we need to create the actual entity just because the user is given the UI for creating a new template type. The user doesn't need to know if it is just an auto-saved version at that point. Though I guess it might make the listing more complicated it could make the publishing simpler. Maybe ContentTemplates are just a unique type of config entity where we know that ID will not change and that is why they could work with just an auto-saved version before first being published🤷

I chatted with @wim leers about #7 and why we want to create the template entity first

I pushing up some changes.

I pushed a little further.

re #7 and #10. I realize now the other reason I originally refactored ApiLayoutController into a base class with 2 extending classes was that for ContentTemplates we need to also pass a "previewEntity" which would be an example entity of the bundle that would be used for the preview. The new MR doesn't cover that case. It might make sense to continue the work on the new MR https://git.drupalcode.org/project/experience_builder/-/merge_requests/1458 but then add `ApiContentTemplateController extends ApiLayoutController`. So in this case we won't need the base class of my original MR but we need a special `ApiContentTemplateController` because it needs the extra info about the preview entity and it would also need it's own routes to pass in the entity ID of the preview entity.

@thoward216 if you want to assign this back to yourself and work on it you can unassign me feel free, otherwise I will work on the preview entity problem tomorrow.

tedbow → closed merge request !1438

tedbow → closed merge request !1439

I created 📌 Refactor(or attempt to) all or most ApiLayoutController*Test classes to also test ContentTemplate entities Active and added a todo this in the code. I did add test coverage but it is minimal right now. It covers GET, POST, PATCH and publishing of Content Templates.

I don't think we should do all the test coverage here because the Controller not in used yet for content templates and this issue blocks front end work such as ✨ Render template and support component operations in preview canvas Active . I would rather unblock the front-end and try to not have duplicated test coverage for content and content template entities in the follow-up

I think this is ready for review. See Issue summary for why this does not yet support dynamic prop sources and the follow-up issues where will be handled

Assigning to myself to create a new MR against "canvas"

tedbow → closed merge request !1458

#15++ for deferring tests to a follow-up to unblock the front-end work 👍

I have some ideas for addressing the concerns I raised, and which @tedbow previously raised in #12. I'll push a few commits.

The outline of the changes makes sense to me though! 😊

I raised 3 main concerns, and wanted to not solve all implementation details, but just set this MR on a path towards addressing 'em:

1. logic living in controllers that should be handled by the routing system (faster & safer, but also easier to reason about and hence maintain long-term)
2. clearly incorrect OpenAPI specs, that were somehow passing 🤯
3. unclear why completely omitting the required presence of DynamicPropSources is the right approach, when 2 alternatives seem viable

Overview of changes made prompted by review

I wanted to make sure I left y'all with a pattern that would work instead of just complaining about what was incorrect. So I did, and here's the overview:

1. Commits 133f7a829d35eb997c60dcb670be55c424ce0d13 through bc0fc76e956f4069f125f7840acecdb345a4580f: use the routing system as intended.
2. Commit 8915df2f923fb6263b5425bb126a18ec26bb45ef: update OpenAPI spec to fix the (many 😅) inaccuracies.
3. Commit 4a40d5b8e4a0a23c5cd0e7be2f52ad78b04f42cc: the prior commit caused widespread failures because the previously incorrect OpenAPI spec was masking lots of problems, so this commits reverts all other invalid changes too
4. Commit d111b4483c9c929a3f5d7bf0a5e1ca0249cca8f0: thanks to OpenAPI spec being correct as of 2 commits ago, it becomes impossible to generate a content template layout GET request without a preview entity without angering OpenAPI, so to keep the original test coverage, we must use the X-NO-OPENAPI-VALIDATION request header
5. phpunit is green again!
6. Commit c53921fcb03478ee3df6d53d67945e14048e7dd6: to address the 3rd main concern: simply ignore "presence" requirements" when the component tree is empty — details: https://git.drupalcode.org/project/canvas/-/merge_requests/9/diffs#note_...

For all commits: see corresponding CI runs that demonstrate the reason for each subsequent commit.

Next/remaining work

1. Repeat the pattern I've introduced for canvas.api.layout.get + canvas.api.layout.get.content_template + associated OpenAPI specs for the canvas.api.layout.patch and canvas.api.layout.post routes.
2. Review the other feedback on the MR, most important: why not isPublished for ContentTemplates?

This became much cleaner while I was sleeping, thanks! 😄

While re-reviewing, I wanted to make sure I understood all long-term consequences. 2 things stood out:

ComponentTreeEditAccessCheck

One of the things I was most worried about is how this MR is improving ComponentTreeEditAccessCheck: I was worried about A) us being able to understand/maintain this going forward given it's become quite complex, B) what else we might have forgot. So, I added assertions to verify my current understanding.

In doing so, I discovered my understanding was incomplete: a single test failed: the translation test coverage that @tedbow added >1 year ago in 📌 Allow Experience Builder fields to support Asymmetric and Symmetric translations Needs review .

For a moment, I was afraid this actually uncovered a bug in \Drupal\canvas\Storage\ComponentTreeLoader::load(), but turns out it's entirely explained by how Drupal core's ContentEntityBase::getTranslatedField() works 👍 Run the test that failed without the expanded assertions, and use a debugger to observe what happens to gain the same understanding.

Hardcoded entity:node upcasting for {preview_entity} route parameter

@tedbow questioned whether this was okay, I +1'd it because in 1.0 we'll only support Node-targeting ContentTemplates.

It'll be up to 📌 Allow XB to be used on all node types Active to expand. So I added a @todo explaining this is supposed to use a ParamConverter. But in having written that … I realized it actually might be trivial to implement? Turns out it took me ~25 mins — 2 fewer @todos, and it's now similar to how core's entity param converting logic-with-error-handling works 👍

That means this issue now is not introducing technical debt/future follow-ups. It's completely solving all net new things, using best practices.

Remaining Q

One question remains: shouldn't ContentTemplate layout API responses also contain isPublished?.

That can easily be a trivial follow-up MR on this same issue though, so … merging!

Next step: actually using the preview entity: 📌 [PP-2] Accept Dynamic Props in ApiTemplateLayoutController for content templates Postponed , which this now unblocks. See y'all there!

Now that this issue is closed, please review the contribution record.

As a contributor, attribute any organization helped you, or if you volunteered your own time.

Maintainers, please credit people who helped resolve this issue.