Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
Account created on 26 December 2006, about 18 years ago
  • Senior Principal Software Engineer β€” Drupal Acceleration Team at AcquiaΒ  …
#

Merge Requests

More

Recent comments

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

⚠️ This assumes that the auto-saved code component is working. If it doesn't, then it'd be the client-side equivalent of πŸ“Œ Improve server-side error handling Active , which needs its own issue. Raised this at #3499919-20: [Meta] Plan for in-browser code components β†’ .

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

πŸ“Œ Code Components should render with their auto-saved state(if any) when rendered in the XB UI Active is missing from the issue summary. Is that intentional?

It also doesn't account for the fact that auto-saves are not guaranteed to be in a working state. Which means that once πŸ“Œ Code Components should render with their auto-saved state(if any) when rendered in the XB UI Active is done, a temporarily broken code component could break all XB previews.

(For example: modifying the code to not output a slot, while the metadata does say that that slot exists.)

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

This is RTBC, but now conflicts with upstream. :) Almost!

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

SingleDirectoryComponentTest::testRewriteExampleUrl() is being added in πŸ› Adding the Image component results in a state considered invalid Active .

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

🀩

Thank you! Excellent initiative πŸ‘

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

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

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

#62 is actually what @lauriii expressed on the meeting I had with him as his ideal.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

WRT PageTemplate validation

πŸ“Œ Page title should not be required Active landed, so #3 cannot happen anymore.

#3501600-17: Split 1 PageTemplate config entity into N PageRegion config entities β†’ describes how we'll be able to drop most validation, probably all the validation that caused #3 in the first place.

So let's wait for πŸ“Œ Consider refactoring page_template into page_region(s) Active to land, and then re-assess this.

Related, at a different level

This is level up from πŸ“Œ Improve server-side error handling Active , but is comparable in intent. A "level up" in the sense that that is about individual component instances, and this is about a tree of component instances.

So let's wait for πŸ“Œ Improve server-side error handling Active to land too, and then re-assess this.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

This improves on the status quo. Let's get it in.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

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

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Consult how this change needs to be represented in the code component's config entity

The way this was implemented in ✨ Config entity for storing code components Active + ✨ Create a ComponentSource plugin for JS components Active :

  • The "code component" (JavaScriptComponent config entity) has its status set to true.
  • This causes \Drupal\experience_builder\EntityHandlers\JavascriptComponentStorage::doPostSave() to create the corresponding Component config entity.

This is already implemented and tested on the back end.

So the UI shown in the issue summary should only need to PATCH the config entity (using the /xb/api/config/js_component/…) and set status to true.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Now all this needs is a nice screencast or GIF πŸ™πŸ˜‡

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

wim leers β†’ changed the visibility of the branch 3485692-create-extendibility-proof to hidden.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

wim leers β†’ changed the visibility of the branch extension-and-example-try-vite to hidden.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

On it!

I had 2 remarks: docs (Jesse added those πŸ‘) and a @longwave review of some clever bypassing of a core API. @longwave +1'd it for here, but did confirm the need for a follow-up, which he created: πŸ“Œ Refactor library discovery for extensions Active . And seems like @effulgentsia implicitly agreed with that too in #13 :)

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

What do you mean? πŸ€”

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Limiting to XB's own Page is IMHO dangerous because it doesn't expose us to the wider reality. Which means we'll end up with known unknowns and unknown unknowns.

Furthermore, the status badge ( ✨ The status badge should indicate if there are changes to the page Active ) would otherwise fail to work correctly for article nodes.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

This is what @lauriii indicated in DM.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

You left it in a great spot, was a pleasure! :)

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

#22: that doesn't address .

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

The same problem exists for "article" nodes. Shouldn't that be fixed in the same way? That's also what I was referring to at #3498525-35: Allow XB to be used on all bundles of all revisionable, publishable content entity types β†’ .

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Added screenshot to show what it should look like instead.

I bet either:

  • XB is messing up the markup somehow
  • Olivero is expecting specific markup, including expectations of BlockPageVariant being used

