Easy Breadcrumb

Does this module add functionality the below module does not?

http://drupal.org/project/custom_breadcrumbs

Please check this code review. You never indicated your version of Drupal

http://ventral.org/pareview/httpgitdrupalorgsandboxsonemonu1395492git

Extended documentation

Git access for anyone requesting it: git clone --branch 7.x-1.x http://git.drupal.org/sandbox/sonemonu/1395492.git

Hi sonemonu, there seems to be quite a few different problems that have been flagged with Ventral. I would recommend going through each one, line by line and trying to fix them up as best you can.

A couple of issues I faced when using your module.

• Firstly, after installing, my drupal 7 test environment died and I got the following error: [Sun Mar 04 13:42:05 2012] [error] [client 127.0.0.1] PHP Fatal error: require_once(): Failed opening required 'EasyBreadcrumb_Constants.inc' (include_path='.:/usr/share/php:/lib/zend/library') in /home/sam/Sites/drupal7/sites/default/modules/1395492/easy_breadcrumb.module on line 10, referer: http://drupal7.sam/?q=admin%2Fmodules&render=overlay
• Your module adds a top level configuration item. "EasyBreadcrumb form settings" appears on the top level next to configuration. You should probably make your config location admin/config/easy-breadcrumb to keep things neat.
• I wasn't able to get the block to appear in the blocks menu for some reason. Not sure what the issue here is.

I'll see how you go with these and come back at a later date and check it out again.

For sites that have URLs that strictly follow a defined menu structure, this could be a great module.

Hi sonemonu,

There are still errors and warnings that appears on automated review: href="http://ventral.org/pareview/httpgitdrupalorgsandboxsonemonu1395492git-7x-1x">PAReview

Automated review

Review of the 7.x-1.x branch:

• Bad line endings were found, always use unix style terminators. See http://drupal.org/coding-standards#indenting
  

  ./easy_breadcrumb.info:        ASCII English text, with CRLF line terminators
  ./README.txt:                  ASCII English text, with very long lines, with CRLF line terminators
  easy_breadcrumb_admin.inc
  easy_breadcrumb_blocks.inc
  EasyBreadcrumbConstants.inc
  easy_breadcrumb.info
  easy_breadcrumb.install
  easy_breadcrumb.module
  README.txt
  tpl/easy_breadcrumb.tpl.php
  

• Run coder to check your style, some issues were found (please check the Drupal coding standards). See attachment.
• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). See attachment.

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. Get a review bonus and we will come back to your application sooner.

Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards

sites/all/modules/pareview_temp/./test_candidate/easy_breadcrumb_admin.inc:
 +97: [critical] Potential problem: drupal_set_message() only accepts filtered text, be sure to use check_plain(), filter_xss() or similar to ensure your $variable is fully sanitized.

Status Messages:
 Coder found 5 projects, 5 files, 1 critical warnings, 0 warnings were flagged to be ignored

FILE: ...es/all/modules/pareview_temp/test_candidate/EasyBreadcrumbConstants.inc
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 1 | ERROR | End of line character is invalid; expected "\n" but found "\r"
--------------------------------------------------------------------------------


FILE: ...al-7-pareview/sites/all/modules/pareview_temp/test_candidate/README.txt
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 1 | WARNING | Line exceeds 80 characters; contains 146 characters
 3 | WARNING | Line exceeds 80 characters; contains 477 characters
 3 | ERROR   | Files must end in a single new line character
--------------------------------------------------------------------------------


FILE: ...iew/sites/all/modules/pareview_temp/test_candidate/easy_breadcrumb.info
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 6 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------


FILE: .../sites/all/modules/pareview_temp/test_candidate/easy_breadcrumb.install
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
  1 | ERROR | End of line character is invalid; expected "\n" but found "\r\n"
 14 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------


