Remove invocation of hook_html_head_alter()

That just sounds like a subset of hook_page_attachments_alter, and therefore unnecessary

https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Render!theme.api....

Agree with #6. So I think just a change notice is needed here.

This is the issue that removed the hook documentation: #2218117: Bring back metatag support for the HtmlPage object, and the corresponding CR does mention the removal of the hook, sort of: https://www.drupal.org/node/2259045

I've documented this change in https://www.drupal.org/node/2160069

It doesn't really need a deprecation as such, given that it's already an undocumented hook.

Just some rough stats on D7 use of this hook: (keep in mind I have only all the themes & modules provided by https://www.drupal.org/sandbox/greggles/1481160)

There are 124 usages in modules and themes.

1. 77 in themes
2. 47 in modules

Some usescase were to remove meta tags, add CSS link tags, add some favicons, modify element attributes to add charsets.

Looks like hook_page_attachments_alter() looks to cover all the cases.

I think we just need to get the removal issue through if possible.

Do we need to deprecate, or can we just remove? It's a really simple replacement, and I'm working on the assumption RC1 will be out next week. Much better if it's not in RC1 at all.

Here's a patch for that. It'll conflict with #2568511: Fix broken test: KernelTestBase::render but at least it shows what is required.

Removed needs change record, as this is now covered by https://www.drupal.org/node/2160069

Looking on interface definition https://api.drupal.org/api/drupal/8/search/processAttachments
There's no mentions about hooks executed but:
1) AjaxResponseAttachmentsProcessor::buildAttachmentsCommands() calls hook_ajax_render_alter
2) the subject conditionally alters hook_html_head_alter()

So my concerns about actual need to do "post-process-alter" here at all?
Obviously we need a way to alter ajax commands but "html_head" and only if that array initialized....

IS and beta eval needs update a bit.

+++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
@@ -187,12 +187,7 @@ public function processAttachments(AttachmentsInterface $response) {
       if (!empty($attached['html_head'])) {
-        $html_head = $this->processHtmlHead($attached['html_head']);
-        // Invoke hook_html_head_alter().
-        $this->moduleHandler->alter('html_head', $html_head);

That looks the only hook here invoked conditionally, and that's looks wrong

#14: We already have hook_page_attachments_alter, which is invoked during the render process. Are you saying we might need hooks that run later than that? I can't think of any scenarios myself.

I don't really understand what you mean by "That looks the only hook here invoked conditionally, and that's looks wrong". The code you've quoted is being removed.

This is rc deadline, so it would be great to get another review on this, or clarification from andypost.

@ianthomas_uk I was trying to point that this hook should be mentioned in @see and get rid of optimization:

if (!isset($attached['html_head'])) {
  $attached['html_head'] = [];
}
$variables['head'] = $this->processHtmlHead($attached['html_head']);

+++ b/core/lib/Drupal/Core/Render/HtmlResponseAttachmentsProcessor.php
@@ -187,12 +187,7 @@ public function processAttachments(AttachmentsInterface $response) {
       if (!empty($attached['html_head'])) {
...
+        $variables['head'] = $this->processHtmlHead($attached['html_head']);

when there's no such key in attachments no hook executed, so this hook does not allow contrib to alter/add head when array is not initialized

@andypost That function (processHtmlHead, it's not a hook) is documented as "Ensure proper key/data order and defaults for renderable head items." - i.e. it cleans up an existing an array, and therefore doesn't make sense to call if you don't have an array. It follows the pattern of other functions called in the surrounding lines.

If you wanted to override processHtmlHead, then you would already be overriding HtmlResponseAttachmentsProcessor and therefore could override processAttachments itself.

More likely, you'd use hook_page_attachments_alter, which is the last thing called in HtmlRenderer::prepare

hook_html_head_alter() wasn't always called. That's a bug, but is irrelevant as that's the hook we are removing.

hook_page_attachments_alter() is always called. invokePageAttachmentsHooks is the last thing that is called in HtmlRenderer::prepare, and is not inside an if. It invokes both module hooks and the theme hook:

  public function invokePageAttachmentHooks(array &$page) {
    // Modules can add attachments.
    $attachments = [];
    foreach ($this->moduleHandler->getImplementations('page_attachments') as $module) {
      $function = $module . '_page_attachments';
      $function($attachments);
    }
    if (array_diff(array_keys($attachments), ['#attached', '#cache']) !== []) {
      throw new \LogicException('Only #attached and #cache may be set in hook_page_attachments().');
    }

    // Modules and themes can alter page attachments.
    $this->moduleHandler->alter('page_attachments', $attachments);
    \Drupal::theme()->alter('page_attachments', $attachments);
    if (array_diff(array_keys($attachments), ['#attached', '#cache']) !== []) {
      throw new \LogicException('Only #attached and #cache may be set in hook_page_attachments_alter().');
    }

    // Merge the attachments onto the $page render array.
    $page = $this->renderer->mergeBubbleableMetadata($page, $attachments);
  }

Nice, it needed the reroll because we were updating the docblock for drupal_process_attached(), which was recently removed.

Are we sure that all use-cases pointed out by joelpittet like removing meta tags, etc. are still fulfillable?

The hook_page_attachments_alter() is called just for the page attachments, but that does not include necessarily things added e.g. in placeholders (maybe we need to move the hook from the HtmlRenderer to the HtmlResponseAttachmentsProcessor instead).

Just some food for thought.

Edit:

Wim Leers points out that we can add another hook later to take care of that.

Are we sure that all use-cases pointed out by joelpittet like removing meta tags, etc. are still fulfillable?

Some usescase were to remove meta tags, add CSS link tags, add some favicons, modify element attributes to add charsets.

Yes, because additions are never a problem, and removing meta tags still works then because they're added in system_page_attachments().

The ability to arbitrarily manipulate any attachments we don't have in 7 either, and we could easily add later.

Can we open a follow-up for that so we don't forget?

Why? We don't have that ability in 7 either, when the need arises, such an issue will surely be opened.

We have a replacement and the hook is completely undocumented and tested. In my opinion it is less risk to remove rather than keep. Committed bbc9852 and pushed to 8.0.x. Thanks!

Hurray!