The latter is easily confirmed:

  const searchWideButtonSelector =
    '[data-drupal-selector="block-search-wide-button"]';
  const searchWideButton = document.querySelector(searchWideButtonSelector);
  const searchWideWrapperSelector =
    '[data-drupal-selector="block-search-wide-wrapper"]';
  const searchWideWrapper = document.querySelector(searchWideWrapperSelector);

β€” core/themes/olivero/js/search.js

which is targeting markup literally only available in core/themes/olivero/templates/block/block--secondary-menu--plugin-id--search-form-block.html.twig πŸ˜…

And that Twig template is specifically targeting a Block config entity in the secondary_menu region with using the search_form_block plugin ID.

Conclusion

Olivero is heavily tightly coupled with the Block module. 😱

I don't see how this is possible to solve without either:

  • hardening Olivero
  • duplicating a lot of Olivero inside XB… 😱
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Yes. πŸ™ˆ Fixed.

I assumed the different states you described in #3505118-4: [PP-1] The status badge should indicate if there are changes to the page β†’ would not need to apply only to the Page content entity type, but to all edited content entities? Is that wrong?

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Also, can we link the designs in the issue summary? πŸ™

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

To clarify: I defer to @lauriii where to draw the line, but if not everything in #19 is addressed here, there will definitely need to be one or more follow-ups.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

#16 seems reasonable to consider part of this issue from a UX POV, but I don't think it makes practical sense.

Because it would need to work generically, not just for the Page content entity type. Plus, it'd require no longer hardcoding the link in the UI. Wrote up a detailed plan at πŸ“Œ Disallow deleting an XB-enabled content entity if it's currently the homepage Active .

The UI doesn't currently convey:

  • that the currently viewed page is the current homepage
  • that the currently viewed page is about to become the current homepage (thanks to auto-saving)
  • which page in the list is the current homepage

I also think that the UI strings are quite confusing, which @tedbow also pointed out in the MR.

This is what it all currently looks like:

@mglaman has indicated these aspects are missing from the design.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Added STR for using article nodes.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Oops, overlooked the cspell violation this introduced, fixing in #3506280-5: Fix tests to handle validation message improvement in Drupal 11.1.2 β†’ .

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

I overlooked the cspell violation that πŸ› Cannot redeclare hook_field_widget_info_alter() Active introduced πŸ™ˆ Let's fix that here too.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

I believe this is indeed the correct place/approach, but it should not hardcode title[0][value].

See the MR for ✨ The status badge should indicate if there are changes to the page Active at https://git.drupalcode.org/project/experience_builder/-/merge_requests/6... for the correct generalized approach, which looks like this:

      $label_field_input_name = sprintf("%s[0][value]", $content_entity_type->getKey('label'));
      $is_new = $this->contentEntityIsConsideredNew($entity_form_fields[$label_field_input_name], $content_entity_type);
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

🀯 Shouldn't phpstan-drupal be configured to not parse *.api.php files? πŸ€”

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

FYI: escalated to @effulgentsia, to decide when to get time from Ben/BΓ‘lint to work on this problem space.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Debugged this.

#5 is inaccurate AFAICT.

Debugging trail

  1. Create a fresh node with status: false. Say, node 4.
  2. Load it in Experience Builder at /xb/node/4/editor. Observe that the toggle is toggled on.
  3. Put a breakpoint in the code that generates this form: \Drupal\experience_builder\Controller\EntityFormController::form(). Observe this is the return value:

    Most importantly: #value === FALSE.
  4. In \Drupal\Core\Render\Element\Checkbox::preRenderCheckbox(), that's still the case.
  5. ⚠️ However, that maps the #return_value Form API property to the value attribute:
    Element::setAttributes($element, ['id', 'name', '#return_value' => 'value']);
    

    which results in

  6. Look at the corresponding FE code: /ui/src/components/form/components/drupal/DrupalToggle.tsx. It contains this:
      <Toggle
        checked={!!attributes?.value}
    

    … which appears accurate but isn't. The value attribute for <input type="checkbox"> is very interesting πŸ€ͺ, see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input/checkbox....

    So AFAICT the problem lies in DrupalToggle, which was introduced in πŸ“Œ Split form components into `Drupal`-prefixed behavioral wrappers and presentational components Needs work .

