🇨🇭Switzerland @berdir

Now it worked.

See 🐛 Remove return value from EntityFormInterface::save Needs work . Those core issues are mostly about submit and save, but it's pretty much the same for validate. It's all pretty weird, we override a method that exists on the parent FormInterface and change how it behaves. Unlike submit/save, validate doesn't set the entity, so you can't access it otherwise, it's a hack for that. It would never have been possible if return types would have existed when we built Drupal 8.

There are at least a dozen implementations in core that aren't doing this either.

And it's related to an even older and trickier issue: 📌 In submit use the entity built during the validation instead of building it again - for performance and consistency ensuring we are saving the entity that has been validated Needs work

I'd love to actually improve things at the source instead of implementing somewhat bogus changes just to make phpstan happy. But I'll merge this for now so we can get pipelines greener, once drupal.org actually lets me do that.

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

Merged.

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

Reviewed.

Merged, thanks.

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

> Exactly. This is the responsibility of NotificationHandler::sendCompat()

Yes, my suggestion is to specifically not move that to the new service at all. Having it there means we'll need to remove sendCompat() in the future. That method currently doesn't explain that you're not supposed to call it, it explicitly "supports" an arbitrary $op and does _not_ trigger a deprecation message. forgot_username could call that directly and then we'll just break that later.

We want to deprecate _user_mail_notify() fully IMHO, we want to deprecate/remove every single .module file function we have left in core. They all need to go.

The current state to me is very confusing, with the inconsistent deprecation and what you're supposed do as a module doing that. I'd also clarify for modules like forgot_username what they are supposed to do (implement their own hook_mail())

As far as I'm concerned, we can fully deprecate _user_mail_notify() with @internal or without on NotificationHandler. IMHO, we don't need an interface to do so. An interface means we support alternative implementations, @internal means you're not supposed to call it. But happy to let a core maintainer decide on that.

Left a comment on missing DI, otherwise +1 to RTBC from me, the deprecation test looks good and verifies that an if condition with a variable triggers the deprecation message, great.

I didn't have time to read the comments properly and review the update, just wanted to add one note for now.

I think we cannot deprecate _user_mail_notify as a whole without providing proper APIs to cover most of these use cases first. That said, I think the best way forward would be to deprecate custom ops here in a first step.

_user_mail_notify() is an internal, underscore-prefixed function. We don't *have* to do anything, we are just nice because a lot of contrib would break. Specifically, we are not forced to provide a 1:1 replacement of anything, we decide how our future API's look like and what we want to support. We can also deprecate an implicitly internal function with an explicitly internal service. It doesn't mean that it can't be called, it just means that we might change it.

What we did in some OOP hook conversion issues is that we deprecated some functions, _underscore prefixed and regular ones, without replacement, because the actual replacements are protected and can't be called. We could do something similar. We could deprecate _user_mail_notify() but *not* call the new handler, just trigger the deprecation message. Any call to the old function continues to work as-is but doesn't go through the new API. It would mean they wouldn't use the new mail API once replaced, but that's perfectly fine IMHO, especially as long as the whole thing is still experimental.

> numeric value like quantity or price

FWIW, the price thing could be confusing for people using commerce, because that has an actual price field type and you must use that, not numeric value. Unless that decides to make it part of the numeric value category, but that seems somewhat unlikely as it has more parts than just a number. Maybe use a different example, we often use numbers for a weight/order sorting field, both terms have multilple meanings as well, especially with translations.

Can confirm that this loads around 16 fewer services using my dependency scripts from 📌 Explore using more service closures to break deep dependency chains and load fewer services Active

Profiling is always tricky. The differences in #21 seem huge and honestly too good to be true :) I tested with just core and standard profile, not sure how many more services cms adds.

When I test with AB, thn it's something like 48ms vs 54ms or so, varies between runs. There's a ton of overhead there, this is on WSL with ddev, so I'm likely mostly testing all kinds of network things. Instead of trusting ab numbers, it might be more useful to parse php fpm logs.

Testing with blackfire gives more useful results I think: https://blackfire.io/profiles/compare/3f98e259-aff4-401b-b363-13523b17fe...

That's 9ms to 7ms, so a 2ms improvement, ~23%. blackfire in my experience blows up requests between 2x to 5x. But much more meaningful than that is the memory improvement, which is ~4MB to ~2MB, almost a 50% improvement.

Note: I was testing with redis enabled as a forgot/was too lazy to switch.

In the new mail system, Contrib would like to be able to trigger generation of emails. I believe Easy Email might include a UI for this feature generally. Commerce has it specifically as "resend receipt".

This API has nothing to do with the new mail system, it's explicitly an abstraction from the actual mail API, so that we can switch it out underneath.

