Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Crell’s picture

For this issue, the right approach would be to use ob_start(), ob_get_contents(), and ob_end_clean() (http://php.net/manual/en/function.ob-start.php) around phpinfo(), then stick the output of that into a Response object and return it directly. Should be fairly simple.

The class should be called SystemInfoController, so that we can also stick system_status() in here as well. (cf #1987830: Convert system_status() to a new style controller)

nick_schuch’s picture

Assigned: Unassigned » nick_schuch
nick_schuch’s picture

Status: Active » Needs review
FileSize
2.59 KB

Here we go.

larowlan’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/lib/Drupal/system/Controller/SystemInfoController.phpundefined
@@ -0,0 +1,31 @@
+ * Returns responses for System routes.

System -> System Info?

+++ b/core/modules/system/lib/Drupal/system/Controller/SystemInfoController.phpundefined
@@ -0,0 +1,31 @@
+    $output = ob_get_contents();
+    ob_end_clean();

$output = ob_get_clean();
One less line++

+++ b/core/modules/system/system.moduleundefined
@@ -1006,12 +1006,9 @@ function system_menu() {
+  $items['admin/reports/system/php'] = array(

this was 'status' not 'system' - not sure of reason for change?

RTBC other than that

nick_schuch’s picture

Status: Needs work » Needs review
FileSize
2.57 KB
1.26 KB

Here we go.

Thanks larowlan!

larowlan’s picture

+++ b/core/modules/system/system.routing.ymlundefined
@@ -75,7 +75,7 @@ system_site_maintenance_mode:
-  pattern: '/admin/reports/status/php'
+  pattern: '/admin/reports/system/php'

This was right originally, the hook_menu() was wrong

nick_schuch’s picture

Sorry, confusion on my end.

Crell’s picture

This looks good to me visually. As long as we're here, let's go ahead and merge in some of the other "info" controllers, as noted in #1. It's probably faster to get them all committed at once.

nick_schuch’s picture

Here is the system status conversion as outlined in #1.

Crell’s picture

+++ b/core/modules/system/lib/Drupal/system/Controller/SystemInfoController.php
@@ -0,0 +1,88 @@
+    usort($requirements, '_system_sort_requirements');

This is a perfect case to get rid of _system_sort_requirements() and replace it with an inlined anonymous function, I think. :-) (Unless it's called from a lot of other places, which I doubt.)

+++ b/core/modules/system/lib/Drupal/system/Controller/SystemInfoController.php
@@ -0,0 +1,88 @@
+    // MySQL import might have set the uid of the anonymous user to autoincrement
+    // value. Let's try fixing it. See http://drupal.org/node/204411
+    db_update('users')
+      ->expression('uid', 'uid - uid')
+      ->condition('name', '')
+      ->condition('pass', '')
+      ->condition('status', 0)
+      ->execute();
+    return theme('status_report', array('requirements' => $requirements));

This should be an injected DB connection.

Also? Holy crap when did we add that fix! That's awesome!

Interdiffs should be named .txt so that testbot doesn't bother with them.

Status: Needs review » Needs work

The last submitted patch, 1987824-9-system-php.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
2.02 KB
6.98 KB

Fixing review comments in #10

1. This is a perfect case to get rid of _system_sort_requirements() and replace it with an inlined anonymous function, I think. :-) (Unless it's called from a lot of other places, which I doubt.) - replaced and it is just one place use.

2. This should be an injected DB connection. - Not sure what need to be done for this?

Status: Needs review » Needs work

The last submitted patch, 1987824-system-php-12.patch, failed testing.

Crell’s picture

For point 2: Do you see how moduleHandler() is being injected using create() and __construct()? Do the same thing for the "database" service. Then replace db_delete() with $this->database->delete().

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
2.07 KB
7.35 KB

Thanks for your reply @Crell. Here is the patch...

Status: Needs review » Needs work

The last submitted patch, 1987824-system-php-15.patch, failed testing.

nick_schuch’s picture

Status: Needs work » Needs review
FileSize
9.61 KB
9.14 KB

Ok. So we had to make system_status into a service so it could still be used in "system_admin_config_page". I have added a @todo for this to use injection once we are converting it to a controller in here #1987810.

nick_schuch’s picture

Also noticed the hook_menu entry added a "PHP" item to the Reports page. This was not there before. This can be left as just a route.

Crell’s picture

+++ b/core/modules/system/lib/Drupal/system/Controller/SystemInfoController.php
@@ -0,0 +1,82 @@
+   */
+  public static function create(ContainerInterface $container) {
+    return new static(
+      $container->get('system.manager'),
+      $container->get('module_handler')
+    );
+  }

It looks like we no longer need module_handler in the controller, since that's taken care of by system.manager.

+++ b/core/modules/system/lib/Drupal/system/SystemManager.php
@@ -0,0 +1,77 @@
+  /**
+   * Displays the site status report. Can also be used as a pure check.
+   *
+   * @return bool|string
+   */

The double use here is a code smell. "Check status" and "Show me a status report" should be separate public methods that call the same internal protected methods for the places where they overlap. They shouldn't be 2 operations mixed together with a boolean.

+++ b/core/modules/system/lib/Drupal/system/SystemManager.php
@@ -0,0 +1,77 @@
+    // MySQL import might have set the uid of the anonymous user to autoincrement
+    // value. Let's try fixing it. See http://drupal.org/node/204411
+    $this->database->update('users')
+      ->expression('uid', 'uid - uid')
+      ->condition('name', '')
+      ->condition('pass', '')
+      ->condition('status', 0)

The MySQL-fixup code should be its own method, too, which is called from the controller, NOT from within the service.

nick_schuch’s picture

I believe this fixes the issues identified in #19.

Crell’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/lib/Drupal/system/SystemManager.php
@@ -0,0 +1,90 @@
+  public function check_requirements() {

All methods should use lowerCamel. So checkRequirements(), listRequirements(), etc.

+++ b/core/modules/system/lib/Drupal/system/SystemManager.php
@@ -0,0 +1,90 @@
+    return drupal_requirements_severity($requirements) == REQUIREMENT_ERROR;

drupal_requirements_severity() looks like it could move onto this service, too. (probably as getMaxSeverity()).

+++ b/core/modules/system/system.admin.inc
@@ -19,7 +19,8 @@
 function system_admin_config_page() {
   // Check for status report errors.
-  if (system_status(TRUE) && user_access('administer site configuration')) {
+  // @todo Use depedancy injection in http://drupal.org/node/1987810.
+  if (Drupal::service('system.manager')->check_requirements() && user_access('administer site configuration')) {

I'm half tempted to say we should go ahead and convert this too, but let's get this patch in and then this is the next step. We can probably move this callback to the same controller class.

nick_schuch’s picture

Thanks for reviewing Crell! One question.

drupal_requirements_severity() is also defined in "core/includes/install.core.inc" and "core/update.php". How do I go about converting those?

Crell’s picture

Oh yuck... The current way it's setup only works by chance, then. :-)

Let's take the easy route. Stick the method in place, but leave the function where it is for the procedural code to still use. We can get rid of the function another time. It's not a long function so a little code duplication is OK.

nick_schuch’s picture

Status: Needs work » Needs review
FileSize
10.17 KB
3.75 KB

Thanks for the feedback. Here is the latest patch.

Status: Needs review » Needs work

The last submitted patch, 1987824-system-php-24.patch, failed testing.

nick_schuch’s picture

Looks like it needs reroll. Will do so shortly.

kim.pepper’s picture

Issue tags: +Needs reroll
nick_schuch’s picture

FileSize
10.16 KB

Just needed an update in the system.routing.yml. Refer to #24 for the current interdiff.

Crell’s picture

Status: Needs work » Needs review
+++ b/core/modules/system/lib/Drupal/system/SystemManager.php
@@ -87,4 +87,24 @@ public function fix_anonymous_uid() {
+    $severity = REQUIREMENT_OK;

REQUIREMENT_OK should be moved to a const on this class. One less external dependency.

nick_schuch’s picture

I have added the constants (I see us using these as we migrate more functionality):

- REQUIREMENT_OK
- REQUIREMENT_WARNING
- REQUIREMENT_ERROR

Unfortunately we cannot take these out of the install.inc yet as functions like drupal_requirements_severity() use these.

nick_schuch’s picture

Issue tags: -Needs reroll

Removing reroll tag.

Crell’s picture

Status: Needs review » Needs work
+++ b/core/modules/system/lib/Drupal/system/SystemManager.php
@@ -0,0 +1,125 @@
+    return $this->getMaxSeverity($requirements) == REQUIREMENT_ERROR;

This needs to use static::REQUIREMENT_ERROR in order to reference the right constant.

+++ b/core/modules/system/lib/Drupal/system/SystemManager.php
@@ -0,0 +1,125 @@
+  /**
+   * MySQL import might have set the uid of the anonymous user to autoincrement
+   * value. Let's try fixing it. See http://drupal.org/node/204411
+   */

Comment needs a one-line summary. I suggest "Fixes anonymous user on MySQL." Then what's there now can be just the longdesc after it.

nick_schuch’s picture

Status: Needs work » Needs review
FileSize
10.56 KB
1.27 KB

Fixed issues found in #32.

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Whew! I think we're finally done. Thanks for sticking with it, Nick! If the bot doesn't throw this back, this is RTBC.

Crell’s picture

Priority: Normal » Major

Bumping to major as this conflicts with and therefore blocks #1623114: Remove drupal_exit(). (That one can be easily rerolled and RTBCed as soon as this is in.)

ParisLiakos’s picture

FileSize
10.21 KB

reroll after manual cron running conversion

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 520f1c5 and pushed to 8.x. Thanks!

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