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.
https://api.drupal.org/api/drupal/core%21modules%21system%21src%21Contro...
The callback returns render array not a string as stated in documentation. I would also change description of the return value to something more meaningful.
Comment | File | Size | Author |
---|---|---|---|
#14 | 2611044-14.patch | 775 bytes | Nikhilesh Gupta |
#12 | 2611044-12.patch | 777 bytes | theMusician |
#9 | 2611044-9.patch | 499 bytes | felribeiro |
Comments
Comment #2
felribeiro CreditAttribution: felribeiro at CI&T commentedComment #3
felribeiro CreditAttribution: felribeiro at CI&T commentedComment #4
cilefen CreditAttribution: cilefen commentedNice work. I noticed a few things.
https://www.drupal.org/coding-standards/docs
The comment should explain what is in the array, notably that it is a render array. Look in core for examples, but something like "A render array representing the current status of the Drupal installation." may work.
Comment #5
felribeiro CreditAttribution: felribeiro at CI&T commentedThank you for your tips.
Comment #6
felribeiro CreditAttribution: felribeiro at CI&T commentedComment #7
cilefen CreditAttribution: cilefen commentedDocumentation improvements are rc eligible.
From the issue summary:
I do not agree with that. If more information about the requirements array that is passed to the template is needed, that could go on listRequirements(), which is called by this function. But #5 explains what the function is.
Comment #8
xjmThanks, that's better!
Let's improve this a bit more. The render array does not exactly represent the current status of the Drupal installation (that's pretty broad). I'd suggest something like:
Comment #9
felribeiro CreditAttribution: felribeiro at CI&T commentedComment #10
felribeiro CreditAttribution: felribeiro at CI&T commentedComment #11
cilefen CreditAttribution: cilefen commentedIsn't it also "...and whether this installation meets the requirements."?
Comment #12
theMusician CreditAttribution: theMusician as a volunteer and at Western Washington University commentedI agree with cilefen about the array output being able to appear on the system requirements install page. If you look at /core/includes/unicode.inc, line 16 outputs the error message one would see if they didn't have an appropriate library at the time of install. The attached patch includes his suggested language.
Comment #14
Nikhilesh Gupta CreditAttribution: Nikhilesh Gupta as a volunteer and at Melity commentedComment #15
johan.gant CreditAttribution: johan.gant at Torchbox commentedHave reviewed the patch from #14 against 8.2.x, looks good to me. Setting status to RTBC.
Comment #17
catchCommitted/pushed to 8.2.x, thanks!
Comment #20
xjmBackported to 8.1.x as well. @catch fixed an indentation error in the patch on commit.