🇨🇭Switzerland @berdir

You have a really awkward conflict between two things that both are problematic.

#13 /app/web/modules/contrib/kint-kint/kint.module(63): Drupal::config('kint.settings')
#14 /app/web/core/lib/Drupal/Core/Extension/Extension.php(162): include_once('/app/web/module...')

the kint module loads config directly in the .module file. That's a bad idea and it shouldn't do that.

#6 /app/web/core/lib/Drupal/Core/Entity/EntityTypeManager.php(192): Drupal\Core\Entity\EntityTypeManager->getHandler('key', 'storage')
#7 /app/web/modules/contrib/key/src/KeyConfigOverrides.php(73): 

key module seems to use the entity API in config overrides, and in this case, in the mide of loading modules, so some are loaded and some are not.

🐛 Uninstalling and installing modules during config:import can lead to fatal errors Active might help with the fatal error, but it then just skip some alter hooks and leave you with a broken and inconsistent state of entity type definitions.

My plan for now is to stick with this and make a new branch. I'll start with a MR when I have some time to drop the replicate dependency as a starting point to see how that looks. That's the first step anyway.

See https://git.drupalcode.org/project/drupal/-/commit/685ad2ba89418998c5912.... You need to stop checking the page variable in your node and term twig templates and look at the view mode instead (which you copied from classy)

Yes, I can create new branches and major releases. The drupal.org permission system does not limit creating branches and major releases. The only thing I couldn't do then is make that new branch the default branch in gitlab.

I understand it's a proof of concept, that doesn't change anything about the scope of the issue, if anything it makes it more important to have a limited scope so that it can be reviewed and assessed. And not being a maintainer does not restrict you in any way to continue working on that.

There are recent, open issues about phpstan and phpcs that I've reviewed, those would be possible issues to move changes related to that, I'm also perfectly fine with creating new issues to do things like improve comments and other cleanup that can be done in a way that is backwards compatible.

I very much welcome contributions, especially on improving the UI, and I'm interested in having more maintainers.

I'm also asking for some patience and caution. Token is an extremely high profile project and when working on such a project together with other maintainers, it is important to follow established processes and patterns. One of that is to limit scope in single issues, and I provided some feedback in that regard that has not been addressed yet. I'm very interested in moving the token UI issue forward, but some of my feedback a critical factor in getting that issue resolved. 8.x-1.x will need to be maintained for at least another year and keeping them in sync as much as possible is important to me.

You're primary contribution so far is in one issue, which is work in progress. A big and important issue, but still, one issue. According to https://www.drupal.org/project/issues/search/token?project_issue_followe... → , you've only been active in a handful of other issues.

To be honest, I'm currently not comfortable giving you maintainer access yet (if I could do that).

I don't think that token is in urgent need of having more maintainers. There are only 3 RTBC issues which likely need an update anyway. There are plenty of things that you can do without maintainer access, from writing code, to reviewing, to helping maintain and clean up issues. Which you started doing a bit.

Also, note that Drupal core is also seeking maintainers for the token component. I think the long term plan as of now is to either move a majority of token module into core or move it all out, not the current mix. You might also want to get familiar with core token issues, the state there and consider contributing there as well. For core, scope management and working in a team is even more important than a contrib project, even one as big as token.

The page deprecation is https://www.drupal.org/node/3458593 → , you need to update your node templates to not check for page.

You can have multiple cron jobs/tasks, but not as cron hooks, you need to define them as config entities explicitly.

The hook API does not offer a way to target one specific hook if multiple ones exist in a module. We would need to apply our own magic and split those out into separate definitions if we detect multiple, but that results in complex transitions, what would happen to automatically updated cron jobs if a second is added in an existing module, what if the second is removed.

I decided to deal with the new hooks for BC as otherwise this is challenging to handle as hooks are converted to OOP.

Doing our own discovery on cron hooks would be expensive. In 11.3, we could access the underlying internal data structure but the edge cases remain.

