🇬🇧United Kingdom @longwave

I don't get why we need to do this. DX is slightly nicer if developers can pass a string in the simple case or an array of strings otherwise. Given it's been over 10 years since this was proposed are there better things to work on?

I am not really sure what this means, but have you tried selecting the "Use AJAX" checkbox in the view settings, which will reload just the view instead of the entire page when you use links inside the view?

@nicxvan this should be suitable for any large field in any entity type that stores duplicated data so it will help reduce database size with Paragraphs as well as Experience Builder.

Some more thoughts on this while I slept on it:

1. Wondering now if this should be attached to a field type instead of a field storage setting - this really only makes sense for blob or longtext type fields; the hash is 64 chars so the data probably needs to be an order of magnitude larger for this technique to make sense.
2. If this is to be a field storage setting, is it somehow worth abstracting out the underlying field storage for "base fields", "configurable fields", "interned fields" into plugins and then allowing a storage plugin selection per field? This will need some rules - base fields can only have cardinality 1, interned fields should only be available for fields that use blob/longtext columns, etc, but it would provide more choice to the site builder. Implementing this is probably out of scope here but it is still worth discussing as it may still shape the solution here.
3. Unsure which hash method to choose, or if it matters that much. I chose SHA256 because we have precedent elsewhere in core and I remember from somewhere that, while shorter, MD5 or SHA1 are discouraged in some cases even if not used as a cryptographic hash.

I think this could be simpler if we do this at FieldItem level?

• Instead of adding the body_value, body_summary, and body_format columns to the node_revision__body table, we only add a body_hash column.
• We add another table, node_revision__body__interned, with columns body_hash, body_value, body_summary, and body_format.

At insert/update time we just calculate the hash, create the interned storage row if it doesn't exist already, and store the hash in the revision table.

Garbage collection can be still be done by deleting rows from the interned storage table where the hash does not appear in the revision table; this can be done out-of-band (on cron) because, apart from disk space, it doesn't matter if we have orphaned hashes.

I had a thought that this could even be more generalised by having a single interned storage table per field type, which might provide slightly better effective compression ratios in some cases where the same data may appear in multiple fields, but the tradeoff there is that garbage collection is much more difficult (we would have to consider hashes from multiple tables), and I don't think the savings will be worth it.

Are we also assuming that this will only be used for revision storage? ie. node__body will not use the interned strings?

This technique could also be useful in a related but slightly different way for the message column in the watchdog table, which can also grow quite large on some sites.

Actually given that this is purely a developer-facing feature I am not even sure this is a product manager decision, removing the tag again.

Opened 📌 Move JSON:API validation to a feature flag or separate module Active to discuss the core change.

For the time being, I think we should land MR!247 in XB, marking RTBC for that.

longwave → created an issue.

Yeah I meant that we should change core so JSON:API validation is explicitly enabled with a feature flag. I wonder even if it should just be a contrib module and the feature removed from core directly, but that is a product manager decision. Will open a core issue to discuss.

Let's change my_module_list's variables to match while we are here, it's much easier to read in multiline format.

Instead of the JSON:API validation magically happening when the package is installed maybe we should move it to a feature flag module (that explicitly depends on the package as well).

kristen pol → credited longwave → .

Decided to backport this one to all active branches as a docs-only fix and to keep things in sync given that it touches a lot of files, otherwise it will make future cherry-picks more tricky in some cases.

I still wonder why we can't do this for entity storage:

Drupal\node\NodeStorageInterface:
  factory: ['@entity_type.manager', 'getStorage']
  arguments: [node]

and then just inject it directly as a service?

Added a few questions, not sure of the answers.

Thanks, I think explicitly returning a list here is more correct behaviour and it's cleaner than having to modify multiple tests (and perhaps some in contrib or custom code).

Committed to 11.x. As a minor behaviour change I don't think it's worth backporting to a patch release or even down to 10.4.x given we only need it in order to pass tests.

Also published the CR.

The changes to the text look good to me, they are the minimum required to reflect the change the core committees agreed on in Burgas.

We could move these out to a feature flag module, disable it by default on new installs but enable it on existing sites, and eventually move it out to contrib.

Backported down to 10.3.x as a test-only fix.

Committed and pushed ee58564958 to 11.x and 1372de8d7e to 11.0.x and 39c6aa5c56 to 10.4.x and 70202c66bc to 10.3.x. Thanks!

As someone who's opened this file in the past and been confused by this, incremental improvements are welcome. Feel free to open followups to improve this further if anyone thinks it is necessary.

