🇦🇺Australia @larowlan

heine → credited larowlan → .

xjm → credited larowlan → .

xjm → credited larowlan → .

poker10 → credited larowlan → .

poker10 → credited larowlan → .

wim leers → credited larowlan → .

Left a review, thanks

Reverted this one, the failure in MediaSourceOEmbedVideoTest is from this change

smustgrave → credited larowlan → .

Committed to 11.x and backported to 11.1.x, 11.0.x, 10.5.x, 10.4.x and 10.3.x as this is a critical bug and there's minimal risk of disruption.

Ah, I'm not opposed to considering an 11.x version of WBE 2.x or to fixing the migration script for D10

smustgrave → credited larowlan → .

poker10 → credited larowlan → .

fubarhouse → credited larowlan → .

8.x-1.8 should work with 11.x
If it doesn't happy to fix any bugs
I have one site left using WBM (rather than CM) and it has a lot of custom integration that makes moving to CM more effort than updating WBM every time there's a new core major

There was a 2.x branch that had an update/drush migration command, I don't know if it still works

Committed to 11.x and backported to 11.1.x

smustgrave → credited larowlan → .

longwave → credited larowlan → .

Rerolled and added a CR

@aaronmchale this requirement came from Drupal CMS/Product managers who don't want people to have to choose upfront. @lauriii and @pameeela would be the best people to discuss it with

smustgrave → credited larowlan → .

amateescu → credited larowlan → .

Element class formatter does this too I think

I'm happy with a sub-module, but we'd have to be mindful about how we merge values from the formatter with user supplied values.

Perhaps we need options 'overrride user options' or 'user options override defaults'

MR is green

Rerunning the fails, the last build was green, so confident they're random

Code in the MR is working, site looks as normal and \Twig\Extension\SandboxExtension::ensureToStringAllowed is no longer running. Couple of random fails that I've rerun

The deprecation links in the MR need to point to a change record, other than that there seems to still be a lot of open threads.
Can we get those resolved please? If the only change is to the deprecation link URLs, fine to self RTBC

I believe this one has required a few re-rolls so feel free to ping me for another look - thanks everyone

I've opened an MR that shows a possible approach. When we're using our default sandbox policy, we don't need to check to string on any objects, they're all allowed, so can sidestep it altogether.

Haven't tested this and haven't linted either, but see if it helps

Committed to 11.x and backported to 11.1.x, thanks all. See you in the followups.

So the difference for twig between these versions is it now loops over any array and checks if any objects inside it are safe to convert to string

But in Drupal we don't do any object specific 'is safe to convert to string' checks because our sandbox policy permits anything that has a __toString method

We can't override the method in the sandbox extension because it's final, so I think we need to propose a PR to symfony for an escape hatch

There's nothing special in this module w.r.t mail delivery so I'm not sure what the issue is.

Is there perhaps something setting 'send' = FALSE on the mail?

There's a couple of open threads on the MR

Pretty keen to see this in the beta

I think the new MR is closer to where we'd want to be - and avoids us adding a new hook/API

Committed to 11.x and backported to 11.1.x

Committed to 11.x and backported to 11.1.x

Published change notice

kim.pepper → credited larowlan → .

hooroomoo → credited larowlan → .

larowlan → created an issue.

Added child 📌 Add HTML comments to twig output to attempt to allow in-place editing Active for the easy-ish bits

Getting HTML attribute support will require a full syntax tree that mixes twig nodes with HTML - which is what I'm working on in a separate PHP library. But I think this is a good step forward.

I was able to do the easy bits here - prop wrappers for when we're not inside an element or attribute, and wrappers for the component.

For the props-no-slots component in the xb_test_sdc module we get this

<!-- xb-start-9fff3623-d783-44c7-bb94-896d322f0d66 -->
  <div  data-component-id="xb_test_sdc:props-no-slots" style="font-family: Helvetica, Arial, sans-serif; width: 100%; height: 100vh; background-color: #f5f5f5; display: flex; justify-content: center; align-items: center; flex-direction: column; text-align: center; padding: 20px; box-sizing: border-box;">
    <h1 style="font-size: 3em; margin: 0.5em 0; color: #333;">
      <!-- xb-prop-start-heading -->
        se3hqtu5
      <!-- xb-prop-end-heading -->
    </h1>
  </div>
<!-- xb-end-9fff3623-d783-44c7-bb94-896d322f0d66 -->

larowlan → created an issue.

📌 [PP-1] Add support for Blocks as Components Active handled this

https://github.com/twigphp/Twig/pull/4441 may help

Is this because our objects reference themselves and we don't have recursion protection?

larowlan → created an issue.

Added MRs all the way back to 10.2.x

Hey folks, there's three MRs here
One of them looks blank - can we confirm there's one for 11.x and one for 10.4

I think this is eligible to backport to 11.0, 10.3 and 10.2 so if we could also get MRs for that it'd be great

I think any arbitrary settings key is fine

Thanks mate, I'll work on those fixes over the coming days/week

I was able to reproduce @hctom's results
Expanded the tests and fixed the issue

Back to needs review

quietone → credited larowlan → .

pameeela → credited larowlan → .

Refactored and added tests

larowlan → changed the visibility of the branch 10.2.x to hidden.

larowlan → changed the visibility of the branch 3424200-media-overwrites-validation to hidden.

larowlan → created an issue.

Can do for media
I did have that issue with block content before one of the later pushes, so perhaps @hctom was testing an earlier version.
I fixed it whilst adding the tests.

