#stable blocker

The last js component failures are because 🐛 Don't raise exception when an enum value is missing from meta:enum Active avoids any way to provide meta:enums, overriding them even when set.

This is a problem when using \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\JsComponent::buildEphemeralSdcPluginInstance for validating js components as sdc components.

🤩🤩🤩🤩 And … the XB UI actually starts to come fully alive! 🧟‍♂️

/me typed "u", return, cmd+space, click "Save" … and didn't spot that it didn't select 😅

At the moment, you can't even access XB if you don't have permission to edit XB pages so if you can access XB then you will always have at least one option in the menu.

Is that true? What if I can create/edit articles?

In any case, it won't be true anymore very soon, because 📌 Add permission for "Use Experience Builder" Active will definitely allow you to load XB if you cannot use Pages at all, assuming you have an editable XB field on article nodes.

Thanks for adding that robustness — I won't test again, I bet you tested it thoroughly 😄👍

I'm not sure it's possible to actually get into those situations (yet?!) but good catch. At the moment, you can't even access XB if you don't have permission to edit XB pages so if you can access XB then you will always have at least one option in the menu.

Anyway I've added some more robustness around the UI when people have no permissions or can delete things but don't have any other items in the menu which should keep this looking tidy in the future no matter what configuration of permissions people end up with!

Very nice end result, and @penyaskito showing off with code elegance rarely seen! 🤓🤩🙇

Paired with @penyaskito on this. We got closer.

1. Original state to start editing a page/article without anything in auto-save, the /xb/api/v0/api/layout/node/1 response contains this after placing a new Heading SDC:
   
2. Then, upon changing the default value for the text prop, the client incorrectly sends this
   

   Note how the original prop value remains present (left) and the value I actually entered is sent with the wrong key (right).

This is what's causing things to not work. This appears to be happening in buildPreparedModel(), which is what generates the value for form_xb_props.

Did thorough manual testing. No problems found.

Tip for fast manual testing: change

          'permissions' => [
            'globalRegions' => $this->currentUser->hasPermission(PageRegion::ADMIN_PERMISSION),
            'patterns' => $this->currentUser->hasPermission(Pattern::ADMIN_PERMISSION),
            'codeComponents' => $this->currentUser->hasPermission(JavaScriptComponent::ADMIN_PERMISSION),
            'contentTemplates' => $this->currentUser->hasPermission(ContentTemplate::ADMIN_PERMISSION),
            'publishChanges' => $this->currentUser->hasPermission(AutoSaveManager::PUBLISH_PERMISSION),
          ],
          'contentEntityCreateOperations' => $this->getContentEntityCreateOperations(),

to

          'permissions' => [
            'globalRegions' => $this->currentUser->hasPermission(PageRegion::ADMIN_PERMISSION),
            'patterns' => $this->currentUser->hasPermission(Pattern::ADMIN_PERMISSION),
            'codeComponents' => FALSE,
            'contentTemplates' => $this->currentUser->hasPermission(ContentTemplate::ADMIN_PERMISSION),
            'publishChanges' => $this->currentUser->hasPermission(AutoSaveManager::PUBLISH_PERMISSION),
          ],
          'contentEntityCreateOperations' => [],

etc. to test any combination of things.

Found two bugs, both in the same area:

Merged in upstream after 🐛 PHPUnit Next Major tests failing Active landed; resolved several conflicts.

wim leers → closed merge request !793

Bumping to priority. Because without this, we'll need to keep doing awkward dances/updates and we'll never match all image fields.

IOW: without this, we won't be able to match any image fields or image media on existing Drupal sites that do not allow AVIF uploads. That's bad! XB users will be (rightfully!) baffled and frustrated that they can't use their years worth of uploaded images.

+1 to this plan

Finally got to the bottom of a number of issues that were causing a lot of friction in writing the Playwright tests for this work. All resolved now and ready for another round of review.

The summary on the MR is the most up to date place for all info on this work.

Tagging needs review. We need to verify #note_543156 and #note_545354 is the expected outcome.

This should do it: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...

The change I made in #21 introduces a new prop: display_width. It uses type: integer, minimum: 1.