Looking at /node/<nid>/edit
  • checked "published" checkbox: <input data-drupal-selector="edit-status-value" type="checkbox" id="edit-status-value" name="status[value]" value="1" class="form-checkbox form-boolean form-boolean--type-checkbox">
  • unchecked "published" checkbox:
    <input data-drupal-selector="edit-status-value" type="checkbox" id="edit-status-value" name="status[value]" value="1" checked="checked" class="form-checkbox form-boolean form-boolean--type-checkbox">
    

πŸ‘† That's the markup being generated outside of XB. That's the starting point for things we do on top.

Tentative conclusion: needs only front-end changes?

So AFAICT updating DrupalToggle should work? But

-    checked={!!attributes?.value}
+    checked={!!attributes?.checked}

didn't do the trick πŸ˜…

I'm getting lost between:

  1. ui/src/components/form/components/Checkbox.tsx
  2. ui/src/components/form/components/drupal/DrupalInput.tsx
  3. ui/src/components/form/components/Toggle.tsx
  4. ui/src/components/form/components/drupal/DrupalToggle.tsx

… because all four of those (!!!) are dealing with the checked attribute πŸ˜… AFAICT only the last 2 are relevant. I changed both like indicated above, without success.

I've done the due diligence, and think it's now down to somebody who knows the Semi-Coupled theme engine well enough to finish it up. It probably takes them only minutes given the digging I've done so far 🀞

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Forgot to signal this is soft-postponed for #9.2: there is a pure BE MR that can already land and should be reviewed.

But for the FE pieces to happen, both this issue's MR and the one at πŸ› Once previewed in XB an entity with no changes will still show up in "Review x changes" Active must have landed.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

🀣 Ran into the infamous d.o bug where concurrent edits cause an issue to become unpublished! Oh irony πŸ˜†

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

#3: Regarding the odd "Published" badge behavior β†’ : agreed. For that we have πŸ› "Published" checkbox is always checked even if the entity is not Active .

#4: No, that diagram does not address @jessebaker's concerns in the 2nd and 3rd paragraphs of #3. Because this diagram refers only to interactions/states at the "XB auto-saving" level. It does not address the confusion between XB's "Publish all changes" button and the "Published" checkbox on content entities that use \Drupal\Core\Entity\EntityPublishedTrait::publishedBaseFieldDefinitions().

@jessebaker in #6 devised (πŸ€©πŸ‘) a way to achieve those 6 XB UI states ("new content", "draft", "published", "changed", "deleted", "archived"), and uses the "Published" field (\Drupal\Core\Entity\EntityPublishedTrait::publishedBaseFieldDefinitions()) in them. But that doesn't mean the UI confusion has been solved πŸ˜…

I think @jessebaker captured well in his 3 points in #6 what is still needed.

  1. A "new" content entity can only be created for Page, not for Node or anything else. Thanks to ✨ Provide a way to create a new page Active . So a general solution for this point is not realistic at this time.

    (We cannot use $entity->isNew(), because that only works for a "new in PHP runtime" check, but here the entity has already been created.)

    However, we can deduce a reasonable heuristic already I think:

    1. a label is likely to always be required (and we could make that another requirement in πŸ“Œ Allow XB to be used on all node types Active )
    2. which is why \Drupal\experience_builder\Controller\ApiContentControllers::post() generates Untitled page as the default
    3. … which is identical to t('Untitled @singular_entity_type_label', $entity_type->getSingularLabel())
    4. So let's use that as the heuristic for now!

    β‡’ Created a draft MR on this issue that adds this information to the /api/layout/{entityTypeId}/{entityId} API response, under isNew.

  2. This require πŸ› Once previewed in XB an entity with no changes will still show up in "Review x changes" Active to be fixed. I refined that earlier today, see details at #3502902-6: Once previewed in XB, a content entity with no changes incorrectly appears in "Review x changes" β†’ .
    β‡’ Postponing this issue on that one.
  3. Since ✨ Provide an API for listing available pages Active , this has been available for Page content entities. But here we need it for:
    • all content entity types supported by XB (not a generic solved problem, see πŸ“Œ Allow XB to be used on all node types Active , where I summarized the discussion here in comment 33 and credited you both!)
    • not for a list of content entities, but specifically for the currently edited content entity. This part is missing.

    β‡’ Created a draft MR on this issue that adds this information to the /api/layout/{entityTypeId}/{entityId} API response, under status.

