πŸ‡¦πŸ‡ΊπŸ.au GMT+10
Account created on 21 November 2008, over 16 years ago
#

Merge Requests

More

Recent comments

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

This is blocked on πŸ“Œ Split model values into resolved and raw Active , πŸ› "There are no users matching '' error message appears after reloading the editor page Active and πŸ› ComponentValidator ignores the set validator and creates a new one Active

Pushed some code that includes a cherry-pick (Squashed) of πŸ“Œ Split model values into resolved and raw Active and πŸ› "There are no users matching '' error message appears after reloading the editor page Active that is working on the backend if the core patch from πŸ› ComponentValidator ignores the set validator and creates a new one Active is applied

On the frontend - current status is the autocomplete value (e.g Some node title (3) doesn't validate with ajv as a uri and hence the field is invalid. I have written a transform to translate this to a uri (entity:node/1) but transforms aren't applied until after the field value validates so we might need to revisit that. Started looking at ajv keywords which are a schema extension that adds support for transforms - but these don't work on strings, only on nested data/objects (because the string is passed by value when we call jsonSchemaValidate)

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

This is blocking for adding autocomplete support to link field widgets in Experience Builder

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Seems reasonable.
I imagine the unit test will be difficult to unpick in the future - its a shame we couldn't modify a functional test to test the real thing - but we can deal with that in the future

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

larowlan β†’ created an issue.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

I wonder if filehash module solves issue 2

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Addressed that thanks!

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

πŸ› ComponentValidator ignores the set validator and creates a new one Active prevents us from being able to do this using the APIs provided by justinrainbow/json-schema

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

larowlan β†’ created an issue.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

I think I have a way to work around #6 in the meantime

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Now there's the answer in #13 there is no distinction now (I think).

This needs to go back to needs work as we should be able to remove a lot more of the FALSE items in this list with custom field overrides.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

πŸ“Œ Allow 6.x version of justinrainbow/json-schema Active is going to be needed here because it uses filter_var to validate URLs and that means entity:node/3 doesn't validate as a valid URI.

https://github.com/jsonrainbow/json-schema/pull/800 is what fixes this upstream but it's only in the 6.x branch.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Yes that's correct

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Added πŸ“Œ Allow 6.x version of justinrainbow/json-schema Active for adding support for both 5.x and 6.x at the same time

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

larowlan β†’ created an issue.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Adding ✨ [PP-1] Make link widget autocomplete work (for uri and uri-reference props) Postponed which also requires the 6.3.1 version - the fix in https://github.com/jsonrainbow/json-schema/pull/800 is needed to support entity:node/3 style URIs in experience builder

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Do we have a follow up to update justinrainbow/json-schema as this has been fixed upstream now

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

What's the core bug? Is "core" referring to Drupal core, or some other meaning of "core"?

I guess the first question is - should we be able to map dynamic prop sources in XB to computed fields in Drupal.

If the answer is yes, then I think we need a core issue to flag those comment fields as computed - but we also need an override in XB to add the string semantics (aka the format key from JSON schema) to that we can ascertain what sort of value they hold.

Can you clarify if the expectation is that you can setup dynamic prop sources against computed fields?

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

larowlan β†’ created an issue.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Repurposing this as a parent meta for tracking widget support.

Adding πŸ“Œ Multiple fields widget should fully work in the contexual form Active , πŸ“Œ Get Options as buttons in Page Data form working Active as additional children

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

larowlan β†’ created an issue.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

larowlan β†’ created an issue.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

larowlan β†’ created an issue.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

larowlan β†’ created an issue.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

This is now green, adding parent meta

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Adding parent Meta

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

The blocker is in

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Yep it's a real fail
Closing the contextual link list used to place focus on the trigger but that isn't happening now.
Tried a few things but they all end up in a loop
Will revisit this week

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Added the service provider

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

There were some underscore references left behind. Got rid of those because they're not needed in 2025 - we have native methods

Spot checked some tests locally and its looking good

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Merged forces with πŸ“Œ Get Options as buttons in Page Data form working Active

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Ok, new tests are passing πŸ™Œ

I think this is small enough to get in without disrupting multi-value.

And then we can tackle the remaining widgets in individual issues with smaller scope.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Got around #5 and media library tests are still passing
The key thing we need to save is the form_build_id, nothing else matters

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

I have gotten past the issue in #4 now

But then we hit a new issue because this gets modified by OptionsWidgetBase::elementValidate which in turn sees the new value (array) get stored in autosave so that when we publish, we end up with the same issue as #4 😭

I think this will likely require some special casing of options_buttons and anything that subclasses OptionsWidgetBase

Before I go down that route I will revisit why we store the updated entity form fields in autosave, it was for Media but perhaps there's a middle ground where we don't have to.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

green now

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Removed support for password with an explicit test

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Fixed the issue spotted by @longwave

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Thanks, committed to 11.x

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

sounds like this might need to run _before_ trash module?

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

@skessler and the converse, you can use scaffolding to prevent fingerprinting .txt files like MAINTAINERS.txt from being added. If we change that to MAINTAINERS.md and someone updates, they may be leaking information they didn't intend to.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Plus one from me but with a caveat - @mstrelan discussed some of the issues with nightwatch with me during the week (he's been doing a deep dive on random fails πŸ’™) and flagged that a lot of the tests are doing module installs and role creation via the browser which is obviously slow. Ideally we don't repeat that same mistake and instead have more setup scripts that do this via the API.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

This is green

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

7 e2e fails feels like real things

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

