Updated: Comment #N

Problem/Motivation

The Statistics module has one job: to count hits to nodes; in D8 HEAD, this is simply not happening. The root cause seems to be that neither the statistics.js file nor the necessary drupalSettings items are being included on any node page. This could be because of a faulty hook_node_view() implementation, or the hook doesn't exist anymore. It's also possible that the way those scripts/settings are being attached is no longer valid.

Marking as critical since this is literally the only thing the Statistics module does and it's broken.

Proposed resolution

We need to get these assets back on node pages; we also need to fix the tests to ensure they're included.

Remaining tasks

Fix, patch, test, etc.

User interface changes

n/a

API changes

n/a

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

iamEAP’s picture

Issue summary: View changes
iamEAP’s picture

iamEAP’s picture

Status: Active » Needs review
FileSize
1.03 KB

Needs tests.

iamEAP’s picture

The last submitted patch, 4: drupal-ensure_stats_js_included-2172859-4-tests_only.patch, failed testing.

The last submitted patch, 4: drupal-ensure_stats_js_included-2172859-4-tests_only.patch, failed testing.

iamEAP’s picture

iamEAP’s picture

Just cleaning up test comments. Reviews welcome.

The last submitted patch, 4: drupal-ensure_stats_js_included-2172859-4-tests_only.patch, failed testing.

The last submitted patch, 4: drupal-ensure_stats_js_included-2172859-4-tests_only.patch, failed testing.

iamEAP’s picture

nod_’s picture

Status: Needs review » Reviewed & tested by the community

That works for me, thanks :)

webchick’s picture

Status: Reviewed & tested by the community » Needs review

Why did we remove the tests that ensure Statistics module continue to log on cached pages? That's also part of the only thing that Statistics module does. :)

nod_’s picture

Well stats are JS only so cached or not as long as there is the stat JS file and the proper settings it will be logged.

Oh I see what you mean. Not sure why :p because the actual logging doesn't depend on cache enabled or not? The logging always goes through statistics.php and that's not a page that drupal can even cache.

iamEAP’s picture

Yup, nod_ covered it. The cached vs. not cached check was important when logging was done in a hook_exit(), but now that it's an asynchronous script + an uncacheable callback, it's not a concern.

Would seem like a waste of test cycles to make sure a piece of JS is still there after a page is cached.

catch’s picture

Status: Needs review » Reviewed & tested by the community

Yeah if we had real browser testing we could do a meaningful test here, but we don't so it would only be checking the js file is added on the cached page. Moving back to RTBC but I didn't review the actual patch here so not committing yet.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Ok, thanks that makes sense.

Committed and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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