Conclusion

  • The BE MR I just created can already land and would fix points 1+3.
  • Point 2 would be fixed by ✨ Provide an API for listing available pages Active
  • After both are in, the remainder of this would be a trivial pure front-end issue.

P.S.: AFAICT this is not blocked on πŸ› "Published" checkbox is always checked even if the entity is not Active , but it sure makes things confusing πŸ˜…

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

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

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Failing tests due to recent upstream core changes. The failure is bogus.

Glad to see this done.

And nice diffstat too: +12, -54 😁

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Per @lauriii's diagram for "XB publishing states" at #3502902-4: Once previewed in XB, a content entity with no changes incorrectly appears in "Review x changes" β†’ and @jessebaker's concrete interpretation of that in concrete content entity terms at #3502902-6: Once previewed in XB, a content entity with no changes incorrectly appears in "Review x changes" β†’ , I think there's one more requirement: to be eligible, the content entity type must implement \Drupal\Core\Entity\EntityPublishedInterface too.

Not 100% certain though. So asking @lauriii to confirm.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Yep, green! πŸ₯³

That's why I also pushed the secondary clean-up bit (point 2 under "proposed resolution").

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

The MR at πŸ“Œ Allow CMS Author to delete a page Postponed is 99% ready, and it does fix this too, and includes test coverage for it πŸ‘

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Approved the BE pieces of this MR. πŸ‘

Still needed:

  1. failing tests, see #10
  2. the (trivial) openapi.yml feedback has not yet been addressed
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

The new e2e test is failing:

    cy:command βœ”  findByLabelText	Page options for Empty Page
      cy:command βœ”  click	
      cy:command βœ”  contains	div.rt-BaseMenuItem, Delete page
      cy:command βœ”  click	
      cy:command βœ”  contains	button, Delete page
      cy:command βœ”  click	
        cy:fetch ➟  GET http://localhost/web/session/token
                    Status: 200
        cy:fetch ➟  DELETE http://localhost/web/xb/api/content-delete/xb_page/2
                    Status: 204
      cy:command ✘  wait	@deletePage
        cy:fetch ➟  GET http://localhost/web/xb/api/content/xb_page
                    Status: 200
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

AFAICT it really is as simple as #5.

Select tests pass locally. Hopefully the MR confirms it for the full test suite.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Isn't this just

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Only do auto-saves for entities that are different than their saved (live) versions.

Alternatively: compute the data_hash for the saved entity and check if it's different.

Having read \Drupal\experience_builder\Controller\ApiLayoutController::buildPreviewRenderable() (which creates the auto-save entry for the edited content entity using client-side representation) and \Drupal\experience_builder\Controller\ApiLayoutController::get() (which computes client-side representation to boot the XB UI), it AFAICT should be possible to just compute the hash, using something like:

diff --git a/src/AutoSave/AutoSaveManager.php b/src/AutoSave/AutoSaveManager.php
index 0cb1419c..322b93cb 100644
--- a/src/AutoSave/AutoSaveManager.php
+++ b/src/AutoSave/AutoSaveManager.php
@@ -5,6 +5,8 @@ namespace Drupal\experience_builder\AutoSave;
 use Drupal\Core\Cache\CacheTagsInvalidatorInterface;
 use Drupal\Core\Entity\EntityInterface;
 use Drupal\Core\Entity\TranslatableInterface;