It's not really part of the work on the new mail system at all, it fits more into the work to deprecate and move all functions in .module files to services/classes. It's just a welcome side effect that this allows to switch out the code with something that uses the upcoming new mail API, within the experimental mailer module. Because having experimental code in non-experimental modules is tricky. And once that API is no longer experimental, it will move back into user module and replace the current default implementation.

And eventually, maybe, we can then decide to deprecate this API again, if we think that working directly with the new mail API is sufficient and covers all aspects, such as enabling/disabling certain mails. Or we maybe we won't, because you could even use this abstracted service to send out SMS/app notifications instead of/additionally to e-mails. But that's several steps in the future and not relevant for now.

As mentioned in 📌 Remove Url generator from router class Active , I wrote a little patch to write out all essentially all Container::createService() with their initiator. Initiator here means the first createService() call when we're not already within a createService() chain. And when leaving that initial createService() all, I write out that list as a single line into a txt file. The result is a list of all dependency chains. Any direct call to get() that is a new server, including such as plugin/storage/controller "DI" is seen as a standalone thing. As well as service locators and similar factories that initiate services lazily.

A separate file is written for each request with timestamp and path, which allow to compare them. It's a single line, but it's actually a tree of services, so not every -> actually represents a chained call. It would be possible to visualize this better, but it might also make it harder to diff. That wasn't a focus yet.

The idea is to track when a given service is used the first time. And the goal of this issue would be to find meaningful and worthwhile ways to push them "back", non-html (such as json api, but also things like autocomplete callbacks and assets) and cached pages are likely where we'll see the most benefit, as "back" might mean that they wouldn't be needed at all. The script can be run before and after a change to visual the change.

I excluded private services because those would differ between cache clears and likely aren't so meaningful here.

That can either be done with a regular diff, or what I prefer, within a fresh git repository with a word-based, as seen in the screenshot in that other issue.

The changes as diff:

