This is the Drupal 7 backport issue of #2172871: Async node hit counter data collection is broken when path-based language negotiation is used which has been fixed for Drupal 8!
On a multilingual site using i18n, with Statistics enabled with both the settings "Count content views" and "Use Ajax to increment the counter" tracking fails, receiving a 404 for the statistics.php request because the wrong URL is generated in statistics.module.
For example: When visiting a node in RUSSIAN (using "russian") as the language prefix, the XHR requests url:
http://www.example.com/russian/modules/statistics/statistics.php
Instead of:
http://www.example.com/modules/statistics/statistics.php
Comment | File | Size | Author |
---|---|---|---|
#17 | 2236165-16.patch | 5.92 KB | Anybody |
#11 | drupal-ajax_node_counter_lang_path_prefix-2172871-2.d7-do-not-test.patch | 5.86 KB | Anybody |
#1 | statistics-ajax-php-file-404-2236165-1.patch | 1.31 KB | ericpugh |
Comments
Comment #1
ericpugha patch where all I did is remove using url() to get the url to statistics.php
Comment #2
tobey_p CreditAttribution: tobey_p commentedsubscribe
Comment #4
errand CreditAttribution: errand commentedHey! You saved my life!
#1 worked for me.
Thank you!
Comment #5
effulgentsia CreditAttribution: effulgentsia commentedThanks for the bug report and the fix. I confirmed this bug does not exist in D8, so setting the version that the fix needs to be committed to to 7.x. To be committed, this needs an automated test added to prevent future regression, so tagging for that. We'll also want to forward port that test to 8.x after it lands in 7.x, to prevent future regressions there as well, but let's start with just 7.x.
The test needs to ensure that when you view a node at a language-prefixed URL, that the JS setting to statistics.php is returned without that language prefix.
Additionally:
Out of scope for this issue as the $langcode parameter isn't used by this patch.
$base_url
can be replaced withbase_path()
. The former results in an absolute URL (includes the domain name), the latter in a relative URL (no domain name). It might not matter which is used in this case, but since current 7.x outputs a relative URL, we should stay consistent with that unless there's a reason to change it.Comment #6
David_Rothstein CreditAttribution: David_Rothstein commentedI think this is a duplicate of #2172871: Async node hit counter data collection is broken when path-based language negotiation is used?
Comment #7
iamEAP CreditAttribution: iamEAP commented@David_Rothstein is correct. This issue duplicates #2172871. A patch for Drupal 7 with tests is available in #2.
Closing as dupe. For those following, please subscribe / review / contribute to the aforementioned issue.
Comment #8
AnybodyReviving this issue as of #2172871: Async node hit counter data collection is broken when path-based language negotiation is used last comment regarding backport policy. The patch still applies and fixes the issue.
This is NOT a duplicate, but the Drupal 7 fix!
Comment #9
AnybodyConfirming RTBC for https://www.drupal.org/project/drupal/issues/2172871#comment-8375123 with tests. This is especially problematic, because you need the AJAX version if you're using boost, otherwise the counter won't increase...
Comment #10
thomas.frobieter+1 RTBC
Comment #11
AnybodyTo make things clearer and hopefully include this in the next Drupal 7 core release, I've uploaded patch #2 from #2172871: Async node hit counter data collection is broken when path-based language negotiation is used.
Please credit @iamEAP for that.
This issue affects ALL users with i18n (paths) and Statistics module enabled.
Comment #12
AnybodyComment #13
AnybodyComment #14
AnybodyComment #15
AnybodyThe patch now seems to fail with 7.72 and needs a reroll.
Comment #16
AnybodyPatch for D7 attached based on https://www.drupal.org/project/drupal/issues/2172871#comment-8375123 by @iamEAP including tests. Please credit him if this works. Let's see what the testbot says, I rerolled it against latest D7 dev.
Hope we'll finally get this fixed after years...
Comment #17
AnybodySorry made a mistake with the diff in #16 -.- Correct patch attached which also addresses #5.
Comment #18
AnybodyYeah, it works! Can we please get this reviewed and RTBC'ed?
- Patch works in our real-world tests
- Patch has tests and they are green
- This issue is open for much too long and we're very close to finish
@David_Rothstein & @iamEAP could you have a short look?
Comment #19
AnybodyComment #20
AnybodyComment #21
thomas.frobieterReroll in #17 works perfectly for a month now, fixing a serious problem for multilang projects which is open since years. Also it provides the required tests. RTBC'd, let's get this in :)
Comment #22
AnybodyComment #23
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedDo we need to make these unrelated changes here?
Yes it seems like these
db_truncate()
calls are not working, but the same is in D9, so maybe it will be better for create a follow-up issue to fix it also in D9. See: https://git.drupalcode.org/project/drupal/-/blob/9.5.x/core/modules/statistics/tests/src/Functional/StatisticsLoggingTest.php#L100Other than that I think the patch looks good.
Comment #24
BerdirDrupal 9+ needs a separate issue anyway (this is already a separate follow-up issue), so we can just file one extra issue for that without having to postpone this change.
Comment #25
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedCreated a separate issue for D10: #3357518: StatisticsLoggingTest does not truncate node_counter
It seems like these truncate calls are not needed, as there are no test failures while they are not working. Therefore I suggest that we remove these two truncate calls in D7 entirely. I have proposed the same solution for D10.
Comment #26
AnybodyWhat do we have to do to get this Drupal 7 fix into core after nearly 10 years?
Comment #27
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedI propose to update the patch - instead of fixing the truncate calls let's remove both of them, because these are not needed at all (I suggested the same approach in the D10 patch). And if possible, please upload a new patch together with test-only patch, so that we can also see the failure in test results.
I will add this issue to the next release META issue: #3325849: [meta] Priorities for 2023-06-07 release of Drupal 7, so that it has a greater chance of being committed. I cannot guarantee it, but we will try to take a look at this issue as well. Thanks!
Comment #28
poker10 CreditAttribution: poker10 at ActivIT s.r.o. commentedJust for info, the removal of trucate call(s) was committed to D10 today, so it seems to be the correct approach: #3357518: StatisticsLoggingTest does not truncate node_counter