Add hook_page_attachments(_alter)() and deprecate hook_page_build/alter()

This certainly can first be started once we have moved the block rendering process out of hook_page_builder()

Having looked at this, we can only remove the block rendering process out of hook_page_build() in #2352155, see #2352155-4: Remove HtmlFragment/HtmlPage for related issues that explain why. In short: we've been working towards moving the block building into DefaultHtmlFragmentRenderer, and since that will go away, we also need to re-evaluate where block building should go in that context (block building is a specific page variant, so: "re-evaluate where instantiating a specific page variant should go" is the actual question).

But we already know we want to remove the concept of hook_page_build(), which allows any module to inject any random crap in any region, so we can already move forward with this, or at the very least begin the review process. This is the most contentious bit, and I've highlighted it in interdiff-contentious.txt.

Noteworthy things that we must tackle in this issue/patch:

1. We still had a block.install that altered block module's weight, specifically to allow other modules to alter its block in hook_page_alter()! Incredible, right?! I removed that.
2. The bubbling of cache tags from the page_top and page_bottom "internal regions" is broken in HEAD, but is only exposed in this patch. This regression was also introduced by #2068471: Normalize Controller/View-listener behavior with a Page object.
3. Fun fact: hook_page_alter() is used to wrap each region in the region "theme wrapper"!
4. The documentation in theme.api.php has been updated to explain when themes should implement hook_page_attachments_alter().
5. One test case in MainContentFallbackTest has become nonsensical/irrelevant: it used to test duplicating the main content to another region, but that is no longer allowed by hook_page_build(). So: removed that test case and documented the existing ones, we still have full test coverage.

1. Could we just call that $attachments? For implementors of this hook, it won't be a fake page any more, just an empty array and they're only adding elements to the top level.

   But the implementations of hook_page_attachments() operate by setting $page['#attached']['library'][] = 'mymodule/library'; — hence $attachments would be weird. Unless you're saying only the code <em>invoking</em> <code>hook_page_attachments() should use this $attachments variable name?

2. Can't use ModuleHandler::invokeAll()? […] looks like an artifact […]

   I'd love that, but AFAICT that doesn't support passing arguments by reference, which this needs.

3. RE: attachments in regions in D7: in Drupal 8, both work. No special cases.
4. RE: _block_page_build(): I agree, that's why I have a @todo in there to fix it in #2352155: Remove HtmlFragment/HtmlPage. Isn't that sufficient?

Yes just the code invoking.

Done.

Does it still work after this patch though? We're only taking attachments out of $page['#attachments'], so if I add something to $page['foo']['#attachments'] I'd expect that to be ignored.

Oh! I thought you were referring to $page['<region>']['#attached'] already being set, but you were referring to it being set in hook_page_build()! Hence you're right.
You said Fine with either answer, it's simpler in the patch but it'd fail silently for an unported 7.x module.. So you either want to see it be supported, or get an explicit failure. Makes sense.
I think explicit failure is better here, because it allows us to also explicitly fail if you're adding new render arrays in hook_page_build(), which we don't support anymore, and hence would also be silent failures (and developers will be wondering Why the hell is my content silently being discarded?! — not good!).
Done.

Code looks good to me. One significant bit of feedback and a couple nits:

1. Why keep around hook_page_build() and hook_page_alter()? There should only be one way to do things. We are still in early beta. I think the minor inconvenience to module devs (a function rename) is less bad than having two ways to do things, for years. The BC implementation is ugly as well, jammed into system_page_attachments()
2. throw new \LogicException('Only #attached and #post_render_cache may be set in hook_page_attachments_alter()');. Missing period.
3. + // and, for backwards compatibility.missing the word: themes

RTBC per #11 and #12, since #12 only required nitpicks to be fixed.

Even if we decide to remove hook_page_build() and hook_page_alter(), that decision doesn't need to hold up this issue; that can be done in a non-critical follow-up. Moshe agreed with this.

Change record created.

I failed to update the exception messages everywhere to have trailing periods. Clearly, the test coverage works :)

How late is the attachment hook executed? It need to be the very very last...

@hass: No difference to before I think. Use hook_attachments_alter() if you want to be after most other attachment hook implementations. But at least devel also wants to be last, and it just dumps the page content, so you probably want it to win.

It looks like hook_page_top and hook_page_bottom runs after hook_attachments_alter(). And blocks runs after hook_page_bottom(), too. That looks like the wrong order to me as I have no access to the final $page in hook_page_attachments_alter(). Just code wise...

And $page['#attached']['library'][] = 'google_analytics/google_analytics'; is also broken in hook_page_attachments() as it does not add the library to the page.

As a note, if the docs need adjusting, all of these hooks have now been relocated to core/modules/system/theme.api.php instead of system.api.php where they were. See #2307859: Move theme/render hooks from system.api.php to theme.api.php

$page['#attached']['library'][] = 'google_analytics/google_analytics'; is broken in hook_page_attachments() as it does not add the library to the page. This is a bug introduced by the patch here.

Please rollback the patch.

#2365985: #attached library is broken in hook_page_attachments()/alter()

Please also explain me the design you are refering, too. The case summary does not explain anything and I have no idea why I cannot alter everything.