Ajax POST to statistics.php fails when using i18n with path prefix language code

subscribe

Hey! You saved my life!

#1 worked for me.

Thank you!

Thanks 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:

1. +++ b/modules/statistics/statistics.module
   @@ -114,7 +114,7 @@ function statistics_permission() {
   -function statistics_node_view($node, $view_mode) {
   +function statistics_node_view($node, $view_mode, $langcode) {
   

   Out of scope for this issue as the $langcode parameter isn't used by this patch.

2. +++ b/modules/statistics/statistics.module
   @@ -123,7 +123,11 @@ function statistics_node_view($node, $view_mode) {
   -      $settings = array('data' => array('nid' => $node->nid), 'url' => url(drupal_get_path('module', 'statistics') . '/statistics.php'));
   +                    'url' => $base_url . '/' . drupal_get_path('module', 'statistics') . '/statistics.php',
   

   $base_url can be replaced with base_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.

I think this is a duplicate of #2172871: Async node hit counter data collection is broken when path-based language negotiation is used?

@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.

Reviving 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!

Confirming 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...

+1 RTBC

To 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.

The patch now seems to fail with 7.72 and needs a reroll.

Patch 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...

Sorry made a mistake with the diff in #16 -.- Correct patch attached which also addresses #5.

Yeah, 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?

Reroll 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 :)

Do we need to make these unrelated changes here?

@@ -75,9 +75,25 @@ class StatisticsLoggingTestCase extends DrupalWebTestCase {
     // Clear the logs.
-    db_truncate('accesslog');
-    db_truncate('node_counter');
+    db_truncate('accesslog')->execute();
+    db_truncate('node_counter')->execute();

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#L100

Other than that I think the patch looks good.

Drupal 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.

Created 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.

What do we have to do to get this Drupal 7 fix into core after nearly 10 years?

I 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!

Just 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