Problem/Motivation

Follow-up from #2832074: Fix hook_page_top implementation for node.module.

NodeHooks1::pageTop() runs on every request, just in case we're on a node preview route, to add the form above the preview.

I think it ought to be possible to change the preview route to include the markup currently being added in node_page_top() - i.e. put this in the controller one way or another.

Steps to reproduce

Proposed resolution

Allow page_top and page_bottom attachments

Remaining tasks

Review

User interface changes

API changes

#3558617: Drupal\Core\Render\MainContent\HtmlRenderer::buildPageTopAndBottom now has $page_top and $page_bottom parameters

Data model changes

N/A

Release notes snippet

N/A

CommentFileSizeAuthor
#21 3339905-nr-bot_f8t1_tzo.txt1.21 KBneeds-review-queue-bot
#10 3339905-nr-bot_czwq8o7c.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3339905

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

catch created an issue. See original summary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Controllers return output that is wrapped into the main content block in page.html.twig. page_top is for things that get printed outside of the normal page wrapper, but maybe the main content block could bubble this up to the top level somehow?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

I thought about this some more; we already allow ['#attached']['html_head'] to add things to <head>, so why not allow ['#attached']['page_top'] to put things in the page top?

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

acbramley’s picture

acbramley commented
Title: Get rid of node_page_top() » Get rid of NodeHooks1::pageTop()
Issue summary: View changes
Status: Active » Postponed (maintainer needs more info)

This is a fairly common pattern in core, contrib and custom code. We have 6 implementations in core (block, navigation, node, system, update, toolbar) that all do some variation of checking route, request, or user permissions to add stuff to the page.

Is this really that much of an overhead or something that shouldn't be done?

catch’s picture

catch
he/him
Primary language English
commented
Priority: Normal » Minor
Status: Postponed (maintainer needs more info) » Active

Navigation and toolbar both add their markup on every page, for me that's a good use of the hook.

System and update add it for all admin users on all admin pages, that's again close to 'site-wide' and seems OK, although to a large extent it's compensation for not having dashboard module or similar in core.

Block's implementation is very similar to node's - it's adding some specific markup for a specific route, for me that should also be done in the route controller one way or another. Feels like a symptom of both block and node preview being very ancient. Moving back to active but yes it's not the worst thing in the world so downgrading to minor.

catch’s picture

catch
he/him
Primary language English
commented
Related issues: +#3511357: Clean up hook implementations in Node module

acbramley opened merge request !13163

acbramley’s picture

acbramley commented
Title: Get rid of NodeHooks1::pageTop() » Get rid of NodeThemeHooks::pageTop()
Status: Active » Needs review

Ran with the idea in #3 a bit, I'm not sure on the implications of allowing page_top and page_bottom in HtmlResponseAttachmentsProcessor. I tried playing around with unsetting them in buildPageTopAndBottom with a pass by reference on $main_content but that didn't work.

Setting to NR to gauge how viable this would be.

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3339905-nr-bot_czwq8o7c.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. 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.

acbramley’s picture

acbramley commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work
Issue tags: +Needs Review Queue Initiative

Tested this out on a local 11.x install with standard profile

Verified that preview is still working
1. Created a random basic page content type
2. Clicked preview and it was correctly rendering

Only thing I see is if a CR could be created about the new parameter

Thanks

acbramley’s picture

acbramley commented
Status: Needs work » Needs review
Issue tags: +Needs change record

I still want a review of the approach before creating a CR

oily’s picture

oily commented

Re: #3:

I thought about this some more; we already allow ['#attached']['html_head'] to add things to , so why not allow ['#attached']['page_top'] to put things in the page top?

There are lots of good reasons to 'add things to <head>' whatever PHP framework you are using. I think that having a piece of the page dedicated to the node preview form was originally because it was felt that the importance of the form markup was sufficient to justify it getting 'red carpet treatment' ie special real estate on the page?

I am not quite sure if the net effect of the refactoring is that node preview form will lose its special status or if there will be a danger of it competing with other markup that might affect UX? Are there any other issues to consider like performance? Other questions are whether the form ever really needed its special status and if it did then, whether it does now.

At the end of the day I think the question is so long as the UX is pretty much the same whether this will make page rendering more susceptible to bugs.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

@acbramley had a chance to look at this today and I don't see anything wrong with the approach you are taking.

acbramley’s picture

acbramley commented
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs change record

CRs added, not sure if we should/need to make the new parameter optional since this method is public API.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Yea think we will have to make it optional. Did a search for buildPageTopAndBottom and modules like Google Tag (all be it in a test) and Canvas both have references (even though they don't do anything with it). Think it would be kind to not break them if possible.

https://git.drupalcode.org/search?group_id=2&scope=blobs&search=buildPag...

acbramley’s picture

acbramley commented
Status: Needs work » Needs review

Should be good to go now

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Bumped to 11.4 and rebased

Believe all feedback for this one has been addressed

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Reviewed & tested by the community » Needs work
StatusFileSize
new 3339905-nr-bot_f8t1_tzo.txt1.21 KB

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.

acbramley’s picture

acbramley commented
Status: Needs work » Reviewed & tested by the community

Agreed with your changes, marking this back to RTBC since they are fairly trivial.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

I just realised we need to add documentation for these new attachment keys in the correct place. Added a comment to the MR>

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

Also we should add a follow-up to convert \Drupal\block\Hook\BlockHooks::pageTop() (I think this is the only other implementation in core that would lend itself to this pattern)

acbramley’s picture

acbramley commented

Created #3571143: Convert BlockHooks::pageTop to attachment in BlockController::demo for the follow up.

acbramley’s picture

acbramley commented
Status: Needs work » Needs review

Moved the comments.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Feedback from @alexpott appears to be addressed

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Title: Get rid of NodeThemeHooks::pageTop() » Add page_top and page_bottom to #attached to get rid of NodeThemeHooks::pageTop()
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 07f90f1f3d7 to main and b73b9291c84 to 11.x. Thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • alexpott committed b73b9291 on 11.x 
    feat: #3339905 Add page_top and page_bottom to #attached to get rid of...

  • alexpott committed 462af0a2 on main 
    feat: #3339905 Add page_top and page_bottom to #attached to get rid of...
acbramley’s picture

acbramley commented

Nice! Do we publish the CRs now? I'm not sure if that process has changed.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@acbramley thanks for the reminder - no the process has no changed.

quietone closed merge request !13163

nod_’s picture

nod_
Primary language French
Location Lille
commented

Updating search link from #17: buildPageTopAndBottom

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.