🇨🇭Switzerland @berdir

I saved the credits, @pfrenssen clearly looked at the contribution UI and used the generated commit message, but it's very easy to miss that you have to actually save them and have to be logged in to do so.

Added some comments.

We've been discussing the user interface thing since comment #32/#40 8 years ago. IMHO, that user is a module and not a core namespace is an implementation detail and just because it contains things that historically had to be and still have to be in a module, such as hooks, routes and so on.

RevisionLogInterface depends on the user entity type. *That* is the dependency, and that's not going away. UserInterface and entity type user is the same thing (while AccountInterface is *not*, as I've said back then, I regret creating that). Just because one is a string and the other is an interface doesn't make it more or less of a dependency. Introducing OwnerInterface doens't change that.

I *do* agree with #32.2 though, reading it again. No reason for preSave() to be on ContentEntityBase, lets move it to the same base class as the preSaveRevision() implementation.

If a an entity type has no bundles then the bundle is the entity type. For user, that is user. That means ->getFieldDefinitions('user', 'user')
See \Drupal\Core\Entity\EntityTypeBundleInfo::getAllBundleInfo() and \Drupal\Core\Entity\ContentEntityBase::__construct().

You can also see this in \Drupal\Core\Entity\Plugin\DataType\EntityAdapter::createFromEntity(). bundle is always set.

I'm not sure where EntityDataDefinition::create('user') is called as opposed to EntityDataDefinition::create('user', 'user').

We could add a special case to fall back to that but I think it's up to the caller to set the bundle in this case, same as EntityAdapter.

I can only repeat what I've been doing for 10 years, multiple bundles and no bundle (which isn't really a thing as there is always one bundle) is very different and shouldn't be mixed up in the same issue.

Reviewed.

This includes a lot more than just line length issues.

No, this wasn't fixed, what I did in pathauto is a workaround, I shouldn't need to duplicate this method.

Agreed with the suggestion, you essentially need to add more paragraph specific checks, don't call label()

On 2, I think what that essentially means is combining Status and Used in into a single column, as it can be a mix of both for different usages. I think "Status: Used in old revisions" is a valid thing to say as "Status"

Multiple things here:

* 🐛 Uninstalling and installing modules during config:import can lead to fatal errors Active might help but we probably want to extend it to alter hooks, but it will just silently not call those hooks, resulting in incomplete entity type definitions.
* It's essentially 🐛 ModuleHandler skips all hook implementations when invoked before the module files have been loaded Needs review again, which we had thought we'd fixed.
* Most proposals/changes are workarounds to the underlying issue that it is not supported to invoke hooks before modules are loaded, as the date formatter example shows, even core hooks are bad at this.
* That's a legacy problem though, OOP hooks will work fine and autoload when needed. It's still not a good idea for performance to do this.

> Drupal\Core\Datetime\DateFormatter->__construct() (Line: 259)

* Something is injecting DateFormatter very early, you could set a breakpoint and figure out what. It calls "$this->dateFormatStorage = $entity_type_manager->getStorage('date_format');" in its constructor, it should not do that, entity storages should not be injected. This is what triggers the hook and breaks stuff. We can and should fix this in a separate issue, just inline that call into the dateFormat() method. Maybe BC for the old property with __get().

Oh, I wasn't aware that I can't give you access here either myself.

Either way, I think there's plenty that you can help without having access, based on the discussions we've had in the linked issue:

* Draft new project pages for both modules (I'd suggest an issue for each module and put it in the issue summary so I can copy the raw HTML), for the current state, and possibly also preparing for the 2.x plan by mentioning that, then once it's starting to be ready, we can switch to making that the focus)
* Create an issue to drop usage of the replicate API and just call $entity->createDuplicate() directly, we can make that the start of the 2.x branch.
* Work on other issues, such as the redirect one, more general, adopt functionality we consider useful from replicate_actions per #3554706: Merge functionality into replicate_ui → , each configurable I'd say. Quick notes on that: unpublish: Yes, but we should also support content_moderation with a configurable target state for replicated entities (publish could be on by default, but content moderation requires a setting as states are configuration, could check if draft is available. see similar logic in tmgmt). set owner: yes (maybe on by default?). redirect: yes, i think even by default/unconditionally, group integration: We actually have that custom, but also exposed in the UI where you can optionally alter the target group. lower prio, maybe as a separate integration module.

