Problem/Motivation

Some server administrators may choose to disable the PHP function phpinfo() for security reasons (see https://www.drupal.org/node/243993). SystemInfoController::php() does not verify that phpinfo exists before using it. Potentially this could create php fatal error in systems where phpinfo is disabled

Proposed resolution

Add a check in SystemInfoController::php()to ensure that phpinfo() exists.

Remaining tasks

create patch.

User interface changes

none.

API changes

none.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

willzyx’s picture

Dom.’s picture

Hi !
- patch review for Drupal standard
- patch manually applied and checked
Therefore, patch #1 is RTBC+1 !

However I would suggest the following, thus not applying the RTBC tag. If you don't agree with the following, please apply the RTBC tag since #1 is ok :
- add "visit" in the last sentence since otherwise it is a fragment (looks weird that there are no verb, but I'm not native english so I'm probably wrong.)
For more information, visit <a href="@phpinfo">Enabling and disabling phpinfo()</a> handbook page.
- remove the empty lign before return statement.

andypost’s picture

Issue tags: +Documentation
+++ b/core/modules/system/src/Controller/SystemInfoController.php
@@ -61,9 +61,14 @@ public function status() {
+      $output = t('The phpinfo() function has been disabled for security reasons. To see your server\'s phpinfo() information, change your PHP settings or contact your server administrator. For more information, visit <a href="@phpinfo">Enabling and disabling phpinfo()</a> handbook page.', array('@phpinfo' => 'http://drupal.org/node/243993'));

Not sure that the text makes sense, you suggest to revert security setting on server to enable phpinfo() that disabled for reason.
The link to d.o page makes a lot of sense, so I think text should be like.
Suppose The phpinfo() function has been disabled for security reasons. For more info....

willzyx’s picture

@Dom.
- it makes sense
- on coding standards there is no reference of the standard to follow for this thing. it's just a matter of preference. For me the change you made is fine

@andypost
It's the same sentence used in the status report page (see system_requirements()); we suggest to revert setting on server to enable phpinfo() IF you want to see your server's phpinfo() information.
However, the requested change makes sense

Dom.’s picture

Status: Needs review » Reviewed & tested by the community

RTBC+1

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Huh. Had no idea this was an option. You learn something new every day. :)

Can't envision a useful way to automated test this, so...

Committed and pushed to 8.0.x. Thanks!

  • webchick committed 3e58aba on 8.0.x
    Issue #2465467 by willzyx, Dom., andypost: SystemInfoController::php()...

Status: Fixed » Closed (fixed)

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