Backported to 11.1.x, 10.5.x and 10.4.x

Committed to 11.x - thanks!

This is a great feature. We need to add an upgrade path.

This should take the form of a post update hook that makes use of the config entity updater.

There should also be a test that asserts the value isn't set before running update, and then checking it is set.

There's a good example of a similar update hook (also for entity view displays) here https://git.drupalcode.org/project/drupal/-/merge_requests/10002/diffs#2...

This MR also contains a good example of an upgrade path test - https://git.drupalcode.org/project/drupal/-/merge_requests/10002/diffs#2...

Unfortunately it doesn't look like there are any instances of this formatter in use in the standard profile, so that means you will have to create a new database dump/fixture file. \Drupal\Tests\views\Functional\Update\EntityArgumentUpdateTest in core has an example of how to add additional test fixtures to the default standard dump.

The docs have more information on how you can do that - https://www.drupal.org/docs/drupal-apis/update-api/writing-automated-upd... →

I backported this to 11.1.x

Setting to 10;5.x, but also needs backport for 10.4.x

I backported this to 11.1.x

I backported this to 11.1.x

Issue credits, including those from UX meeting
Crediting @manibharathi ezhimalai ravi for manual testing

Committed to 11.x
I think it is worth back-porting this to 10.4 for API compatibility and because its a bug.
I don't think we can back port it to 10.3/11.0 because the behaviour change could be disruptive.

Needs re-roll for 10.4.x

Published the change record.

Thanks

Committed to 11.x - thanks!

Committed to 11.x - thanks!

I think the suggestion to make use of a cache collector is a great idea. I think ::getIconsFromDefinition would likely be the best place to add it.

Setting to needs work for that

I wonder if we can expand the description of the permission to convey that there is a performance impact of granting this permission. And whether we should also have a hook_requirements that shows a warning if this permission has been granted. Will report back on other committers' thoughts.

I'll discuss this with other core committers

Got some path integration going in the branch

griffynh → credited larowlan → .

Updated issue summary
This now includes tests and is ready for review
Testing on a client project its working nice 🙌

larowlan → created an issue.

@catch's suggestion to make use of https://www.drupal.org/node/3155569 → fixed 3 and 4 in a real nice manner

1 I think we have to leave as is because its used in teaser view mode etc
2 I think we should discuss in a follow up, technically its nice because you _could_ create all the content for a node type and then turn on the display to launch that content type.

I'll work through any remaining test fails. I addressed all the fails locally from the last pipeline except nightwatch, which look random

Will continue with tests tomorrow

In practice most complex projects will use a bundler, but this pure esm approach seems like something that could grow over time.

Yeah it would allow us to move away from everything being global

But in practice I think there are concerns about the resource waterfall impact on performance

If you don't have a container, you don't have a site

The cache rebuild rebuilds the container so your site works

In this initial screen, it's weird to have 'Display author and date information' enabled since there's nowhere for this to appear without a page display. So this should maybe be unchecked and disabled if there is no page display?

This is in node.html.twig in the node module, so it is used even in teaser view mode

  {% if display_submitted %}
    <footer>
      {{ author_picture }}
      <div{{ author_attributes }}>
        {% trans %}Submitted by {{ author_name }} on {{ date }}{% endtrans %}
        {{ metadata }}
      </div>
    </footer>
  {% endif %}

I think we can fix items 2-4 if we:
* Instead of handling it in the controller, handle it in routing access
* Return a access denied in the routing access which fixes item 2 and 4
* Add a custom exception subscriber that turns the 403 into a 404 in this case _or_ redirects the user to the edit form if they can edit - fixing item 3

Thoughts?

Kept it at node-type until ✨ Support adding additional routes for view modes other than 'full' Active

Updated the change record and addressed the feedback. I think the only thing remaining from reviews are follow ups as follows:

* How can SDC opt in
* Is there a nicer way for a .libraries.yml file to declare itself a module - than the current attributes approach

I'm not sure where SDC fits in with regards to exposing importmap entries - I guess if you had one component that had an ES module that you also needed to import in other places you might want to do that. But really only if another module or theme needed to import it. In all other cases it would be fine to let your bundler do code-splitting and have the consuming code import it from the chunks split during bundling.

The importmap functionality is really only when you have unknown consumers. E.g. React module doesn't know what themes/modules will need to load react. Experience builder doesn't know what contrib modules will want to ship React powered things for field widgets.

But I think it would be worth exploring in a follow-up for sure.

We can do that and store it in the same spot for any future functionality

There's nothing to review here now that the direction has changed. I think our next step is to propose and agree on a better default.

Another emerging option is vitest in browser mode https://vitest.dev/guide/browser/ - it runs on top of webdriver or playwright

In general I'm in favour of this approach but I would prefer it was done via console commands rather than via a browser. Pretty sure that can be done with custom steps

I think we should postpone this on 📌 Consider dropping Nightwatch in favor of Functional Javascript tests Active as there's no point spending additional effort on it if we choose a different option

The default target of an entity reference field is the user entity. Comment::bundleFieldDefinitions sets this to the entity type configured on the comment type
In this test it was not being set because the comment type wasn't created
As a result the commented entity was loaded as a user
On MySQL/sqlite it was entity id 1, on pgsql it was 2
This should have been loading an entity test entity, but instead was loading a user
User 1 existed, user 2 did not

Kudos to bradjones1 for working out it was loading the wrong entity type, at the weekend I pointed him towards why on slack

Yeah that sounds good
We can also trigger a deprecation if it's not so sites like this client project get notification that they're doing it wrong