FILE: ...w/sites/all/modules/pareview_temp/test_candidate/easy_breadcrumb.module
--------------------------------------------------------------------------------
FOUND 13 ERROR(S) AND 4 WARNING(S) AFFECTING 17 LINE(S)
--------------------------------------------------------------------------------
   1 | ERROR   | End of line character is invalid; expected "\n" but found
     |         | "\r\n"
   9 | WARNING | Line exceeds 80 characters; contains 116 characters
  10 | WARNING | Line exceeds 80 characters; contains 136 characters
  11 | WARNING | Line exceeds 80 characters; contains 156 characters
  19 | ERROR   | Whitespace found at end of line
  22 | ERROR   | Whitespace found at end of line
  35 | ERROR   | Whitespace found at end of line
  38 | ERROR   | Whitespace found at end of line
  65 | ERROR   | Whitespace found at end of line
  68 | ERROR   | Whitespace found at end of line
  79 | WARNING | A comma should follow the last multiline array item. Found:
     |         | $path
  86 | ERROR   | Whitespace found at end of line
  89 | ERROR   | Whitespace found at end of line
 102 | ERROR   | Whitespace found at end of line
 103 | ERROR   | Last parameter comment requires a blank newline after it
 107 | ERROR   | Whitespace found at end of line
 119 | ERROR   | Files must end in a single new line character
--------------------------------------------------------------------------------


FILE: ...ites/all/modules/pareview_temp/test_candidate/easy_breadcrumb_admin.inc
--------------------------------------------------------------------------------
FOUND 12 ERROR(S) AND 1 WARNING(S) AFFECTING 13 LINE(S)
--------------------------------------------------------------------------------
  1 | ERROR   | End of line character is invalid; expected "\n" but found
    |         | "\r\n"
 10 | ERROR   | Whitespace found at end of line
 11 | ERROR   | Last parameter comment requires a blank newline after it
 15 | ERROR   | Whitespace found at end of line
 23 | ERROR   | Whitespace found at end of line
 26 | ERROR   | Whitespace found at end of line
 32 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
    |         | question marks
 43 | ERROR   | TRUE, FALSE and NULL must be uppercase; expected "TRUE" but
    |         | found "true"
 68 | WARNING | A comma should follow the last multiline array item. Found: )
 79 | ERROR   | Whitespace found at end of line
 82 | ERROR   | Last parameter comment requires a blank newline after it
 84 | ERROR   | Whitespace found at end of line
 98 | ERROR   | Files must end in a single new line character
--------------------------------------------------------------------------------


FILE: ...tes/all/modules/pareview_temp/test_candidate/easy_breadcrumb_blocks.inc
--------------------------------------------------------------------------------
FOUND 11 ERROR(S) AND 3 WARNING(S) AFFECTING 13 LINE(S)
--------------------------------------------------------------------------------
  1 | ERROR   | End of line character is invalid; expected "\n" but found "\r"
 10 | ERROR   | Whitespace found at end of line
 13 | ERROR   | Whitespace found at end of line
 23 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
    |         | question marks
 49 | ERROR   | Inline comments must start with a capital letter
 49 | ERROR   | Inline comments must end in  full-stops, exclamation marks, or
    |         | question marks
 64 | WARNING | A comma should follow the last multiline array item. Found:
    |         | $separator
 71 | WARNING | Line exceeds 80 characters; contains 95 characters
 73 | ERROR   | Whitespace found at end of line
 76 | ERROR   | Last parameter comment requires a blank newline after it
 77 | WARNING | Line exceeds 80 characters; contains 88 characters
 80 | ERROR   | Whitespace found at end of line
 87 | ERROR   | BREAK statements must be followed by a single blank line
 90 | ERROR   | BREAK statements must be followed by a single blank line
--------------------------------------------------------------------------------


FILE: ...es/all/modules/pareview_temp/test_candidate/tpl/easy_breadcrumb.tpl.php
--------------------------------------------------------------------------------
FOUND 6 ERROR(S) AFFECTING 5 LINE(S)
--------------------------------------------------------------------------------
  1 | ERROR | End of line character is invalid; expected "\n" but found "\r"
  4 | ERROR | Whitespace found at end of line
 13 | ERROR | The control statement should use the ":" alternative syntax
    |       | instead of curly braces in template files
 18 | ERROR | The control statement should use the ":" alternative syntax
    |       | instead of curly braces in template files
 18 | ERROR | There should be no white space after an opening "{"
 25 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

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

Manual Review

