Refactor update.fetch.inc into a MailHandler

Moving this under the umbrella of #1803948: [META] Adopt the symfony mailer component.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Fixed a misnamed variable. The approach in #3539178: Extract _user_mail_notify() into a user NotificationHandler changed a little bit (internal service, no interface). Let's see how that goes over there and adapt here if necessary.

Can we get a change record for the new interface?

I have added the change record. Probably It would need more updates. Please review.

Looking at the MR vs the CR think we need to deprecate _update_cron_notify before we can remove it.

Similar discussion over in #3539178: Extract _user_mail_notify() into a user NotificationHandler . I do not have much of an opinion, but we should do it consistently here and in the user module.

Also in the other issue we went without an interface.

Just in case we don’t end up needing deprecation may want to update the title too

We've made exceptions on the underscore-prefixed rule before, when it's not too much trouble and used in contrib. we did that with _field_multiple_value_form_sort_helper() and I think it makes sense for _user_mail_notify() because there are dozens of usages of that in contrib, with that one there's some disagreement on how far that BC logic should go though. That one probably should always have been an API, it is a pretty common thing that other modules need to do as proven by the usages.

That said, this one seems pretty much unused, all I could find is a bunch of copy-pasted and bogus @see references from copied hook_mail() implementations: https://git.drupalcode.org/search?group_id=2&scope=blobs&search=_update_...

IMHO we don't need to be consistent but can decide on a case-by-case basis for these functions.

If it were doing anything else but sending mails, I'd suggest to just inline it into the hooks class, that's what we did in other cases too. We can keep the old code as-is and deprecate it without attempting to call the new code. However, the semi-secret reason that znerol is doing that MailHandler service here is that this helps with the completely separate effort to replace hook_mail with the new symfony mailer API, the idea there is that the experimental module would, if enabled, replace/subclass all those services to use the new API until it's no longer experimental. But that won't work in all cases anyway, so I'm undecided myself and can live with either the service or inlined in the hooks class. Either way, +1 on no interface.

Applied the same approach as in #3539178-22: Extract _user_mail_notify() into a user NotificationHandler to get rid of the interface and still permit a subclass with a completely different kind of mail manager.

It is possible to mark methods for setter injection with the #[Required] attribute. That way, a subclass doesn't need to receive and pass on a MailManager via its constructor. It only needs to declare the services which it really needs.

Updated the CR.

Should we mark Drupal\update\MailHandler as @internal.

Updated search link for #25: _update_cron_notify seems like it's not used anywhere

Adapt to approach discussed in #3539178: Extract _user_mail_notify() into a user NotificationHandler.

There are a couple of contrib modules including the file though which is weird because they don't use the function.

https://git.drupalcode.org/project/dash/-/blob/6fc17292c04d903f1285db629...

That whole method is... interesting to say the least.

I think this means we should deprecate the file, although in one of the locale issue alexpott said we should explicitly not deprecate the file and just delete the file when ready, the immediate white screen is a better signal.

So in not sure we change anything now that I think further.

include() doesn't fatal when a file is missing. require() does.

That's right, so nothing to change, I'll review it again when I have a moment.

I took a careful look at this. All of the code that was moved has been faithfully moved.

The actual send code has been moved with the reasons stated in 25.

This may get kicked back for consistency and require deprecation. But as noted, it is not used anywhere, and there is only the one function in that file so I think we can make the exception and clean up the whole include.

I reviewed the IS and the CR.

The CR is a bit sparse, but I question if we need it since we're not even deprecating it.

Performance test needs tweaking.

Asset Aggregation Across Pages (Drupal\Tests\demo_umami\FunctionalJavascript\AssetAggregationAcrossPages)
     ✘ Assets across multiple pages
       ┐
       ├ Failed asserting that two arrays are identical.
       ┊ ---·Expected
       ┊ +++·Actual
       ┊ @@ @@
       ┊  Array &0 [
       ┊      'ScriptCount' => 15,
       ┊ -····'StylesheetCount'·=>·8,
       ┊ +····'StylesheetCount'·=>·7,
       ┊  ]
       │
       │ /builds/core/tests/Drupal/Tests/PerformanceTestTrait.php:692
       │ /builds/core/profiles/demo_umami/tests/src/FunctionalJavascript/AssetAggregationAcrossPagesTest.php:116
       │ /builds/core/profiles/demo_umami/tests/src/FunctionalJavascript/AssetAggregationAcrossPagesTest.php:30
       ┴
    
    FAILURES!
    Tests: 1, Assertions: 43, Failures: 1.

Also a question on the MR.

I don't see why this would affect asset aggregation, maybe a random fail? Agree on the lazy services, most cron executions won't need them.

I don't see why this would affect asset aggregation, maybe a random fail?

Re-running again now, but I think it's failed at least twice.

It did fail again yes, but this was a few commits behind and I think it just managed to hit the HEAD fail introduced by #3502993: Convert Navigation messages component to SDC, there was a follow-up commit to that that fixed that test. I rebased it through the UI to confirm.

Feedback has been addressed.

Consulted with @quietone and @catch about the how the service closure properties were documented, such as

    /**
     * @var \Closure(): \Drupal\update\MailHandler
     */

Decision was that this is fine, though we may open a coding standards issue if there's a way we want to standardize going forward.

Committed and pushed to 4081563 main and df22840 to 11.x. Thanks!