Part of #1971384: [META] Convert page callbacks to controllers

For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.

Files: 
CommentFileSizeAuthor
#36 1987824-system-php-36.patch10.21 KBParisLiakos
PASSED: [[SimpleTest]]: [MySQL] 55,427 pass(es).
[ View ]
#33 1987824-system-php-33.interdiff.txt1.27 KBnick_schuch
#33 1987824-system-php-33.patch10.56 KBnick_schuch
PASSED: [[SimpleTest]]: [MySQL] 55,807 pass(es).
[ View ]
#30 1987824-system-php-30.interdiff.txt843 bytesnick_schuch
#30 1987824-system-php-30.patch10.5 KBnick_schuch
PASSED: [[SimpleTest]]: [MySQL] 55,800 pass(es).
[ View ]
#28 1987824-system-php-27.patch10.16 KBnick_schuch
PASSED: [[SimpleTest]]: [MySQL] 55,773 pass(es).
[ View ]
#24 1987824-system-php-24.interdiff.txt3.75 KBnick_schuch
#24 1987824-system-php-24.patch10.17 KBnick_schuch
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1987824-system-php-24.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#20 1987824-system-php-20.interdiff.txt4.89 KBnick_schuch
#20 1987824-system-php-20.patch9.64 KBnick_schuch
PASSED: [[SimpleTest]]: [MySQL] 55,829 pass(es).
[ View ]
#18 1987824-system-php-18.interdiff.txt494 bytesnick_schuch
#18 1987824-system-php-18.patch9.67 KBnick_schuch
PASSED: [[SimpleTest]]: [MySQL] 55,980 pass(es).
[ View ]
#17 1987824-system-php-17.interdiff.txt9.14 KBnick_schuch
#17 1987824-system-php-17.patch9.61 KBnick_schuch
PASSED: [[SimpleTest]]: [MySQL] 55,873 pass(es).
[ View ]
#15 1987824-system-php-15.patch7.35 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 55,668 pass(es), 71 fail(s), and 18 exception(s).
[ View ]
#15 1987824-diff-12-15.txt2.07 KBvijaycs85
#12 1987824-system-php-12.patch6.98 KBvijaycs85
FAILED: [[SimpleTest]]: [MySQL] 55,884 pass(es), 71 fail(s), and 18 exception(s).
[ View ]
#12 1987824-diff-9-12.txt2.02 KBvijaycs85
#9 1987824-9-system-php.interdiff.patch5.09 KBnick_schuch
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1987824-9-system-php.interdiff.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#9 1987824-9-system-php.patch6.16 KBnick_schuch
FAILED: [[SimpleTest]]: [MySQL] 55,746 pass(es), 71 fail(s), and 18 exception(s).
[ View ]
#7 1987824-7-system-php.interdiff.txt976 bytesnick_schuch
#7 1987824-7-system-php.patch2.46 KBnick_schuch
PASSED: [[SimpleTest]]: [MySQL] 56,468 pass(es).
[ View ]
#5 1987824-5-system-php.interdiff.txt1.26 KBnick_schuch
#5 1987824-5-system-php.patch2.57 KBnick_schuch
PASSED: [[SimpleTest]]: [MySQL] 56,917 pass(es).
[ View ]
#3 1987824-1-system-php.patch2.59 KBnick_schuch
PASSED: [[SimpleTest]]: [MySQL] 56,069 pass(es).
[ View ]

Comments

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)

Assigned:Unassigned» nick_schuch

Status:Active» Needs review
StatusFileSize
new2.59 KB
PASSED: [[SimpleTest]]: [MySQL] 56,069 pass(es).
[ View ]

Here we go.

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();

<?php
$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

Status:Needs work» Needs review
StatusFileSize
new2.57 KB
PASSED: [[SimpleTest]]: [MySQL] 56,917 pass(es).
[ View ]
new1.26 KB

Here we go.

Thanks larowlan!

+++ 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

StatusFileSize
new2.46 KB
PASSED: [[SimpleTest]]: [MySQL] 56,468 pass(es).
[ View ]
new976 bytes

Sorry, confusion on my end.

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.

StatusFileSize
new6.16 KB
FAILED: [[SimpleTest]]: [MySQL] 55,746 pass(es), 71 fail(s), and 18 exception(s).
[ View ]
new5.09 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1987824-9-system-php.interdiff.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

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

+++ 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.

Status:Needs work» Needs review
StatusFileSize
new2.02 KB
new6.98 KB
FAILED: [[SimpleTest]]: [MySQL] 55,884 pass(es), 71 fail(s), and 18 exception(s).
[ View ]

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.

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().

Status:Needs work» Needs review
StatusFileSize
new2.07 KB
new7.35 KB
FAILED: [[SimpleTest]]: [MySQL] 55,668 pass(es), 71 fail(s), and 18 exception(s).
[ View ]

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.

Status:Needs work» Needs review
StatusFileSize
new9.61 KB
PASSED: [[SimpleTest]]: [MySQL] 55,873 pass(es).
[ View ]
new9.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.

StatusFileSize
new9.67 KB
PASSED: [[SimpleTest]]: [MySQL] 55,980 pass(es).
[ View ]
new494 bytes

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.

+++ 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.

StatusFileSize
new9.64 KB
PASSED: [[SimpleTest]]: [MySQL] 55,829 pass(es).
[ View ]
new4.89 KB

I believe this fixes the issues identified in #19.

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.

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?

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.

Status:Needs work» Needs review
StatusFileSize
new10.17 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1987824-system-php-24.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new3.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.

Looks like it needs reroll. Will do so shortly.

Issue tags:+Needs reroll

StatusFileSize
new10.16 KB
PASSED: [[SimpleTest]]: [MySQL] 55,773 pass(es).
[ View ]

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

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.

StatusFileSize
new10.5 KB
PASSED: [[SimpleTest]]: [MySQL] 55,800 pass(es).
[ View ]
new843 bytes

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.

Issue tags:-Needs reroll

Removing reroll tag.

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.

Status:Needs work» Needs review
StatusFileSize
new10.56 KB
PASSED: [[SimpleTest]]: [MySQL] 55,807 pass(es).
[ View ]
new1.27 KB

Fixed issues found in #32.

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.

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.)

StatusFileSize
new10.21 KB
PASSED: [[SimpleTest]]: [MySQL] 55,427 pass(es).
[ View ]

reroll after manual cron running conversion

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.