πŸ“Œ Split model values into resolved and raw Active needs to go in first because without that we always use the sourceType from the config entity

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

πŸ› Component transforms need to be per sourceType, not per component prop Active for the critical found in the final test fail

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

I'll hoist my work into your branch πŸ‘οΈ

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

FWIW we used #attached csp_hash support from the CSP module to allow-list the inline JS
There's a lot of great stuff in that module that probably belongs in core as an API

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

@heyyo mentioned this in πŸ“Œ Improve backend API when lots of components Active

From slack

It looks like a POST request, why we need to post anyhting to backend when just loading the page ?

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

The only open thread is to open a dedicated issue, this is green and ready for review

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Annotated the MR

Two open threads, one to file a dedicated issue for the data corruption issue and another to review the use of empty

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Welcome to the team @dieterholvoet - goes without saying but please use one issue per commit for transparency sake.

Thanks for coming onboard

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Let me just confirm with @acbramley the other maintainer

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Looks good to me

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Here's a concrete example use-case - https://www.previousnext.com.au/blog/creating-cards-section-layout-builder

A 'cards' component that has a slot for cards and props for title, intro text and read more link.

You want to prevent adding anything other than card-like components into the slot.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Just to reiterate from above, there's a UX implication here as well as a visual identity one.
One of the issues with LB is the overwhelming amount of component options and restrictions can yield a much leaner experience.
In terms of implementations in the FE, in decoupled LB we were sending restrictions with block metadata and using them to control dropzones - if you visit https://project.pages.drupalcode.org/decoupled_lb/?path=/story/component... and click the βž• then try to drop the Umami disclaimer into the two col layout you see it doesn't let you.

We should be able to do something similar here hopefully

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Committed to 11.x - thanks!

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

The module sends an email and needs a subject

Can you elaborate on the use case for hiding the subject?

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Committed to 11.x, thanks

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

We had at one point code that auto created a media entity based on example URIs but remove it because it relied on the magic in findTargetForProps finding a file with the exact uri of the example, which wasn't very precise with things like FileExists::rename.

I feel we're going to keep coming up against this issue.

I'm trying to think of creative ways to get around this.

As I see it the issue is stemming from when an SDC component documents an object as the $ref: json-schema-definitions://experience_builder.module/image we automatically apply a certain storable prop shape that assumes a media entity

But we need flexibility to be able also handle the default value.

We have πŸ“Œ Split model values into resolved and raw Active for some of this but I don't know if its going to solve this issue.

I'm spending today on both this and that to try and propose a solution.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

We have a default content import solution in core that is used by recipes.

It has support for files in \Drupal\Core\DefaultContent\Importer::copyFileAssociatedWithEntity

We don't yet have support for exporting (still requires contrib default_content module) but perhaps we could depend on default_content module and do the actual exporting of the media/file entities as part of config export/save of patterns.

We could then listen to the 'missing content' event in config sync and run the import?

Longer term we could work with the Recipes initiative to add the exporter bits of default content to core.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Re #22 sorry, will focus on this today

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Hey πŸ‘‹

The contents of the file can be imported via copying it's content and pasting it in the 'single import' form from admin/config/development/configuration/single/import after selecting 'View' as the configuration type and then 'aggregator_rss_feed' as the view - just leave the UUID from the existing one.

If you're using drush, you could also copy the contents of that file into your exported .yml file - again leaving the UUID from the existing one.

The drush approach would allow you to visit admin/config/development/configuration and see a diff between the two files - or you could also do this with git if you were using source control for your configuration files.

In terms of what changed - https://git.drupalcode.org/project/aggregator/-/commit/620743b3e053a660e... is the only change which is:

  • Adding a sort
  • Ensuring empty items aren't output

LR

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Opened πŸ“Œ EntityFormController should load auto save state if it exists Active but we might be talking about different things
From your GIF I wonder if that's re-rendering the initial state rather than Drupal not taking into account the saved state in the form.
IE a frontend issue rather than a backend one.

Will tackle both over there if they're related.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

larowlan β†’ created an issue.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Ah yeah we don't consider auto save state in the form controller

I'll open an issue on Monday

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

There's some test fails here because I didn't update the tests to include the 'Article author's password' as a source.

I'm thinking this is a red flag that we shouldn't support password fields ever.

Thoughts?

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

larowlan β†’ created an issue.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Got to the bottom of the issue ✨ Add an "Include image in display" checkbox to the image field Needs review

Basically FileItem sets a 'display_default' setting to FALSE which ends up in form state because \Drupal\file\Plugin\Field\FieldWidget\FileWidget::formElement sets a #default_value drawn from that.
But because this field is not visible, in \Drupal\file\Plugin\Field\FieldWidget\FileWidget::process it is set to TRUE.
And the code that processes the hidden field in \Drupal\Core\Form\FormBuilder::handleInputElement refuses to set a value in form state because a value already exists and was set from the #default_value.

So that is a bug in ImageItem - we already have an override so I'll add the fix there but opened a concrete core issue - πŸ› ImageItem::defaultStorageSettings should override display_default Active

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

String data needs the StringSemantics constraint in order to match against formats etc, so added those to password item props.

The comment_last_name field is computed, so marked that as unsupported (core bug).

Also updated the comments for path fields because the whole field item list is computed.

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

Looks like this was pretty simple in the end, sorry for being the one who introduced the install in beforeEach

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10

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

πŸ‡¦πŸ‡ΊAustralia larowlan πŸ‡¦πŸ‡ΊπŸ.au GMT+10
Production build 0.71.5 2024