API page: https://api.drupal.org/api/drupal/modules%21system%21system.admin.inc/fu...
> requirements: An array of requirements.
Gee, I wonder what those could be!
Tentatively tagging as novice.
You'd need to look first at https://api.drupal.org/api/drupal/modules!system!system.admin.inc/functi..., which is probably the only caller of this theme function, and then look at what the https://api.drupal.org/api/drupal/modules!system!system.api.php/function... implementations return, and work from there.
Comment | File | Size | Author |
---|---|---|---|
#43 | Screenshot from 2017-08-21 16-43-32.png | 43.27 KB | Yago Elias |
#42 | theme_status_report-2120459-42.patch | 1.2 KB | mahesh.gupta |
#32 | system.admin_._inc-2120459-31.patch | 765 bytes | Himanshu5050 |
#26 | interdiff-2120459-21-26.txt | 653 bytes | amitsedaiz |
#26 | system.admin_.inc-2120459-26.patch | 1.03 KB | amitsedaiz |
Comments
Comment #1
amitsedaiz CreditAttribution: amitsedaiz commentedI have made changes to the documentation as suggested. Please have a look
Comment #2
jhodgdonThanks for the patch!
I am not sure that this is accurate for Drupal 8 as to when it is called. I'm only seeing two uses instead of 3... I also think it's not a good idea on this theme function to document when it is called -- this kind of documentation is difficult to maintain, because someone can add a theme() call and not go back here and update the documentation. So let's just take it out. We don't normally document where theme hooks are used on the theme function/template documentation anyway.
The other part of the patch:
This needs a bit of work:
- It needs to be made a bit more grammatical. Actually, I think the line that was there before "An array of requirements" was a good way to begin the documentation. Then you could continue with something like "Each requirement is an associative array with the following elements:".
- Needs to end in a .
- Needs to follow list formatting guidelines (indentation): http://drupal.org/node/1354#lists
- The elements in the associative array should also be formatted as a list, and each element can then be given some documentation about what it means and what its values can be (especially important for severity).
Thanks!
Comment #3
amitsedaiz CreditAttribution: amitsedaiz commentedThanks jhodgdon. Yes, mentioning when does it get called would be difficult to maintain.
Based on your suggestion , I have updated the patch. Please have a look.
I did a check once again, it gets called in all the 3 phases: During install, update and when viewing status report page.
Comment #4
joachim CreditAttribution: joachim commentedWe should also mention -- sorry should have said earlier -- that this theme function is dependent on install.inc being loaded, because that's where the constants are defined.
If you try to make use of this in your own code (as I did) you get errors until you load the include.
I think this should maybe take a step back from calling them requirements. That's a bit of an abstract notion. Also, the theme function's name is about 'status report', not requirements. Each item is a status item. It has two, optionally 3 pieces:
- the first column, which is the title
- the second column which is the actual status
- and an optional 2nd row which can have longer description.
Comment #5
vijaycs85There are some space at the end.
Comment #6
amitsedaiz CreditAttribution: amitsedaiz commentedI suppose the the variable name should change from $requirements to $status_items(or something better) for the documentation to make better sense.
However, during install phase for some status items like PHP version, file system etc can act like both status report as well as requirement(if they are not satisfied by the server system).
Also apart from title, value, description and severity, weight is also is also available as optional.
I am getting a bit confused on how should I take this forward in documentation change.
Comment #7
amitsedaiz CreditAttribution: amitsedaiz commentedAttaching a new patch with minor changes.
The value part of the associative is also optional, as suggested by code
// hook_requirements does not require value to be set. Set to null if not:
if (isset($requirement['value'])) {
$value = $requirement['value'];
}
Please do let me know if any other changes are required :)
Comment #8
jhodgdonThis is really close -- thanks! I think what you've written here is very clear.
A few things to fix:
a) List formatting:
The second line needs a 2 space more indentation.
b) List formatting:
Should be
c) List formatting; Each subsequent level of list needs to be indented an additional 2 spaces.
d)
Can this be just "REQUIREMENT_INFO: Status information." ? We should not use the word "info".
Comment #9
amitsedaiz CreditAttribution: amitsedaiz commented@jhodgdon: Please find the new patch based on your suggestions. Thanks.
Comment #10
jhodgdonPlease read https://drupal.org/node/1354#lists and follow it carefully. Still not quite right (indentation; capitalization of "optional").
Also in REQUIREMENT_INFO I don't think Information should be capitalized.
Also can you use the term "associative array" instead of "associative list" in your documentation?
I'm also a bit concerned about:
What does this mean? I am not sure what "version, time, level, etc." means. Value of what? Also, please say "for example" instead of "e.g.". "e.g." is usually misused and mispunctuated (as it is here). Or better yet, since you have "etc.", just leave out "e.g.".
Thanks!
Comment #11
amitsedaiz CreditAttribution: amitsedaiz commentedPlease find the updated patch attached.
Comment #12
jhodgdonthis second line still needs 2 more spaces of indentation.
Other than that, I think this is OK. Thanks!
Also, when you upload new patches, please make an interdiff
https://drupal.org/documentation/git/interdiff
and set the status to Needs Review. Thanks!
Comment #13
amitsedaiz CreditAttribution: amitsedaiz commentedThanks. As suggested, please find the new patch and interdiff.
Comment #15
jhodgdonHm. Odd that a docs-only patch would trigger that many errors. ?!? I'll re-test.
Comment #16
jhodgdon#13: system.admin_.inc-2120459-13.patch queued for re-testing.
Comment #17
jhodgdonAnyway, assuming we can get the bot to agree, this looks good. Thanks!
Comment #18
amitsedaiz CreditAttribution: amitsedaiz commentedThank you jhodgon. It was good learning :). Please feel free to direct me to any documentation or novice code related work. Happy to help. Thanks.
Comment #19
jhodgdonWe have a "Novice" tag for issues in Drupal Core. If you search for issues with that tag (from Advanced Search), you can find more to work on. Also, you may be interested in the Core Mentoring program. Links:
https://drupal.org/project/issues/search/drupal [after the D7 upgrade today, this may be a different link in a few hours?]
https://drupal.org/core-mentoring
Comment #20
jhodgdonCommitted to 8.x. Thanks again!
It looks like the 7.x function is slightly different and it may have slightly different variables, so I think we need to port this patch rather than committing it as it is to D7. Can someone take a look please?
Comment #21
amitsedaiz CreditAttribution: amitsedaiz commentedThere are few differences though. This function gets called during install and when viewing Status Report and not during update phase. Secondly, both title and value elements in the associative array are mandatory.
I have also made one small change to modify Numeric 0(hard coded) to REQUIREMENT_INFO.
Comment #22
amitsedaiz CreditAttribution: amitsedaiz commentedMy Bad. Changing REQUIREMENT_INFO to REQUIREMENT_OK for hard-coded value 0.
Comment #23
jhodgdonThe patch in #22 is probably OK, but if you make this code change (even though it looks correct), it makes this patch harder to get committed.
We also lost the documentation from the D8 patch that said you needed to make sure install.inc was being loaded. Was that intentional?
One other comment: looking at the D7 code, there is reference to $requirement['#type'], which is not documented here. It looks like if it's set, the whole requirement is not output. Maybe a look at the calling code would illuminate this so it could be documented?
Comment #24
joachim CreditAttribution: joachim commented> Changing REQUIREMENT_INFO to REQUIREMENT_OK for hard-coded value 0.
I've already filed a separate issue for that.
Comment #25
joachim CreditAttribution: joachim commentedI can't edit my own comment at the moment... issue is #2120461: theme_status_report() uses an integer instead of a constant.
Comment #26
amitsedaiz CreditAttribution: amitsedaiz commentedI tried to find for #type in requirements array but could not find any. The calling code in system.admin.inc is system_status($check = FALSE), which in turn calls module_invoke_all('requirements', 'runtime'). I had a look at system_requirements($phase) in system.install but there was no mention of '#type' for requirements.
For install.inc to be loaded was derived from #4 by joachim. However, I didnt test it myself.
Attached is the new patch without the code change.
Comment #27
amitsedaiz CreditAttribution: amitsedaiz commentedComment #28
jhodgdonI will try to be clearer this time. :)
a) The function theme_status_report() in D7 (same as in D8) uses a bunch of constants like REQUIREMENT_INFO. Those constants are defined in the includes/install.inc file, and the d8 patch from above mentioned this. I think the d7 patch should too.
b) In the code for this function in D7 I am seeing:
So it is being used for something in the code -- it looks like if it's not empty, the requirement is not printed. I agree that I don't see anywhere in D7 that is passing this in, but it is there in the code and should probably be documented what it would be used for if it was passed in. Or else, for fun, we could try removing it from the code since it is totally undocumented (there is not even a comment) and see what happens. :) Just kidding on that... in D7 this is not a good idea at this point.
Comment #29
joachim CreditAttribution: joachim commented> if (empty($requirement['#type'])) {
Wow, that's weird. It's completely skipped if that is set! I've had a quick look through the implementations of hook_requirements() and the theme calls to this in core, and I can't find any use of #type in any of them!
Comment #30
vijaycs85Unassigning, so that someone can pick this up.
Comment #31
Himanshu5050 CreditAttribution: Himanshu5050 commentedHi, added description on comments.
Comment #32
Himanshu5050 CreditAttribution: Himanshu5050 commentedComment #33
jhodgdonPlease go back to the patch in #26 and start from there, fixing the problems identified in the following comments. Your new patch is missing all the work that has already gone into this.
Comment #34
jhodgdonComment #37
stefan.r CreditAttribution: stefan.r commentedMarking as a Novice task for taking #26 and addressing the issues identified in the following comments
Comment #40
shashikant_chauhan CreditAttribution: shashikant_chauhan as a volunteer and at Iksula commentedCleaning tag, we are working this in Drupal mumbai code sprint.
Comment #41
mahesh.gupta CreditAttribution: mahesh.gupta at Iksula commentedI am working on it
Comment #42
mahesh.gupta CreditAttribution: mahesh.gupta at Iksula commentedPatch for drupal 7
Comment #43
Yago Elias CreditAttribution: Yago Elias as a volunteer and at CI&T commentedPatch #42 looks good for me.
I compared to the hook_requirements documentations.. text looks good.
Comment #44
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedCommitted to 7.x - thanks!
As for the whole
empty($requirement['#type'])
thing, that is indeed weird... it looks like it's been there for a decade or more and is probably dead code. So our best bet at this point is probably to not remove it, but also to not encourage its use - and therefore I think not documenting it is OK :)