Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Hi,
I've installed Drupal 8.3.2 in a subfolder of my public_html webfolder. The website is accessed by https://www.mydomainname.com/subfolder
I enabled the statistics module, but this one is now making calls to:
https://www.mydomainname.com/subfolder/<b>index.php</b>/core/modules/statistics/statistics.php
The 'index.php' should not be in the URL. The URL is constructed in: core/modules/statistics/statistics.module:statistics_node_view (line 44). But I think the root cause is in the Url implementation which should not add the 'index.php'?
Also this problem can be reproduced for the usual installation of Drupal (without a subfolder):
- Install statistics module and checked 'Count content views' in configuration of this module.
- Go to page
<site>/index.php/node/add/article
and create Article node - Go to page
/admin/reports/dblog
and see the message'page not found'
.
Comment | File | Size | Author |
---|---|---|---|
#56 | interdiff-53-56.txt | 664 bytes | Anonymous (not verified) |
#56 | 2881999-56.patch | 4.06 KB | Anonymous (not verified) |
#37 | 2881999-37.patch | 1.97 KB | Wim Leers |
#37 | interdiff-36-37.txt | 546 bytes | Wim Leers |
#36 | 2881999-36.patch | 1.48 KB | Wim Leers |
Comments
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedI installed the 8.3.2 in the subdomain, installed and enabled the statistics module and could not reproduce the problem (see attached gif with confirm it)
Comment #3
Jochus CreditAttribution: Jochus commentedHi vaplas,
Thanks for your very fast feedback. I just tried it also on a 'fresh' installation, and indeed, the problem is not in there. I'll have to find out which contrib module/theme brakes the functionality.
You can close this ticket. As soon as I find the root cause, I'll add it here for future reference.
Comment #4
Jochus CreditAttribution: Jochus commented(sorry, it seems I can close the ticket myself :-))
Comment #5
Anonymous (not verified) CreditAttribution: Anonymous commented@Jochus, no problem. Maybe you have some changes in the .htaccess file?
Comment #6
Jochus CreditAttribution: Jochus commentedOk, found how I can reproduce it on a fresh install. Make sure to execute the following steps:
Scenario:
* Set up clean Drupal 8 installatin
* Create content: Article
- make sure the article gets a an URL alias ($ALIAS)
* Access your article using the URL http://localhost/subfolder/index.php/$ALIAS
Problem:
call to statistics.php result in 404 NOT FOUND
Comment #7
Jochus CreditAttribution: Jochus commentedAnd your question will be: why do you add the "index.php"? Well, I have no idea, it happens automatically:
Scenario:
* Browse to http://www.jochenhebbrecht.be/site/
* Click 'an article' (doesn't matter which one)
* Check the URL
Problem:
index.php is automatically added in the URL (this problem does NOT occur when I'm logged in to the Drupal site)
Update:
After reading this page: https://www.drupal.org/node/256410, I've changed:
to
But the problem still occurs ...
Comment #8
Anonymous (not verified) CreditAttribution: Anonymous commentedGood research, @Jochus!
Unfortunately (or fortunately), I can not reproduce the appearance
index.php
via enable URL alias. Maybe this is problem of Apache config or php version. See also #2839167-3: index.php in pathes.But the problem with
'statistics.php'
path can be reproduced for the usual installation of Drupal (without a subfolder) too:/admin/config/system/statistics
)./index.php/node/add/article
and create Article node/admin/reports/dblog
and see the message'page not found'
.I'm update issue summary and title, but do feel free to correct it :)
This patch should fix problem for both cases (with/without subfolder installation), but i'm not sure that this is ideal solution.
Also I found an unpleasant bug. Saving node with 'index.php' in path (
/index.php/node/1/edit
) leads to the appearance of 'index.php' in all actions links (even if after go to the '/node/1' page). The reason for this is the caching of generated action links (you cann't reproduce this on dev regim with'cache.backend.null'
). But looks like this out of scope this issue.Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedComment #10
Jochus CreditAttribution: Jochus commentedHi vaplas,
Title has been modified correctly by your side! :-)!
With your patch, I'm unable to reproduce the problem.
Thank you!
Comment #11
alayham CreditAttribution: alayham at EGHNA commentedI am seeing this error a lot recently. Patch applied. testing for a few days.
Comment #12
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks @Jochus and @alayham for feedback!
A bit test coverage (test-only is interdiff with #8 patch).
Comment #14
BerdirI 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.
Comment #15
Anonymous (not verified) CreditAttribution: Anonymous commentedThank you for this tip, @Berdir!
Comment #16
timmillwoodLooks good.
I tested the test without the fix to statistics.module and it failed as expected.
Comment #17
tstoecklerSo
update_authorize_update_batch_finished()
contains the following to - if I am not mistaken - to achieve the same thing: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.Comment #18
Wim LeersThis is very very very wrong. This is not what
file_create_url()
orfile_url_transform_relative()
are designed for.Comment #19
Berdir> 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?
Comment #20
Wim LeersBut 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.
Comment #21
Anonymous (not verified) CreditAttribution: Anonymous commentedLooks like this unrouted url has problems due to
script
option (UnroutedUrlAssembler::addOptionDefaults()).Test only remains like #12.
Comment #22
dawehnerDid you tried using something like this:
?
Comment #23
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks, @dawehner! And it works fine with index.php (including a subfolder case) too. And this approach is not yet in our collection:
base_path()
file_url_transform_relative(file_create_url(...))
['script' => '']
\Drupal::request()->getBasePath()
Comment #24
dawehnerOh yeah, I should have actually listed the cases I tested:
Comment #26
websiteworkspace CreditAttribution: websiteworkspace commentedIs the bug described in this thread the source of the constant:
warning/errors in the recent log messages for nearly every URL on a site?
Comment #27
dawehnerFor me at least this is ready to go. We have test coverage and a working fix.
Comment #28
catchI'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?
Comment #29
dawehnerAt least for me having a absolute URL instead of a relative URL feels safer, but that's just me :)
Comment #30
websiteworkspace CreditAttribution: websiteworkspace commentedThis 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.
Comment #31
Anonymous (not verified) CreditAttribution: Anonymous commentedWe have two big problem for statistics module on page with 'index.php' prefix:
We have two additional issues, which can solve this problem by another way:
#23 contains a list of possible solutions for the get correct path to file. (All that were offered here, except #17, because it does not help to protect against the appearance of a
script
key, see #21).We have a white-box test for these solutions. Probably also a black-box test will be useful. Here it is with solution from dawehner (#22).
Comment #33
Anonymous (not verified) CreditAttribution: Anonymous commentedopps, incorrect namespace.
Comment #36
Wim LeersI just started looking at the
statistics
module (because I want to use it on my own site) and noticed that its JS is usingdrupalSettings.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.
Comment #37
Wim Leers/me curses https://www.drupal.org/node/2815083 for not running automatically
Here's an updated patch with the ES6 transpiled to ES5.
Comment #38
timmillwoodThanks Wim, looks good to me
Comment #39
BerdirNo, 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 :)
Comment #40
dawehnerI'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.
Comment #41
Anonymous (not verified) CreditAttribution: Anonymous commentedrestoration of tests from #33 + tests with language prefix.
Comment #43
BerdirThe 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.
Comment #44
Anonymous (not verified) CreditAttribution: Anonymous commented#43: Here are two versions for it:
testLoggingPage
andtestLoggingPage2
. It also seems that BTB::StatisticsLoggingTest is not needed anymore.Comment #45
Wim Leers#38: you're a bit trigger-happy with that RTBC — this still needed test coverage and actual manual testing.
#39:
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.Makes sense!
#40:
Because I started from scratch.
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.
Comment #46
timmillwood@Wim Leers - Re #38 I was so excited someone was actually working on Statistics.
Comment #47
Berdir> 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.
Comment #48
dawehnerNo, 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 :)
Nice test coverage!
That is some really nice test!
Nitpick: "Gets" instead of "Get"
Comment #49
Anonymous (not verified) CreditAttribution: Anonymous commented@dawehner thanks for review and kind words about tests!
This will make it possible to verify that we have not statistics block, not "0 view".
Comment #50
dawehnerThis looks really good for me now, but maybe @timmillwood as the statistics module maintainer should have another look.
Comment #51
timmillwoodLooks 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.
Comment #52
Wim Leers#46: 😃❤️
#48:
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!
Comment #53
Anonymous (not verified) CreditAttribution: Anonymous commented#51: Done :)
Comment #55
alexpottWhy is the block module being installed?
Comment #56
Anonymous (not verified) CreditAttribution: Anonymous commentedEagle eyes! Just unsuccessful copy-paste from Drupal\Tests\statistics\Functional\StatisticsLoggingTest
Comment #57
dawehnerThank you for fixing!
Comment #58
alexpottAdding credit to reviewers who've helped the patch progress and steered it towards a solution.
Comment #59
alexpottCommitted d85463b and pushed to 8.6.x. Thanks!
Setting to ptbp for cherry-pick to 8.5.x once 8.5.0 is out.
Comment #61
alexpottCommitted 684bdfa and pushed to 8.5.x. Thanks!