This caused 2 test failures:

Drupal\Tests\experience_builder\Kernel\Config\ComponentTest::testComponentAutoUpdate
Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for experience_builder.component.sdc.experience_builder.video with the following errors: 0 [active_version] The version da2ffec495563dd6 does not match the hash of the settings for this version, expected facbb275ecb9699a.

+

Drupal\Tests\experience_builder\Kernel\MediaLibraryHookStoragePropAlterTest::testUniquePropSchemaDiscovery
Drupal\Core\Config\Schema\SchemaIncompleteException: Schema errors for experience_builder.component.sdc.experience_builder.video with the following errors: 0 [active_version] The version 44d35b0af1837caf does not match the hash of the settings for this version, expected f828aa2fb0fe4c76.

Note that '' is literally the default value for max in \Drupal\Core\Field\Plugin\Field\FieldType\IntegerItem::defaultFieldSettings(). So the instance settings we're validating:

[
  "max": "",
  "min": 1,
]

Root cause:

1. Component::save() eventually calls
2. \Drupal\Core\Config\Config::save(has_trusted_data: FALSE), which eventually calls
3. \Drupal\Core\Config\StorableConfigBase::castValue(key: 'versioned_properties.active.settings.prop_field_definitions.display_width.field_instance_settings.max', value: '')
4. which contains this beauty:
   

           // Special handling for integers and floats since the configuration
           // system is primarily concerned with saving values from the Form API
           // we have to special case the meaning of an empty string for numeric
           // types. In PHP this would be casted to a 0 but for the purposes of
           // configuration we need to treat this as a NULL.
           $empty_value = $value === '' && ($element instanceof IntegerInterface || $element instanceof FloatInterface);
   
           if ($value === NULL || $empty_value) {
             $value = NULL;
           }
   

   😬😬😬😬😬😬😬😬

🎉

Approved by proxy: since @larowlan +1'd the BE.

Unfortunately:

Rebase failed: Rebase locally, resolve all conflicts, then push the branch. Try again.

@tedbow: can you rebase locally & merge once it's green again? 🙏

wim leers → credited bnjmnm → .

I trust Lee & Ben, so going ahead and merging! 🚢

Still had one major concern about the new experience_builder:video SDC, because it conflated:

1. intrinsic width: width of the video itself — a fact
2. display width: the width the content author would like the video to be displayed at — a subjective decision

So, made that a reality:

1. StaticPropSource matching was already working 👍
2. DynamicPropSource matching was not yet working, in part due to core bugs. While this also doesn't work in HEAD for multi-bundle image media references, it's important that the shapes we define are known to be matchable to avoid painting ourselves into a corner by the time we work on ContentTemplates in the near (post-beta1) future. So, fixed that.
   There are still remaining caveats though, but that's definitely a pre-existing problem, and the most critical part (the video URL) is working 👍 Follow-up: 📌 Add `IntegerSemanticsConstraint` Active .
3. The approach @f.mazeikis took for oEmbed videos was based on a suggestion by @lauriii: add a new Twig function. That's pragmatic, and makes things work, but … it also makes the subsequent issue 📌 [PP] FE implementation of Video Prop Active impossible. So the solution is to do what we've done for the file_uri field type's url property or better yet: the wholly new url property we just added to the link field type in ✨ [PP-1] Make link widget autocomplete work (for uri and uri-reference props) Postponed .

The latter is what I'm currently working on, and is very unlikely to get finished in the next 1.5 hour (before the next and final sprint before beta1 starts).

So, being pragmatic: creating an MR to land just local video shape matching support, to unblock 📌 [PP] FE implementation of Video Prop Active with at least local videos, because as of finishing point 2 above, that's completely working. Here it is in action in both the all-props SDC and the new video SDC:

wim leers → credited balintbrews → .

Discussed with @balintbrews, he's on board 👍

I going to RTBC this based on @bnjmnm MR approval and @larowlan saying the backend changes look good

So, my concern in one sentence: we must be able to provide a long-term update path for code components, which includes not breaking any code components created on beta1.

Implementation outline

My interpretation of what @effulgentsia and I are saying:

1. client-side to pass drupalSettings.xbData.v0 dependency information however it sees fit — note this COULD be all of the data initially, and just more granularity later! What matters is that we know which code components use this at all, and which version they're on. Which is exactly what the pragmatic approach in #3531249 (i.e. in HEAD did)
2. update \Drupal\experience_builder\Entity\JavaScriptComponent::updateFromClientSide() to consume it
3. remove the parsing logic in ::getRequiredXbDataLibraries() and it with a new config entity property on the JavaScriptComponent config entity type to store it instead
4. update the logic in ::getAssetLibraryDependencies() to use that new config entity property

IOW: the exact same pattern that 🐛 Components sourced from JsComponent are missing source component cacheable metadata Active did, just not stored as an enforced config entity dependency.

Choice: beta blocker 🆚 stable blocker + update path by relying on heuristics

This means that this can indeed be a , not a , but only if the logic in getRequiredXbDataLibraries() can be updated to work with reasonable reliability, because then we can transform whatever that logic computes to explicit dependency information in an update path.

Per #3531249-18: Populate data in `drupalSettings` to enable simple use cases in dynamic code components → :

The front-end API will consume this settings with methods (not decided yet) like getPageData() (for title and breadcrumb) and getSiteData() (for base URL and branding).

That's why I think detecting the presence of such calls in the JS is doable in a saved config entity, which means that an update path after beta1 would be feasible.

So: I propose to indeed keep this as a , but I might have missed something 😇