+use Drupal\experience_builder\Controller\ApiLayoutController;
+use Drupal\experience_builder\InternalXbFieldNameResolver;
 
 /**
  * Defines a class for storing and retrieving auto-save data.
@@ -27,6 +29,22 @@ class AutoSaveManager {
     return $this->tempStoreFactory->get('experience_builder.auto_save', expire: $expire);
   }
 
+  public function hash(EntityInterface $entity, ?array $data): string {
+    // Generate a hash for the saved entity.
+    if ($data === NULL) {
+      $field_name = InternalXbFieldNameResolver::getXbFieldName($entity);
+      $tree = $entity->get($field_name)->first();
+      $data = [
+        // @todo ::buildRegion is not currently callable, but *could* be.
+        'layout' => ApiLayoutController::buildRegion('content', $tree, $model),
+        'model' => $model,
+        // @todo ::getEntityData is not currently callable, but *could* be.
+        'entity_form_fields' => ApiLayoutController::getEntityData(),
+      ];
+    }
+    return \hash('xxh64', \serialize($data));
+  }
+
   public function save(EntityInterface $entity, array $data): void {
     $key = $this->getAutoSaveKey($entity);
     $auto_save_data = [
In \Drupal\experience_builder\Controller\ApiPublishAllController::__invoke we have logic to convert auto-save data to entities ready for validating/saving. We may want to move this logic into the auto-save manager(or somewhere else) so we can then compare the entities to the saved version. If there no changes to the saved versions we would then not create the auto-save entry and if there already is one delete it.

AFAICT \Drupal\experience_builder\Controller\ApiPublishAllController::validateExpectedAutoSaves() does not look at the saved data at all currently? So I don't think that logic helps.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

So this needs to undo what \Drupal\Component\Utility\Html::escape() did, interesting! πŸ˜…

This is 99% correct AFAICT, but this would make it 100%: https://git.drupalcode.org/project/experience_builder/-/merge_requests/6... β€” asking @longwave to confirm.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Reviewed the back-end bits.

Who should review the FE bits? Or, do you feel this needs no further review, @bnjmnm?

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

@lauriii opened πŸ“Œ Page title should not be required Active for #10, and I fixed it.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

@lauriii: I proposed to do something similar for the "messages" block over at #3501600-17: Split 1 PageTemplate config entity into N PageRegion config entities β†’ .

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Adding to #8: πŸ“Œ Page title should not be required Active landed too, so the "validation across the component trees for all regions in a given theme" just became simpler.

To recap, there are 3 special kinds of blocks: "main content", "page title" and "messages".

  1. as of πŸ“Œ Page title should not be required Active , the page title no longer gets special treatment β€” thanks to @lauriii's response to #3505872-5: Page title block should not be required β†’ .
  2. per #8, the "main content" one no longer needs any special treatment either, at least not at the validation level. Because we'll hardcode that block being the sole one in the content region.
  3. that leaves only "messages"! I personally think it merits getting almost the same treatment as the page title one, with one difference: if \Drupal\experience_builder\Plugin\DisplayVariant\PageTemplateDisplayVariant didn't encounter >=1 such block during rendering, then XB's page variant should just place it at the top of the content region automatically, which is exactly what BlockPageVariant does:
        // If no block displays status messages, still render them.
        if (!$messages_block_displayed) {
          $build['content']['messages'] = [
            '#weight' => -1000,
            '#type' => 'status_messages',
            '#include_fallback' => TRUE,
          ];
        }
    

If @lauriii agrees with that, then we'll have eliminated all of the validation challenges in this MR! It'll allow this MR to delete \Drupal\experience_builder\Plugin\Validation\Constraint\ComponentTreeMeetsRequirementsConstraint::$nested and related code.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

That test looks like a nice start! πŸ‘ πŸ˜„

#2: seems like the expected Workspace config entity does not exist yet, since that NULL can only come from

Workspace::load($id)
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

WFM! :D Go ahead and merge as far as I'm concerned :) But … I think it'd be better/simpler to first get tests to pass on the current MR? πŸ˜‡ That keeps the focus on the most important thing?

Just walked @lauriii through what I did in #58 (at the end of a meeting about something else). He VERY much does not like the https://example.com prefix πŸ˜… He agrees with the overall approach, but just explicitly doesn't like that prefix.
I respectfully disagree, but given @lauriii has a much stronger front-end development past than I do, I defer to him 😊 Done: https://git.drupalcode.org/issue/experience_builder-3501902/-/commit/457...

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Re-testing again. Locally it passes. 😬

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

I believe I see a solution for

  1. agree upon the syntax for letting an SDC specify a default image, knowing it CANNOT be a resolvable image URL, and perform the necessary transformation to a resolvable image URL

in ~100 LoC (nutshell: require example values to start with https://example.com, add new method to ComponentSource to rewrite such example values to resolvable URLs):

The only thing that's still missing: updating \Drupal\experience_builder\ComponentMetadataRequirementsChecker::check() to require examples values for type: string, format: uri|iri|uri-reference|iri-reference to start with https://example.com.

See the second branch:

  1. making it work in ~100 LoC: https://git.drupalcode.org/issue/experience_builder-3501902/-/commit/3dc...
  2. getting rid of the auto-created Media entity for the 600x400.png sample image: https://git.drupalcode.org/issue/experience_builder-3501902/-/commit/bf1...

P.S.: this also paves the path for examples for images to become optional if min/max dimensions are specified, then we could auto-generate a relevant sample image. See ✨ Feature request: Ability to specify minimum image resolution for an image upload Active .

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

I was thinking about this some more last night.

When and why do we need this FallbackPropSource? We need it for:
preview purposes, never storage purposes
never for booleans, integers, or numbers
never for any string format other than one of the uri + uri-reference + iri + iri-reference ones: https://json-schema.org/understanding-json-schema/reference/type#format
… and even then, only when they must be resolvable/fetchable, to make certain tags work: <img>, <video> …

So AFAICT these special needs do not apply to anything else. The most common "resolvable URL" case is links (<a href="<SDC PROP HERE">link text</a>), but there it doesn't apply either, because the URL does not have a visible impact: a dummy URL would do just fine for preview purposes!

Although, come to think of it: a "call to action" component would want to have a required link, without wanting that link to ever be saved? So, perhaps there is a difference in preview impact (image/video URLs vs link URLs), but the saving impact is the same: nobody wants links pointing to https://example.com/campaign, right? :D

Given all that, I think that UrlPreviewPropSource would be a better name, that is much narrower and hence better captures both the need and when it is appropriate.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Thanks for raising this β€” and looks very cool!

How do you imagine this would work? Which SDC props should accept this?

  • Is it an existing shape (type + format …)?
  • Is it a new one? The icon equivalent of $ref: json-schema-definitions://experience_builder.module/image?
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

#53 πŸ˜±πŸ™ˆπŸ€£ gitnoob And you got it character-by-character identical except for one line :O

#55: Not opposed to that, but it does mean that the literal originally reported issue does not get fixed. How about we just do 2 MRs for this single issue? And just merge this first MR … first? Avoids issue queue overhead.

The issue title has been lacking precision since the start, which adds to the confusion. I'll think of a better title with a fresh head in the morning.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Thanks for confirming.

Re-testing.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

✨ Auto-save code components Active is in, and responded on the MR πŸ‘

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

✨ Auto-save code components Active is in. πŸŽ‰

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

See review on MR; AFAICT this is blocked on ✨ Auto-save code components Active . Notified @tedbow over at #3500042-37: Auto-save code components β†’ an hour ago.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

For #37.

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Keeping issue summary relevant. The current MR should do only one thing, but there's a second part to this…

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

@larowlan is out this week 🏝️

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

We're threading very closely to this over in the MR for πŸ› Adding the Image component results in a state considered invalid Active .

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

Thanks, I was similarly confused.

@lauriii: Are you saying it's okay for the page title to not appear anywhere? Because if so, that's a trivial change πŸ˜„

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

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

πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί
  1. 😱 Excellent sleuthing! πŸ‘
  2. Yep, I've noticed there's plenty of missing OpenAPI validation. But looking at it more closely again: why is that not triggering \League\OpenAPIValidation\PSR7\Exception\NoPath?! 😱
  3. 😭

This makes it all the more important to have proactive OpenAPI maintainers on this project, because it's supposed to save us time, not cost us time. And if the infrastructure doesn't let us do

It's feeling more and more that OpenAPI is less and less worth it?! 😭 Feels like we need to spend some dedicated time to make it actually robust.

Production build 0.71.5 2024