This module complements the core Status report page by providing an additional, more in-depth and technically oriented status report, designed to help technical support identify issues with the site. The generated report has some features:

  • It is plain text, so it's easy to inspect and copy-and-paste.
  • It is in fact a valid Markdown Extra document -- so developers and support tech staff can transform it to nicely formatted HTML or PDFs files, if desired.
  • It can be downloaded for off-site reviews and/or to be sent out in attachments or uploaded elsewhere.
  • It anonymizes or masks irrelevant and perhaps sensitive data. See the module page for more information.
  • It is extensible -- modules can provide hooks that augment the report in a much more flexible way than via hook_requirements().

Project page: https://www.drupal.org/sandbox/flaviovs/2426407

GIT clone command: git clone --branch 7.x-1.x http://git.drupal.org/sandbox/flaviovs/2426407.git advstatus

p.s.: There is an issue with the http://pareview.sh review of this module that I didn't manage to workaround. The module implements some hooks on behalf of core nodes that are currently enabled. Obviously, those interception hooks have the original module name as a prefix, so although the warning is valid, it is not useful in this case. BTW, the mechanism used is similar to the one used by some other modules (Rules comes to mind) to implement custom hooks on behalf of the core modules.

Manual reviews of other projects

CommentFileSizeAuthor
#9 coder-results.txt3.19 KBklausi
#6 8031891.png37.51 KBwebcodin
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

PA robot’s picture

PA robot CreditAttribution: PA robot commented

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

kala4ek’s picture

kala4ek
Primary language Russian
Location 🇷🇺 Novosibirsk
CreditAttribution: kala4ek at DrupalJedi commented

advstatus.module

  • advstatus.module:14 - There is no needed initialize of array;
  • advstatus.module:45,45 - doubled empty line;
  • advstatus.module:69,70 - the same.

advstatus.pages.inc

  • advstatus.pages.inc:8,9 Doubled empty line;
  • advstatus.pages.inc:19 there is no needed to set title, you already did it (in hook_menu());
  • advstatus.pages.inc:35,36 doubled empty line;
  • advstatus.pages.inc:35,36 the same.
InviteReferrals’s picture

InviteReferrals CreditAttribution: InviteReferrals commented
Status: Needs review » Needs work
Issue tags: +Fix the automated issues first

https://www.drupal.org/node/318#naming

./modules/node.inc:
all functions should be prefixed with your module/theme name to avoid name clashes