Backported down to 10.3.x as a docs-only fix.

Committed and pushed d820d8ed3e to 11.x and a21edc2188 to 11.0.x and db4e4e5426 to 10.4.x and 723d4d718a to 10.3.x. Thanks!

Backported to 10.3.x as a docs-only fix. And congratulations on reaching 9000 merge requests in core!

Committed and pushed e1b0e54225 to 11.x and e2ff94dc96 to 11.0.x and 73ced19530 to 10.4.x and 2b685fb4e9 to 10.3.x. Thanks!

Backported to 10.3.x as a docs only fix.

Committed and pushed 11996c7a16 to 11.x and 658f18f920 to 11.0.x and af2a2c5a14 to 10.4.x and 8069f0030a to 10.3.x. Thanks!

Committed and pushed 1dde6f5ed9 to 11.x and 7d320cb441 to 11.0.x and b0a4cd3e5d to 10.4.x and 8e040e5df0 to 10.3.x. Thanks!

Committed and pushed 239e6f638d to 11.x and 8d9b67396d to 11.0.x and 2f5393f842 to 10.4.x and fd1c79fa51 to 10.3.x. Thanks!

Looks like PHPUnit will be bumped in 📌 sebastianbergmann/comparator:5.0.2 Introduces (for Drupal) breaking changes Fixed , removing tag.

Let's get this into 11.x and 10.4.x and see if anything breaks downstream, hopefully nothing. I think it's highly unlikely the PerformanceTestBase method will be overridden.

Committed and pushed 22e2a1285a to 11.x and 67b91bcb42 to 10.4.x. Thanks!

MR has merge conflicts.

Committed and pushed 41122f85dd to 11.x and a4f5580c86 to 10.4.x. Thanks!

Backported to 10.4.x to keep things in sync there, not worth the effort of backporting to bug fix branches.

Committed and pushed 0b17a57492 to 11.x and 58c6687aa2 to 10.4.x. Thanks!

Added some nits where the trait is not required, and there is a case where the trait is added to a test case itself which is not correct - we shouldn't be using t() at all in test code.

I think the error message needs work. Is there a handbook page that we can link to on d.o that explains how and why you should configure private files? If so I think we should link there in either case, we don't need to separate out the two cases.

While we're in this method the docblock is wrong as well:

  /**
   * Overrides \Drupal\Core\Block\BlockBase::blockForm().
   *
   * Adds body and description fields to the block configuration form.
   */

It doesn't do either of those things, it does allow the view mode to be set if more than one is available, but it probably should just be @inheritdoc.

longwave → created an issue.

Adding the trait to \Drupal\Core\Plugin\PluginBase (not Component) seems to work, I autowired the BlockContentBlock plugin to prove it.

Does this even need to be a trait? Should it just be included directly in PluginBase?

Not sure we need explicit tests, given we are exercising it in Views tests now, so removing the tag.

Added AutowirePluginTrait to PluginBase to enable autowiring by default (simply omit the create() method) and autowired some Views plugins to test that it all works.

No, #8 doesn't work because ContainerFactoryPluginInterface::create() doesn't match.

+1 - unless we have a good reason to have a friendly alias name for a service, it should just be named after the class it is implemented in, because naming things is hard - we are very inconsistent in our service naming at present - and so this removes the decision entirely.

📌 Autowire core modules that do not require explicit configuration Postponed proposes making the autowiring change to existing core modules and 📌 Use autowiring for core modules and services Needs work to most core services.

If this is accepted I think we need a sister issue to rename core module services (with deprecated aliases for the old names), and eventually, one for core services too.

Urgh, the field formatters case is pretty horrible.

  public static function create(ContainerInterface $container, array $configuration, $plugin_id, $plugin_definition) {
    return new static(
      $plugin_id,
      $plugin_definition,
      $configuration['field_definition'],
      $configuration['settings'],
      $configuration['label'],
      $configuration['view_mode'],
      $configuration['third_party_settings'],
      $container->get('entity_type.manager'),
      $container->get('language_manager')
    );
  }

Shouldn't that extraction work be done in the constructor instead? But changing all existing constructors and providing BC is going to be no fun at all.