diff --git a/core/lib/Drupal/Component/DependencyInjection/Container.php b/core/lib/Drupal/Component/DependencyInjection/Container.php
index dc68c9c8a24..5f4cc3d6521 100644
--- a/core/lib/Drupal/Component/DependencyInjection/Container.php
+++ b/core/lib/Drupal/Component/DependencyInjection/Container.php
@@ -46,6 +46,8 @@
  */
 class Container implements ContainerInterface, ResetInterface {

+  protected array $currentDependencyChain = [];
+
   /**
    * The parameters of the container.
    *
@@ -225,6 +227,15 @@ public function reset(): void {
    *   and cannot be instantiated.
    */
   protected function createService(array $definition, $id) {
+
+    $initial = FALSE;
+    if (!str_starts_with($id, 'private__')) {
+      if (empty($this->currentDependencyChain)) {
+        $initial = TRUE;
+      }
+      $this->currentDependencyChain[] = $id;
+    }
+
     if (isset($definition['synthetic']) && $definition['synthetic'] === TRUE) {
       throw new RuntimeException(sprintf('You have requested a synthetic service ("%s"). The service container does not know how to construct this service. The service will need to be set before it is first used.', $id));
     }
@@ -299,6 +310,11 @@ protected function createService(array $definition, $id) {
       call_user_func($callable, $service);
     }

+    if ($initial && defined('DEPENDENCY_DEBUG_FILE')) {
+      file_put_contents(DEPENDENCY_DEBUG_FILE, implode(' -> ', $this->currentDependencyChain) . "\n", FILE_APPEND);
+      $this->currentDependencyChain = [];
+    }
+
     return $service;
   }

diff --git a/index.php b/index.php
index 750dc282dc2..d4fe93a2b0a 100644
--- a/index.php
+++ b/index.php
@@ -16,6 +16,9 @@
 $kernel = new DrupalKernel('prod', $autoloader);

 $request = Request::createFromGlobals();
+define('DEPENDENCY_DEBUG_FILE', __DIR__ . '/dependencies/' . $request->server->get('REQUEST_TIME') . '-' . str_replace('/', '-', $request->getPathInfo()) .  '.txt');
+touch(DEPENDENCY_DEBUG_FILE);
+
 $response = $kernel->handle($request);
 $response->send();

I attached the current state on head as a text file, it's a bit much as a snippet and wrapping makes it hard to read.

In some cases, we'll want to specifically test dynamic page or even page cache hits, I'll do some testing with this in the middleware issue with those scenarios.

Couldn't resist to try my debug file idea, it's a bit tricky to diff, there are also some seemingly unrelated changes that I'm seeing, I think the whole thing isn't quite stable yet, possibly due to caches warming up, so I did a git diff words, as a screenshot.

Essentially, the url_generator moves down to the maintenance mode subscriber. Which is likely another good target for some service locators, it has a ton of dependencies that only needed if maintenance mode is on.

This also shows the middleware issue that 🐛 Regression: All stack middlewares are constructed at the same time even for cached pages Active is working on quite nicely.

I'll share more of that and some example files later in 📌 Explore using more service closures to break deep dependency chains and load fewer services Active .

This looks good, nice little cleanup.

Updated the @see references.

I removed @see template_preprocess_status_report(), as that was removed 9 years ago. I'll add a note to double check that we got all of them in the meta issue before we close it, so that we won't forget any, they're easy to miss.

file_uri_scheme() was removed a long time ago. the test fail is unrelated, those tests just sometimes randomly fail. If it doesn't look related to your changes, you can trigger a retest. I did that now.

I'm missing at least some template @see references here.

I created 📌 Explore using more service closures to break deep dependency chains and load fewer services Active for those dependency chains.

I created a sister issue for this one to start using service closures in places that are not yet using lazy service proxies: 📌 Explore using more service closures to break deep dependency chains and load fewer services Active

berdir → created an issue.

Did the proposed split.

I also checked ModuleExtensionList and it's also initialized during bootstrap, in this lovely dependency chain:

router_listener -> router -> router.no_access_checks -> url_generator -> renderer -> theme.manager -> theme.registry -> extension.list.module

The url generator confused me a bit, it's the MetadataBubblingUrlGenerator wrapper, the renderer there might be a candidate for a service closure? and possibly some others in that chain as well.

I think it's fine to close this. There were actionable issues that were done, we have performance testing now, which doesn't provide exactly the same data, but whatever was shown in this specific flamegraph is long outdated. We can always create new issues if we spot things.

While I agree that this is a bug in core, webform is still unnecessarily relying on automatic .inc file loading for helper functions called by that hook.

This is deprecated for removal in D12, while legacy hooks are supported until D13 (and not even formally deprecated yet), so you likely want to keep them around longer to support D10.

That means eventually, all functions in tokens.inc must move to .module and there is no reason to not do it now. that would fix this particular error but the token system is going to be extremely broken due to all non-converted hooks that are still in tokens.inc, such as the ones in token module.

So, changing this to a normal task to move that.

Personally I prefer not mixing in coding standard and spelling fixes in a compatibility issue, it just makes it much larger and those aren't necessary to have a green MR. But it's done, so leaving that to a maintainer to decide.

I'd also completely remove the composer.json, it doesn't add any value, that will be overwritten anyway on drupal.org and the description is outdated, this isn't a core theme anymore. But that too is a maintainer decision.

Tests are failing pretty hard due to the composer.json. I don't think that's required, defining repositories like that is not valid in a module/theme and there are no dependencies except core. Lets remove that?

berdir → created an issue.

I haven't tested this, just edited the composer.json through the web UI.

berdir → created an issue.

Needs a phpstan baseline update.

I think all but maybe ModuleExtensionList are pretty safe. ThemeExtensionList is used more actively as we don't have that info in the container like we do for modules (I think?) and I don't see how we'd do any preprocessing without the renderer or module handler already being actively involved.

That said, I realized that we now have a ThemeHook and a SystemThemeHooks class due to the generic transformation of preprocess hooks, so I'll use that to untangle this a bit and split it up, more for architectural purposes than performance. The plan is to move hook_theme() and non-admin theme hooks (preprocessEntityAddList kind of, also has no dependencies) to SystemThemeHooks and rename the current ThemeHook to SystemAdminThemeHooks.

Not only that, but 📌 Use LRU Cache for static entity cache Postponed landed this year, which is a full on duplicate of this.

I still have mixed feelings on this. I'm not convinced that we need to have those two methods and a dedicated, node specific repository service for this. there is only one non-test call to revisionIds() and it's something we already have a generic replacement for. Similar for userRevisionIds(), two calls and both are something that we really should have a generic answer for (user cancel handler).

Leaving at RTBC to get thoughts on this from a core committer.

Looking at the change in NodeController (which isn't strictly needed anymore for now but doesn't hurt), I also realized that deprecating NodeStorage is going to be extremely tricky, because if we just stop using it then everyone who has a method or property with that type will have a fatal error. But that's a problem for another day.

">=10.1.3" is definitely wrong, that would include any future major version.

Don't care if it requires 10.3 or the helper is used, but this should be fixed, that will cause problems in the future.

requiring 10.3 would not need a new major version, even 10.3 is unsupported now and updating requirements is fine in a minor.

I don't really see a scenario where this could be invoked and ConfigFactory isn't already initialized, at which point using a service closure is just overhead?

Currently, ConfigFactory is initialized as part of the dependency chain of \Drupal\Core\EventSubscriber\OptionsRequestSubscriber, a prio 1000 request subscriber (through route provider and path processors). This seems like a good candidate for a service closure as it only needs the route provider on OPTIONS requests.

If I disable that, it's the TimeZoneResolver, a prio 299 request subscriber, that also does the first actual get() call (possibly information that could be put in the container, like language), but there are more actual get() calls.

\Drupal\Core\Theme\DefaultNegotiator::determineActiveTheme (system.theme)
\Drupal\Core\Extension\ThemeHandler::listInfo (core.extension, due to theme status check)
\Drupal\Core\Session\UserRolesAccessPolicy::calculatePermissions (user roles, we removed killed a persistent cache here as that's slower than config get)

And that's on mostly warm caches, such as the request matching, inbound path processing involves more config for example.¨

system has more services, that's a bit more complicate, I'll check that separately.

Putting back to RTBC to evaluate that again.

> those who may have been relying on createBodyField to make a text_with_summary?

createBodyField was introduced in 📌 Deprecate node_add_body_field() Active , as a replacement for node_add_body_field(), that's a non-issue, but you can of course ask the same question about node_add_body_field(). But that's not really a problem IMHO. There are going to be very, very few cases that specifically want a summary field. We'd need to deprecate and move this trait again to text_with_summary module, and then those contrib module would need to depend on that.

In all my contribs in my project, I see 7 calls or so to node_add_body_field(), I doubt any of them care about the summary. In contrast to that, there are 200+ calls to FieldStorageConfig::create() where tests set up their own fields explicitly. Which I think is where we want to go.

Same with core really, I don't even see any calls to createBodyField() except the one in createContentType(). Maybe there were and we refactored them away in that issue and shouldn't have introduced that trait and method at all? Core has 400+ FieldStorageConfig::create() calls where it sets up specific fields with specific configuration. We keep the concept of a "body" field being a default thing of a node type in tests while removing this on actual sites, that should IMHO be an intermediate step just to deal with the amount of tests relying on it for one reason or another.

Instead, what I think we should do is introduce an API version of \Drupal\Tests\field_ui\Traits\FieldUiTestTrait::fieldUIAddNewField(). That uses the UI, but most cases don't actually want to test the UI with this, they just want a field with a given name, type and configuration for their entity type.

sounds like umami already does exactly what I'm proposing for standard, so that shouldn't need changes at all, makes sense. we also didn't have to change it in the field storage issue I think.

Yes, I would assume that standard would only add such a teaser field to articles and not page, solving the page node type summary issue you referenced.

Removing those node types completely does sound like a sensible direction considering we have Umami and Drupal CMS, but also sounds like a more complicated change than adding a teaser field. you can still somewhat use standard as an evaluation of the basic capabilities of Drupal. Removing Page and Article would definitely many non-trivial steps to that. So I'm certain that's going to result in some concerns.

> My main concern was that someone who used to use custom OPs would now have that option taken away from them. But giving it some more thought, nothing stops them from calling the mail service with said custom OP themself.

I'm unsure where the notion of of custom $ops comes from exactly, do we have any data that supports that this is a thing people did?

This is technically an internal function that you weren't even supposed to use, it's just a very common thing to do and convenient to use, and the two other issues that this references would have updated the docs to specify the allowed values and the other introduced an enum. both specifically disallowing the notion of a "custom $op".

Same with the underlying config. you can not add more things because you can't provide schema for it.

> The new service copies the old code in ways that seem to make sense, but also do not go any further. One could argue now would be a good time to revisit some of the comments, but if you want to limit scope to "just move the code" then I could get onboard with that too.

The idea is that we isolate the code in a way that allows us to provide an alternative implementation using the new mail API, first in an experimental module, then later moved back to user (at least that's my recommendation).

Added some quick thoughts.

> Would this need a CR or is it covered under another?

IMHO we don't, because we already have all the deprecations that explain the hook_requirements() deprecations, we just didn't formally deprecate it.

> We're concerned about performance, maybe we do a naive check somewhere for the existence of hook_requirements without one of the three replacements and set a message there.

No, performance is not a concern here. It is complex code, but this isn't going to be a measurable performance regression. It's specific to requirements hooks. system_requirements() alone can take multiple seconds with all the entity schema and other checks. If anything, it will be faster, because we won't run the same requirements checks twice for modules that want to be backwards compatible.

The only concern is code complexity, I agree that this is quite a lot, but also managable.

Does this also fix 🐛 navigation top bar edit button has strange active (?) colors Active ?

I find it suspicious that \Drupal\Tests\node\Kernel\NodeTokenReplaceTest::testNodeTokenReplacement doesn't need a change. That probably works because it never reloads the node, so the summary is set as an undefined property and the token works.

1. Convert the storage.body fields we just moved to be text_long vs text_with_summary. Probably can do this in one ticket but can see the argument to breakup. But 0 idea how to address migration

It's not just changing the type. We do want a comparable feature of having teasers I think, even on standard. Also as an example on how you'd do that. Not sure if umami has something like that too. That means we need a separate new field_teaser_text or something like that, set up form and view displays, update tests. Should that be a plaintext field or one with a format? Not sure. In our distribution, we plan for customization and make it a text with format, but by default limit it to plaintext. That feature is in core now.

I think it is pretty much postponed, in the way that it's maybe step 7 or something on 🌱 Requirements and strategy for Core Mailer Active . It shouldn't be what we focus on, not now, so postponed to me is a good state. It's not blocked on a specific issue, but we're not yet at this level of abstraction in the new mail API. Lets build it from the bottom up and then we'll see if provides an additional benefit.

Posted a review. And because my comments surely weren't annoying enough yet, I'll finish up with a bonus: The node:body and node:summary tokens. What on earth are we going to do with those? Deprecate them? See also 🐛 "summary" token for text+summary field is empty when using anything other than the core "body" field Active , metatag relies on this as default configuration.

Also, as I found out in 📌 Introduce Adapters instead of different cache/queue/lock classes per backend Active , the multi in queue and lock are very much required, tests fail without that. I might switch to pipeline there for cache.

This is now passing tests. I want to improve docs and so on, but it's seems to be working pretty well.

the multi stuff was easier than expected, I just can't use chained methods, then multi() seems to be working fine for both phpredis and predis. I might remove some or all of the methods I had to add and use non-chained multi() or pipeline calls.

_user_mail_notify() is technically an underscore prefixed function and not covered by BC. But there are dozens of calls to it, so we should deprecate it properly.

A bit unsure if we should do a straight conversion or actually rethink things. There are various issues open about improving docs, arguments and changing how it works, such as ✨ Use an enum for user email notification types Needs work , as a mailHandler, we should also split it into dedicated methods and make the $op stuff at least an internal thing. Deprecation should be easier than changing params on the existing function. But it will make this more complicated.

Two more things:

* I think I saw 3-4 similar/older issues like this one just when searching for _user_mail_notify, there are likely more. We should close some old plan/meta issues or relate them if they're still relevant. Like ✨ Create a plug-in system to build emails with Symfony Mailer Active .
* As mentioned to @znerol, the current meeting slot really doesn't work well for me, due to work travel/family. Calls can be useful for specific discussions, but other initiatives have adopted async meetings (e.g. discussion threads over 24h or so), which I think would help getting people from other timezones into this as well. I also like to have some time to collect and order my thoughts (and then write very long issue comments), I usually struggle to provide meaningful feedback in calls.

Oops, restoring the issue summary.

Some thoughts on the updated proposal and comments:

I agree that code is useful to experiment and review the direction and it is specifically also useful to see the bigger picture, not just one single decision/class/... But I'd essentially try to do a "vertical prototype" in this phase, convert 1-3 examples, e.g. one fixed mail like update and one for user or something like that. A lot of work was put into defining every single config entity for user e-mails which wasn't really necessary to understand the concept.

Just like with the prototype, I'd avoid creating too many issues to discuss and implement every single bit. It's hard to get things into core piecemeal. And a single issue (fork) can fit many different merge requests, if different people want to experiment/work on different parts separately.

IMHO, the hardest thing of the whole process is experimental code/module and BC in all kinds of directions. Usually with experimental modules, we keep them everything isolated to/in them. The plugin/config entity MR had to make changes to lots of modules, with feature flags and module checks, but still. AFAIK experimental modules still get removed form releases, that will be very hard to handle then. It's probably not realistic that this is beta stability by the time we ship 11.3. We need to have answers for this, both for core while this is experimental as well as contrib, which might want to start adopting this without breaking compatibility with earlier versions.

Rough ideas on that:
* Instead of per module, I think we should have a single flag that switches all (supported) mails to the new API. maybe the experimental module can be that flag and that's enough, or we have something else. It seems unlikely that sites want to switch user mails but not contact for example. If we allow sites to decide, then we might be able to get away to not have bidirectional BC support for something like hook_mail_alter (which sounds like a nightmare)
* Not much we can do about modules that don't support this yet. Don't think we want or can automatically transform hook_mail() implementations, there are too many custom things going on there with attachments, HTML and what not. However, we could adopt something that helps sites identify modules that aren't converted yet. Inspired by OOP hook conversion, we could maybe support an attribute on hook_mail() / alter implementations like `#[SupportsMailer]`, then we can check if a site has any modules installed that don't have this yet. They'll still work, but sites will need to be aware to configure their mail transports in two places and so on.
* Contact was/is interesting in the MR because it's a separate service. Instead of replacing that conditionally in contact, we can do that in the module. It might be beneficial to push _user_mail_notify() into a service to do the same thing there as well. Then mailer could provide alternative services for all core modules that mails. And once it's no longer experimental/part of core, we move it back.

> So if three blocks have one link each, three links will be loaded in one round trip. But if one block has three links, it will require three round trips - because the first link has to be loaded before the second.

For routes, we have \Drupal\Core\Routing\PreloadableRouteProviderInterface::preLoadRoutes(), which is used by the global preloader that I'm trying to change, but we also call it in \Drupal\Core\Routing\RouteProvider::getRoutesByNames(), which used for example by \Drupal\Core\Menu\MenuLinkTree::load to preload all routes for a given menu.

Maybe as a follow-up, we can think of something similar for specific cases where we know that we have multiple links/url objects, we can explicitly request those to be preloaded? We need to make some assumptions and path aliases did move into a technically optional module now but maybe we can think of something, could go through the UrlGenerator and an event?

So just like my route preloader issue, we still have the concept of preloading, but we move from global preloading to per-block/element preloading, which fits better with partial/render caching.

> Cache tag isn't great since Drupal de-dupes multiple invalidations.

Yes, but only if there were no new caches set since the last invalidation, that's not an issue.

However, cache tags still need to be actually invalidated, and there are _many_ dynamic permissions based on bundles and other config entities, all those would suddenly need to add cache tag invalidations.

This is going to require a lot of work and it might be easier to deal with specific issues on a higher level. For example, the recipe action could support a list of permissions instead of saving the role config entity 30 times, that's always going to be slower.

I'm not sure how the deprecation message would work. We'd need the differentiate the between internal and external calls, that sounds complicated and slow (checking backtrace?).

These were never meant to be called directly and doing so would be result in incomplete and broken results. Making it protected won't break subclasses, it's possible to extend the visibility of a protected method in the parent class.

I can't find any calls that aren't $this->getDefautOperations(), I'd say we either just do it or we don't bother and leave it?

BC is complicated, but I think this looks fine now. Seems a bit overkill to go to all that trouble for a template that almost certainly nobody uses, but makes sense to me that we either do it properly or not at all. If we can remove routes and controllers, then possibly we can just directly remove such a template, but that's a decision for whoever is going to commit this.

Back to RTBC.

Added a review.

My understanding would be both the standard profile and the page recipe, yes.

Catching up on the discussion and merge requests. Long comment incoming...

I don't think LOC is the primary factor to consider for this. Also because the current examples are way more complicated than they need to be due to feature flags, the experimental module and BC. The actual implementation would be a lot less.

That said, I do agree that the config entity approach seems complex. Yes, config entity + plugin are a very common pattern, but sometimes also overused. This feels similar to action plugins and config entities, which are often pretty awkward as it's typically a 1:1 relationship and tedious for modules to maintain the config. In general, code that is hardcoding specific config entity ID's is a bit of a code smell to me. As @adamps said, it's unclear what would happen if that config is missing. For example now with recipes, which pretty much *encourages* installing modules without their config ( I don't really agree with that, but that's another topic). Deleting the password reset template

Naming: I think the template part is misleading. That makes me think of twig templates, HTML and customizing their output. Looking at DSM, it uses build() instead of render(), which makes a lot more sense to me. We build an Email object, which might or might not involve rendering something or using templates, but it's not the main thing. It's also similar to terminology used in simplenews.

An issue with the current architecture is that there's a bidirectional dependency between the plugin and the config entity, as it's passed to render. If you compare with block plugins for example, they don't know about block.module block config entities and can be used in other contexts as well. Similar for field formatters, which are also used by views. That's what $configuration is for, the config entity is responsible for storing and passing that in.

Now, there are cases where a fully implemented config entity + plugin could be useful. For example with user, the plugin would be provide a subject and body setting which would make it self-contained instead of accessing global config (which currently bypasses your attempt at making not just mails but also their data and arguments documented and discoverable). and status is built-in, so the code could check that. But it would mean that the UI would move from user settings to generic mail configuration, which is likely harder to find. It's also not as flexible as you'd be used to from plugins. you can't make up your own notifications, you can only enable/disable and edit the specific settings of them. In the end, pretty limited benefit over the current config and some drawbacks.

contact, update are examples that don't really benefit from there being a config entity, and that's going to be a lot of contrib and custom examples. That's combined with the "problem" that the mail rendering/preparing sits entirely on top of the actual mail send API and is there fore optional. People looking at this and trying to send mails will just directly create the Email object and send it directly. And if they do that, we lose the ability to use a mail theme because rendering of stuff possibly already happened. I think we should make it as easy as possible to use the mail API without losing features like language. And then optional features on top. If you look at commerce_mail(), they essentially bypass the whole thing by just passing in the body and rendering it there. I wonder if we should actually allow to use a callback based approach for those minimal cases, similar to what renderInContext() essentially already is.

I'm not sure how important discoverability and reusability of mail "templates" really is, it might in some cases even be undesired. We recently discussed that ECA explicitly doesn't support those special user password reset mails, to not allow to send them out in the wrong context, that could even be a security issue.

On the other side of the spectrum, use cases that actually have generic and complex use cases around mails might struggle to fit into this architecture. Simplenews has multiple things that make up a newsletter, specifically a entity that sent to a subscriber and there's a lot of rendering and logic going on that should happen within the mail render context. And ECA would need to redefine its mails that sit in its configuration or have a super generic one that would almost become recursive.

I think I get the intent with the types and data, but that really feels complex to use. the user confirm thing requires a data type plugin, a definition class, it's passed in as a raw value, converted to typed data and then used. It values (theoretical) reusability over DX, despite all that complexity, what you work with when writing the code is an array with arbitrary keys. I see that DSM has the concept of mail params on the object. That's still keys and not types that can be enforced, but I think that's an improvement over the single magic mixed type that this has. With mulitple params, we can still document them and their type.

I've even been thinking about going entirely with plain value objects implementing a basic interface, but haven't fully thought that through. But overall I'd clearly recommend keeping the actual mail builder interface/API as plain and un-opinionated as possible and start with that.

That there is no specific exception for this makes it IMHO pretty hard to detect and identify affected code, I'd expect most is custom code that won't see deprecation messages, not without the issue that logs them.

That's my reason to make this a D13 deprecation, and as a result of that, have a way to explicitly bypass the exception to use the nullsafe operator. If you think D12 is fine, then I won't push back against that further and we can possibly skip the argument, if we think that maintaining that magic parameter isn't worth it.

One thing that I'm not sure about yet is the pipelines. I see two options:

* Add specific methods. That worked well for cache, it's just two and they are fairly generic. The reliable queue however has some very specific ones.
* Add our own generic version of that to the client adapter, with our own class to pass it through. the cluster version of that can then for example just not actually do a pipeline and pass it along as separate commands.

Ah, jsonapi, yeah, I didn't test with that. The call chain for me was views data.

Maybe 📌 Change JSON:API routes from every bundle/field to per entity with arguments Active would help then? I'm assuming those calls are coming from route building. If we have those generic routes, then the need to fetch all fields and bundles would just go away... not a quickfix like this obviously.

This is still postponed and contains 4 or so other issues and a bunch of manual fixes, but with all that, it is now green. We might need to add a BC test though for this. could be combined with a BC test for template_preprocess.

FWIW, I don't think the *importer* (which this also has to integrate with) is pluggable enough yet, as we discussed the pre import event is called once for all entities. I think we should add an import event as well, fairly certain that ERR will need that as well.

Forget about the moving part, this does enough. I'll take care of that in 📌 Remove 'includes' support from hook_theme() Postponed

Certainly, I also dropped file, had this open in an old tab and sometimes browser remembering input of textareas is counter-productive. Restored.

Just three modules left, with 5 template preprocess functions in total. One less than I expected, also because block_content is being taken care of in a separate issue and fully deprecated.

@xjm: I think you mean 📌 "Promoted to front page" for new content types should default to Un-Checked Needs work . we did that and this was postponed on that.

> We already spend a lot of time deliberately removing as many descriptions as possible from core fields

I think what we're doing is removing bogus/technical descriptions from entity fields that are exposed to editors. This is the description on the node type settings form, which explicitly states that users with enough permissions can override those checkboxes. The first sentence in the screenshot in #48 is in HEAD now. Except with the new default config, they no longer can. I'm fairly certain that site builders *will* be confused about this.

But I'm also not going to try and convince anyone about this. I like this change and it won't affect me and we can always add such a description later.

And apparently there's no test code that would throw the exception there, so no deprecation message emitted.

I'm suspect there's something more sinister going on.

I found that code because just removing it did result in a fatal error. See the very first pipeline of the original MR: https://git.drupalcode.org/issue/drupal-3490787/-/jobs/3542502. Drupal\Tests\options\FunctionalJavascript\OptionsFieldUI failed there with a fatal error.

That _should_ be a deprecation now.

But this is most likely triggered by an ajax call through the browser. And because our deprecation handling relies on HTTP headers, deprecations in ajax requests are silently dropped. I've seen that before.

It looks like the goal of this issue is move towards more of a 'garbage in garbage out' where typos fail silently, but fields that may or may not exist are more convenient to deal with.

I've been on a project or two where fields are removed from entities, but devs miss removing code like $entity->get({REMOVED FIELD NAME}). Without adequate test coverage, it goes to prod and results in a WSOD.

Well, that is then in many cases traded into php warnings (accessing property on null) or fatal errors ($entity->get('field')->anyMethodCall()). This is isn't meant to address that.

For me, this is specifically about enabling the usage of the nullable operator as mentioned in the issue summary. It would be neat if we could specifically detect that, but obviously that's not possible.

Not sure we'd have to deprecate for Drupal 13 in this case because in general you should not be using exceptions for code control, hasField() already exists and is the actual API to use

Your call, but I think we shouldn't underestimate the impact this will have on custom code.

This change will result in code like this:

      try {
        $entity->get('field_name')->getValue();
      }
      catch (\InvalidArgumentException) {
      }

Should you do that? No. But I'm fairly certain there are hundreds of cases of custom code out there doing exactly that. IDE's like PhpStorm typically even tell you that you should add that try/catch because it's documented to throw that. With the exception removed, this will be a fatal error.

I'm confused why MR !12845 doesn't need the change in \Drupal\field\Entity\FieldStorageConfig::getOptionsProvider. That should trigger the deprecation now, no?

I'm not very fond of that approach without a way to explicitly disable the exception now. It makes it impossible to actually do if ($entity->get('field_name')?->value) until we are on D12/13 and remove the exception.

My reason for pushing this is proceeding with 📌 [meta] Deprecate __get/__set() on ContentEntityBase Needs work and this came up as an argument why people prefer $entity->$field_name over $entity->get($field_name), because that does return NULL and allows this.

This should also deprecate the node_add_list template I think.

And I think 📌 [PP-1] NodeRouteProvider should extend DefaultHtmlRouteProvider Needs work should take care of the node_list_add template, although it doesn't yet. closing as duplicate of those two.

template_preprocess_block_content_add_list() is being removed now, but the template is just deprecated. It won't be working without that preprocess, so we need to keep it. The same was done with template_preprocess_authorize_report for example.

To prepare for 📌 Remove 'includes' support from hook_theme() Postponed , we might still want to move it to .module. Also, in #3504381-25: [meta] Convert Template Preprocess hooks to OOP equivalent → , we discussed that for .inc files, we should keep them with a deprecation message, see for example 📌 Convert template_preprocess in views Active . That's not really done consistently, and it might not block getting it committed.

berdir → created an issue.

I saw that comment and was surprised, we've been using redis on platform.sh since 2015 and never needed this.

Started a proof of concept, all cache classes merged into one, getClient() actually returns an instanceof ClientInterface, which has some new methods for special things (really just multi/pipeline related things atm)

Currently using __call() to pass through all not explicitly defined methods directly to the actual client. Not sure yet if I want to rely on that (with @method on the interface maybe) or explicitly define all the methods used in our implementations and only keep that as a fallback.

Still need to update all the other things and refactor the factory part.

berdir → changed the visibility of the branch 3080449-deprecate-clientinterface-and to hidden.

Yes, but it only calls it if the information isn't already available. stats/info/config output seems to vary wildly between different, it is very much possible that while config() might not work on Azure, info() might not work somewhere else. There also are patches here wit different approaches. if necessary we can extend the logic with a try/catch, removing it completely does not seem like a viable option.

timeout isn't configurable for others. There are no long running scripts or anything like that in 8.x-1.x, so the only reason to have this would be to avoid a long wait in case the connection can't be established, and for that it should be 0.5s like phpredis.

I believe this has been addressed by 📌 Abstract info-collection RedisController to deal with special environments Active , if not, feel free to reopen.

Included this in 📌 Abstract info-collection RedisController to deal with special environments Active , since it would have conflicted with that again.

This will need another rebase, I have extracted and committed the reports stuff in 📌 Abstract info-collection RedisController to deal with special environments Active (kept the cluster things, so should be able to just drop those parts here). This needs work per #87 and also the docs will conflict and should be renamed to .md and added to mkdocs.yml, there are some pending comments on them as well to not repeated general docs, which are also outdated (chained fast stuff)

new valkey testing exposed some fails, memory warnings were also broken, fixed some more things and committed.

berdir → created an issue.

According to tests, this fails relay when the redis extension isn't available.

Only one way to figure out: Creating a merge request to run tests.

🐛 TTL handling broken, always permanent Needs work has been fixed, and this has always been a duplicate of that.

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

Good enough for now.

Merged.

> What would be the problem with removing the exception entirely, without adding the parameter?

Not 100% sure I understand the question, at first I thought it's related to the BC layer, there we have to deal with existing code that uses the exception instead of a hasField() check, like the core/modules/field/src/Entity/FieldStorageConfig.php change in core.

But I realized that I think you mean in the "final form" in D13 and whether we need the ability to ever throw an exception. That's a very good point. Initially we had this inverted, so you had to pass an argument to not get an exception, similar to Container::get() or PluginManager::getDefinition(). Then it was necessary.

I think we do need it now, so that code can benefit from it in D11.3-D13 without triggering the exception/deprecation message, stuff like:

      if ($body = $comment->get('comment_body', FALSE)?->value) {

But we wouldn't need it in D13. So maybe instead of planning to define the argument explicitly in D13, we just plan to remove it again then? We could invert the BC layer to then trigger a deprecation message if you pass in FALSE. And we could also change it now so that only FALSE is valid and not accept TRUE as we'll remove that ability?

Something is there, but not really working yet.

Merging, will need to verify on default branch.

This took some work.

Fixed extended the tests, took a while to not have any random fails, time stuff is tricky, especially as this needs to account for both the fake internal request time and the real time that the redis server uses.

Did a lot of work on the docs as well, not just on offset, but also very outdated docs on ttl in general, documented stuff around maxmemory policy. Created two follow-ups and merged.