It's unfortunate, especially since it works with just core, but there is no trivial solution here.

Before OOP hooks happened, I pushed to move cron out of the hook to a dedicated API but parts of the reason why I did that have kind of been superseded by OOP hooks, but only when using core and not this module. See 📌 Replace hook_cron() with a more modern approach Needs work .

I think there should be another module in there, so replace it with something that doesn't get removed.

This is very new, added by 📌 [PP-1] core.extension should be validatable Postponed , I asked there for input.

This added a usage of the contact module which is being removed from core. I don't see contact mentioned here, so I assume we just picked a random module? See 📌 Remove use of Contact from ExtensionAvailableConstraintValidatorTest Needs review

This is worse than I thought because it also affects Tests that enable all modules of course.

We'd need a trait that unifies skipping some modules and update them all .

But I wouldn't bother. Not now. This is not a blocker, it will not fail if contact is removed, it is an exclude list.

And I think I disagree with how it was done in the first place. We imho shouldn't trigger errors for something that can't be fixed.

> The reason for handling this change here as well is that none of the two separate MRs would ever pass tests in isolation, that's why they can only come together. And the reasoning for why this is needed in the first place has been quoted from the Slack discussion with @lauriii.

What tests? Drupal CMS tests? There are no tests in this MR. I don't think this needs to satisfy Drupal CMS exceptations on day 1. Removed contrib integrations will also need to find a new home, it'll need some new dependencies for that anyway. The discussion with @laurii is on how it should be done, which I agree with, not that it needs to be done and must be included.

I also left some review comments on that part, I really think this needs more scrutiny and careful review (and tests).

I'm not exactly sure in which issue I should comment and if I should at all.

I'm a bit concerned about the scope of this (just looking at the PHP code, this might be the biggest code addition since views got into core in a single MR?). There are thousands of lines of preprocess and other hooks. It's not realistic that this is all meaningfully reviewed. I understand that the decision was made and there isn't really an alternative for this initial merge, but still, there is a lot of very opinionated code that contains all kind of customizations and as already discussed, various workarounds for older versions and contrib and so. I added one more comment on a media alter.

I also noticed that the info.yml file hasn't been adjusted yet. I'm not sure if we support the lifecycle definition in themes, but if so, this should probably be experimental? also stuff like package, version and so on.

I saw that the user login theme negotiator issue was closed in favor of this, making this even bigger and also including changes to non-experimental extensions. Do we really need that feature in the initial merge of this?

Mostly makes sense, I'm not sure I fully follow your proposal in regards to status vs used in columns. "Published revision" doesn't add much value to me over just "Published" and you didn't really specify what it would say for a default revision that isn't published. I'd consider just a merged version that is Published/Unpublished/Old revision(s).

I also saw a recent comment from someone that the paragraph location bit in the title is actually useful information for them and I agree that it can be on longer, more complex entities. If we're going with custom logic for that, it could be a secondary row below the title, something like:

Linked Node Title
Paragraph: Container > Card

That's a pattern we use pretty often in our admin views to add more info without having too many columns.

The system table thing isn't the table name but the database, see ✨ Do not use the system "mysql" database to install Drupal Active .

\Drupal\FunctionalJavascriptTests\Ajax\DialogTest also explicitly relies on contact, not (only) through ajax_test. That will need to be adjusted too. The test will need to use a different entity type.

I'm not sure if this can just be removed. It would need to be duplicated into all contact tests that trigger this, which could be a lot. Including contrib tests such as contact_storage which would then break without this as well. I guess it will need to happen eventually as they are moved out of core. Start by removing it and seeing which tests break I suppose. Then add it back in setup() of those specific tests.

Lets keep this focused on this test. Changes make sense.

The bundle deleting test seems a little but strange, that doesn't really test that validator, that doesn't care about null storage, it just indirectly tests the getQuery() implementation of the null storage, which I guess doesn't hurt. maybe there was a specific bug there in the past.

