Path to statistics.php is not correct when the path start with index.php

I am seeing this error a lot recently. Patch applied. testing for a few days.

+++ b/core/modules/statistics/statistics.module
@@ -41,7 +41,7 @@ function statistics_help($route_name, RouteMatchInterface $route_match) {
     $build['#attached']['library'][] = 'statistics/drupal.statistics';
-    $settings = ['data' => ['nid' => $node->id()], 'url' => Url::fromUri('base:' . drupal_get_path('module', 'statistics') . '/statistics.php')->toString()];
+    $settings = ['data' => ['nid' => $node->id()], 'url' => base_path() . drupal_get_path('module', 'statistics') . '/statistics.php'];
     $build['#attached']['drupalSettings']['statistics'] = $settings;

I don't think we should deprecate on base_path(), which is more or less deprecated.

What about using file_url_transform_relative(file_create_url()) for this? Kinda weird to do it for a .php file, but we *are* creating a URL to a file.

This might break someone uncondtionally changing all file URL's to be passed through a different domain/CDN but I think you shouldn't do that anyway.

We should ask @dawehner about this.

Looks good.

I tested the test without the fix to statistics.module and it failed as expected.

So update_authorize_update_batch_finished() contains the following to - if I am not mistaken - to achieve the same thing:

  $results['tasks'][] = [
    '#type' => 'link',
    '#url' => $url,
    '#title' => t('Run database updates'),
    // Since this is being called outsite of the primary front controller,
    // the base_url needs to be set explicitly to ensure that links are
    // relative to the site root.
    // @todo Simplify with https://www.drupal.org/node/2548095
    '#options' => [
      'absolute' => TRUE,
      'base_url' => $GLOBALS['base_url'],
    ],
    '#access' => $url->access(\Drupal::currentUser())
  ];

That code predates the existance of file_url_transform_relative(), though, and using that might actually be a nicer solution. So not downgrading, as I'm not sure, but wanted to point this out. I agree @dawehner would be great voice here.

+++ b/core/modules/statistics/statistics.module
@@ -41,7 +41,8 @@ function statistics_help($route_name, RouteMatchInterface $route_match) {
+    $statistics_path = file_url_transform_relative(file_create_url(drupal_get_path('module', 'statistics') . '/statistics.php'));

This is very very very wrong. This is not what file_create_url() or file_url_transform_relative() are designed for.

> Creates a web-accessible URL for a stream to an external or local file.

statistics.php *is* a file. Any route based thing doesn't work because it for example includes index.php/ in the URL if you are currently on such a path.

as far as I see, the only other alternative is to use the base_url function/global, any other suggestion?

But it's not a static file, an asset. Something like an image, a PDF, a CSS file. It's an executable that you want to be executed.

For now, I think it's best to do what @tstoeckler quoted in #17. That's the most similar use case.

Did you tried using something like this:

$settings = ['data' => ['nid' => $node->id()], 'url' => \Drupal::request()->getBasePath() . '/' . drupal_get_path('module', 'statistics') . '/statistics.php']; 

?

Thanks, @dawehner! And it works fine with index.php (including a subfolder case) too. And this approach is not yet in our collection:

Oh yeah, I should have actually listed the cases I tested:

• localhost/d8
• localhost/d8/index.php
• d8.loc/index.php
• d8.loc

Is the bug described in this thread the source of the constant:

"page not found" 

http://{site}/core/modules/statistics/statistics.php

warning/errors in the recent log messages for nearly every URL on a site?

For me at least this is ready to go. We have test coverage and a working fix.

I'm not sure why we can't use file_url_transform_relative() here. The docs don't indicate it's for this use case, but that's exactly what it's for?

At least for me having a absolute URL instead of a relative URL feels safer, but that's just me :)

This bug still exists as of the 8.4.0 update.

The log keeps showing statistics.php not found errors.

I've uninstalled the core statistics now, since it is basically neutered in 8.x, now also broken, and therefore useless.

I just started looking at the statistics module (because I want to use it on my own site) and noticed that its JS is using drupalSettings.statistics.url to determine which URL to send data to.

That's … weird … because we have Drupal.url() for that since several years (new in D8 though). There's no need to send this data in the body of every HTML response.

This optimization also happens to fix the bug originally reported here.

/me curses https://www.drupal.org/node/2815083 for not running automatically

Here's an updated patch with the ES6 transpiled to ES5.

Thanks Wim, looks good to me

No, that's not so weird.

statistics.php is a file. Drupal.url() is about a (routed) path, it adds the path prefix, e.g. the language prefix. But that doesn't work for files and results in a 404 on a multilingual site.

That said, we can possible use the same implementation and just leave out the pathPrefix? (No we can not as I found out below)

Also, according to my test, it actually does *not* fix the index.php problem either. If I am on d8/index.php/node/1, then drupalSettings.path.basePath is "/index.php/", again because it is about routed paths and not files.

We really need a JS test that makes sure it works with language prefix and with index.php in the path :)

I'm confused why we lost the test coverage from earlier versions of this patch.

@Berdir
You made a good point here. I also believe that this should be done in PHP vs. JS, because the file might be somewhere different, for example when you don't want to put your code into the webroot. Based upon that changing this value in PHP seems easier than changing it in JS.

+++ b/core/modules/statistics/tests/src/FunctionalJavascript/StatisticsLoggingTest.php
@@ -0,0 +1,83 @@
+
+  /**
+   * Data provider for testLoggingPage().
+   */
+  public function providerTestLoggingPage() {
+    return [
+      ['node/1'],
+      ['en/node/1'],
+      ['index.php/node/1'],
+      ['index.php/en/node/1'],
+    ];

The thing is that means is that we have to run through the whole install process 4 times, as each provider is a completely separate test run.

I would just make this a loop inside the test method. Not so nice, but IMHO we should only use providers for browser tests if they provide a real benefit and we need to actually install/configure different and conflicting things that we can't do in a single test run.

That does mean that we have to change the logic around the "1 view" check, but actually seems like a good thing, making sure that it also works correctly on cache hits and so on?

As a follow-up, maybe we could convert most of the other test coverage that fakes actual hits with PHP to this JS test. I think that could both improve and simplify our test coverage.

#38: you're a bit trigger-happy with that RTBC — this still needed test coverage and actual manual testing.

#39:

statistics.php is a file.

It's not a file. A "file" in Drupal parlance is a static asset. An image. A video. A font. A CSS file. A JS file. A document. This is a PHP file, with code we want to execute. Hence it is not a file. Files/assets can be served from a reverse proxy. statistics.php cannot be served from a reverse proxy.

Also, according to my test, it actually does *not* fix the index.php problem either. […]

Makes sense!

#40:

I'm confused why we lost the test coverage from earlier versions of this patch.

Because I started from scratch.

because the file might be somewhere different, for example when you don't want to put your code into the webroot.

Really? Isn't this tantamount to hacking core? And if you're hacking core's PHP code, you might as well also hack core's JS code.

I see no reason for burdening every HTML response with metadata that can be perfectly computed on the client side.

@Wim Leers - Re #38 I was so excited someone was actually working on Statistics.

> A "file" in Drupal parlance is a static asset

> This is a PHP file

Would be news to me that the term "file" can be used only for static assets. file_create_url() for example doesn't ever talk about static assets only, it only talks about managed and shipped files. This is IMHO a shipped file.

Either way, that's not really the relevant part of my review, the relevant part is that is *not* a route. I think that we can agree on :)

And that means that your approach or anything that just relies on the the existing information in drupalSetings.path can not work.

because the file might be somewhere different, for example when you don't want to put your code into the webroot.

Really? Isn't this tantamount to hacking core? And if you're hacking core's PHP code, you might as well also hack core's JS code.

No, this is considered a good security practice. You can read more about it here https://www.drupal.org/node/2767907

I like that the current solution allows you to get the best of everything possible. There is a sensible location set by default, but you can also alter the settings and point it to somewhere else.

Some of the language in this thread seems a little aggressive, let's please remember to be mindful of how our words come across. Thanks :)

1. +++ b/core/modules/statistics/tests/src/FunctionalJavascript/StatisticsLoggingTest.php
   @@ -0,0 +1,126 @@
   +      'node/1',
   +      'en/node/1',
   +      'index.php/node/1',
   +      'index.php/en/node/1',
   

   Nice test coverage!

2. +++ b/core/modules/statistics/tests/src/FunctionalJavascript/StatisticsLoggingTest.php
   @@ -0,0 +1,126 @@
   +    $this->assertSame(0, $this->getStatisticsCounter('node/1'));
   +    $this->assertSame(1, $this->getStatisticsCounter('node/1'));
   +    $this->assertSame(2, $this->getStatisticsCounter('en/node/1'));
   +    $this->assertSame(3, $this->getStatisticsCounter('en/node/1'));
   +    $this->assertSame(4, $this->getStatisticsCounter('index.php/node/1'));
   +    $this->assertSame(5, $this->getStatisticsCounter('index.php/node/1'));
   +    $this->assertSame(6, $this->getStatisticsCounter('index.php/en/node/1'));
   +    $this->assertSame(7, $this->getStatisticsCounter('index.php/en/node/1'));
   

   That is some really nice test!

3. +++ b/core/modules/statistics/tests/src/FunctionalJavascript/StatisticsLoggingTest.php
   @@ -0,0 +1,126 @@
   +   * Get counter of views by path.
   

   Nitpick: "Gets" instead of "Get"

This looks really good for me now, but maybe @timmillwood as the statistics module maintainer should have another look.

Looks good to me.

Only issue is the test-only patch in #49 isn't only the test. Feel free to reupload both the test-only and full patch for #49 to help committer review.

As this is a bug fix it should be possible to backport to 8.5.x too.

#46: 😃❤️

#48:

Would be news to me that the term "file" can be used only for static assets. file_create_url() for example doesn't ever talk about static assets only, it only talks about managed and shipped files. This is IMHO a shipped file.

I am the one who wrote those docs in #499156: CDN integration: allow file URLs to be rewritten by hook_file_url_alter() … and at the time my English was far less proficient. Sorry for the ambiguity :( I totally agree that the wording could and should be much better!

+++ b/core/modules/statistics/tests/src/FunctionalJavascript/StatisticsLoggingTest.php
@@ -0,0 +1,87 @@
+  public static $modules = ['node', 'block', 'statistics', 'language'];

Why is the block module being installed?

Thank you for fixing!

Adding credit to reviewers who've helped the patch progress and steered it towards a solution.

Committed d85463b and pushed to 8.6.x. Thanks!

Setting to ptbp for cherry-pick to 8.5.x once 8.5.0 is out.

Committed 684bdfa and pushed to 8.5.x. Thanks!