Structure: Maybe it could be cleaner to put all ".inc" files into an "includes" directory.

.module: You should remove @version 1.0 - 2012-01-05. Version should be .info and when your project will get into full project, d.o will add these information automatically.

Hi,

If you're working on netbeans for Windows, you can add this plugin which will allow you to convert line endings or use dos2unix in a unix workstation

Also, in my mind, you should completly review your .tpl.

You should use preprocess on your module and only display the result on your tpl.
Also you should consider to use "print" instead of "echo"

Hi,

sorry for the delay!

as there are currently many applications in queue we need more reviewers,
so think about getting a review bonus and we will come back to your application sooner.

• there is not tooo much logic in your tpl, but doing count($breadcrumb); as preprocessing would be good
• print is 4µs slower then echo.. not much, so please use print as it is used in most other tpl's too (just for consistency)

•       'description' => t('Perform administration tasks for the EasyBreadCrumb
          module.'),

  the 80 characters thing, is only for comments, in cases like above there is no sense of doing a linebreak

•  * Implementation of hook menu
  

  should be

   * Implements hook_menu().
  

• you should check your general spelling, eg .module, line 6: "tipically"
• you got only one tpl file so there is no need for putting it into its own directory, then you can use the 'file' key instead of the 'path' key in
• generally you should think about using a theme function instead of a tpl-file. (this would be a real performance enhancement)
• you use DRUPAL_CACHE_GLOBAL for your breadcrumb block, that means that your block is on every page the same and should be cached globally.. It's a breadcrumb block so it's probably not the same on every page, use DRUPAL_CACHE_PER_PAGE instead
• Drupal 7 has class/interface lazyloading, if you tell drupal which files contain classes/interfaces , you don't have to include them yourself. to make drupal recognize these files you should add them as file parameter to your .info
• All comments should end with .,? or !
• Functions containing a lot of custom code and no typical hook_ stuff should contain much more inline comments so other developers (or yourself in some years) can understand what your doing and why your doing things.
• Generally I'm not sure if it is really necessary to use an interface just to define some constants, it makes things more complicated then they could be (IMHO). btw, feel free to define your own constants with define('EASY_BREADCRUMB_VARNAME', 'VALUE')

Okay, so lets get those issues managed and I'll continue! :)

You can raise the issue priority up again after 2 weeks of no reviews.

You can raise the issue priority up again after 2 weeks of no reviews.

switching back without comment is a little strange ?

Hi,

Made a quick look and your admin page is not available.
Looks like you have an error in admin.inc at line 18.

function _easy_breadcrumb_settings_page($op = NULL) {
  return drupal_get_form('easy_breadcrumb_general_settings_form');
}

I thin it should be

function _easy_breadcrumb_settings_page($op = NULL) {
  return drupal_get_form('_easy_breadcrumb_general_settings_form');
}

• You should add into your README.txt information about configuration and block to activate.
• You should add a direct link to "admin/config/easy-breadcrumb" into Configuration page.
• If you can, when you add link into each URL items, you should check if the URL really exists. Eg: I have a content type which have rewrite path as mysite/test/mycontent bu mysite/test page doesn't exist.
• Also, you should add an option to disable drupal default breadcrumb in front if it is set.

Change status to need work.

I believe that @jygastaud is talking about adding a configure link to your .info file. See http://drupal.org/node/542202.

In your .info file add the following:

configure = admin/config/easy-breadcrumb

Adding options

Hi,

What I suggest is to add a link which can be saw directly in admin/config page as you can see into the file attach.

To do that, I only transformed your menu link from admin/config/easy-breadcrumb to admin/config/user-interface/easy-breadcrumb into .info and .module.
Maybe it's not the better package to add this, you should look about other module to see where they add it.

I will try to make an in-deep review soon.
But, as previously mentionned by patrickd at comment #16, you really should consider to made some reviews of other projects to get the review bonus tag and have your project review sooner by official reviewers.
I do that for mine and it's working very well.

Corrected mixed option

add module git path

There still seems to be quite a few errors in coding standards (see http://ventral.org/pareview/httpgitdrupalorgsandboxsonemonu1395492git).

Also, in the .install file you have used a SQL query to remove variables used by the module on uninstall. Have you considered using variable_del()?

just minor coding style isses
using variable_del() should not be a requirement

-> no reason for needs work

Hi patrickd, maybe I was a little too harsh changing the status for these findings, but after installing and playing with the module this was all I could really find. If these minor coding standards issues were fixed, not only would this be a nice little module, but also one that is fully compliant.

Hi,

For me module is really close to RTBC once small issues previously mentioned will be solved.

I added an issue #1548896: URL items to clarify one point I previously mentioned and you made something different but it is not a reason to no have RTBC.

For me it's RTBC.

Only one minor coding standards issue found via Coder module but these should not locked approval.

On easy_breadcrumb.blocks.inc

Line 86: missing space after comma
          'attributes' => array('class' => $segments_classes),)

Didn't try but with these code, both coder and ventral should be OK

      // Only adds the segment as link if its URL really exists; adds it as text
      // otherwise.
      if ($valid_path) {
        // Adds the segment to the breadcrumb.
        $breadcrumb[] = l($segment_text, $segment_url, array(
          'attributes' => array('class' => $segments_classes),
          )
        );
      }

Added credits

1. Your project page looks a little unstructured, you may use some html tags for enhancing its readability.
2. drupal_parse_info_file() does not give me any information about "recommends[]", am I missing something ? otherwise you should remove this from your info
3. easy_breadcrumb_block_info(): 'info' => 'Easy Breadcrumb', you should make this translatable
4. Please use underscores (_) instead of hyphens (-) in your variable names.
5. Please consider using variable_del() instead of a delete query as you only have constant variable names (this is best practice for this case)
6. There's no reason for wrapping drupal_get_form() with _easy_breadcrumb_settings_page() for the path "admin/config/user-interface/easy-breadcrumb".
7. t("¿Should this module disable the default Drupal's breadcrumb?"); Please don't use these upside down question marks, this will only make the live of translators harder.

8. $success_msg = t("The module's settings have been saved.");
     $success_msg = check_plain($success_msg);
     drupal_set_message($success_msg);

   It makes no sense to check_plain() static/hardcoded text.

9. Consider using system_settings_form() for saving most of the variable values (you can also use system_settings_submit for just saving the variables within your own submit handler).
10. $i $n $k $v .. please avoid using such short variable names, please choose some self-explaining ones
11. Even if "Disable the default Drupal's breadcrumb" is ticked, the standart drupal breadcrumb is still shown (using bartik) did I miss something?
12. The "Segments separator" is not filtered before it's displayed, I don't think that makes sense here. Use check_plain() to avoid security issues!
13. "EasyBreadcrumb general form settings" this is pretty long and looks a little ugly in admin_menu, maybe you could short this
14. The "reset to defaults" button is never shown, you can remove it.

Major issues you should fix: 3, 4, 7, 12. The rest is strongly recommended.
Leaving RTBC, tell me if you fixed them, I'll have a final look asap.

Improved format

3. We should keep all possibilities open, as general practice advises.
4. hmm, well that's not the general practice, but after some searching I could not find a reason against it, therefore - okayy if you really want to do it this way
8. You don't have to fully satisfy any issues of automated tools - these things are pretty dumb - especially on finding possible security issues
these tools should point you to possible issues - and suggest you to make proper changes - but nobody can force you to have a clean report result (that's sometimes just not possible) - but whenever you can and it makes sense, try to follow best practices and coding standarts
11. nope sorry, no idea, but I'm sure it's possible

Sidenote: your other module "easy module" sounds like a duplication of module builder. please always make sure to spend some time searching for existing modules (I know this can take up to two hours sometimes ;D) before you create a new module.

Well, so far this looks fine to me.

Thanks for your contribution and welcome to the community of project contributors on drupal.org!

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best. Remember this power brings responsibility ;-)

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.

I was using bartik while testing, maybe it's doing some untypical stuff in it's preprocessing function (maybe you can try to override this through hook_theme_alter()) - just a guess

hmm, maybe you should just create an issue at the module builders queue and tell them about your approach. Then you can decide whether you want to merge your modules, always good to think about joining forces first ;-)