There are already other issues in the meta issue for the dialog/ajax test, I think we might be missing one for ExtensionAvailableConstraintValidatorTest.php.

I don't see how we could disallow it, it's a valid callable reference, we have no way to decide which objects are allowed and which aren't in callbacks.

it's bad because if you do that inside a hook class for example, you serialize that object including it's dependencies (hook services are regular services and do not use DependencySerializationTrait), that might include non-serializable objects such as the database connection if you inject that. Even when it works, it creates duplicate objects on re-initializing and uses extra time for serialization and unserialization and storage.

I'm been thinking about this a fair bit and still uncertain on how to approach this exactly and advantages and disadvantages. Implications on big pipe, how pages are rendered and cached (such as a view with 50 teasers).

> I think the first step is to check what breaks.

The thing is that we wouldn't necessarily see what breaks exactly with just core, but I can imagine all kinds of BC issues. Changed rendered arrays that are altered and extended in preprocess as mentioned, but also the _referringItem feature of entity reference formatters for example. It's complex but when correctly used, very useful to influence entities based on where they are rendered.

So far, the best idea I have is to make it an opt-in per entity type, or maybe even a different formatter or formatter setting. I'm struggling to find a way on how it could work with fibers, maybe combined with 📌 Render children in fibers Active combined with an opt-in flag, like #render_children_with_fibers = TRUE. But that will presumably only work on a single list of render items.

> If it does what I think it will do, it could also mean we can remove the 'prepare view' step from entity reference fields, which would be a significant simplification there.

I know it's the kind of the opposite of what you'd like but I'm currently considering to instead experiment with expanding on an explicit prepare view step (which might actually just be view, see note below). Essentially, I think it's possible to recursively traverse references on entities considering their view mode setting and configured components for that and preload all entities that will be rendered in a few steps. That currently feels far less daunting, with fewer side effects than fully relying on placeholders/fibers. This should work well for something like a page with lots of paragraphs which render nodes, medias, terms, ...

Based on my custom performance test, I'm currently loading 14 media entities one by one on my example page. Since each has a bunch of fields and also loads the respective file, that's a total of about 200 queries. If I can get them all then my example page is down form 800 queries in 11.2, to around 400 on a cold cache, so a 50% improvement. And at least half of those are config queries and other things that are only needed on really cold caches and following pages will see even bigger relative improvements.