Can we just extend the existing AutowireTrait to work here?

  public static function create(ContainerInterface $container) {
    $args = [];

becomes

  public static function create(ContainerInterface $container, ...$args) {

ie. additional arguments passed to create() become the initial set of arguments?

Will try and add some BC.

longwave → created an issue.

https://git.drupalcode.org/project/drupal/-/merge_requests/8735/diffs is the changes that were made in core and so similar changes likely need to be made here.

Project Browser reported that their Nightwatch tests are failing when run against "next major" which uses 11.x and Nightwatch 3.7, which is probably related to this issue.

https://git.drupalcode.org/issue/project_browser-3466307/-/jobs/2508579#L40

⚠ Failed to connect to Selenium Server on localhost with port 9515.
...
  Error
    An error occurred while creating a new Selenium Server session: [SessionNotCreatedError] session not created: Missing or invalid capabilities
  (Driver info: chromedriver=123.0.6312.58 (6b4b19e9dfbb93aa414dc045bd445287977d8d7a-refs/branch-heads/6312_46@{#3}),platform=Linux 5.10.220-209.869.amzn2.x86_64 x86_64)

This is possibly because Nightwatch 3 is trying to connect to the older drupalci/chromedriver:production container, I think the templates will need the newer selenium/standalone-chrome container to be added from 📌 Use selenium/standalone-chrome instead of our chromedriver image Needs work ?

TwigNodeTrans::compileString() calls trim() on the supplied text:

    return [
      new Node([new ConstantExpression(trim($text), $body->getTemplateLine())]),
      $tokens,
    ];

This means that whitespace either side of the text inside the tags is removed, so it doesn't matter which syntax we use.

This is an event subscriber and I doubt anyone would be overriding it (you would swap out the renderer service instead) and so I don't feel we need to provide BC. We could deprecate the unused compiler pass instead of deleting it outright if that is deemed necessary.

 5 files changed, 18 insertions(+), 73 deletions(-)

The MR so far adds the route and a controller that returns the form HTML wrapped in JSON.

I tried to start writing a test but I get a 404 from /xb/api/entity-form/node/1 and I'm not sure why.

Reran stylelint, seems you have to run it from /core otherwise it only picks up some of the config?

MR!185 renders all blocks as suggested in #16 but as per #17 I think we need to restrict this - we don't want the toolbar showing up, and we probably want to remove other blocks too. This could be a region setting, but it could also be a block condition? It seems likely there are some blocks people will want to show in XB and others that they don't, and perhaps hardcoding it by region in the theme isn't enough?

To me MR!184 should be merged here to unblock progress and then the decisions about additional blocks deferred to a followup.

So if we render the full page instead of the bare page, we get the header and footer but also the toolbar...

Happy to change this so it renders more of the page (e.g. header/footer blocks as well) but it's not clear whether that is required or if it would get in the way in some cases.

First attempt at this, the Olivero footer and background show up in the XB preview.

longwave → made their first commit to this issue’s fork.

longwave → created an issue.

Wim Leers → credited longwave → .

Is it possible to naively identify meta fields by finding all fields that are not displayed in any view mode? This probably doesn't hold true for all cases, but generally, if a field is never displayed it must be meta information of some kind?

+1 for not making this specific to XB, it could be useful elsewhere.

To decide between options 1 and 2: is there a reason why some fields would be metadata in some form layouts but not others? I can't think of anything immediately which would point to option 1, but if there is, then option 2 would let us distinguish that.

+1 from me, we should definitely be announcing new major/minor versions of core in the feed.

Not sure if it was in an issue or in Slack but somewhere @alexpott wondered whether NULL is really the correct thing to do for config string values and whether we should just use empty string instead to avoid having to handle conversion from UI input.

Should we be using CSS.escape instead of a custom regex? https://developer.mozilla.org/en-US/docs/Web/API/CSS/escape_static

Aren't we losing some test coverage by deleting those lines? It looks minimal but I assume it was added for a reason, if we can work around the issue instead of deleting the code then we probably should.

Added the UI changes section, don't think this needs a release note as it only affects new installs but tagged as a highlight in case we want to point it out in a blog post.

Evaluation looks good to me and as release manager I have no issues with adding this as a dev dependency.

However the MR currently has merge conflicts and also the package is added as a non-dev dependency so that needs to be fixed.

Great to see this hack be removed, surprised there was only one fix needed.

Not eligible for backport as it may affect contrib tests as per #4.

Committed c17cc49 and pushed to 11.x. Thanks!

One benefit is this becomes trivial:

An upgrade path for a `component` would require logic somewhat like this:

1. SQL query to search the _tree_ JSON blob for uses of this `component`, capture the UUIDs.

SELECT uuid FROM table WHERE component = "provider:person-card"

@Wim Leers additional note: we also have to consider langcode for asymmetric translations

Wim Leers → credited longwave → .

Wim Leers → credited longwave → .

Some feedback where the change makes the comment misleading - generally where we are listing punctuation in some way - plus some nitpicks and some weird cases where only some comments appear to be taken into account (especially in TaxonomyFieldAllTermsTest and EntityQueryTest).

If we're going to do this it would be good to have some test coverage to prove that it does work (or indeed, as #6 implies, doesn't work).

RendererPlaceholdersTest::testNonScalarLazyBuilderCallbackContext() should be expanded to cover the updated message, which will also alleviate the need for manual testing.

See MR comment.

Committed e4d0bea and pushed to 11.x. Thanks!

As it's just a cleanup I don't see the point in backporting any further.

Wim Leers → credited longwave → .

Do we need a separate trait? Can we just say

  use ApiRequestTrait {
    makeApiRequest as request;
  }

directly in ResourceTestBase?

nod_ → credited longwave → .

Needs the backward compatibility dance in the constructor → .

Wondering if this should just be @inheritdoc in most cases except where we add additional information?

Committed f501275 and pushed to 11.x. Thanks!

Not eligible for backport as we only enable new PHPCS rules in minors.

Fair enough. Let's get this in.

Committed d38aac1 and pushed to 11.x. Thanks!

Looking at this sort of thing makes me think there is some value in a version of BTB that only installs the site at the start of the entire class instead of each test run, which would probably be useful in a lot more places.

Committed ccca0ab and pushed to 11.x. Thanks!

Marking PTBP as this needs a new MR for 11.0.x.

I think 🐛 Generic recipe functional tests ar discovered as a unit test Active can be closed as duplicate.

Is it not simpler just to do this to explicitly test the tearDown() method, and avoid this code running on all other teardowns in this class?

-  protected function tearDown(): void {
+  public function testTearDown(): void {
     parent::tearDown();

Note also that this only affects development dependencies and can't be exploited at runtime, the Drupal Security Team will not be issuing a security release or advisory about this change.

yarn audit also reports problems with ws, let's fix that at the same time.

Published the CR.

The changes look good but as this is a change to default PHPUnit config it can only go in a minor release and needs a change record.

FWIW on @mondrake's suggestion I split the MockClient class out to its own file instead of keeping it inside WebAssertTest and the deprecation is triggered in 11.x:

$ ddev test core/tests/Drupal/Tests/Core/Test/WebAssertTest.php 
PHPUnit 10.5.29 by Sebastian Bergmann and contributors.

Runtime:       PHP 8.3.8
Configuration: /var/www/html/drupal/core/phpunit.xml.dist

D........................                                         25 / 25 (100%)

Time: 00:00.129, Memory: 6.00 MB

1 test triggered 1 deprecation:

1) /var/www/html/drupal/vendor/symfony/error-handler/DebugClassLoader.php:341
Method "Symfony\Component\BrowserKit\AbstractBrowser::doRequest()" might add "object" as a native return type declaration in the future. Do the same in child class "Drupal\Tests\Core\Test\MockClient" now to avoid errors or add an explicit @return annotation to suppress this message.

Triggered by:

* Drupal\Tests\Core\Test\WebAssertTest::testResponseHeaderExists
  /var/www/html/drupal/core/tests/Drupal/Tests/Core/Test/WebAssertTest.php:75

OK, but there were issues!
Tests: 25, Assertions: 65, Deprecations: 1.

Added some review comments/feedback, happy for all of it to be pushed back on but just some thoughts that came to mind while reviewing.

#2805849: WebAssert does not track assertions, Test marked as risky → is semi related, there were no functional fails here but only because BTB explicitly increments the assertion count to avoid risky tests.

Backported down to 10.3.x as test-only changes to keep things in sync.

Committed and pushed d43e2ba8f3 to 11.x and b3f2f8de04 to 11.0.x and 492839829a to 10.4.x and 50496b5e22 to 10.3.x. Thanks!

In which case as that was the only nit back to RTBC and I can commit.

Yeah, sorry, was wrong there, forgot this was all about strict types :)

What's the difference between declaring it in the argument and casting, it works the same?

> $f = new Drupal\Component\Render\FormattableMarkup('markup', []);
= Drupal\Component\Render\FormattableMarkup {#10099
    markup: "markup",
  }

> $callable = fn(string $elements) => $elements;
= Closure(string $elements) {#10093 …2}

> $s = $callable($f);
= "markup"

> gettype($s);
= "string"