Problem/Motivation

There's no link to phpinfo at admin/reports/status but the route still works

Introduced in #2388255: (followup) Limit PDO MySQL to executing single statements if PHP supports it because 'php' key is used to show this info

Proposed resolution

provide a link at report page

Remaining tasks

decide and fix

User interface changes

tbd

API changes

no

CommentFileSizeAuthor
#14 d8-status-report-phpinfo-2464605-14.patch3.65 KBdom.
#7 interdiff-4-7.txt843 byteswillzyx
#7 d8-status-report-phpinfo-2464605-7.patch3.74 KBwillzyx
#4 d8-status-report-phpinfo-2464605-4.patch3.84 KBwillzyx
#4 d8-status-report-phpinfo-2464605-4-test-only.patch1.72 KBwillzyx
#2 d8-status-report-phpinfo-2464605-2.patch2.12 KBwillzyx
status-report.png30.25 KBandypost

Comments

dawehner’s picture

dawehner
Primary language German
commented
Issue tags: +Needs tests

Ups :(

willzyx’s picture

willzyx commented
Status: Active » Needs review
StatusFileSize
new d8-status-report-phpinfo-2464605-2.patch2.12 KB

I'm not sure how to write tests that cover all possible cases

dom.’s picture

dom. commented

Hi!
I'm okay with your patch, although I'm not sure of this lign regarding Drupal standards :
$phpversion = $phpversion_label = phpversion();
RTBC+1 otherwise.

Regarding tests, I can make a WebBaseTest that would check :
- URL admin/reports/php is accessible as admin
- URL admin/reports/php is not accessible as standard user.
- URL admin/reports shows a link called "more information"
- This link points to admin/reports/php.
Would it be enough ?

willzyx’s picture

willzyx commented
StatusFileSize
new d8-status-report-phpinfo-2464605-4-test-only.patch1.72 KB
new d8-status-report-phpinfo-2464605-4.patch3.84 KB

The last submitted patch, 4: d8-status-report-phpinfo-2464605-4-test-only.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue tags: -Needs tests +Documentation

Great! test just need to be cleaned up.
There's docs to disable function https://www.drupal.org/node/243993

And https://www.drupal.org/node/59680 should be updated about this case

  1. +++ b/core/modules/system/src/Tests/System/StatusTest.php
@@ -37,4 +39,38 @@ public function testStatusPage() {
     $this->assertResponse(200, 'The status page is reachable.');
...
+  public function testPhpInfoPage() {
...
+    $this->assertResponse(200, 'The phpinfo page is reachable.');
...
+  public function testPhpRequirements() {
+    $this->drupalGet('admin/reports/status');

    There's no reason to split this into different test methods, because each test* method causes fresh install of core for testing. 

  2. +++ b/core/modules/system/src/Tests/System/StatusTest.php
@@ -37,4 +39,38 @@ public function testStatusPage() {
+    if (function_exists('phpinfo')) {
+      $this->assertLinkByHref(Url::fromRoute('system.php')->toString());
...
+    else {
+      $this->assertNoLinkByHref(Url::fromRoute('system.php')->toString());

    then try to visit status/php and check responce

willzyx’s picture

willzyx commented
StatusFileSize
new d8-status-report-phpinfo-2464605-7.patch3.74 KB
new interdiff-4-7.txt843 bytes

changes as per #6

willzyx’s picture

willzyx commented

We should also check if phpinfo() exists in SystemInfoController::php() since admin/reports/status/php can be reached directly. Opened #2465467: SystemInfoController::php() should check if function phpinfo() exists

dom.’s picture

dom. commented

- patch is okay regarding Drupal standards
- test manually that patch works and tests properly when phpinfo is activated and when desactivated.

However, I would suggested to add this (just haggling but still) :
- should we add a test that the admin/reports/status/php page is not accessible when unlogged as admin ? I'm not sure if it's interesting because basically it tests the permission key of the .routing.yml file which is not the purpose here !

+    $this->assertResponse(200, 'The phpinfo page is reachable.');
+
+    $this->drupalLogout();
+    $this->drupalGet('admin/reports/status/php');
+    $this->assertResponse(403, 'The phpinfo page is not reachable for non admin users.');

Also I have created a follow up issue regarding the display of admin/reports/status/php when phpinfo is disabled:
#2465473: Page admin/reports/status/php display blank when phpinfo is disabled

willzyx’s picture

willzyx commented

I think #2465473: Page admin/reports/status/php display blank when phpinfo is disabled is a duplicate of #2465467: SystemInfoController::php() should check if function phpinfo() exists (in which I have already provided a patch).

dom.’s picture

dom. commented
Status: Needs review » Reviewed & tested by the community

Sorry you wrote your comment #8 while I was doing mine and did not see your issue (no keyword I was looking for gave me results !). I will review it also. Thus this one is RTBC.

dawehner’s picture

dawehner
Primary language German
commented
Component: base system » system.module

Moving to system module

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/system/src/Tests/System/StatusTest.php
@@ -37,4 +39,33 @@ public function testStatusPage() {
+  public function testPhpRequirements() {

Let's just add this into testStatusPage since it is part of testing that page. Multiple test methods in WebTestBase require a re-install of Drupal and in this case nothing is gained by doing that.

dom.’s picture

dom. commented
Status: Needs work » Needs review
StatusFileSize
new d8-status-report-phpinfo-2464605-14.patch3.65 KB

Just rewrited patch for comment #13.

willzyx’s picture

willzyx commented
Status: Needs review » Reviewed & tested by the community

RTBC + 1

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 6435e2e and pushed to 8.0.x. Thanks!

  • alexpott committed 6435e2e on 8.0.x 
    Issue #2464605 by willzyx, Dom.: Regression: no link to admin/reports/...

Status: Fixed » Closed (fixed)

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