(FWIW, prepareView is already now barely needed since almost nothing in core except comment rendering actually uses buildMultiple(), we render entities one-by-one. We could drop that in favor of just using view() with almost no regression, but deprecating it sounds pretty complicated, as just no longer calling it could would drop optimizations from things relying on it. I just fixed/improved the implemention in entity_reference_revisions but didn't bother making it deal with multiple entities.)

I updated the docs in 📌 Document the most basic set up for DDEV and server Active , this should only require a minimal addition now.

I did a full pass on the README.md and restructured it. All things that are configured in Drupal are now in one section and all things for redis itself in another.

I also added docs on igbinary, updated and elaborated on the expiration policy and TTL configuration and recommendations for that.

Merged.

Thanks for sticking with this. I've updated the comment on why those revisions of node rev2 aren't deleted (It's because those are the default revisions, so more checks are needed to verify that all revisions of the reference are unused, in case there are any.

This made me realize what I think the issue here is that you're seeing. Exactly that default revision check. I think we can do this more efficiently and avoid those extra queries on revision deletions by instead adding them to the queue in that case. With that change, your tests are passing for me without the revision delete hook.

Merged this with some basic test coverage that did fail on earlier iterations of this. Will test this more extensively through paragraphs test coverage.

I'm not sure if the fallback is still needed with the new loaded check, no idea if IEF sets only the target id or also the entity, but kept that just in case. FWIW, using this with IEF might not have expected results as that indicates that it just saves the default revision, which means that updating it will update the reference.

See also 📌 Preload cache tags for set/get multiple Fixed for the redis issue to implent this for both set and get.

Ah, render caching has getMultiple() but only a singular set().

Stepping through, the only setMultiple() calls I see on umami frontpage are config and entity writes, and those only have the same single cache tag, if any at all. So we'd likely need a unit test or so to assert the expected behavior for this.

The assert makes it a bit awkward too, so we assert before we assume it's valid, but then I have to do the loop even if the checksum service doesn't support preload or add additional conditions. Without the assert, this could also be done with an array_column()

berdir → created an issue.

Didn't see a difference in the umami performance tests locally, that's a bit unexpected, nothing sets multiple caches with distinct cache tags that weren't already loaded it seems.

I did verify that this works in my project, the cache tag lookups go from initially 65 to 90 without this and then back to 69 with this change.

Drupal core now finally has caching on loading revisions: 📌 ContentEntityStorageBase::loadRevision() should use the static and the persistent entity cache like ContentEntityStorageBase::load() Needs work

It would have already been beneficial before, but It's considerably more beneficial now.

I've added version checks for 11.3 in \Drupal\entity_reference_revisions\EntityReferenceRevisionsFieldItemList::referencedEntities(), in \Drupal\entity_reference_revisions\Plugin\DataType\EntityReferenceRevisions::getTarget and using referencedEntities() in \Drupal\entity_reference_revisions\Plugin\Field\FieldFormatter\EntityReferenceRevisionsFormatterBase::prepareView() so older core versions also benefit from this.

I've also addressed 🐛 referencedEntities() causes data loss Needs review by adding a new API to check if the target has already been loaded. This is what I've also suggested in the referenced core issue, if this API is pulled up we could eventually remove it again: 🐛 EntityReferenceFieldItemList::referencedEntities might not return up-to-date entity objects Needs work

I've validated this with a performance test in our distribution, tested on page with about 25 paragraphs, some of which are nested, the diff on the metrics of a cold cache page load looks great:

@@ -280,27 +280,27 @@ public function testAnonymous(): void {
}, 'after_full_cache_clear');

$expected = [
-      'QueryCount' => 637,
-      'CacheGetCount' => 529,
+      'QueryCount' => 569,
+      'CacheGetCount' => 511,
'CacheGetCountByBin' => [
'page' => 1,
'config' => 130,
'bootstrap' => 25,
'discovery' => 162,
'data' => 85,
-        'entity' => 65,
+        'entity' => 47,
'dynamic_page_cache' => 1,
'default' => 32,
'render' => 22,
'menu' => 6,
],
-      'CacheSetCount' => 495,
+      'CacheSetCount' => 477,
'CacheSetCountByBin' => [
'config' => 92,
'bootstrap' => 19,
'discovery' => 159,
'data' => 85,
-        'entity' => 64,
+        'entity' => 46,
'default' => 30,
'render' => 39,
'menu' => 4,
@@ -309,7 +309,7 @@ public function testAnonymous(): void {
],
'CacheDeleteCount' => 0,
'CacheTagInvalidationCount' => 0,
-      'CacheTagLookupQueryCount' => 65,
+      'CacheTagLookupQueryCount' => 90,
'ScriptCount' => 3,
'ScriptBytes' => 253500,
'StylesheetCount' => 3,

A reduction of queries from 637 queries to 569 and fewer cache lookups due to using loadMultiple during rendering. revision cache uses cache tags so there's an extra 25 cache tag lookups, but that should be reduced to 4 or so by 📌 Preload cache tags in DatabaseBackend::setMultiple() Needs review .

berdir → created an issue.

It works for *new* entities, but it doesn't work for existing, changed entities, which is what this issue is trying to fix. \Drupal\Core\Field\EntityReferenceFieldItemList::referencedEntities() in HEAD already accounts for this, because it falls back to that if target_id is null. That's consistent with hasNewEntity(), which also explicitly checks for target_id === NULL, so it doesn't matter whether that's checked first or not.

Your code will also work in HEAD. What is the bug that you are trying to fix?

We can not solve this and keep multiloading without exposing something like isTargetLoaded() on the reference property.

Updated the issue summary and created a CR: https://www.drupal.org/node/3556785 →

I tried to explain that it should be very rare that this needs to be considered.

Looks good to me, I tried to explain a bit better what exactly "invalid" means.

A client we have is still seeing this on multi-page forms, as regular users, without any assistive technology.

I assume this only happens on subsequent steps. Could we set a flag in local storage or something like once user successfully passed the check and immediately unlock the form for them for any future request, possibly for a given period of time?

Updated the performance tests.

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

@catch is it possible you had an older local branch? I rebased this yesterday a few hours after that issue got in and I just did another local rebase against 11.x, no conflicts there.

Looked over the MR, seems ok, left one note about the return type.

Having the hash calculation in one place should make it easier to exchange it, I think this would be a good case for using xxhash over md5, but that would invalidate all existing URL's and definitely shouldn't be done in this issue.

Not sure if we should document the includes bits as it's just for the BC layer and we'll need to remember to remove them again, but I think this addresses the issue.

In the CR, I also mentioned that these structures are considered internal, including keyvalue entries. Maybe we could also put this in code somewhere?

I pushed what I asked for, but it's not quite so simple it seems. they don't push their 2.5 rc releases to packagist, so it doesn't find them, and the dev release seems to to confuse composer. Would need to be spend a bit more time on this which I don't have right now.

Also note that once again, the dependency chains and conflicts here are extremely challenging. Right now it requires symfony 7.3, which means it's not compatible with 11.x/11.3, and once it switches to 7.4, it will require 11.3. It depends on things like guzzle in very specific minor versions that too make it challenging to require this, in my own project I also saw conflicts on psr/http-message when testing quickly.

> Or have you effectively been the sole maintainer, such that the project may become unsupported without one or more new active maintainers coming aboard?

Pretty much, @mingsong was interested, but I can't add new maintainers myself, so the issue I linked earlier would need to be escalated to so that an admin can give either or both of use full admin access. And now that he is working on the fork, I don't know if there's still interest. I'm willing to continue to maintain this on a professional/paid basis if someone reaches out to me, just not willing to spend my personal time on something as painful as this.

As mentioned before, I strongly recommend checking out samlauth if you don't need specific features provided by simplesamlphp.

The changes make sense to me. Moving to block as it's not language specific.

The comment changes to the test where the behavior now changes for this specific block are IMHO fine, it doesn't solve the issue, it just no longer means that this specific block is affected by it.

Is this typo in your actual path or just the issue?

web/modules/custom/va_core/src/Plugin/monotoring/SensorPlugin/AzureBlobStorageSensorPlugin.php

Also, status is not a metric type, remove that, and you don't need to set the provider for your own module.

We could also do this without the OOP conversion, but we have to do this anyway, so why not together.

It's an internal function, no known calls, so I don't think we need a change record, kept the old function with a reference to this MR. Can't make it private due to the BC layer.

performance tests need to be fixed.

> @berdir we can deprecate __get('container') to discourage use and eventually remove it - not sure if we want to do that here or in a followup as it's likely quite disruptive.

Yes I was thinking the same thing, doing this would give us a way to properly deprecate it. No strong opinion whether we do this here and have a follow-up for actually deprecating it or if we keep this issue for that. IMHO, that part is/was the scope of this issue, to me it would make more sense to keep this open for that, but no strong feelings. We could also decide to not deprecate it and keep it, but I'm certain that we'll still have discussions in the future about $this->container vs \Drupal::service() if we keep both, because using \Drupal::service() == bad (even though it's now exactly the same)

FWIW, I get mostly the same behavior on xhprof with ddev as with blackfire. demo umami, fresh install, frontpage 11.x shows it, 18 calls, 3,204 microseconds wall time (0.9%).

On this branch, it does not show up. I verified with xdebug that it stops there.

Unlike blackfire, it does show up when I keep the static cache. There, from 18 calls, there are 12 calls to unpackOptions(), so the static cache avoids 6 calls. But all unpackOptions() calls that we do run only takes up 40% of that time, and calculating the static cache key is also around 20% with the serialize and hash calls.

Worth nothing that with such low numbers, the variation is likely very high and overhead of profiling is high. but I think it's sufficient to show that the actual logic is less expensive than the caching, definitely for persistent cache and a bit of a wash for static cache, would at best be helpful if we'd hit that a lot

persistent cache (11.x)

with static cache (earlier version if this)

What happens on <11.2 when that menu doesn't exist? does that link end up in a weird place, is it ignored? And on 11.2+ when navigation is not enabled? Do we need to move it to the menu links alter hook with a navigation module exists and version check?

Changes make sense, phpstan should have caught that but didn't with our configuration (\Psr\Container\ContainerInterface::get() does not have that argument either).

Failing tests are just the usual and frequent random fails, theme switch and media library.

Yes, agreed, updated comment looks good.

> In xhprof if you view the full list of functions, need to make sure initDisplay() is actually in the list somewhere, otherwise it's possible the page didn't execute it at all and have to tweak the steps to reproduce.

I haven't verified this with xhprof, but with blackfire, initDisplay() was actually not visible anymore at all list of functions, they do filter out some things that only make up a tiny fraction of the time.

Instead of clearing cache, I prefer to set the page/dynamic page cache/render bins to null, then you can run it repeatedly. You can also set a breakpoint there to make sure it's called on your request. But do not run/enable profiling and xdebug at the same time.

> I also ran phpstan on max level before and after and there was no difference in the total number of violations, not sure what to make of that.

I think a lot of work went into the phpstan integration to actually make it understand those services, whether that's a class/interface or any other string. So I'm fairly certain that the claim in the issue summary ("However there is no way for PHPStan to know that.") is actually wrong.

The same is not true for phpstorm and possibly other tools though. It doesn't work as well there, even with the symfony plugin and so on.

FWIW, this did a considerable shift in direction in the last day, I do want to point out that @donquixote explicitly argued that it should be separate in earlier discussions here. I don't agree with that option, and I think we shouldn't add more API endpoints for non-injected code when we are now at a point with autowire+autodiscovery+relaxed doc standards that it's actually becoming easier to do DI than not (or maybe restricted to tests where we don't want to use DI). I'm just saying that we should consider that opinion/direction change.

Yes, it should, on a regular request with one/multiple views, you should see that without caching, the initDisplay() method should either be faster or completely vanish from the profiling results as loading from cache is actually more expensive than the work it caches.

Yes, there should be, the BC test just verifies the initialization and not the theme manager/ rendering

This should not include general coding standards cleanup, removal of legacy hooks and so on, makes it way harder to keep two branches in sync and review this.

I'd argue this is a duplicate of that yes. You can publish/unpublish, it's just a confusing/broken UI combined with translations.

Makes sense, I just have one small comment about a comment in the test that should be updated to reflect the changed assertion.

also, I wonder if permissions could be used to avoid displaying this link when the user wouldn't have access, but that's already not done now and the access API around this is not standardized.

This makes sense as a quickfix for the issues with this, but I still think we should discourage usage of $his->container and deprecate it or something like that. So maybe that should be a child issue of this and this would be a meta/long-term-plan?

It will also fix 🐛 Switching themes in a functionaljavascript test causes backend code to run as anonymous Active that was reported as a bug/behavior change now that themes rebuild the container. Was always the case for modules.

Going a step further, I've been wondering if we should do something like 📌 Alternative to Drupal::service() or $container->get() with return type promise Active but specifically limited to test code, so that we get better phpstan/phpstorm (?) detection while not encourage non-injected services outside of test code.

I think the current suggestion in 🌱 Use \Drupal consistently in tests Needs work will fix this.

Updated the performance tests, very nice diff. As an additional positive side effect, it also removes/pushes back a number of views cache tag lookups, which means better grouped lookups.

I also removed the static caching, I agree with #4. The calculation of the static cache is non-trivial too with the serialize. Hard to proof since the whole thing doesn't even show up in profiling anymore, but that also adds to the fact that we're maintaining complex code for no measurable benefit.

To add to the reasons in #4, PHP as a language also made *significant* improvements to performance since 2009, even if we'd run those things as much as we used to, it would be a lot faster.

+1 to RTBC.

On the cache operations, I was just confused by because the total didn't seem to add up, but I think I got confused, maybe I mixed up query count and cache get operations or maybe I compared the get per bin and the total of cache set operations.

Sorry, I partially misunderstood your example. I thought you do a theme install after login.

I see now, this is the classic $this->container getting out of sync problem. Set that back to \Drupal::getContainer() and it should work again

Or patch drupalLogin to do that

Or also just what the last line o \Drupal\Tests\UiHelperTrait::drupalLogin() does.

That's what I expected, it doesn't work for either, but for themes, it's new.

The user is also kept in $this->loggedInUser, see for example \Drupal\Tests\ckeditor5\Traits\SynchronizeCsrfTokenSeedTrait::rebuildContainer, you can do that for your tests.

Instead of.

installing a theme rebuilds the container now, just like modules. so we need to actively do something to carry over the current user from one container to the next. it works in actual, session based requests, but I guess not within the manually adjusted container in within the test runtime.

Does the same happen when you install a module?

Makes sense and great to have tests that make this visible.

Jsonapi tests need an update though.

Did another review, all I have time for now, I'll probably do a last pass myself if you can address the feedback

Redis is pretty much expected to fill up, that's what the eviction policy configuration is for: https://redis.io/docs/latest/operate/rs/7.4/databases/memory-performance..., this should absolutely be set anything but noeviction. I'd recommend one of the volatile options.

Another option is to use the database for page/dynamic page cache as those tend to be the largest by size, and use redis for smaller high frequency bins.

> On the other hand, it’s better to have a single global configuration than to set up multiple ones.

It's not better, because there is no need to set a max ttl for many of the high frequency/small size bins like discovery.

14 days isn't as problematic as setting it to a day/hours as the example I mentioned, but I disagree with the added docs. It's not necessary or recommended to set this. using redis with noeviction is not supported/recommended, redis eviction doesn't rely on invalidation/cron and can't "fail". the docs also conflict with initial paragraph in that section which states that the there is a default (it's badly written and should be reworded, but it still conflicts)

I don't have permissions to appoint new maintainers, see 📌 Apply for joining as an co-maintainer Active (that needs to be moved to the appropriate issue queue to escalate it)

berdir → created an issue.

I removed the second point, the method still exists but is protected. There is currently no public API to invalidate a specific revision, but you can invalidate all revisions and there's not really a reason to do so, the main one was always tests, but the static cache is now automatically invalidated.

The last point (before 4, now 3) is still correct *if* called in that order (and only for the static cache). This is in fact the primary driver for the performance test results, as content moderation (EntityRepository::getActive()) tends to load the default revision after loading the entity ID, this now immediately hits the static cache. This is explained further in the change record.

I still don't really understand why you'd want this.

The example configuration in #26 limits for example discovery and bootstrap cache bins to just one day. They are small and they essentially never change between deployments/module installs. It's extra confusing because they are behind fast chained, so the fast cache bin (apcu) will actually live longer than the data redis unless it's reset.

1h cache bins for data/render are also going to be almost useless and unless you have very specific scenarios, it will result in a bad cache hit ratio. you'd be almost better of to just skip render completely and set it to the null backend if it's that low. Also, 11.2/11.3 make better use of render caches and split them out from dynamic page cache. data is a very useful cache, for example for views results, menus and so on. There can be quite a few of them but they are usually fairly small. 11.3 is also removing the per-page path cache, which removes a lot of entries from that bin.

Left one specific comment, if we add this then it should come with clear explanation of why you'd want to use this and I think the example should be set to a high value, not just 3 days.

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.