I think the next steps here are to perform some EXPLAIN queries against this data in JSON and string format and see whether we can query it efficiently. If there isn't an efficient way to query the multi-value nature of the field then I think a separate table makes more sense. SqlContentEntityStorage wraps hook_entity_insert and hook_entity_update in a transaction so we can ensure writes to that table are atomic. We can keep the logic @wim leers has written in the current MR but mark the fields as computed and use those hooks to update the dependencies table. We can make use of the new OO hooks to put the update/insert hook logic in a service and write tests for it. The service an make use of things like just-in-time table creation like we have for other services in core (i.e. no need to have a hook_schema)
This still works manually, though, so hopefully it's just a matter of tweaking widget-multivalue.cy.js a bit. Unassigning myself for now.
I had a thought about this while I was on leave - I think the issue is the realDnd code we have is confused by the fact the field is in a fixed height scrollable pane. When the test was written we had a small number of fields but as we've expanded things it has now grown. I'll see if making the weight of this field lighter so that is shows in the viewport without scrolling makes a difference.
poker10 → credited larowlan → .
penyaskito → credited larowlan → .
wim leers → credited larowlan → .
Added a fallback source plugin that can step in and continue to render children when a JS component is removed if any instance exist.
FYI In 🐛 Cannot delete JS components due to component depending on them Active I've added a fallback plugin that can step in and continue to render children when a JS component is removed if any instance exist.
So the difference is strict schema checking in tests or rather SchemaCheckTrait also checks each individual value has a schema.
So whilst validation is checking that 'this data validates against the schema we have', strict config schema checking as seen in tests (and in FunctionalTestSetupTrait) fails if there is no schema at all. Which is what was happening. I've expanded the test to mimic what SchemaCheckTrait does.
The test did indeed fail with the missing schema. So then I reinstated the new schema and updated the test to validate invalid props against the new schema.
Then I removed it again and added a new schema class that could auto derive the mapping from the $ref.
Reviewed and manually tested, works great!
Opened a new MR with the stuff I had in an in progress review that I finished off this morning after it had already been merged - should have finished it yesterday - sorry!
Opening a follow up for a few things spotted in posthumous review
Feels like we just need to store this in the ephemeral field definition
From slack via @heyyo
The first example is used as default, but not the default property itself. What's the purpose of default as?
i.e. if a default is provided, we're ignoring it and using the example value. That feels like a bug and we should address it here or open a separate issue - thoughts on which you'd prefer?
Assigning to Wim for direction on how we should go here.
I think trying to calculate the usage in real-time in a deletion check is arduous.
I think something like layout builder's inline block usage where we store usage in a table that we write when a component tree is saved AND that can be re-calculated (ala entity usage module) is probably more robust.
I've written a test here
And I think this behavior might be by design.
If we delete the JS component then the component is deleted too.
And we don't want that if the component has usages, so that opens a can of worms about finding usages.
Do we want to tackle that here too or more likely we'll have to create issues to handle those pieces and postpone this.
I've pushed up a test, but because of the 'are we using this component question - I don't think this one is a simple fix .
Linking a related entity usage system - but we could also implement something like layout builder does for inline block usage.
Ah the curse of a unit test with mocking :) https://git.drupalcode.org/project/experience_builder/-/merge_requests/7...
Follow up 🐛 Cannot delete JS components due to component depending on them Active
larowlan → created an issue.
hooroomoo → credited larowlan → .
mglaman → credited larowlan → .
larowlan → created an issue.
Bumping to critical bug because I marked
🐛
Page Data Form + Options as Buttons error on AJAX submits
Active
as a duplicate
Assigning to @bnjmnm to review
This is fixed in 📌 Add e2e tests for multi-value textfield widget in page data form Active
Got to the bottom of this, the code we added for the media library to keep track of form state cache didn't work if the triggering element in an ajax callback had limit validation errors.
Fixed those issues
Added a test to reproduce the scenario, which is unique in that there are enabled global regions but they're all empty.
Fixed per #6
We do this on line 366 but missed line 344, I'll add a test and a fix.
This is needed to ensure an empty model is json encoded as {} and not []
I think the calling code should cast it to an array.
Pushed the required fix here.
The nuance is this in FormBuilder::handleInputElement
// Get the input for the current element. NULL values in the input need
// to be explicitly distinguished from missing input. (see below)
$input_exists = NULL;
$input = NestedArray::getValue($form_state->getUserInput(), $element['#parents'], $input_exists);
// For browser-submitted forms, the submitted values do not contain
// values for certain elements (empty multiple select, unchecked
// checkbox). During initial form processing, we add explicit NULL
// values for such elements in FormState::$input. When rebuilding the
// form, we can distinguish elements having NULL input from elements
// that were not part of the initially submitted form and can therefore
// use default values for the latter, if required. Programmatically
// submitted forms can submit explicit NULL values when calling
// self::submitForm() so we do not modify FormState::$input for them.
if (!$input_exists && !$form_state->isRebuilding() && !$form_state->isProgrammed()) { // 👈️👈️👈️👈️👈️👈️👈️
// Add the necessary parent keys to FormState::$input and sets the
// element's input value to NULL.
NestedArray::setValue($form_state->getUserInput(), $element['#parents'], NULL);
$input_exists = TRUE;
}
With \Drupal\experience_builder\ClientDataToEntityConverter::setEntityFields we're doing a $form_state->setProgrammed() so that set value never happens. Therefore the old code that was unsetting (or rather filtering out) unticked checkboxes actually needs to be setting them to NULL rather than removing them.
With that change in place, the test passes 🙌
Looks good to me, gave it another review and a manual test
larowlan → made their first commit to this issue’s fork.
Adding related 📌 Add e2e tests for multi-value textfield widget in page data form Active
FWIW this is not working as expected when there is another button on the form above the multi-value widget.
I've traced it down to issues in the client data converter w.r.t cached form state where the form state is not set to programmed - which causes Drupal to find a triggering element.
Will continue with unpicking it over there.
This is postponed on waiting for core to tag 11.1.7
larowlan → made their first commit to this issue’s fork.
Fixed thanks mate.
Side effect of rebooting a D5 era module namespace
Agree, done
Added a reference to multiple cardinality too, because adding the test for
📌
Add e2e tests for multi-value textfield widget in page data form
Active
has revealed a fatal error from the backend for multi-value widgets.
bnjmnm → credited larowlan → .
I still think it makes sense to use \Drupal\Core\Render\PreviewFallbackInterface::getPreviewFallbackString as there are some blocks that might be expecting this
We could have the component source interface extend that and defer to each source on how to handle an empty component
https://git.drupalcode.org/project/experience_builder/-/blame/0.x/ui/src... is the likely cause (disclaimer didn't check the MR)
When we click remove there's no new elements in the ajax response and hence nothing to trigger the model update via this event
We're getting fatals from the backend on adding a third item, so this is catching an actual regression.
Will dig into it tomorrow
FYI in testing
📌
Add e2e tests for moderation state in page data form
Active
I found that the moderation state takes precedence over the setPublished call in the auto save controller.
i.e. With content moderation enabled for a node if the user selects 'Draft' in the page data form and then hits 'publish all changes' their wish to keep something as a draft is respected
Might be something worth taking into account here.
Committed to 11.x - thanks!
Ah, I think instead of using .local() in day JS I can force the TZ to Australia/Sydney instead.
Leave it with me
@penyaskito when XbTestSetup runs, the test site installer has already loaded bootstrap.php which has set the PHP default timezone to Australia/Sydney which means that for the logged in user their timezone is set to Australia/Sydney and therefore Drupal shows the default values in that TZ. In this MR we set the timezone in CI to Australia/Sydney - I don't see a way around it because this happens before Drupal is installed and impacts the site's default timezone
we have some prior art in \Drupal\experience_builder\Plugin\ExperienceBuilder\ComponentSource\JsComponent::renderComponent albeit for modulepreloads
I wonder if we could/should do this in requestIdleTimeout instead so it doesn't impact the initial load time of the rest of XB
Although we could also use a fetchpriority of low and leave the browser to work it out
larowlan → created an issue.
penyaskito → credited larowlan → .
Postponed on 🐛 "There are no users matching '' error message appears after reloading the editor page Active but there is a working MR
The oembed widget only works on media entities per the \Drupal\media\Plugin\Field\FieldWidget\OEmbedWidget::isApplicablecode> method
I think we should remove it from the test module and then postpone this on 📌 Allow XB to be used on all node types Active
Updating children
Rebased this off 0.x which includes a conflict free approach from 📌 Add e2e tests for telephone widget in page data form Active
Rebased this off 0.x which includes a conflict free approach from 📌 Add e2e tests for telephone widget in page data form Active
Rebased this off 0.x which includes a conflict free approach from 📌 Add e2e tests for telephone widget in page data form Active
Merged to 0.x - thanks!
Made a change to the test structure for maintainability and to avoid collisions
Rebasing this off 0.x as it need not be postponed
+1 to the changes - great stuff!
Did that, thanks
@attilatilman that test is passing locally for me, so I think it might just be a random fail
I've merged origin/0.x and pushed to your branch
larowlan → made their first commit to this issue’s fork.
Ah, I didn't read your comment properly, you're saying we can set the timezone in the config.
I'll do that.
FYI it's not my timezone, its what core uses in bootstrap.php - mine is slightly different (no daylight savings)
The timezone needs to match bootstrap.php
So you need to run the cypress command with TZ=Australia/Sydney @penyaskito
New children
larowlan → created an issue.
larowlan → created an issue.
larowlan → created an issue.
larowlan → created an issue.
larowlan → created an issue.