While not as widely used, I'm hesitant to leave behind the project and it's 10k user base. People are slow in adopting new projects, they're also somewhat slow to adopt a a new major version, but I think that's easier.

I think we can solve the discoverability problem. Searching for duplicate ( https://www.drupal.org/search/site/duplicate?f%5B44%5D=&f%5B46%5D=&f%5B3... → ) actually finds replicate first of the various duplicate/clone projects. Just not replicate UI, which currently doesn't mention the "duplicate" term at all.

We could for example name the project Replicate UI (Duplicate) or so, and also list the related/alternative duplicate/clone modules (which has the side effect that it's also finding it by searching for clone). If you want to draft a new project page that explains this better that would definitely be welcome. I also plan to add you as a maintainer.

We can also update Replicate to replace all the D7 noise with a clear set of instructions prominently list (only?) Replicate UI as the module you want. I don't have full maintainer permissions there and can't add you, but I can edit the project page.

In regards to making it "THE Drupal entity cloning module", you won't have to convince me but the other projects. I'd suggest issues, especially in the smaller ones to suggest listing alternatives and specifically this project. But we should only do that once we're more "presentable".

I'm derailing this, but in the slack thread about this, we discussed that we need to find new places for all remaining functions in .module files anyway. It could be a new service with those methods ported 1:1, but I'm not sure it's really worth it. Using the entity API is not that much more complicated (we could add 1-2 helpers such as a a way to add multiple permissions, and then this would be the responsibility of the calling code to decide what to do in that situation.

Also, recipe config actions are kind of a replacement for this as well, think we should re-evaluate whether node_install() and media_install() should exist at all or if it should be the job of recipes and distributions to do this. There is after all a very long comment in node_install() why you shouldn't do that.

I think it's fine to close this. this predates media in core, file entities are mostly an invisible, non-translatable technical entity type. The media name is the visible part and that can be fully customized and translated.

FWIW, that's exactly what I suggested to @nicxvan as well in a slack message when he pinged me about this, this is something that user module should be responsible for.

inline blocks aren't reusable, it's not supported/possible through the UI to use an existing non-reusable content block on a different entity. They are reference by revision for two reasons:

* Support for content moderation/pending revisions
* Multilingual support, specifically in combination with content moderation (I have no idea how well this works in layout builder, never really used it myself, but it's not unlike ERR)

Especially the first isn't really a concern for content export/import and I wouldn't worry about that too much. IMHO, I'd treat this as a composite/embedded entity, exactly like ERR, because it's very much inspired after ERR/paragraphs. That means load the revision, inline it into the nested structure, and on import, create it as an entity again with the id and revision id that get from that.

Extended the comment a bit. Feels a bit redundant to add it twice, but also overkill to refer from one to the other.

Ah, partially misremembered. The previous revision bit actually comes from paragraphs, in its label method: \Drupal\paragraphs\Entity\Paragraph::label().

entity_usage would need to duplicate similar logic into the list controller that generates that table to pull that out of the label and instead display it in the Used in column.

This a known issue and there's only so much that can be done about this with the current architecture. Published is actually the paragraph I think and not the node, unless there's custom code for that too.,

entity usage tracks paragraphs. And then the usage table attempts to backtrack that paragraph reference to the host entity which can involve multiple jumps, each of which can no longer be in active use. As in, the paragraph references to it's host, but the current revision of that host does no longer use that paragraph.

The code for that predates the "Used in" column and wasn't adjusted for that. It should be possible to improve it a bit and report it in the same way, but it will always be a complicated and slow approach that requires working up the parent tree and comparing the back-reference against the actual reference.

A long time ago, I proposed a different approach to tracking usages where such composite entities that don't really exist on their own are all assigned directly to the host, but that requires API changes to the plugin structure, see #3002332: Track composite (e.g. paragraph) entities directly and only as their host → . There's some work there that I haven't reviewed yet.

The failing EntityRevisionsTest actually explicitly asserts that the created timestamp is not revisionable, so more conflicts between tests but I can just revert that for now.

I opened 📌 Stop discovering theme preprocess twice for theme engine discovered templates Active , it's green.

I had one fail in a test that first looked related in ThemeTest about switching the default theme, but it was an assert on a config value and must have been a random fail/race condition?

Discussed on slack, we agreed to remove this optimization. I kept and adjusted the test and extended it to ensure that caches of another entity aren't invalidated and also checking both the persistent and static cache as there is separate code for that.

I'll open a follow-up to add this back.

berdir → created an issue.

I'm not sure. A better name has advantages, it's one of the reasons people don't find this project. But I'm not really interested in a fresh start, most of the code has a reason. not sure if yet another clone module will help with discoverability.

Also with the API in core, we don't really need to have just one UI module/approach as long as the functional stuff below is consistent between them. Drupal CMS for now decided on now UI and just build something on top of ECA. That should still be compatible with the duplicate hook.

The entity issue with the event there is IMHO similarly obsolete and I don't like relying on entity module, despite being a maintainer. For me, entity is similar to the original idea of ctools for D8+, an incubator for ideas and experimental features with the long term goal of getting useful things into core. But if they do, they will then often be different. Same for this case, instead of that event, it's much better to rely on the core hook now. To properly manage that in entity module, it would need frequent new major releases and deprecating things as they get in core. That's pretty tedious for everyone. Maybe it should consist of multiple modules with a separate lifecycle.

With a 2.x branch, I'm happy to make some breaking changes, for example the redirect to edit form, I think it's clear that it's better in almost all cases, just tricky as a behavior change for existing sites. With 2.x, we could just change that, doesn't even need a setting IMHO. We can also improve the project page, even consider using Duplicate instead of the very technical Replicate in the UI and so on.

I saw you tried to add the deprecation, but that caused a of deprecations, the pipeline on the version where only deprecated theme engines never completed.

The problem is that this logic with the theme prefix is not OOP aware. Either it's not needed or it's something we need to convert. We run through themes again a second time a below, I suspect with that, this might be completely necessary or even harmful because it also won't consider legacy hooks, so might add both legacy and OOP preprocess for themes that rely on that. I think we have no test coverage for LegacyHook on preprocess for themes?

We can investigate in a separate issue as it's not directly related to this, but I think we should see what happens when we just comment out $prefixes[] = $theme;. My guess is nothing breaks. I only did a quick manual test but confirmed that nothing breaks visually on olivero frontpage, the later loop catches some manually verified preprocess functions and they're still called.

Pushed some fixes, both the tests and the implementation had some bugs, it clearly was too late last night and I was too tired to work on something like this.

Still not fully passing. The whole invalidation optimization stuff with the only-revisionable-fields check is very complex and the tests contradict each other. As mentioned in comment #203 above, I had to change the revision flag of the created field to actually be able to get to the optimized code flow and being able to test that part of the code explicitly. But now that this is finally improved and seems to be correct, the test with a non revisionable field is now broken, because that never really made sense. The logic in \Drupal\Core\Entity\ContentEntityStorageBase::doResetCacheOnSave explicitly excludes readonly fields, but the test field the test uses actually *is* readonly. So I would need to remove that readonly flag, but then the other tests would fail again because they now no longer go into the optimized case. So we'd need two different entity types and likely a new test entity type that's fully revisionable.

I'm very close to just removing the whole optimization and always invalidate all revisions. The regular entity cache, which would likely have the biggest impact on performance isn't optimized yet anyway, so we could look into both things together in a smaller, more focused issue.

This module is essentially obsolete with https://www.drupal.org/node/3268812 → . That is the actual way forward because there is now a core API that ever one can implement. Paragraphs already does.

My plan is to create a 2.x version of replicate UI that only depends on core.

I'd really prefer to do the big themes properly grouped and not just one big hooks class. And those in a dedicated issue.

We know better now how to group them and otherwise it will result in another chain of follow ups

A lot of failures, something isn't working yet.

The changes from 🐛 Impossible to save a pending revision as the default one without creating a new revision RTBC exposed a bug in the revision invalidation logic. I had removed the separate default revision cache tag earlier, back then, getOriginal() was still fairly reliably the default revision, that's no longer the case and it was never 100% guaranteed as you could set something yourself.

In that failing test, it resulted in the old revision still thinking that it was the default revision as it was loaded from persistent cache where it was not invalidated when an existing draft revision was resaved. Surprised no other test caught this, but our loadRevision() test coverage around state changes is still pretty slim.

I solved this by skipping the optimization when saving a default revision when the original revision wasn't the default revision. With the cache write back changes, we could also consider to reintroduce the default revision cache tag if we want to optimize that, but I think we can explore such optimizations in a follow-up.

What I did optimize was to not invalidate the loaded revision when not updating and it wasn't the default revision. I also added pretty extensive test coverage for revision cache invalidation. For this, I had to make the created field on EntityTestRev (and the used subclass EntityTestMulRev) revisionable.

This also exposes an additional possible improvement: Right now, the regular entity cache is always invalidated, we can also explore not doing that for an entity where all fields are revisionable and it's being saved as a non-default revision. Definitely out of scope for this issue but could be very worthwhile when working with workspaces/content moderation.

This needs a rebase now that the theme oop issue is in. The refactoring I did there should make it easier to trigger a deprecation on theme engine preprocess functions which I just see now we don't have yet as only those still us the prefixes loop.

Note: 25 of the 45 default cache bin lookups in \Drupal\Tests\demo_umami\FunctionalJavascript\OpenTelemetryAuthenticatedPerformanceTest::testNodePageAdministrator() are this. So we could save 24 of those in that scenario.

Always a bit awkward with those internal functions, which are then actually used in completely different places (hooks and the ckeditor plugin), we can't just inline it into the hooks class, we need to make it a service, maybe tagged with @internal. We could make it a private service, then it would be a separate object instance, with the shared memory cache instance.

We could also put this in discovery (probably additionally, 25 lookups to apcu with unserialize still costs us _something_), because it's tiny, frequently used and a single, stable cache ID.

> So I believe the abused form passes some POST requests to /antibot for some reason. Needs more work on this.

Yes, because it's a form submission which are POST by default, antibot currently doesn't change that. it would need to switch the method as well and not just the form action.

I agree this would be useful because POST requests are not render cached, so those antibot pages tend to be quite slow. But it doesn't even need the routing, mostly it would need changes on the form/the antibot JS.

Two updates:

a) There is now a hook in antibot, not sure if there's already a new release: ✨ Add a hook to allow modules to respond to validation failures Fixed
b) We actually managed to hit a case with a client that did run into antibot and then blocked themself, so I think a separate setting for this would be valuable. it should also be configurable as a whole O think, not everyone might want this. It's mostly useful for spam bots that keep trying.

I agree, but it's a behavior change and to be clear, it does not reflect the current state of this MR. Right now, 404 pages are completely uncacheable, internally and externally. We'll need to open another issue and align this, which means this is again/still unblocked.

Lets make sure we open and link that follow-up, but the fix makes sense as discussed in the review thread and we can revisit in the follow-up.

I had no idea you could add attributes like that to a closure function, inline in the same line even.

This is interesting should be fast and safe, the main downside I guess is that it relies calling that specific method. We could still try to match it up with the autowire traits to make it more convenient, but there's only so much we can do.

The trait is used in a lot of places (57 use statements in core), but I think the most prominent ones in core that are commonly extended and might contain closures in the future are indeed controllers, plugins and forms. If it's not one of those things then you also need to remember to use this trait.

Would be good to have a real example of this somewhere, or are we relying on the search block issue for that?

I'm not sure what about the simplesamlphp_auth project creates expectations that lead to comments like #42. It happens a lot. I've outlined in #39 what the next steps are, it really just needs adjustments of .info.yml and composer.json versions. And yes, given that there is a release candidate per #41, it should allow that release and shouldn't need any changes to the simplesamlphp/simplesamlphp dependency in composer.json. You're welcome to either work on that yourself or get in touch with a maintainer to pay them for their work.

I also want to repeat again that per comments #24-27, we have migrated away to samlauth as have several other people that were originally interested in this. I no longer have a personal interest in this project.

This needs a rebase.

This changes langcode for all tokens, that's not OK.

This needs to be fixed where source gets passed through, with getUntranslated(), this is done correctly for nodes and needs to be the same here.

This adds a ton of empty docblocks. I'll not commit this just to make phpcs happier.

Reviewed, lets actually make the phpstan job fail then with this if there are errors.

This adds back code that was removed. Instaed, the @todo above needs to be dealt with now.

In combination with 📌 {% trans %} does not support render array and MarkupInterface valued placeholders Needs work , is likely enough to just remove that.

The language block blocker is resolved. Started a look at the remaining test failures, but it gets "interesting" quickly.

PageCacheTest fails on the fact that with this, 404 pages are no longer cacheable in page cache. That is because \Symfony\Component\HttpKernel\EventListener\RouterListener::onKernelRequest throws a non-cacheable exception (as symfony doesn't know about cacheable exceptions), and then in \Drupal\Core\EventSubscriber\DefaultExceptionHtmlSubscriber::makeSubrequest(), we run this code:

      // Persist the exception's cacheability metadata, if any. If the exception
      // itself isn't cacheable, then this will make the response uncacheable:
      // max-age=0 will be set.
      if ($response instanceof CacheableResponseInterface) {
        $response->addCacheableDependency($exception);
      }

So this was kind of always the intention, but at the same time, we also want those responses to be cacheable and have a built a way to deal with invalidations with the 4xx-response cache tag. On HEAD, we set Cache-Control to must-revalidate, no-cache, private, set the debug header to Uncacheable... but we actually still cache it. I'm not sure what the right change here is. Should we change DefaultExceptionHtmlSubscriber::makeSubrequest() to not break caching? That will mean it will also be cached externally and in dynamic page cache. Maybe only ignore it for a client error to keep it in sync with \Drupal\Core\EventSubscriber\ClientErrorResponseSubscriber::onRespond? At a glance, the dynamic page cache test coverage does not seem to have explicit test coverage for a 404, so it might not cause test failures, but it will be a behavior change.

Interesting idea but how hard is to wire this this into the actual container building? And this would be a BC break for anything that already has a type on \Closure, I guess we could try to add autowire or something to detect that, but then it will only work if you use the correct type?

The Drupal\Tests\language\Functional\LanguageSwitching test fails I think means we're running into similar "complications" with 🐛 Big Pipe, logged-in users and 4xx pages Active as in the CacheOptionalIssue, some implementations there also "solved" that problem and I think this now does as well.

Thanks for sticking with this. Added some comments but didn't fully review the tests yet. You can include the fixes too, MR's have the test only job that should allow us to verify the test fails without the changes. And we can possibly also extend the test to ensure that we're not doing more queues than we have to on top of the fixes.

I'd rather just disable the phpstan job on previous major. This is a rolling target and complicated maintenance. We'll need another with D12 and so on.

There's minor benefit such as identifying possible usage of non-existing things that aren't conditional, but it's IMHO not worth the effort.

Added it to powered, messages and also page title at first but reverted that again because page title is already not cached further up, so this has no meaning there, we never get so far.

We'll want to review render times vs cache load times for this. I did some tests before around this. Performance tests are of course "happy", as in fewer cache lookups. But they don't measure if it's actually a good thing in terms of memory/cpu, at least not as assertions.

This has a bigger impact than I expected and skipping several config and even bootstrap caches, we probably want to investigate exactly why that is.

Thanks. I'm not sure how often we want to apply this pattern , at least not before the two related issues of which one is D12+ to deal with extra code and DX. But In this specific case, it results in a measurable improvement (maybe not in terms of actual time savings but in number of services instantiated) in a common scenario.

Added one optional suggestion to the MR, but I think this can go back to RTBC.

I think search module is a better component here as it's not (yet) a generic plugin system thing.

This looks good I think. Not sure I understand why the container dump wasn't necessary before, but it makes sense to add that.

Merged.

Expanding the scope here a bit. field UI changed again in 11.2 (field types are now a link, form structure changed again), I'm not going to add a 4th layer of BC conditions, so I'm dropping 10.2 and below, which means I can remove a good chunk of test and and actual version logic.

That includes removing the hal normalizer and the service provider class. I considered deprecating the normalizer, but it's code that would be a fatal error on D10 anyway, so there's no point in keeping that, nobody could possibly be using that.

Also have to add some new checks for body changes in 11.3.

berdir → created an issue.

I reverted thee original issue. FWIW, this was only in the development release, what is shown there in the screenshot was always consistent.

(The reason it was updated was the use of autowire, and it will soon be back, I'll try to keep it consistent then)

Also, the change to drush is unrelated.

I realize this is awkward after so long, but there hasn't been a release and I'd like to better understand what problem this actually solves, so I decided to revert this. What I'd like to see per #21 is a test that fails by pointing out a revision that has not been deleted as expected without this.

Yes, but we need to give the new properties a new name, for example formBuilderClosure, or __get() won't be triggered. If we'd adopt that naming convention, it could even be generic, similar to \Drupal\Core\DependencyInjection\DeprecatedServicePropertyTrait.

I'm less concerned about the constructor and more about the properties which change type and behavior. I think there is an option on how we could make this work, the question is if we care enough (short: renamed properties + __get()). It's unlikely that someone has customized the constructor and we BC for that, but if someone would access the properties it would be a fatal error now.

This doesn't apply.

Did a rebase after theme_get_setting() got in, added DI, relevant change:

$ git diff drupal-3544715/3544715-keyvalue-theme-previous core/themes/olivero/src/Hook/OliveroPagePreprocessHooks.php
diff --git a/core/themes/olivero/src/Hook/OliveroPagePreprocessHooks.php b/core/themes/olivero/src/Hook/OliveroPagePreprocessHooks.php
index dc1899b572f..bcd4c66a351 100644
--- a/core/themes/olivero/src/Hook/OliveroPagePreprocessHooks.php
+++ b/core/themes/olivero/src/Hook/OliveroPagePreprocessHooks.php
@@ -4,6 +4,7 @@

use Drupal\Core\Asset\AssetQueryStringInterface;
use Drupal\Core\Extension\ThemeExtensionList;
+use Drupal\Core\Extension\ThemeSettingsProvider;
use Drupal\Core\Hook\Attribute\Hook;
use Symfony\Component\HttpFoundation\RequestStack;

@@ -16,6 +17,7 @@ public function __construct(
protected RequestStack $requestStack,
protected ThemeExtensionList $themeExtensionList,
protected AssetQueryStringInterface $assetQueryString,
+    protected ThemeSettingsProvider $themeSettingsProvider,
) {
}

@@ -26,12 +28,12 @@ public function __construct(
*/
#[Hook('preprocess_html')]
public function preprocessHtml(array &$variables): void {
-    if (theme_get_setting('mobile_menu_all_widths') === 1) {
+    if ($this->themeSettingsProvider->getSetting('mobile_menu_all_widths') === 1) {
$variables['attributes']['class'][] = 'is-always-mobile-nav';
}

// Convert custom hex to hsl so we can use the hue value.
-    $brand_color_hex = theme_get_setting('base_primary_color') ?? '#1b9ae4';
+    $brand_color_hex = $this->themeSettingsProvider->getSetting('base_primary_color') ?? '#1b9ae4';
[$h, $s, $l] = _olivero_hex_to_hsl($brand_color_hex);

$variables['html_attributes']->setAttribute('style', "--color--primary-hue:$h;--color--primary-saturation:$s%;--color--primary-lightness:$l");

This really should have been a merge request, but wanted to get this in for a new release.

Merged.

This shouldn't result in a measurable difference *under normal circumstances*. At most it would maybe be a second here and there if we don't have to wait another second. But if your browser is somehow acting up like mine does then it makes a huge difference and it's ~30s faster per profiled request.

> To use the static and persistent cache with all entities, you can do an entity query to get the IDs then load them all by ID, but that can be a lot.

My proposal would essentially force you to do that by removing support for passing NULL (at least for content entities, or maybe all and a new method for config entities. Config entities could maybe also have specific cache support for loading all with a cache tag. non-cached config queries for that are still fairly common.

for content entities, you'd still need to take care yourself to cache the query if it really happens multiple times per request.

I updated the issue summary to better explain this, it was very updated. If we don't add type now, it will be very hard to add it in the future, see the code snippet I linked. Unlike services, method calls and so on, attributes can't be conditional.

We already have multiple attributes like this in the hook system, And we do have in fact ConfigEntityType and ContentEntityType for entity types. And are considering to support it for plugins too to avoid very large array structures and possibly BC issues like this.

Also, note that HookDependsOnTheme does not and will never make sense for how this is implemented. This check is made during discovery, these hooks do not exist then at all, including their Hook "service". While we will know if a theme is *installed* during container build once 📌 Convert theme hooks to OOP Active lands, whether or not a theme is active is a runtime thing. And I think installed or not doesn't really matter for a theme, it's whether it's active or not, so you need to do a runtime check for that.

> But at least we won't have: #[HookDependsOnModule(name: 'some_theme', type: 'theme')] then.

No, but we don't need to. We can always add HookDependsOnTheme. Or HookDependsOnCallback, or HookDependsOnFullMoon or whatever other use case that we didn't foresee where name/type doesn't fit. We can have multiple purpose-built attribute classes, it doesn't need to be just one.

berdir → created an issue.

Agreed, there aren't many good cases for that. The theme_get_settings() issue specifically struggled with the drupal_static() usage in ThemeInstaller. We have a neat solution now using cache tags, without that it's IMHO a grey zone and a shared cache between two services in the same component might be the lesser evil than having a public API that also exposes internals. Some languages have concepts for this (like protected in Java isn't just for the same class and child classes but classes in the same package). PHP doesn't.

drupal_static() comes with drupal_static_reset(), We have \Drupal\Core\Test\RefreshVariablesTrait::refreshVariables(), which specifically calls out to some methods that we keep out of interfaces, as they are not meant to be called by anyone else. More grey zones. Should a component like cache factory know about the testing system, or should the testing system know about cache factory internal static caches.

This was already RTBC and my comments were addressed. It's big and it does a lot in one single issue, has some drawbacks like more container rebuilds but it brings themes up to speed on what we've been working on around module hooks since 1y+.

We need follow-ups to convert the .theme files, I think one issue per theme would make sense and one or multiple for all the test themes (minus a few for BC testing I guess). Only a handful of hooks are already converted here, but there's quite a bit of testing, including updated unit tests around preprocess. Also, Gin has already been fully updated on top of this, so we have a good amount of real world testing.

We discussed this. We decided specifically for this for BC and a new feature would be a new attribute. Having a non-existing attribute class is just fine. Providing a named parameter to an existing attribute class that doesn't exist is a fatal error (this will be fun on plugin attributes too).

🌱 [policy] Standardize how we implement in-memory caches Needs work is now in, which means the generic cache bin is in place, and we can rebase this and build on top of that.

The Symfony API maps more to our cache contexts than cache tags (they have cache tags too, I don't if they scale as well as our checksum based logic). They say invalidation, but there is no invalidation going on there. The clarified that in the docs that are linked in the comment. The idea is that you'd dynamically calculate a namespace based on the current user or it's changed date or something that. So you don't invalidate, you just vary cache keys, which is what we call cache contexts.

Symfony does not enforce/automate namespacing, it just provides the API. They say "This is handled transparently" in the blog post, but it's only transparent *after* you manually and explicitly get your namespaced cache. I disagree that we should do this automatically and enforce it. It is IMHO valid to share memory caches between services and it would be impossible with this. Maybe with an explicit notation, I'm open to discussing that, providing an API for it to make it more convenient, but I'm against enforcing it.

I added a few more comments of things we should update.

I added a note to the CR, I'm not sure it really needs more docs or even enforcement. We standardized on common and shared cache bins like default, data, bootstrap, discovery early on in D8 instead of per-module bins like we used to have before. I don't think I ever had an issue with a conflict in any of these between different modules. This is the same API, so I think this is a well established pattern and people are aware of this. drupal_static() didn't enforce it, and it would result in different behavior for aan injected service vs \Drupal::cache() vs using the cache factory directly. I'm not sure that's a good thing.

Restored the missing tag, thanks for catching this. That's the whole reason we have to add the ignored deprecation, not sure why I missed that.

I had another look at the altered query. It's a revision query, so there can be multiple different revisions there, and it has a sort on it. I tried to think of a case where there could be a problem with this with revisions or sorting or something, but I couldn't really think of one. Limit 1 is still Limit one and should always result in the same ID being returned.

FWIW, I doubt that specific query results in an improvement at all, there's still the sort and joins and what not. But it also certainly doesn't make it worse.

I think we can just remove because it just got added and was not part of a release yet.

berdir → changed the visibility of the branch 3544715-preprocess-ignore to hidden.

With 📌 Remove Contact module from the Standard profile Active committed, this can no longer be done like this.

I'll let others decide if we want to close this issue as outdated or rescope it to removing this link without replacement, as it's still wrong of contact to provide this in a hardcoded way.

The changes look sensible to me, since a default is not set, it can possibly be simplified and the check for whether the scheme is set can probably be removed.

The changes also make it clear that the goal of the refactoring in 2.x was achieved, the patch is half the size and requires no adjustments to methods or interfaces as we pass around an array of settings now.

Docs were moved to a more sensible place, although the wording could possibly be improved a bit.

However, I do not have access to an environment where I can test this, so if someone needs this then I suggest you test the MR and set it to RTBC and I will commit this.

There is an existing issue for this: ✨ Support TLS for Predis Needs review

You do see a difference, the executed query changed. Performance tests assert which queries run and how many. We do not assert any time constraints, because that is too variable. Even if we would, with only a handful of nodes, we're talking about nanoseconds if anything at all. If you run those tests locally, you'll see likely see seconds of differences between each run, depending on what your system as well as those cloud kubernetes servers are doing at the same time. In other words, the savings of this are a tiny fraction of the differences this test has without any changes.

But if you have a million revisions, then things will look quite different, as you can see in the example in #5, where those queries then run 12x faster and save 5.5s.

Yes, it will not be possible to profile this in a reliable way, but less GROUP BY is always good.

The impact is visible in one query in the performance tests, no existing tests break, I don't think there is a straightforward way to test this explicitly as we afaik do not test anywhere what kind of query are actually generated by entity queries, just their results and that doesn't change.

Didn't fully think this through yet, so just setting back to needs review for now.

Proposed a fix that seems to be fixing this for our case.

berdir → created an issue.

See https://www.md-systems.ch/de/blog/2025-10-19/converting-legacy-hooks

All legacy hooks are scheduled for removal in d13, I don't want to move code twice, that just results in breaking patches twice.

See recent token and pathauto commits for examples

I'll look at the performance tests when 📌 ContentEntityStorageBase::loadRevision() should use the static and the persistent entity cache like ContentEntityStorageBase::load() Needs work which is more important.

berdir → created an issue.

@catch: correct. Both with disabled render caches.

Created a change record.

I realized that I messed up persistent revision caching in my refactoring and the only thing that broke was some performance tests. So I added some explicit test coverage for that.

I did a bunch of refactoring, split the revision and regular cache methods cleanly apart to avoid weird BC issues if someone would have overridden them. I also moved the persistent cache logic from SqlContentEntityStorage up to ContentEntityStorageBase. This was modelled after doLoadMultiple() I think, but it's different here because ContentEntityStorageBase defines the persistent cache methods and also handles cache clearing, so it was weird to have this split. No more changes to SqlContentEntityStorage with this, which means it should also work with other implementations if they exist.

Also worked a bit on the issue summary, next up is a change record.

On umami, usleep() looks like this:

Updated this to default to delayed, fixed some stuff and tested a bit on umami. I also get 37 calls to usleep() here:

I get huge variations for the same request (one request was 400ms, the other 500ms), but I see a clear reduction in IO wait:

I/O Wait-19 ms (-41%)
45 ms → 26.4 ms

I think this is at least major, because it currently more than undoes the actual improvements we got from the suspend issues.

This issue in this scope will not be accepted. It's way too big and has many incorrect changes due to the complicated rebase. It's failing tests and other jobs.

There other issues. Focus on one job, for example just phpstan. There are likely existing other issues for that. Start from scratch, a lot has changed, especially for phpstan, there are only a handful of issues remaining. Change the job to fail, see for example redirect.