Problem/Motivation
hook_page_build()
and hook_page_alter()
are deprecated, but still around for BC purposes. Unfortunately, they're broken.
Worse yet: they are broken in such a way that they break their successors: system_page_attachments()
executed hook_page_build()
and hook_page_alter()
, and if there are any implementations of those BC hooks, the attachments added in system_page_build()
and any hook_page_build()
implementation executed before it (i.e. most of them) will be overwritten!
But, not only are they broken, everybody is wondering why we still have them. After all, they're still around for BC, but can't do the same things anymore, so it's actually quite misleading.
Proposed resolution
Remove them.
Remaining tasks
None.
User interface changes
None.
API changes
The already deprecated hooks hook_page_build()
and hook_page_alter()
are removed.
Comment | File | Size | Author |
---|---|---|---|
#30 | rm_hook_page_build__hook_page_alter-2362987-20.patch | 9.01 KB | Wim Leers |
Comments
Comment #1
Wim LeersComment #2
Wim LeersExisting CR https://www.drupal.org/node/2357755 updated.
Comment #3
Wim Leers(Reported by berdir, discussed with catch, who really wanted the BC layer originally, but now agrees that we can remove it.)
Comment #4
catchIf we don't remove it, we need to 1. Fix it 2. Add tests for it.
Given this is just a function rename and the original issue was critical, I think adding the bc layer was a mistake.
Comment #5
BerdirAgree with removing this...
Comment #6
alexpottWe still have mentions of hook_page_build() in
DefaultHtmlFragmentRenderer
andDefaultHtmlPageRenderer
.Comment #7
hass CreditAttribution: hass commentedThis is no longer working in hook_page_attachments(). Where is the test for this?
Comment #8
Wim Leers#6: I intentionally didn't remove those mentions because it's not clear what to replace them with; we already removed "D7-style
hook_page_build()
", where you can add stuff to the page; HEAD'shook_page_build()
can only add attachments. #2352155: Remove HtmlFragment/HtmlPage will removeDefaultHtmlFragmentRenderer
andDefaultHtmlPageRenderer
and hence fix this for us automatically. However, I understand your request, and I've complied with it as well as possible, given the transitional state we're in.#7: You're right, test coverage for this is incomplete, which is why the IS says that the BC
hook_page_build()
andhook_page_alter()
are broken. In the discussion until #5 (and in IRC), we've decided to removehook_page_build()
andhook_page_alter()
, because they were misleading anyway.Removing them also fixes the problem of them breaking
hook_page_attachments()
andhook_page_attachments_alter()
: if there were anyhook_page_build()
orhook_page_alter()
implementations, they would override (!!!) what allhook_page_attachments()
andhook_page_attachments_alter()
had added before and including insystem.module
's implementation.Clarified that in the IS.
My apologies to all of you, I really dropped the ball on this one. Clearly I should have added more test coverage — even when like in this case I should've been able to extend existing test coverage, but there wasn't any. That's no excuse. Sorry!
Comment #9
hass CreditAttribution: hass commentedWe need such a "library" test for
hook_page_attachments()
andhook_page_attachments_alter()
. Please update the tests to run this with library. This is currently broken and I'm highly interested to see a test that verifies that this regression is fixed by this patch.Comment #10
BerdirWe figured this out, it does work.
Comment #11
hass CreditAttribution: hass commentedBut we had tests for this and now we have no tests.
Comment #12
Wim LeersNo, we never had tests for attaching assets in
hook_page_build()
. I did not remove any tests. Look at the patch in #2350949: Add hook_page_attachments(_alter)() and deprecate hook_page_build/alter().Tests now added.
Comment #13
hass CreditAttribution: hass commentedPatch in #8 removed the code in #9, but if this is added back now all is good.
Comment #14
catchI shouldn't have asked for a bc layer in the first place, this is a big reason to differentiate between wrappers and layers when thinking about backwards compatibility. Thanks for the extra test coverage. RTBC.
Comment #15
alexpottIt looks like hook_page_alter is still being fired in DefaultHtmlPageRenderer::renderPage()
Plus the comment is mentioning hook_page_alter.
Comment #16
Wim LeersI'd reroll this, but the remark in #15 is addressed by #2352155: Remove HtmlFragment/HtmlPage already, which is already RTBC. Trying to fix this here would be wasting time. Especially because if this'd land first, I'd have to reroll the other patch, which is 20 times bigger.
This will need to be rerolled once #2352155 lands.
Comment #17
catchComment #18
PinoloComment #19
PinoloComment #20
PinoloRerolled patch. Obtaining an interdiff turned out to be a PITA, so it's not here, sorry.
Comment #21
Wim LeersThank you very much! Just one problem:
You're missing this hunk from #12.
Comment #22
Codenator CreditAttribution: Codenator commentedGoing to re-roll from #12
Comment #23
Codenator CreditAttribution: Codenator commentedApply patch resolve #21 and make re-roll from #12
My file patch to big so my re-roll is not right.
Going to re-roll soon again.
Comment #24
Codenator CreditAttribution: Codenator commentedComment #25
Codenator CreditAttribution: Codenator commentedComment #27
Codenator CreditAttribution: Codenator commentedComment #28
Codenator CreditAttribution: Codenator commented@Wim Leers try to re-roll patch and find out
drupal_prepare_page($page);
removed from core so patch from #20 is okComment #29
Pinolo@codenator, @wim leers: exactly, I thought that hunk was superseded by the landing of #2352155: Remove HtmlFragment/HtmlPage
Comment #30
Wim LeersHaha, oops! And I'm the one who landed that patch! :P I just diffed my last patch and the one rerolled here, and that was the only hunk missing. I should've inspected that hunk more closely.
Reuploading the patch in #20. Thanks again, Pinolo! :)
RTBC per #14, Pinolo just rerolled it after #2352155: Remove HtmlFragment/HtmlPage landed and I just reuploaded his patch after my criticism was wrong.
Comment #31
alexpottThis issue is a critical task and is allowed per https://www.drupal.org/core/beta-changes. Committed b0f2463 and pushed to 8.0.x. Thanks!
We need to update the CR to reflect that hook_page_build and hook_page_alter are gone - https://www.drupal.org/node/2357755
Comment #33
Wim Leers#31: the CR was already updated in #2.
Comment #34
alexpottre #33 well without looking at it I can tell the title has not been updated :)
Comment #35
Wim Leers#34: HAH! Fixed.