- πͺπΈSpain penyaskito Seville π, Spain πͺπΈ, UTC+2 πͺπΊ
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. - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
π€©π€©π€©π€© And β¦ the XB UI actually starts to come fully alive! π§ββοΈ
-
wim leers β
committed 7fd7f19b on 0.x authored by
jessebaker β
Issue #3516641 by jessebaker, wim leers, penyaskito, mglaman, larowlan:...
-
wim leers β
committed 7fd7f19b on 0.x authored by
jessebaker β
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
/me typed "u", return, cmd+space, click "Save" β¦ and didn't spot that it didn't select π
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
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
Page
s 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 ππ
- π¬π§United Kingdom jessebaker
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!
-
wim leers β
committed dc60e4f5 on 0.x authored by
penyaskito β
Issue #3532454 by penyaskito, wim leers, tedbow, larowlan: Add field...
-
wim leers β
committed dc60e4f5 on 0.x authored by
penyaskito β
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Very nice end result, and @penyaskito showing off with code elegance rarely seen! π€π€©π
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Paired with @penyaskito on this. We got closer.
- 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:
-
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 forform_xb_props
. - Original state to start editing a page/article without anything in auto-save, the
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
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:
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Merged in upstream after π PHPUnit Next Major tests failing Active landed; resolved several conflicts.
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
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.
- π¬π§United Kingdom jessebaker
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.
- πͺπΈSpain penyaskito Seville π, Spain πͺπΈ, UTC+2 πͺπΊ
Tagging needs review. We need to verify #note_543156 and #note_545354 is the expected outcome.
-
wim leers β
committed 2932feb8 on 0.x
Issue #3524130 by f.mazeikis, wim leers, lauriii: Define JSON Schema $...
-
wim leers β
committed 2932feb8 on 0.x
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
This should do it: https://git.drupalcode.org/project/experience_builder/-/merge_requests/1...
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
The change I made in #21 introduces a new prop:
display_width
. It usestype: 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 formax
in\Drupal\Core\Field\Plugin\Field\FieldType\IntegerItem::defaultFieldSettings()
. So the instance settings we're validating:[ "max": "", "min": 1, ]
Root cause:
Component::save()
eventually calls\Drupal\Core\Config\Config::save(has_trusted_data: FALSE)
, which eventually calls\Drupal\Core\Config\StorableConfigBase::castValue(key: 'versioned_properties.active.settings.prop_field_definitions.display_width.field_instance_settings.max', value: '')
- 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; }
π¬π¬π¬π¬π¬π¬π¬π¬
-
tedbow β
committed 9f99e82f on 0.x authored by
jessebaker β
Issue #3526907 by tedbow, jessebaker, larowlan, bnjmnm: For content...
-
tedbow β
committed 9f99e82f on 0.x authored by
jessebaker β
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
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? π
- πΊπΈUnited States bnjmnm Ann Arbor, MI
wim leers β credited bnjmnm β .
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
I trust Lee & Ben, so going ahead and merging! π’
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Still had one major concern about the new
experience_builder:video
SDC, because it conflated:- intrinsic width: width of the video itself β a fact
- display width: the width the content author would like the video to be displayed at β a subjective decision
So, made that a reality:
- @wim-leers opened merge request.
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
StaticPropSource
matching was already working π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 onContentTemplates
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 .- 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'surl
property or better yet: the wholly newurl
property we just added to thelink
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 newvideo
SDC: - Issue created by @wim leers
- π³π±Netherlands balintbrews Amsterdam, NL
wim leers β credited balintbrews β .
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Discussed with @balintbrews, he's on board π
- πΊπΈUnited States tedbow Ithaca, NY, USA
I going to RTBC this based on @bnjmnm MR approval and @larowlan saying the backend changes look good
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
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:
- 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) - update
\Drupal\experience_builder\Entity\JavaScriptComponent::updateFromClientSide()
to consume it - remove the parsing logic in
::getRequiredXbDataLibraries()
and it with a new config entity property on theJavaScriptComponent
config entity type to store it instead - 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.The front-end API will consume this settings with methods (not decided yet) like
getPageData()
(for title and breadcrumb) andgetSiteData()
(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 π
- client-side to pass
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
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.
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
#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 β .
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
@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!
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
Thanks for the updates here folks, the change to remove the extra field.
Cross-linking a core issue about that behavior
- πΊπΈUnited States effulgentsia
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.
- πΊπΈUnited States tedbow Ithaca, NY, USA
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
- πΊπΈUnited States effulgentsia
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. - πͺπΈSpain penyaskito Seville π, Spain πͺπΈ, UTC+2 πͺπΊ
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.
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
Thanks, that's helpful. I suggest you switch to something else for a bit, it sounds like this has been a frustrating ride :/
-
tedbow β
committed f49278a0 on 0.x authored by
larowlan β
Issue #3525829 by wim leers, larowlan, tedbow, effulgentsia, lauriii,...
-
tedbow β
committed f49278a0 on 0.x authored by
larowlan β
- Issue created by @isholgueras
- πΊπΈUnited States tedbow Ithaca, NY, USA
tedbow β changed the visibility of the branch 3525829-user-without-access-no-clobber to active.
- πΊπΈUnited States tedbow Ithaca, NY, USA
tedbow β changed the visibility of the branch 3525829-user-without-access-no-clobber to hidden.
- πͺπΈSpain penyaskito Seville π, Spain πͺπΈ, UTC+2 πͺπΊ
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. - π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
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.
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
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();
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
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:
buildPreviewRenderable(updateAutoSave: FALSE)
ClientDataToEntityConverter::convert([..., entity_form_fields: []], validate: FALSE)
- β¦ which then proceeds to do
$original_entity_violations = $entity->validate();
, when AFAICT there's absolutely no reason for it, because bothentity_form_fields == []
andvalidate == 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 .
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
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
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
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.
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
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
ininputBehaviors.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.
- π¦πΊAustralia larowlan π¦πΊπ.au GMT+10
@tedbow's response regarding using the hash makes sense - thanks
- πΊπΈUnited States tedbow Ithaca, NY, USA
I wanted to make sure I understood why this works and I think it is bit difficult to understand.
\Drupal\experience_builder\Controller\ApiLayoutController::get()
calls$entity_form_fields = $this->getEntityData($entity);
- 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
orEntityFormTrait::filterFormValues
. Also the namegetEntityData
to doesn't imply that we would be doing any filteringSeems 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.
- πΊπΈUnited States tedbow Ithaca, NY, USA
Assigning back to @larowlan because I answered his question on the MR
Automatically closed - issue fixed for 2 weeks with no activity.
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
#19: that sounds a lot like π Provide visibility into field types with different matching SDC prop shapes depending on field storage settings Active ? π€
- πΊπΈUnited States tedbow Ithaca, NY, USA
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
- π§πͺBelgium wim leers Ghent π§πͺπͺπΊ
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.