Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amitsedaiz’s picture

Status: Active » Needs review
FileSize
903 bytes

I have made changes to the documentation as suggested. Please have a look

jhodgdon’s picture

Status: Needs review » Needs work

Thanks 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:

  * @param $variables
  *   An associative array containing:
- *   - requirements: An array of requirements.
+ *   - requirements: Contains list of system status report as an associative
+ *   array containing following elements for each report: title, value, 
+ *   description and severity as return value
  *

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!

amitsedaiz’s picture

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

joachim’s picture

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

+++ b/core/modules/system/system.admin.inc
@@ -466,11 +466,20 @@ function theme_system_admin_index($variables) {
+ *    - title: The name of the requirement.
+ *    - value: The current value (e.g., version, time, level, etc).
+ *    - description: The description of the requirement/status.

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.

vijaycs85’s picture

+++ b/core/modules/system/system.admin.inc
@@ -466,11 +466,20 @@ function theme_system_admin_index($variables) {
+ * ¶
...
+ * ¶

There are some space at the end.

amitsedaiz’s picture

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

amitsedaiz’s picture

Assigned: Unassigned » amitsedaiz
Status: Needs work » Needs review
FileSize
1.21 KB

Attaching 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 :)

jhodgdon’s picture

Status: Needs review » Needs work

This is really close -- thanks! I think what you've written here is very clear.

A few things to fix:

a) List formatting:

+ *   - requirements: An array of requirements/status items. Each requirement
+ *   is an associative list containing the following elements:

The second line needs a 2 space more indentation.

b) List formatting:

+ *    - value(Optional): The current value (e.g., version, time, level, etc).

Should be

- value: (optional) ...

c) List formatting; Each subsequent level of list needs to be indented an additional 2 spaces.

d)

+ *     - REQUIREMENT_INFO: For info only.

Can this be just "REQUIREMENT_INFO: Status information." ? We should not use the word "info".

amitsedaiz’s picture

@jhodgdon: Please find the new patch based on your suggestions. Thanks.

jhodgdon’s picture

Please 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:

+ *     - value: (Optional) The current value (e.g. version, time, level, etc).

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!

amitsedaiz’s picture

Please find the updated patch attached.

jhodgdon’s picture

+ *   - requirements: An array of requirements/status items. Each requirement
+ *   is an associative array containing the following elements:

this 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!

amitsedaiz’s picture

Status: Needs work » Needs review
FileSize
654 bytes
1.22 KB

Thanks. As suggested, please find the new patch and interdiff.

Status: Needs review » Needs work

The last submitted patch, system.admin_.inc-2120459-13.patch, failed testing.

jhodgdon’s picture

Hm. Odd that a docs-only patch would trigger that many errors. ?!? I'll re-test.

jhodgdon’s picture

Status: Needs work » Needs review

#13: system.admin_.inc-2120459-13.patch queued for re-testing.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

Anyway, assuming we can get the bot to agree, this looks good. Thanks!

amitsedaiz’s picture

Thank you jhodgon. It was good learning :). Please feel free to direct me to any documentation or novice code related work. Happy to help. Thanks.

jhodgdon’s picture

We 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

jhodgdon’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed 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?

amitsedaiz’s picture

Issue summary: View changes
Status: Patch (to be ported) » Needs review
FileSize
1.56 KB

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

amitsedaiz’s picture

My Bad. Changing REQUIREMENT_INFO to REQUIREMENT_OK for hard-coded value 0.

jhodgdon’s picture

Status: Needs review » Needs work

The 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?

joachim’s picture

> Changing REQUIREMENT_INFO to REQUIREMENT_OK for hard-coded value 0.

I've already filed a separate issue for that.

joachim’s picture

I can't edit my own comment at the moment... issue is #2120461: theme_status_report() uses an integer instead of a constant.

amitsedaiz’s picture

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

amitsedaiz’s picture

Status: Needs work » Needs review
jhodgdon’s picture

Status: Needs review » Needs work

I 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:

    if (empty($requirement['#type'])) {
 

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.

joachim’s picture

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

vijaycs85’s picture

Assigned: amitsedaiz » Unassigned

Unassigning, so that someone can pick this up.

Himanshu5050’s picture

Hi, added description on comments.

Himanshu5050’s picture

FileSize
765 bytes
jhodgdon’s picture

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

jhodgdon’s picture

  • jhodgdon committed e98a74f on 8.3.x
    Issue #2120459 by amitsedaiz, vijaycs85, joachim: Fix up docs for...

  • jhodgdon committed e98a74f on 8.3.x
    Issue #2120459 by amitsedaiz, vijaycs85, joachim: Fix up docs for...
stefan.r’s picture

Issue tags: +Dublin2016

Marking as a Novice task for taking #26 and addressing the issues identified in the following comments

  • jhodgdon committed e98a74f on 8.4.x
    Issue #2120459 by amitsedaiz, vijaycs85, joachim: Fix up docs for...

  • jhodgdon committed e98a74f on 8.4.x
    Issue #2120459 by amitsedaiz, vijaycs85, joachim: Fix up docs for...
shashikant_chauhan’s picture

Cleaning tag, we are working this in Drupal mumbai code sprint.

mahesh.gupta’s picture

Assigned: Unassigned » mahesh.gupta

I am working on it

mahesh.gupta’s picture

Status: Needs work » Needs review
FileSize
1.2 KB

Patch for drupal 7

Yago Elias’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
43.27 KB

Patch #42 looks good for me.

I compared to the hook_requirements documentations.. text looks good.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Fixed

Committed 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 :)

  • David_Rothstein committed 09f1141 on 7.x
    Issue #2120459 by amitsedaiz, mahesh.gupta, Yago Elias, jhodgdon,...

Status: Fixed » Closed (fixed)

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