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.
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.
Comment | File | Size | Author |
---|---|---|---|
#36 | 1987824-system-php-36.patch | 10.21 KB | ParisLiakos |
#33 | 1987824-system-php-33.interdiff.txt | 1.27 KB | nick_schuch |
#33 | 1987824-system-php-33.patch | 10.56 KB | nick_schuch |
#30 | 1987824-system-php-30.interdiff.txt | 843 bytes | nick_schuch |
#30 | 1987824-system-php-30.patch | 10.5 KB | nick_schuch |
Comments
Comment #1
Crell CreditAttribution: Crell commentedFor 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)
Comment #2
nick_schuch CreditAttribution: nick_schuch commentedComment #3
nick_schuch CreditAttribution: nick_schuch commentedHere we go.
Comment #4
larowlanSystem -> System Info?
$output = ob_get_clean();
One less line++
this was 'status' not 'system' - not sure of reason for change?
RTBC other than that
Comment #5
nick_schuch CreditAttribution: nick_schuch commentedHere we go.
Thanks larowlan!
Comment #6
larowlanThis was right originally, the hook_menu() was wrong
Comment #7
nick_schuch CreditAttribution: nick_schuch commentedSorry, confusion on my end.
Comment #8
Crell CreditAttribution: Crell commentedThis 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.
Comment #9
nick_schuch CreditAttribution: nick_schuch commentedHere is the system status conversion as outlined in #1.
Comment #10
Crell CreditAttribution: Crell commentedThis 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.)
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.
Comment #12
vijaycs85Fixing 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?
Comment #14
Crell CreditAttribution: Crell commentedFor 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().
Comment #15
vijaycs85Thanks for your reply @Crell. Here is the patch...
Comment #17
nick_schuch CreditAttribution: nick_schuch commentedOk. 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.
Comment #18
nick_schuch CreditAttribution: nick_schuch commentedAlso 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.
Comment #19
Crell CreditAttribution: Crell commentedIt looks like we no longer need module_handler in the controller, since that's taken care of by system.manager.
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.
The MySQL-fixup code should be its own method, too, which is called from the controller, NOT from within the service.
Comment #20
nick_schuch CreditAttribution: nick_schuch commentedI believe this fixes the issues identified in #19.
Comment #21
Crell CreditAttribution: Crell commentedAll methods should use lowerCamel. So checkRequirements(), listRequirements(), etc.
drupal_requirements_severity() looks like it could move onto this service, too. (probably as getMaxSeverity()).
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.
Comment #22
nick_schuch CreditAttribution: nick_schuch commentedThanks 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?
Comment #23
Crell CreditAttribution: Crell commentedOh 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.
Comment #24
nick_schuch CreditAttribution: nick_schuch commentedThanks for the feedback. Here is the latest patch.
Comment #26
nick_schuch CreditAttribution: nick_schuch commentedLooks like it needs reroll. Will do so shortly.
Comment #27
kim.pepperComment #28
nick_schuch CreditAttribution: nick_schuch commentedJust needed an update in the system.routing.yml. Refer to #24 for the current interdiff.
Comment #29
Crell CreditAttribution: Crell commentedREQUIREMENT_OK should be moved to a const on this class. One less external dependency.
Comment #30
nick_schuch CreditAttribution: nick_schuch commentedI 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.
Comment #31
nick_schuch CreditAttribution: nick_schuch commentedRemoving reroll tag.
Comment #32
Crell CreditAttribution: Crell commentedThis needs to use static::REQUIREMENT_ERROR in order to reference the right constant.
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.
Comment #33
nick_schuch CreditAttribution: nick_schuch commentedFixed issues found in #32.
Comment #34
Crell CreditAttribution: Crell commentedWhew! I think we're finally done. Thanks for sticking with it, Nick! If the bot doesn't throw this back, this is RTBC.
Comment #35
Crell CreditAttribution: Crell commentedBumping 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.)
Comment #36
ParisLiakos CreditAttribution: ParisLiakos commentedreroll after manual cron running conversion
Comment #37
alexpottCommitted 520f1c5 and pushed to 8.x. Thanks!