function node_advstatus_report($anonymizer) {
function node_advstatus_report_init($report) {
./modules/dblog.inc: all functions should be prefixed with your module/theme name to avoid name clashes.

function dblog_advstatus_report($anonymizer) {
./modules/comment.inc: all functions should be prefixed with your module/theme name to avoid name clashes.

function comment_advstatus_report($anonymizer) {
function comment_advstatus_report_init($report) {
./modules/path.inc: all functions should be prefixed with your module/theme name to avoid name clashes.
function path_advstatus_report($anonymizer) {

flaviovs’s picture

flaviovs CreditAttribution: flaviovs commented
Status: Needs work » Needs review

@kalak,

advstatus.module:14 - There is no needed initialize of array;

Whilst there's no error or warning message when you do that, it's generally considered a bad practice do add elements to an undefined array. The PHP documentation is strong about this:

$arr[key] = value;

(...)
If $arr doesn't exist yet, it will be created, so this is also an alternative way to create an array. This practice is however discouraged(...)

There's also a good discussion about this on Stackoverflow.

advstatus.module:45,45 - doubled empty line;
advstatus.module:69,70 - the same.
advstatus.pages.inc:8,9 Doubled empty line;
advstatus.pages.inc:35,36 doubled empty line;
advstatus.pages.inc:35,36 the same.

Can you point out where is said that you cannot have two blank lines between functions?

All functions must have a doc block, and they must be separated at least by a blank line. That's all I could find about it.

advstatus.pages.inc:19 there is no needed to set title, you already did it (in hook_menu());

Good catch. Fixed.

@InviteReferrals,

https://www.drupal.org/node/318#naming

./modules/node.inc:
all functions should be prefixed with your module/theme name to avoid name clashes
(...)

Please, read the original post:

The module implements some hooks on behalf of core nodes that are currently enabled. Obviously, those interception hooks have the original module name as a prefix, so although the warning is valid, it is not useful in this case. BTW, the mechanism used is similar to the one used by some other modules (Rules comes to mind) to implement custom hooks on behalf of the core modules.

flaviovs’s picture

flaviovs CreditAttribution: flaviovs commented
Issue summary: View changes
Issue tags: -Fix the automated issues first +PAreview: review bonus
webcodin’s picture

webcodin CreditAttribution: webcodin commented
FileSize
8031891.png37.51 KB

Handle review:
In the report maybe part of names of entities, Nodes and Comment displays wrong see attached 8031891.png

flaviovs’s picture

flaviovs CreditAttribution: flaviovs commented

It is expected behavior.

Initial post on this issue:

It anonymizes or masks irrelevant and perhaps sensitive data. See the module page for more information.

Module page (https://www.drupal.org/sandbox/flaviovs/2426407):

Albeit being very detailed, considerate effort was made to protect sensitive information you may care about your site. For example, content type names, database user names etc., are all "anonymized" — i.e. they are replaced with generic identifier, which will still allow technical people to understand the inners aspects of your site and relate them when needed, without the need to publicize the exact names of the anonymized identifier.

zbombicz’s picture

zbombicz CreditAttribution: zbombicz commented

Automated Review

pareview.sh: only the mentioned function naming issues.

I think, it is wrong method to implement hook functions in your module behalf of other modules (node, comment, etc.). With this solution you exclude the possibility for these modules to implement your hooks in future. I know, that these are core modules, and probably any of these modules will not implement your hooks as own, but I have concerns in general with implementing hooks behalf of other modules. I think the proper solution could be not to implement these reports as hook functions, but creating of internal function for these (for example: advstatus_report_default_node), and put all of these function calls into advstatus_report (with verification the existence of these modules). Sorry for my bad english, If I was not fully understood, please tell me than.

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.
No duplication
Yes: Does not cause module duplication and/or fragmentation.
Master Branch
Yes: Follows the guidelines for master branch.
Licensing
Yes: Follows the licensing requirements.
3rd party assets/code
Yes: Follows the guidelines for 3rd party assets/code.
README.txt/README.md
Yes: Follows the README Template.
Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.
Secure code
Yes: Meets the security requirements.
Coding style & Drupal API usage
  1. Some function's PHPDoc comment doesn't contaions a @return tag.
  2. Some function arguments are not documented (advstatus_mrange, advstatus_anon_file, ...).
  3. I think, it is unnecessary to define variables, if it is not used more than one times:
    - function advstatus_support_report_page - $my_path

    - function advstatus_anon_file - $droot

    - ...
  4. Typos:
    - advstatus.pages.inc: 233. obect

    - advstatus.pages.inc: 341. formated

    - advstatus.pages.inc: 657. curent

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

Personal notes: This module is very handful. I would really like feature, that make "anonymization" optional (on/off). In some cases I would really like to see statistics about my nodes, node types, etc., with real names, and exact data. I agree, that protecting sensitive information is very important, but for self use I really want if it was available too.

Anyway: Hands up, very useful module, I hope it will be contrib as soon as possible.

This review uses the Project Application Review Template.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Assigned: Unassigned » mpdonadio
Status: Needs review » Reviewed & tested by the community
Issue tags: -PAreview: review bonus
FileSize
coder-results.txt3.19 KB

Review of the 7.x-1.x branch (commit ee9f2f4):

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

  • I think "Advanced status" is a bad name because it does not tell me what this is about. Better could be "Status report markdown", "Text status report" or "Plain status report". The "Advanced" just has no meaning, which makes this functionality harder to find.

But otherwise looks good to me. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Assigning to mpdonadio as he might have time to take a final look at this.

flaviovs’s picture

flaviovs CreditAttribution: flaviovs commented
Title: [D7] Advanced status » [D7] Advanced status report

@zbombicz,

I think, it is wrong method to implement hook functions in your module behalf of other modules (node, comment, etc.). With this solution you exclude the possibility for these modules to implement your hooks in future. I know, that these are core modules, and probably any of these modules will not implement your hooks as own, but I have concerns in general with implementing hooks behalf of other modules. I think the proper solution could be not to implement these reports as hook functions, but creating of internal function for these (for example: advstatus_report_default_node), and put all of these function calls into advstatus_report (with verification the existence of these modules). Sorry for my bad english, If I was not fully understood, please tell me than.

It is not wrong if you really meant to implement the hook on behalf of a core module. There are a bunch of modules that do that, mainly because this is the most elegant way to code this, IMHO . I mentioned Rules, but I think they did this only on Rules 1.x -- Rules 2.x use another scheme. However, another big player that do this is Views.

I think that general sense is that it's very unlikely -- not to say wrong -- to core modules implement every other hook that pops up on contrib. And if they do it someday, it is just a matter of copying the hook from the origin module. (In my module, I took care of testing if the core module did not define the hook -- no matter how unlikely it sounds -- and include() the proxy only if not. That way, we never risk to break a site with double-defined functions errors. Not sure if Views do that, though.)

- Some function's PHPDoc comment doesn't contaions a @return tag.
- Some function arguments are not documented (advstatus_mrange, advstatus_anon_file, ...).

This module will still undergo an alpha phase before an stable release. When in alpha, the API may still change, so I think this is not the best time to document API parameters and return values yet.

I think, it is unnecessary to define variables, if it is not used more than one times:
- function advstatus_support_report_page - $my_path
- function advstatus_anon_file - $droot

I also think so. But in this case, those variables used to be used more that once, and I choose to left then alone when the other parts of the code were removed. Why, you may ask... Just common sense. I have a strong feeling that they may be used again if another unanticipated user cases pops in. Also, removing the variables will means to put function inside string concatenation, inside other function calls... This usually mean we will move beyond the dreaded 80 column, and it is going to sacrifice code readability a lot. This alone is a very strong reason to keep the variable to me. See Line length and wrapping in the Coding standards page for a similar example where "unnecessary" variables are used to improve readability.

Typos:
- advstatus.pages.inc: 233. obect
- advstatus.pages.inc: 341. formated
- advstatus.pages.inc: 657. curent

Oops... I wonder why Codespell did not catch this here.

Fixed. Thank you!

@klausi,

Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.

Which version of CodeSniffer are you using? I am on 1.5.6 and I do not get these warnings here (but got and fixed a lot of similar "Closing parenthesis not on a new line" previously.)

Fixed, anyway.

I think "Advanced status" is a bad name because it does not tell me what this is about. Better could be "Status report markdown", "Text status report" or "Plain status report". The "Advanced" just has no meaning, which makes this functionality harder to find.

Mea culpa, you are absolutely right. The actual name of the module is "Advanced status report". That's how it calls itself all over the UI. This was changed some time ago and I just forgot to update the module page (and this issue). I am doing this right now.

I also committed a fix some comments and help text using the old name. Thanks.

flaviovs’s picture

flaviovs CreditAttribution: flaviovs commented

BTW, I reverted the commit with the drupal_set_title() fix for the issue reported in #2. The reason is that without it, the page never get the right title set on hook_menu(). It looks like this is caused by a bug in core (see #891892: Page/Overlay title doesn't change when MENU_LOCAL_ACTION is used).

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented

Automated Review

Review of the 7.x-1.x branch (commit 9c396df):

  • ./modules/comment.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See https://www.drupal.org/node/318#naming
    function comment_advstatus_report($anonymizer) {
function comment_advstatus_report_init($report) {
  • ./modules/dblog.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See https://www.drupal.org/node/318#naming
    function dblog_advstatus_report($anonymizer) {
  • ./modules/node.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See https://www.drupal.org/node/318#naming
    function node_advstatus_report($anonymizer) {
function node_advstatus_report_init($report) {
  • ./modules/path.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See https://www.drupal.org/node/318#naming
    function path_advstatus_report($anonymizer) {
  • Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
    
FILE: /Users/mattd/Dropbox/Drupal/par/pareview_temp/modules/node.inc
---------------------------------------------------------------------------
FOUND 1 ERROR AFFECTING 1 LINE
---------------------------------------------------------------------------
 38 | ERROR | [x] Closing parenthesis of array declaration must be on a
    |       |     new line
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------

Time: 877ms; Memory: 17Mb
  • No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

Manual Review

@flaviovs, klausi and the other review admins use the bleeding edge review tools from: https://github.com/klausi/pareviewsh

Having a subdirectory called modules that doesn't contain any Drupal modules is kinda weird and goes against conventions.

(+) advstatus_permission should have the 'restrict access' and possible the 'warning' keys set. Your node_advstatus_report() calls
db_select() agains the {node} table w/o the node_access tag, so whoever gets this permission will get access to all nodes. I am not
deeming this a security problem, as this is a reporting tool for admins, and no real information would be revealed anyway (I don't
consiuder node counts sensitive). There may also be things in the logs, and you aren't checking the access log permission in your dblog_advstatus_report().

Yeah, fix the namespace of the module.

Not seeing any blocking issues.

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented
Assigned: mpdonadio » Unassigned
Status: Reviewed & tested by the community » Fixed

Thanks for your contribution, flaviovs!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well.

flaviovs’s picture

flaviovs CreditAttribution: flaviovs commented

Thanks for the upgrade and for your review!

BTW:

Coder Sniffer has found some issues with your code (please check the Drupal coding standards).
(...)
@flaviovs, klausi and the other review admins use the bleeding edge review tools from: https://github.com/klausi/pareviewsh

Strangely enough, I'm also using the pareview.sh script from the above URL (commit 2907f4be). But I did not get the above Code Sniffer issues.

Code Sniffer version mismatch maybe? What your phpcs --version says?

(Mine says "PHP_CodeSniffer version 1.5.6 (stable) by Squiz (http://www.squiz.net)")

Having a subdirectory called modules that doesn't contain any Drupal modules is kinda weird and goes against conventions.

This is exactly the same pattern used by Views, Rules and Entity API (at least)!

(Not that I do not think it's weird -- I do. But I really thought that that was the convention...)

(+) advstatus_permission should have the 'restrict access' and possible the 'warning' keys set.

Nice suggestion. I will open a feature request for this.

Your node_advstatus_report() calls db_select() agains the {node} table w/o the node_access tag, so whoever gets this permission will get access to all nodes. I am not
deeming this a security problem, as this is a reporting tool for admins, and no real information would be revealed anyway (I don't
consiuder node counts sensitive).

I think that the warnings in the permission you suggested take care of this. As an admin/developer tool, this module should provide a accurate picture of the Drupal installation -- that's why I went directly to the db_select() route.

But giving a notice on the report about node_access is a great suggestion. I will also open an issue for this.

There may also be things in the logs, and you aren't checking the access log permission in your dblog_advstatus_report().

The dblog report only list the last PHP errors (currently). Yep, they may contain information that some user may not be able to see, but I again think that the permission warning does suffice. OTOH, I recognize that it really makes sense to omit (with a warning) the dblog report if the user does not have the appropriate permission. Well... I will open an issue to discuss this question latter. Maybe with some more feedback from users we will be able make a better decision about this.

Yeah, fix the namespace of the module.

I will look into this.

mqanneh’s picture

mqanneh
he/him
Primary language English
Location Amman
CreditAttribution: mqanneh at Vardot commented
Status: Fixed » Needs work

pareview.sh report

Review of the 7.x-1.x branch (commit 1436c6e):

  • ./modules/node.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See https://www.drupal.org/node/318#naming
    function node_advstatus_report($anonymizer) {
function node_advstatus_report_init($report) {
  • ./modules/dblog.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See https://www.drupal.org/node/318#naming
    function dblog_advstatus_report($anonymizer) {
  • ./modules/comment.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See https://www.drupal.org/node/318#naming
    function comment_advstatus_report($anonymizer) {
function comment_advstatus_report_init($report) {
  • ./modules/path.inc: all functions should be prefixed with your module/theme name to avoid name clashes. See https://www.drupal.org/node/318#naming
    function path_advstatus_report($anonymizer) {
  • Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
  • No automated test cases were found, did you consider writing Simpletests or PHPUnit tests? This is not a requirement but encouraged for professional software development.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.


FILE: /var/www/drupal-7-pareview/pareview_temp/modules/node.inc
---------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
---------------------------------------------------------------------------
 38 | WARNING | [x] A comma should follow the last multiline array item.
    |         |     Found: )
---------------------------------------------------------------------------
PHPCBF CAN FIX THE 1 MARKED SNIFF VIOLATIONS AUTOMATICALLY
---------------------------------------------------------------------------


FILE: /var/www/drupal-7-pareview/pareview_temp/advstatus.inc
--------------------------------------------------------------------------
FOUND 2 ERRORS AFFECTING 2 LINES
--------------------------------------------------------------------------
 199 | ERROR | [x] Line indented incorrectly; expected 22 spaces, found 2
 202 | ERROR | [x] Line indented incorrectly; expected 22 spaces, found 2
--------------------------------------------------------------------------
PHPCBF CAN FIX THE 2 MARKED SNIFF VIOLATIONS AUTOMATICALLY
--------------------------------------------------------------------------

Time: 464ms; Memory: 17Mb

Source: http://pareview.sh/ - PAReview.sh online service

mpdonadio’s picture

mpdonadio
he/him
Primary language English
Location Philadelphia/PA/USA (UTC-5)
CreditAttribution: mpdonadio commented
Status: Needs work » Fixed

This application was already approved by two admins. No need for further review. In addition, the output of the automated tools is not an automatic reason for pushing an application back to needs work. In the report you posted, the namespace prefix problems don't apply as they are hook implementations for other modules, and therefore wouldn't get the namespace. The rest is fairly minor as far as code standard go.

Status: Fixed » Closed (fixed)

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