Step 1 here is the front end providing that dependency information, because what 📌 Populate data to drupalSettings to enable Dynamic Code Components Active introduced (and what's quoted in the issue summary) is the best the back end can do, since it cannot parse JS.

#2++

That's literally what I wrote at https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...

We'll need something similar for code components fetching data via HTTP (either local aka Drupal-served URLs or remote URLs), because that is what needed to in the future add server-side prefetching of data without breaking existing code components or leaving them behind.

All of this is basically to say: dependency information is crucial! Without dependency information, the backwards compatibility break risk assessment is "unavoidable, will be painful and require manual intervention", whereas otherwise it can be "avoidable, will either be transparent or precise instructions can be provided".

IOW: it's a matter of future DX for code component creators and update path feasibility for XB maintainers. See point 3 of my comment #12 in the issue that landed this → .

@larowlan here it was not unlimited cardinality though — just a specific cardinality (3) with only a subset (2) populated. If cardinality was 7, then not 1 but 5 empty items would've been added!

Thanks for the updates here folks, the change to remove the extra field.

Cross-linking a core issue about that behavior

I discussed this with @lauriii and according to him it's sufficiently rare to have CSS that's finicky to whether the wrapper tags are there or not that he's okay with this being an area where we break BC between beta and RC.

adding blocker tag as this blocks 📌 For config entities add client support conflict detection via the `autoSaves' key Active and 🐛 Add rudimentary conflict prevention to the Config Auto-save end-point Active

I think what we'll want to do here is something similar to how we do first-party imports, where the XB front-end parses the JS to determine the dependencies and includes that information in the request payload when saving the component. We might need to add a new key to the js_component config schema to store the info if dependencies doesn't work for depending on Drupal libraries.

I tested this and with this MR the values aren't set for any prop, not only enum, even when the SDC has no enums.

Thanks, that's helpful. I suggest you switch to something else for a bit, it sounds like this has been a frustrating ride :/

🎉

tedbow → changed the visibility of the branch 3525829-user-without-access-no-clobber to active.

tedbow → changed the visibility of the branch 3525829-user-without-access-no-clobber to hidden.

What I've found is that this might be that we need some changes in the client UI that I didn't spot yet.

A symptom:

1. Add a heading component (has 2 enums)
2. Add a druplicon component
3. Edit the heading style + element props (enums)
4. Click on the druplicon. The form fails to load.
5. Back to heading, the style + elements have the defaults instead of our changes.

The new failures are now about

-    15 => 'revision_log[0][value]',

… which seems fine, because the user didn't enter anything for those, it was just auto-generated 👍

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

I hope I got it c🤞

AFAICT my analysis in #27 was both right and wrong 😅

It was right in that I did spot a genuine bug or at least something that could/should be simplified. I was a bit overzealous with my early return, and in fixing that, the original symptom analyzed in #27 reappeared! 😬

WTF?!

Note that

    $test_node = $this->createTestNode([
      $date_field => [
        [
          'value' => $date->setTimezone($utc)->format(DateTimeItemInterface::DATETIME_STORAGE_FORMAT),
        ],
        [
          'value' => $date->setTimezone($utc)->modify('+2 days')->format(DateTimeItemInterface::DATETIME_STORAGE_FORMAT),
        ],
      ],
    ]);
    self::assertSame([], self::violationsToArray($test_node->validate()));

passes, but then the next line fails:

$result = \Drupal::classResolver(ApiLayoutController::class)->get($test_node);

Why?!

Because
ApiLayoutController::get() calls
\Drupal\experience_builder\Controller\ApiLayoutController::getFilteredEntityData()
which builds the entity form
and then filters it using \Drupal\experience_builder\Controller\ApiLayoutController::getFilteredEntityData()

The building of the entity form is what introduces "invalid" data, which really is just this: \Drupal\Core\Field\WidgetBase::formMultipleElements()'s

      // Add a new empty item if it doesn't exist yet at this delta.
      if (!isset($items[$delta])) {
        $items->appendItem();
      }

🤪

Yes, it's an empty field item that DateTimeFormatConstraintValidator is crashing on!

Arguably that validator is far too brittle. But also arguably, all this infrastructure should match how entity forms work, and entity forms do not validate empty field items — they use \Drupal\Core\Field\FieldItemListInterface::filterEmptyItems() to strip those away, see \Drupal\Core\Field\WidgetBase::extractFormValues():

      // Let the widget massage the submitted values.
      $values = $this->massageFormValues($values, $form, $form_state);

      // Assign the values and remove the empty ones.
      $items->setValue($values);
      $items->filterEmptyItems();

The sole failure is:

    Client Data To Entity Converter (Drupal\Tests\experience_builder\Kernel\ClientDataToEntityConverter)
     ⚠ Convert with 0
     ✔ Convert with 1
    
    1 test triggered 1 PHP warning:
    
    1) /builds/project/experience_builder/web/core/modules/datetime/src/Plugin/Validation/Constraint/DateTimeFormatConstraintValidator.php:22
    Undefined array key "value"
    
    Triggered by:
    
    * Drupal\Tests\experience_builder\Kernel\ClientDataToEntityConverterTest::testConvert#0
      /builds/project/experience_builder/tests/src/Kernel/ClientDataToEntityConverterTest.php:94

But oddly, this is happening in this call chain:

1. buildPreviewRenderable(updateAutoSave: FALSE)
2. ClientDataToEntityConverter::convert([..., entity_form_fields: []], validate: FALSE)
3. … which then proceeds to do $original_entity_violations = $entity->validate();, when AFAICT there's absolutely no reason for it, because both entity_form_fields == [] and validate == FALSE 🤔

I think that we can add an early return in ClientDataToEntityConverter::convert() when $entity_form_fields is empty — because then all that was needed is converting the client-side representation of the XB component tree to the server-side representation. Nothing else.

(Until 🐛 Page status changes from "Published" to "Changed" even when no actual changes are made Active , that method always returned $updated_entity_form_fields, which is why this is much easier to see now.)

Did that, and it seems to work.

P.S.: this seems tangentially related to 📌 [PP-1] Return validation errors from LayoutController::post and ::patch Active .

So the correct behavior is to not only record those form violations, but also make sure the entity in the auto-save state isn't mangled beyond repair. And to only do this for invalid deltas as we don't want to wipe new deltas or valid updates to existing deltas from the auto-save entry.

This seems an unavoidable consequence of 🐛 Page status changes from "Published" to "Changed" even when no actual changes are made Active switching from "pure client-side representations stored in auto-save" to "validate le entities stored in auto-save".

But AFAICT #24 does mean that *certain* invalid values would not be recorded in auto-save, which in theory is equivalent to data loss. But … pragmatically speaking, it's A) a tiny bit of information (an invalid delta of a field), B) client-side validation on the entity form likely would've prevented this too. IOW: it affects only "very" invalid data. That seems acceptable 👍

Scenario that needs to be addressed with this fix :

• Create code component named {code01}
• Create code component named {code02}
• Import code01 to code02
• Delete code01 able to delete code01
• Code02 gives error as below

Ok was able to trace this down, it was this change in behaviour in 🐛 Page status changes from "Published" to "Changed" even when no actual changes are made Active

This was another 'fun adventures in debugging submitting form api forms over JSON' 😭

tl;dr if there are form validation errors, we can't assume what's in the entity field value is anything near valid.

In the case of #23 and #19 it is indeed an array with keys 'date', 'time' and 'object' as seen in DateTime element value callback, and not a string as the entity-api expects.

So the correct behavior is to not only record those form violations, but also make sure the entity in the auto-save state isn't mangled beyond repair. And to only do this for invalid deltas as we don't want to wipe new deltas or valid updates to existing deltas from the auto-save entry.

I've added extra tests and handling for that.

Yes ideally we could do this client-side, and I went down that path but without much joy.

Reverting that behavior change feels like the more robust approach.

Found the cause of #19 - and I think it exists in HEAD, and did even before 🐛 Page status changes from "Published" to "Changed" even when no actual changes are made Active because its this in inputBehavioursEntityForm in inputBehaviors.tsx

const validateNewValue = (e: React.ChangeEvent, newValue: any) => {
    // @todo Implement this.
    return { valid: true, errors: null };
};

i.e. we don't do any client-side validation for entity-forms.

In HEAD we don't see this because each POST is loading the default entity.
Before 🐛 Page status changes from "Published" to "Changed" even when no actual changes are made Active we didn't see it because we didn't convert until we hit publish.

We see it now because we load the last submitted auto-save entity during POST

The simplest way to trigger this is to add a date timestamp field to the entity form, delete the time and note you get a client-side (browser) error for the required field. Then edit the date field and this triggers a POST to the server, even though other fields on the form are invalid.

I think the fix will be to prevent any submissions while there are HTML5 errors in the form, no different to what any other Drupal form submission does.

@tedbow's response regarding using the hash makes sense - thanks

#21 excellent points - I've:

• Renamed getEntityFields to getFilteredEntityFields as that's a private method so it should convey what it is doing
• Updated openapi.yml
• Added a docblock to ApiLayoutController::get

I wanted to make sure I understood why this works and I think it is bit difficult to understand.

1. \Drupal\experience_builder\Controller\ApiLayoutController::get() calls $entity_form_fields = $this->getEntityData($entity);
2. then ApiLayoutController::getEntityData has
   

   // Filter out form values that are not accessible to the client.
       $values = self::filterFormValues($form_state->getValues(), $form);
   

So to understand this you actually read the code line by line. There are no function comments for ApiLayoutController::get(), ApiLayoutController::getEntityData or EntityFormTrait::filterFormValues. Also the name getEntityData to doesn't imply that we would be doing any filtering

Seems like at the very least <code>$entity_form_fields = $this->getEntityData($entity); should have comment explaining it is getting just the data accessible to the user. otherwise you really have to dig down.

Also in our openapi.yml file we have paths./xb/api/v0/layout/{entityTypeId}/{entityId}.get.responses.200.content.application/json.schema.properties.entity_form_fields

 entity_form_fields:
    type: object
    description: The full entity data.

which is no longer true, if it ever was

I think if I having trouble keeping track of how this works I think others will be too.

Assigning back to @larowlan because I answered his question on the MR

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

#19: that sounds a lot like 📌 Provide visibility into field types with different matching SDC prop shapes depending on field storage settings Active ? 🤔

re #15 yes, ✨ Publish all changes within a database transaction to allow rollbacks Active looks good! sorry you probably asked me before it was committed

Adding stable block here because I see 📌 [PP-2] Create UI for selective discarding of changes Active blocker and then that would be all the issues done

Will leave assigned to myself to read and make sure the current summary is up-to-date

I think @tedbow's Q should be answered by test coverage. 😇 This is a security matter, so here it makes sense to err on the side of caution.