Updated: Comment #0

Problem/Motivation

If any hooks that run before history_node_view_alter() add assets to a node, they will be completely removed.
I found this while testing Display Suite module, its CSS wasn't being loaded.

Proposed resolution

Fix it
Consider adding a regression test, but not sure that's necessary

Remaining tasks

User interface changes

API changes

CommentFileSizeAuthor
#6 history-2191543-6.patch1.54 KBtim.plunkett
#1 history-2191543-1.patch1.55 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Active » Needs review
FileSize
history-2191543-1.patch1.55 KB

Mostly outdenting.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented
Issue tags: +JavaScript

Tagging for JS ninjas.

nod_’s picture

nod_
Primary language French
Location Lille
CreditAttribution: nod_ commented

Lol what the hell is that thing. I'll open a follow-up to clean that up. Thanks jibran.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

1: history-2191543-1.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1: history-2191543-1.patch, failed testing.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented
Status: Needs work » Needs review
FileSize
history-2191543-6.patch1.54 KB
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented 
+++ b/core/modules/history/history.module
@@ -135,20 +135,14 @@ function history_cron() {
+      'data' => 'window.addEventListener("load",function(){Drupal.history.markAsRead(' . $node->id() . ');},false);',

Do this inline add dependency for Drupal object? (Drupal)

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett commented

No idea, I don't change that line, it's just an indentation change.

tim.plunkett queued 6: history-2191543-6.patch for re-testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

I thought this went in :)

I don't really see the point in adding a test, we would have to add a test module to add some #attached thing before history and then verify it's on the page, but it's seems like a very one-off thing to test for.

The fix is RTBC, going to let a maintainer decide if we really want to add YATM (yet another test module) to be able to test this.

alexpott’s picture

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

Committed 57cf7f0 and pushed to 8.x. Thanks!

  • alexpott committed 57cf7f0 on 8.x 
    Issue #2191543 by tim.plunkett: Fixed history_node_view_alter()...

Status: Fixed » Closed (fixed)

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