🇨🇭Switzerland @berdir

I have mixed opinions on this.

One one side, DI for things that are being serialized has some problematic drawbacks (and entities are serialized _a lot_), it requires trickery such as DependencySerializationTrait. And there's discussions around whether or not entities should be value object, which would imply they shouldn't do stuff that requires DI.

But also, we have bundle classes now (although they probably mostly serve as nice getters and setters), pre/post methods (Re #48, Hooks being services changes things bit, but I still generally prefer to have entity related logic on the entity itself) and ContentEntityBase itself has a number of \Drupal:: calls.

One problem is that, unlike other plugins, we never got the factory stuff working, so we don't have a single thing responsible for creating entity objects. We did make it work for bundle classes though, it's fairly well isolated in the storage handler.

To summarize, not against it, but I do very much expect that there will be cases where it will come back to bite us.

I updated the issue summary a bit, as I made some changes. The changes to the performance tests look nice, but unlike some other improvements, this comes with some drawbacks, and it's really hard to estimate how sites will be affected by the possible drawbacks. See issue summary for details.

I was actually considering that recently, but then saw that you pushed a new release. I did a bit of a review and testing around updating 1.x to 2.x and just wanted to write down my findings.

Feel free to add me, so there's another person around if a release is necessary or so, but I can't really promise much beyond that. I might or might not work on this issue (and others), depending on how much it's a problem for us, haven't yet tested where the limits are.

berdir → created an issue.

> This is incorrect. A past revision can be changed.

Why should a revision get deleted if it has been updated?

that doesn't really make sense. If a revision gets deleted, then all fields will get deleted, that's pretty certain.

2.x works with a queue, you have to run cron for it to actually delete revisions, did you do that?

Just a drive-by note while reviewing similar modules.

node_revision_delete 2.x kind of works like that, it puts entities in the queue, not revisions. And there are multiple plugins to then decide which revisions to delete. And it only puts entities in the queue as they are saved, not on cron. So it scales quite nicely. One downside is that age-based logic only works if an entity is saved again. so if you keep 50 revisions for at least 2 months, if there are 100 revisions within a short time and then the entity isn't updated anymore it is not cleaned up ever (except if you do a full queue processing through the UI, but has the same limitations currently that it loads all ids at once, there should be no need for that).

I'm not convinced that this needs to be in core. I understand the use cases, but this is not extensible. There is no hook here that something could use to intercept the process and actually do something with the translation. You still need to write a custom AI integration to actually do something.

This also doesn't tie into the content translation permissions and settings, it just checks regular entity update access. Content translation has per-bundle settings to enable/disable translation UI, and it has multiple permissions and its own API to check if a user is allowed to translate into a specific language. It also doesn't tie into the revision API's that were created to deal with content moderation and stuff like this, doesn't check for things like existing drafts and what not.

The configuration approach means you need to configure this before you can use it and then it's fixed for those specific, and core removed the action UI module from core. So one more reason why you can't use this without additional modules.

The reference handling is also very opinionated and non-configurable.

Content Translation has had no official maintainer for 5 years, plenty of issues and adding more features isn't really helping with that. Sorry, but -1 from me. This can live in a contrib module for those that consider it useful.

Thank you.

For the record, I'd also fully support it if you want to become a core entity API and improve some things there. I think you're more than qualified to do that. I've actually been able to make _some_ improvements recently, such as $entity->getOriginal(), the duplicate hook, entity attributes finally landed in 11.1 and so on. The way to get past endless circular discussions is to have more maintainers.

And I'd love to get reviews on some of of my issues (such as 🐛 Disallow saving the current default revision as a non-default revision Active ).

Added you with full maintainer rights.

self uninstall can be a bit of a problem, at least for regular numbered update functions but I think for post updates too.

The problem is that Drupal stores information about executed updates in key value storage. But if a module uninstalls itself, it results in a situation where drupal then stores that for a module that is no longer installed and won't be cleaned up. For regular updates, it may then result in warnings, post updates less so, but might still end up with a little bit of cruft in the database. I'd consider moving it to navigation module just to be extra sure.

I'm absolutely open to having more active maintainers, want to create a separate issue to make it an official request? You're welcome to start a 2.x branch, there's quite a bit of cruft that did in fact make it into core, such as 🐛 uses of drupal_set_message() Needs review , would be a chance to clean that up.

That said, a separate project, or an approach with multiple modules within the project or something might make it easier to deprecate and remove functionality that does make it into core. Similar to ctools, I originally thought this would become some kind of sandbox/incubator project to test stuff before it goes into core but that's not really compatible with being a required dependency of projects like commerce/group/. Your call.

No, this is definitely not in the release. It might conflict with that though, I didn't test that.

I'm not sure.

To be perfectly honest, I'm just looking a little bit after the entity module because nobody else does. And I just had a quick look through recent issues while preparing a new release. I'd rather remove/deprecate things than add more in general.

And duplicate features has like 7 competing contrib projects that are all very similar (we use replicate/replicate_ui)

We finally just managed to get hook_entity_duplicate() into core ( https://www.drupal.org/node/3268812 → ) and there's a core issue to add a duplicate operation directly into core as well.

Drupal CMS adopted a purely configuration based approach using ECA for duplicating features and just directly links to a pre-filled entity form. And with the alter hook in core, that is sufficient for many use cases.

Merged. field_group and search_api still cause some fails, removed max testing for now.

Merged. Not actually a 2.x issue.

Also, duplicate of 🐛 Views result cache does not apply cacheable metadata from Entity API's query access conditions Needs review I think?

the applyCacheability method is probably meant to do that, but i guess it doesn't apply to the data cache? Seems strange to do both, that might not be necessary then?

And why only cache contexts and not cache tags/max age?

A duplicate action would either go to the duplicated canonical or edit form of that entity, no?

> I also wondered if maybe we could do something clever and auto detect if a parent item is a "useful" page, but that requires us to define what a "useful" page is, and that's probably an unsolvable task given the variety of different implementation of "useful" pages in contrib and custom

I think it's not that unsolvable. We can treat it as an ignore list and only list things we know are save to exclude. On top of my mind, there's one obvious candidate, and that is the controller being \Drupal\system\Controller\SystemController::systemAdminMenuBlockPage. That should cover all the config overview pages that just list the links below. I don't know how readily that information is available where we need it, we'd need to fetch the route object I guess.

Also interested in this being resolved, I've already been getting bug reports about this in contrib projects, so I don't want to hold this up, could be a follow-up to optimize it?

this was added in ✨ Add re-index menu Active with basically no review. The whole feature seems rather naive, it basically assumes that indexing is done in a single cron run.

Seems OK as to get rid of the error, but I'd consider just reverting that feature.

Thanks, merged.

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

Merged.

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

Thanks for following up on this!

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

Merged.

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

I closed my merge request, this gets more complicated with 🐛 Message body missing from logs Active because we no longer have the ConfigFactory in OpenTelemetryLogs. What we did in our project now is alter the logger_provider service with this:

class ConditionalLoggerProvider extends LoggerProvider {

  /**
   * The config factory.
   */
  protected ConfigFactoryInterface $configFactory;

  public function setConfigFactory(ConfigFactoryInterface $configFactory): void {
    $this->configFactory = $configFactory;
  }

  /**
   * {@inheritdoc}
   */
  public function getLogger(string $name, ?string $version = NULL, ?string $schemaUrl = NULL, iterable $attributes = []): LoggerInterface {
    if ($this->configFactory->get('opentelemetry.settings')->get('endpoint')) {
      return parent::getLogger($name, $version, $schemaUrl, $attributes);
    }
    else {
      return NoopLogger::getInstance();
    }
  }

}

This works for us, I still think that something like this should be the default behavior though.

berdir → created an issue.

I was wondering about the backtrace string, noticed that too but wasn't sure if that's too opinionated in case some backend doesn't support attributes like that. But agree that seems like a sensible default and if someone wants that they can propose a change to make it configurable.

Rebased and updated 📌 Triggering deprecations for plugins using annotations when core plugin type has been converted to attributes Needs work , next step is removing the simple remaining conversion that we missed into separate issues. That now also includes a deprecation for plugin managers that only provide an annotation class. That passes now, there was a single deprecation caused by a special test plugin manager which is now updated too.

So yes, I can confirm that core is now fully converted to attributes, minus the small bits in that issue.

As for this meta, I agree that 📌 Triggering deprecations for plugins using annotations when core plugin type has been converted to attributes Needs work is essential but IMHO, 🌱 [policy, no patch] Allow both annotations and attributes in Drupal 11 Active is not a child of this critical task but a follow-up. It is not required to get rid of annotations, "only" a performance improvement, and it will only become realistic for most sites once majority of contrib projects have caught up with this.

Yes, dealing with html tags is a good idea. I wonder if we want to do something specific about
tags though, kind of the opposite of nl2br(). The perimeter module for example uses that, right now they're pretty unreadable (Banned: %ip for requesting %pattern <br />Source: %source <br /> User Agent: %browser). But half of that is duplicating information that's already logged (ip and referrer), so this could likely be simplified and shortened anyway.

I'm OK with removing link, I forgot that it's a HTML link and not just a URL in my test additions and it's the reason they failed as well afaik (checkout prefix on Gitlab CI). We could try to extract the URL, it can be useful info, but likely more trouble than it's worth.

I'm not sure if #7 has been addressed. Still feels very weird and problematic that we have two parallel ongoing process that save the role entity and both make their own changes to it, as I wrote there, this only works because they rely on the static cache, their changes are combined and saved twice.

I did rebase the linked issue (and reopened the closed MR), and then merged in this MR, the resulting diff is:

$ git diff
diff --git a/core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryNodePagePerformanceTest.php b/core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryNodePagePerformanceTest.php
index 903c4fba006..9aaef449198 100644
--- a/core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryNodePagePerformanceTest.php
+++ b/core/profiles/demo_umami/tests/src/FunctionalJavascript/OpenTelemetryNodePagePerformanceTest.php
@@ -260,7 +260,6 @@ protected function testNodePageWarmCache(): void {
       'SELECT "t".* FROM "node_revision__field_summary" "t" WHERE ("revision_id" IN ("75")) AND ("deleted" = 0) AND ("langcode" IN ("en", "es", "und", "zxx")) ORDER BY "delta" ASC',
       'SELECT "t".* FROM "node_revision__field_tags" "t" WHERE ("revision_id" IN ("75")) AND ("deleted" = 0) AND ("langcode" IN ("en", "es", "und", "zxx")) ORDER BY "delta" ASC',
       'SELECT "t".* FROM "node_revision__layout_builder__layout" "t" WHERE ("revision_id" IN ("75")) AND ("deleted" = 0) AND ("langcode" IN ("en", "es", "und", "zxx")) ORDER BY "delta" ASC',
-      'SELECT "config"."name" AS "name" FROM "config" "config" WHERE ("collection" = "") AND ("name" LIKE "workflows.workflow.%" ESCAPE \'\\\\\') ORDER BY "collection" ASC, "name" ASC',
       'SELECT "revision"."vid" AS "vid", "revision"."langcode" AS "langcode", "revision"."revision_uid" AS "revision_uid", "revision"."revision_timestamp" AS "revision_timestamp", "revision"."revision_log" AS "revision_log", "revision"."revision_default" AS "revision_default", "base"."nid" AS "nid", "base"."type" AS "type", "base"."uuid" AS "uuid", CASE "base"."vid" WHEN "revision"."vid" THEN 1 ELSE 0 END AS "isDefaultRevision" FROM "node" "base" INNER JOIN "node_revision" "revision" ON "revision"."nid" = "base"."nid" AND "revision"."vid" IN (75)',
       'SELECT "revision".* FROM "node_field_revision" "revision" WHERE ("revision"."vid" IN (75)) AND ("revision"."vid" IN ("75")) ORDER BY "revision"."vid" ASC',
       'SELECT "t".* FROM "node_revision__field_cooking_time" "t" WHERE ("revision_id" IN ("75")) AND ("deleted" = 0) AND ("langcode" IN ("en", "es", "und", "zxx")) ORDER BY "delta" ASC',
@@ -286,7 +285,7 @@ protected function testNodePageWarmCache(): void {
     $this->assertSame($expected_queries, $recorded_queries);

     $expected = [
-      'QueryCount' => 172,
+      'QueryCount' => 171,
       'CacheGetCount' => 247,
       'CacheGetCountByBin' => [
         'page' => 1,

I suspect the test fails here were related to the JS problems we had with not running tests. The script bytes count was way off.

I also looked at adding an explicit CacheTagGroupedLookups assertion, but it's a lot of cache tags. many, many blocks, many image styles and entities.

Like in a similar issue I commented on, I'm not too fond on asserting those 170 queries 1:1. It's a lot. If umami changes it's default content, resulting in different ids then it's going to be a pain to update those performance tests. Or a new field or something like that. Also, the default_content api of recipes/default_content module doesn't guarantee ids, so if we switch to recipes, then we can't really rely on that anymore. My vague idea is that we'd filter out any entity load queries and just replace them with counts. So we'd assert 172 queries total, 100 node load queries, 20 media load queries. Or maybe even go a step further and handle it like cache lookups. we don't track single node load queries (except maybe one specific test, but just track how many nodes we've loaded, in groups, similar to how we assert cache and cache tag operations now. Or yet another option, we run the test on warm entity caches by loading all nodes upfront.

berdir → changed the visibility of the branch 3476422-add-assertions-to to active.

This (authenticated user + content moderation + content translation) is currently not a scenario that has explicit enough test coverage to cover this. The test methods in \Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryNodePagePerformanceTest hit this scenario and there are issues to expand explicit assertions on them. I specifically see the query that this removes in 📌 Add assertions to OpenTelemetryNodePagePerformanceTest::testNodePageWarmCache() Active (search for workflows in the query list) and others might have hat it too of that meta, one that has a warm cache is likely more interesting as it has fewer queries in total.

But those issues have been stuck for a while and aren't trivial to get in due to their risk of random fails.

And it doesn't help that much to add explicit test coverage here, because those things are only really useful if they are already in HEAD and you can see the difference they make in a diff.

That sounds good to me. Updated the logic and also adjusted the tests, to use proper context and extend with an exception to get test coverage for what I could.

The tests are written in nightwatch and it took me a while to get them to run and have opentelemetry running as it expects, but eventually got it to work. But it didn't like the escaped, I also don't think it could do any other kind of asserts on those logs, such as that the backtrace exists without checking the exact value, or that certain things are _not_ logged. so I just put in there what I could assert.

This is being incorporated into 🐛 Message body missing from logs Active , which contains massive changes to how logging works, reviews welcome.

edit: wrong issue.

I pushed a few improvements to the MR now, implementing option 1 from 15 with a single logger (not sure if the logger should be kept in a property now with this, creating one does go through several factories and collects a bunch of stuff).

I had to refactor the attributes part a bit, as it was running after message replacement, so the logic with the placeholders didn't work, including the existing match on %function and so on.

This now results in:

{
  "body": "Request GET /admin/config/development/opentelemetry, trace id 1cadec4301a25007cf21634df06114ac",
  "id": "2tAzfwrLk2SggLshd3y2GxgxobC",
  "timestamp": "2025-02-17T16:29:33Z",
  "attributes": {
    "client.address": "172.19.0.9",
    "org.drupal.channel": "opentelemetry",
    "org.drupal.referer": "https://DOMAIN/admin/config/development/opentelemetry",
    "url.full": "https://DOMAIN/admin/config/development/opentelemetry",
    "user.id": "1"
  },
  "resources": {
.....
  },
  "scope": {},
  "severity_text": "DEBUG",
  "severity_number": 5,
  "scope_name": "org.drupal.opentelemetry_logs",
  "scope_version": "",
  "span_id": "8e88c0dec8755205",
  "trace_flags": 1,
  "trace_id": "1cadec4301a25007cf21634df06114ac"
}

I was quite confused about the event.name thing. Because the issue summary currently talks about that, but when testing the MR, it's gone.

Took me a while, but that's because of the EventLogger that's removed here, this was apparently deprecated and is already removed from the documentation (the deprecated docs are a 404: https://github.com/open-telemetry/opentelemetry-specification/pull/4353). As @znerol said, the whole logs stuff seems quite unstable and in flux.

That explains why the event.name attribute is gone and the channel is now set as the scope name instead.

I also found https://opentelemetry.io/docs/concepts/instrumentation-scope/, which to me sounds like the channel is indeed a reasonably good scope, but it also links to https://opentelemetry.io/docs/specs/otel/glossary/#instrumentation-scope, which says the library is a good default choice, which would again by the default org.drupal.opentelemetry I suppose. But it also says "A typical approach to ensure uniqueness is to use the fully qualified name of the emitting code (e.g. fully qualified library name or fully qualified class name).".

So IMHO, it's one of two options:

1. We always use scope name "org.drupal.opentelemetry", and put the channel into attributes per #12). That gives us a stable, reliable way to identify drupal logs that doesn't require partial matching or something.

2. We normalize the channel into org.drupal.

I think I'd prefer 1, like @znerol.

Merged.

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

Yes, something like that. There's still the question of whether or not the scope name (that now uses the channel) should also be handled like that (org.drupal.access_denied instead of "access denied".

The fix looks good, my concern with this is that there is always a prefix even if not explicitly set, this means that all current queue items will become inaccessible with this change.

One fairly complex option would be an update function that loops over all queues, if they use the redis backend, we'd use https://redis.io/docs/latest/commands/rename/ to change it to the new prefix.

The current key prefix actually includes a hardcoded "drupal:" prefix. We could remove that, then people who were using this would have the choice to name their cache prefix just "drupal" and essentially match the current keys.

I see, the new API only has a single return URL. I guess relying on separate vs cancel URL was a problem as you could manually alter it. One option would be to just verify that the payment was a success without storing/placing that information and letting the webhook deal with it.

I've added a setting for the wait time and I removed the locking. A lock is exactly what loadForUpdate() does, and that's now committed in commerce, there's no benefit to having two locks.

I see that https://git.drupalcode.org/project/otlog/-/blob/1.x/src/Logger/OtLog.php... does set the backtrace on the standard attribute, no reason to not add that here as well I guess.

As for string vs array, not sure how much configuration there should be, if the backtrace is there as well, there isn't much left. currently, it's already passed through twice (as string and as array), so doesn't really hart to reduce that to one as the official attribute.

One option would be to unset anything from $context that's explicitly used (essentially covering 🐛 Opentelemetry logs - context/ placeholder replacements gettingprefixed to log message Active ) and any keys that are left in there coud be added as a drupal.XY attribute.

Yes, this is what I do now:

// Automatically set the environment for OpenTelemetry if not already set.
$otel_resource_attributes = getenv('OTEL_RESOURCE_ATTRIBUTES') ?? '';
if (!str_contains($otel_resource_attributes, 'deployment.environment')) {
  if ($otel_resource_attributes) {
    $otel_resource_attributes .= ',';
  }
  $otel_resource_attributes .= 'deployment.environment=';
  if (getenv('PLATFORM_ENVIRONMENT_TYPE') == 'production') {
    $otel_resource_attributes .= 'production';
  }
  elseif ($platformsh->inRuntime()) {
    $otel_resource_attributes .= $platformsh->branch;
  }
  else {
    $otel_resource_attributes .= 'development';
  }
  putenv('OTEL_RESOURCE_ATTRIBUTES=' . $otel_resource_attributes);
}

It's fairly complex, but managable.

I tested this a bit, and from what I see, the event.name that currently is there isn't just replaced, it is completely removed. I didn't understand that there's a scope name (which this now set to the channel, which is clearly much better than the service name which is currently duplicated into this).

Looking at https://github.com/open-telemetry/opentelemetry-specification/blob/main/..., logs having an event.name makes them "events", a special type of more structured logs? I think it makes sense to keep that, we can easily do both, and keep event.name = drupal-log and set the correct scope? IMHO it is useful to keep a way to identify all logs coming from Drupal vs other logs? The alternative would be a namespaced scope_name as mentioned.

As to the actual log body, in HEAD that is as a JSON structure that contains the message. With this change, it's *only* the body. Quite a bit of stuff in the current body is duplicated and repetitive, but not everything. For example, Drupal logs often contain a referer, this is lost. Exceptions also contain a backtrace, this information is also lost (if not used as a placeholder in the message). Per the specification, both is allowed.

Merged.

Revered one unnecessary change that is an API change, merged.

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

Finally green again! Removing ckeditor (4) dependency was easier than expected, just 2 default configs and 1 test that needed to be removed and everything else is already done with ckeditor5.

berdir → created an issue.

Did you see my proposal in the other issue? I think there needs to be an option to disable it completely, and the easiest option would be to require an explicit config. that can be the default config, then for a new install, nothing changes.

And this scenario wouldn't be a problem, it will just not log until the config is in place.

> The module should be able to be enabled without requiring configuration and without generating any log messages that could prevent deployment.

I don't disagree, that's exactly what I'm arguing for as well in the other issue. Which IMHO confirms my point that this is a duplicate of the referenced issue.

I'm using SigNoz and the message is part of the body. Signoz has features to extract things from the body and populate into attributes.

I'm also doing that to make the channel available, and I'm using the event.name to identify logs from Drupal vs system logs and other applications. If the channel is set as event name, how would you do that? Also, I'm still pretty new to OpenTelemetry but channels are often user readable strings with spaces, which I think violates naming guidelines, at least what it should be according to https://opentelemetry.io/docs/specs/semconv/general/naming.

> Please do not force api module to chase an entire chain of ancestors, always define both constants.

I don't know how complex that would be, didn't look at the code. But either we enforce that or we'll need to support it (or live with hook documentation/discovery in api.module not working). People will extend hook classes and expect that to work because it works for actual discovery.

If that really is a considerable complexity and we want to enforce it, we need to make those classes final. Which I very well know how much you dislike that. And then Alter maybe shouldn't exist in the first place, my suggestion for adding that (here in this issue specifically) was exactly so it can be a base class.

It's still not clear to me if the expectation is that eventually all hooks should have a custom Hook attribute class (and we use it to document said hook) or just common/frequently used ones.

There's an older, existing issue, please ensure that covers the same changes: 📌 Nullable types must be explicit Active

I don't really see the point of this, what's that action supposed to do exactly? This doesn't "translate", it just copies the existing values into a translation.

This is something that for example belongs into TMGMT, not core?

This is a duplicate of 📌 Do not allow existing or reserved paths as aliases Needs work .

berdir → created an issue. See original summary → .

Pushed a fix, was missing the assignment of configFactory completely now.

This doesn't happen by default, it needs something that calls toUrl() on that node in preprocess, a template, formatter or so.

Didn't try to reproduce yet, but I can't imagine that we don't have test coverage for this, between dynamic page cache tests, performance tests and so on.

Ah, the comment talks about the fast404 *module*. But Drupal has a built-in fast404 mechanism. I think that has been improved/fixed somewhat recently, so maybe that's the difference?

berdir → created an issue.

Merged my MR 2 which is identical to 6.

Created a test but I'm still unsure about this scenario, something else might have been off when I've been running into this problem.

Obviously, any write or delete directly on the consistent backend would not update the fast backend correctly, so I'm not sure this is worth fixing.

Setting to postponed for see if anyone else runs into this.

This can happen if \Drupal\Core\Cache\ChainedFastBackendFactory::get() bypasses the fast chained backend and just returns the consistent one directly.

That should only happen during the installer or if apcu_fetch isn't defined, implying different extensions for CLI and webserver.

The safe thing to do is to always update/delete the last write timestamp explicitly in that case.

Also, per that issue, when implementing a static cache here, EntityUuidResolver must be updated to actually use this API instead of doing a query on its own for jsonapi to be able to benefit from this.

Verified that there's nothing left in entity_test.module except the two deprecated functions.

I'm not sold yet on the Callbacks class/pattern used here in general, but for this module, it's an existing pattern that's just used here too.

Yes, \Drupal\Tests\navigation\Functional\NavigationTopBarPageContextTest is a very similar test that doesn't have content_moderation enabled.

Yes, would make sense to not having to go through a real browser here. I was considering for a second if I should try to use the trait on a functional test and expected some kind of problem when using that to access non-HTML pages, but it worked so I just went with that.

FWIW, I think it makes sense to get this committed without any improvements first, and then we can use the test in issues like ✨ Add static cache for loadEntityByUuid function to store uuid-id pairs in memory Active to show the improvements.

Those are false positives, this module is fully compatible with D11

\Drupal\jsonapi\ParamConverter\EntityUuidConverter() is the problem, and the fact that it does the upcasting when generating routes, so we do that once for every route that contains the node UUID.

It also doesn't even go through \Drupal\Core\Entity\EntityRepository::loadEntityByUuid() at the moment, so a static cache won't help without changing that. We should be able to change that though.

And on a collection with 50 nodes, it's going to do that with 50 different UUID's.

The request/route is based on the UUID, so one UUID lookup is expected, but no idea why it's then doing it again so many times, looks like something during normalization does that. Should be pretty easy to find, a static cache would hide that, but maybe we can also prevent it from happening.

And yes, block_content was done as a cache collector under the assumption that there's a somewhat limited amount of block_content blocks placed as regular blocks. that doesn't scale and I think a 1:1 cache isn't that useful either.

There's a german saying that's basically "Trust is good, control is better". As always with the performance tests, they're full of interesting things.

I added a test that visits a json api resource (node detail page) on a cool cache (frontpage visited, but no jsonapi route yet), then one with a warm dynamic page cache cache, then invalidates that node and visits the page again.

The most obvious thing that I saw is that there are around 9 uncached loadbyUuid lookup queries for the same UUID, didn't look into that yet but that seems like a very obvious target for optimizations, especially considering that uuid entity queries are really inefficient as they join the data table for absolutely no reason at all (existing open issues).

The route lookups are as expected, 6 separate route lookups that are then separately cached, and with the current implementation in [##3503843] would no longer be cached. Note that it's 8 data cache lookups (1 for the route match, then 7 route preloads, including the full frontend route preload even though that actually excludes jsonapi routes. I think my suggestion in the other issue to keep track of routes by format could be interesting as we could keep all of them in discovery cache.

Another thing is the resource type caches, those could be candidates for the discovery or bootstrap bin instead of default?

🐛 RoutePreloader loads a lot of routes Active is a good example of something that impacts JSON:API. It might not have the same impact as a complex and slow filters on a very large database but in its current state, it would add 10 or so queries to entity types, even single ones with a bunch of reference fields.

Just a drive by comment, but there might be a completely different "solution" for this: Don't include a body field at all.

The reason Drupal core is phasing out the (automatically added) body field is that it's in many, many cases replaced with a different content building mechanism. Paragraphs, layout builder, experience builder and so on.

Is a body field crucial to this recipe? Or could you just say that site builders likely know how to add such a field if they want to add more content to their locations and will do so using whatever tools they are used to.

I've given up trying to argue that we shouldn't support the complex grouping stuff, but I still think we can at least get rid of the use case we have in core: 📌 Simplify ckeditor5_module_implements_alter() in favor of checking for ckeditor5 in media module Active . It might not save much in lines of code (given that we have to keep support for such use cases) but it makes it IMHO self-contained and easier to understand.

media module does now rely on the implicit default hook order based on module name. It also is completely unclear to me why we implement this twice instead of using the base form id like editor and ckeditor5, but wasn't sure about the scope here.

berdir → created an issue.

The threads are mostly just explanations on the changes because several of them were very complex to figure out and resolve. I'm not sure how extensive the inline comments should be. I'm both looking for general reviews and feedback and suggestions on if and how code documentation should be extended/adjusted.

Phew, things are moving fast at the moment, created a change record just in case and also updated https://www.drupal.org/node/3505248 → accordingly.

I agree that this is a very rarely used feature, but it is sometimes used, http://codcontrib.hank.vps-private.net/search?text=field_type%3A%20float... has 3 pages of results (quite a lot are just tests). default config of course still works, just linked to that for use cases.

Unlike an optional module that non-developers can install, the no_ui flag requires that you implement an alter hook to be able to still add them through the UI.

As a compromise, I added an untested example hook to revert this, if someone creates a module for doing that they can share it there.

Yes, this is still relevant, and it's still an issue. There are still core implementations that do it incorrectly such as Menu. save and delete methods really should be final, but I don't know if we have a process to announce that.

avpaderno → credited berdir → .

czigor had been looking after the project in recent years, as we only have one small client that's still using it.

I've given you maintainer access and I also posted a review on https://www.drupal.org/project/commerce_datatrans/issues/3428427 📌 Automated Drupal 11 compatibility fixes for commerce_datatrans Needs review , feel free to address that and then commit and release it.

We could do something like that, but it's a bit unclear to me how it helps us in the longterm.

I'm a bit unclear about the scope of ✨ Configurable views filters to allow for different widgets Active as it is. There are kind of two rather different goals I think. One is essentially BEF in core, where a sub-type sounds interesting, but that's just the filter UI then.

The other part is dealing with views issues like entity_reference, datetime as well as 📌 Allow @FieldType to customize views data Needs work (essentially, expanding views/configurable fields integration to base fields), where we need a system that allows us to offer improved plugins (not just the form element, but also impact on queries, settings and so on) but also have a BC system for existing views.

Did a very basic test and I counted 10 or so router queries on a jsonapi route for a single article, they are now all uncached, on HEAD they are single data cache lookups.

I didn't verify, that's why a test would be useful, but I think so? JSON:API contains lots of relationships and references to other pages, and I think they are created as routes?