[D7] Site Inventory

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxmralexho2113499git

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.

Automated Review:

################################ Coder Sniffer #################################

FILE: /home/jribeiro/Projects/git/2113499/site_inventory.pages.inc
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
  63 | WARNING | Only string literals should be passed to t() where possible
 256 | WARNING | Only string literals should be passed to t() where possible
--------------------------------------------------------------------------------

############################### DrupalPractice #################################

FILE: /home/jribeiro/Projects/git/2113499/site_inventory.module
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 7 | WARNING | @author tags are not usually used in Drupal, because over time
   |         | multiple contributors will touch the code anyway
 8 | WARNING | @author tags are not usually used in Drupal, because over time
   |         | multiple contributors will touch the code anyway
--------------------------------------------------------------------------------

Hi,

1.I have reviewed it by coder module .find the comments below

site_inventory.admin.inc
severity: criticalreview: security_fapi_titleLine 42: Potential problem: FAPI elements '#title' and '#description' only accept filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized. (Drupal Docs) [security_fapi_title]
'#description' => t('Enable this checkbox if blocks should be inventoried.') . $block_custom_dependencies,

2. During unit testing i have found some warnings due to invalid arguments in your form submit . image attached below.

The 'Select All' option in the General Settings page is not working as expected when jQuery Update module is installed.Select All is functioning only in the first click.

1. Update your git clone command in the description to include the project name:
git clone --branch 7.x-1.x http://git.drupal.org/sandbox/mralexho/2113499.git site_inventory

2. Not a huge deal but checkout using the README template.

3. Credits should take place in README.txt not in @file blocks.

4. Delete all your variables on hook_uinstall().

5. Not sure if you tested with a lot of nodes but when I ran on my site which has a decent amount of nodes I got the error "Maximum execution time of 30 seconds exceeded .....includes/database/database.inc on line 2171"

Hi mralexho,

I found this module's fundamental concept really interesting, because its currently reduced scope may be extended (as you stated in your TODO paragraph), adding new "targets" looked for, and looking for them not only in nodes.
In the other hand, it also would become dramatically more useful if those targets were customizable.

Here is my review, which regards only Coding style & Drupal API usage, since all other aspects have been already covered, with no remaining issue.
I installed the module, and my comments are all about how it achieves its goals.

Though it works fine technically speaking, I found some issues due to what I expected vs what I got: it comes partly from a lack of some information about what are currently the limits of the features, and partly from real lack of how these features are designed to work.
Below I focused on the first subject I really tested (types recognition), for which I wrote detailed explanation of my point of view.
In the other hand, I simply reported some basic considerations about the other subject of how fields and languages are managed.

1. (*) Email address: having inserted some email addresses in a node, I didn't get them reported in Site Inventory.
   I had to look at the code to realize that an email address is registered only when preceded by mailto: .
   Obviously you decided this is a feature (and this is not I'm pointing out, even if I find it rather restrictive), but it might be clear for the user.
   At least you should add somme explanatory text about it in front of "Email address" in admin/config/content/site_inventory.
2. (*) Even with the above information, users may face the case of email addresses not registered, because you're strictly testing for something like mailto: my.address@myisp.com ("mailto:" followed by 1 space), while it could be mailto:my.address@myisp.com or mailto: my.address@myisp.com, and so on.
3. BTW there are several points I don't understand in the code about email registration. Let me know if I missed something more subtle (site_inventory.module, lines 193-202):
   1. why a separate (and rather complicated) test to locate this mailto: , while it might be part of the regex (and at the same time solve point 2 above)?
   2. in the regex, why to look for [A-Za-z0-9.\+\-_~] as 1st character, rather than [\w.\+\-_~]+ as for the next ones (and I can't see the difference between "A-Za-z0-9" and "\w")?
   3. in the regex, why to capture the ISP domain name, which is not used furtherly?
   4. regarding the eternal debate about "what is a good regex for an email address" (someones even argue such a "good" regex can't exist!) you may be interested to look at this article, that I consider as a reference for a long time
4. (*) Phone number: your regex expects a strictly limited template for the phone numbers to be exposed, while there are plenty of ways to do it, even taking in account only the most frequent ones.
   For example, as French I didn't had any of my phone number registered, since they obey to different templates.
   I didn't take time to analyze it deeper but it notably seems that you don't look for any international number: AFAIK the regex should at least begin with something like (\+\d{2,3})?
   So here, again, minimum information should be available in front of "Phone numbers" in admin/config/content/site_inventory.
5. Even with this information, this remains of very restricted use. So it seems almost indispensable to give a way for administrators to enter a list of custom templates.
6. "Media" and "Documents": not so important than the previous cases, but of interest, should be a list of the considered file extensions.
7. About which contents are explored; again, I didn't seriously analyzed this, and here are only the points I suggest to pay attention to, in order to clearly determine which cases are currently covered or not (and BTW inform users about the current limitations):
   1. As you already stated it in your TODO, only nodes are currently explored,not blocks.
   2. In nodes, you only look for body contents: this lets other (user- or module-defined) text fields out of scope.
      NOTE: it may even happen that some modules don't have any body at all; this case would result in a PHP error in line 181 of site_inventory.module.
      Look at this page:
      $node->body: Array. Body content of node. Long text field with summary.
      Note: Don't assume that this field will exist, as it is possible to remove it via Manage Fields on each content type. Similarly, modules that define a custom node content type may not even attach a body in the first place.
   3. Even in a the standard body you only look at the value part, not at the summary one
   4. Still in the body you explore only the [0] item, not taking in account the other ones when "Number of values" is not 1 in the field definition
   5. Finally something I can't talk about with certainty is the language scope: I'm not familiar enough with the field structure at this point, but I see you only look at [LANGUAGE_NONE] part, so probably excluding what is not the site language

PLEASE NOTE: don't be offensed by what might appear like a flood of criticism!
The only 3 points flagged (*) are not a so great challenge to fix, and all the rest is merely a lot of suggested enhancements: it is the common way for any project to evolve, and now this is NOT a list of points to correct before this module appears acceptable.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

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