Picon allows site administrators to edit many aspects of the 'Powered By Drupal' icon, including the actual image used, the directory where images can be discovered, the image alt attribute, the link title attribute, whether it is an actual link and the placement of the icon within the theme output.

Project Sandbox
http://drupal.org/sandbox/raynimmo/1326130

6.x-1.x Git Repository
http://drupalcode.org/sandbox/raynimmo/1326130.git/shortlog/refs/heads/6...
Git clone
git clone --branch 6.x-1.x http://git.drupal.org/sandbox/raynimmo/1326130.git picon

Drupal Version
Drupal 6

Comments

attiks’s picture

attiks commented
Status: Needs review » Needs work

Automated test results:

Master Branch
It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
coder on minor
I noticed some very small code style issues. Please run the Coder module on "minor" setting to help catch these. The coding standards have even more information in this area. I noticed things like ...

Please note: The Coder module currently has an unresolved flaw which will prompt you to add file declarations to your .info file even when it's not necessary to do so. Please do not try to make this warning go away by declaring files which do not contain classes or interfaces.

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

/picon.install:
 +18: [minor] Comment should be read "Implements hook_foo()."
 +26: [minor] Comment should be read "Implements hook_foo()."
 +27: [minor] There should be no trailing spaces
 +78: [minor] There should be no trailing spaces

/picon.module:
 +8: [minor] There should be no trailing spaces
 +10: [minor] There should be no trailing spaces
 +31: [minor] There should be no trailing spaces
 +44: [minor] There should be no trailing spaces
 +77: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
 +107: [minor] There should be no trailing spaces
 +118: [minor] There should be no trailing spaces
 +126: [minor] There should be no trailing spaces
 +132: [minor] There should be no trailing spaces
 +139: [minor] There should be no trailing spaces
 +146: [minor] There should be no trailing spaces
 +150: [minor] There should be no trailing spaces
 +160: [minor] There should be no trailing spaces
 +166: [minor] There should be no trailing spaces
 +167: [minor] There should be no trailing spaces
 +171: [minor] There should be no trailing spaces
 +182: [minor] There should be no trailing spaces
 +186: [minor] There should be no trailing spaces
 +195: [normal] The $string argument to t() should not begin or end with a space.
 +196: [minor] There should be no trailing spaces
 +200: [minor] There should be no trailing spaces
 +203: [minor] There should be no trailing spaces
 +206: [minor] There should be no trailing spaces
 +207: [minor] There should be no trailing spaces
 +208: [minor] There should be no trailing spaces
 +211: [minor] There should be no trailing spaces
 +214: [minor] There should be no trailing spaces
 +223: [minor] There should be no trailing spaces
 +226: [minor] There should be no trailing spaces
 +229: [minor] There should be no trailing spaces
 +243: [minor] There should be no trailing spaces
 +250: [minor] There should be no trailing spaces
 +253: [minor] There should be no trailing spaces
 +262: [minor] There should be no trailing spaces
 +264: [minor] There should be no trailing spaces
 +265: [minor] There should be no trailing spaces
 +266: [minor] There should be no trailing spaces
 +269: [minor] There should be no trailing spaces
 +272: [minor] There should be no trailing spaces
 +290: [minor] There should be no trailing spaces
 +292: [minor] There should be no trailing spaces
 +298: [minor] There should be no trailing spaces
 +304: [minor] There should be no trailing spaces
 +305: [minor] There should be no trailing spaces

Status Messages:
 Coder found 2 projects, 2 files, 2 normal warnings, 46 minor warnings, 0 warnings were flagged to be ignored
coder_tough_love and comments on minor
This is optional.
Severity minor, Project application tests - Comments, Coder Tough Love

/picon.install:
 +4: [minor] All comments should end with a ".".
 +5: [minor] All comments should start capitalized.
 +5: [minor] All comments should end with a ".".
 +6: [normal] Remove the empty commented line in your function documentation.
 +10: [minor] All comments should end with a ".".
 +18: [minor] All comments should end with a ".".
 +26: [minor] All comments should end with a ".".
 +26: [normal] Function summaries should be one line only.

/picon.module:
 +4: [minor] All comments should end with a ".".
 +5: [minor] All comments should start capitalized.
 +5: [minor] All comments should end with a ".".
 +6: [normal] Remove the empty commented line in your function documentation.
 +12: [minor] All comments should end with a ".".
 +16: [normal] Use sentence case, not title case, for end-user strings. (Wikipedia)
 +17: [normal] Use sentence case, not title case, for end-user strings. (Wikipedia)
 +27: [minor] All comments should end with a ".".
 +29: [minor] All comments should end with a ".".
 +29: [normal] If you define a @param or @return, you should document it as well.
 +29: [normal] @param and @return syntax should not indicate the data type.
 +30: [minor] All comments should end with a ".".
 +31: [minor] All comments should end with a ".".
 +32: [minor] All comments should end with a ".".
 +32: [normal] If you define a @param or @return, you should document it as well.
 +33: [minor] All comments should end with a ".".
 +54: [minor] All comments should end with a ".".
 +56: [minor] All comments should end with a ".".
 +56: [normal] If you define a @param or @return, you should document it as well.
 +56: [normal] @param and @return syntax should not indicate the data type.
 +57: [minor] All comments should end with a ".".
 +72: [minor] All comments should end with a ".".
 +75: [minor] All comments should start capitalized.
 +75: [minor] All comments should end with a ".".
 +79: [minor] All comments should start capitalized.
 +79: [minor] All comments should end with a ".".
 +101: [normal] Use sentence case, not title case, for end-user strings. (Wikipedia)
 +124: [normal] Use PHP's master function, not an alias. (List of PHP aliases)
 +155: [minor] All comments should end with a ".".
 +170: [normal] Use sentence case, not title case, for end-user strings. (Wikipedia)
 +176: [minor] All comments should end with a ".".
 +185: [normal] Use sentence case, not title case, for end-user strings. (Wikipedia)
 +191: [minor] All comments should end with a ".".
 +205: [minor] All comments should start capitalized.
 +205: [minor] All comments should end with a ".".
 +205: [normal] Function summaries should be one line only.
 +213: [minor] All comments should start capitalized.
 +213: [minor] All comments should end with a ".".
 +213: [normal] Function summaries should be one line only.
 +225: [normal] Function summaries should be one line only.
 +226: [minor] All comments should end with a ".".
 +227: [minor] All comments should end with a ".".
 +228: [minor] All comments should end with a ".".
 +248: [minor] All comments should start capitalized.
 +248: [minor] All comments should end with a ".".
 +257: [minor] All comments should end with a ".".
 +258: [minor] All comments should end with a ".".
 +259: [minor] All comments should end with a ".".
 +260: [minor] All comments should end with a ".".
 +262: [normal] Separate comments from comment syntax by a space.
 +292: [normal] Separate comments from comment syntax by a space.

Status Messages:
 Coder found 2 projects, 2 files, 19 normal warnings, 40 minor warnings, 0 warnings were flagged to be ignored
README.txt
Please take a moment to make your README.txt follow the guidelines for in-project documentation.
raynimmo’s picture

raynimmo commented

Thanks for taking the time to review this for me.

I dont know if its my mistake with regards to commits, but the most recent commit at http://drupal.org/commitlog/commit/29368/860f05cf4f12f6f44dc9465a486bc28... takes care of all the 'There should be no trailing spaces' errors, my text editor was making tabs regardless of how many spaces I used, drove me crazy for a while until I used a different editor.

When ran through with Coder my local version is showing :
Coder found 1 projects, 2 files, 10 minor warnings, 0 warnings were flagged to be ignored

I will have a look at the 'minor' issues and re-commit.

raynimmo’s picture

raynimmo commented

I have checked the code through the Coder module again and it reports no errors or warnings.

Coder found 1 projects, 2 files, 0 warnings were flagged to be ignored

I realised that I was working on the master branch earlier today when reading other peoples rejection reasons. I have attempted to create a 'dev' branch and push the code to that but have somehowfailed miserably, Im pretty new to Git, still getting my feet wet.

You can see from here, http://drupalcode.org/sandbox/raynimmo/1326130.git that I managed to create a tag for a 6.x-1.0-dev branch and actually pushed that to the server, tried a few times to push the code to that but it didnt go, maybe try again later as its getting late now :).

I have pushed the reworked code back onto the master branch (I know!), the commit is here http://drupalcode.org/sandbox/raynimmo/1326130.git/commit/c657ce985ec395... and includes all of the ...

Yay!, its seems that I have now managed to create a dev branch at http://drupal.org/commitlog/commit/29368/c84f242b347f8f10a79a5bf920ba19b....

The README.txt is however still in the previous branch at http://drupalcode.org/sandbox/raynimmo/1326130.git/blob/c657ce985ec39507...

I will change the project status to "needs review", fingers crossed this time.

raynimmo’s picture

raynimmo commented
Status: Needs work » Needs review
raynimmo’s picture

raynimmo commented

I have refined some of the module comments and now have all files within the same commit directory at http://drupal.org/commitlog/commit/29368/af58a14872f27bf900bb9c9184a2a9f...

raynimmo’s picture

raynimmo commented

Just committed an edit to the modules README.txt to ensure that lines wrap at 80 characters, saw a few people getting pulled for this in their module review, thought I would pre-empt that :)

commit at http://drupalcode.org/sandbox/raynimmo/1326130.git/commit/5a0aca2e900c58...

raynimmo’s picture

raynimmo commented

I have Taken care of all the errors that attiks listed above tagged with :

[normal] Use sentence case, not title case, for end-user strings. (Wikipedia)

and also those listed as;

[minor] All comments should start capitalized.

and

[minor] All comments should end with a ".".

With regards to the errors listed as;

+124: [normal] Use PHP's master function, not an alias. (List of PHP aliases)

I have changed the PHP sizeof() call to the PHP count() function.

Ran through Coder, which reported;

Coder found 1 projects, 2 files, 0 warnings were flagged to be ignored

The most recent commit is at http://drupal.org/commitlog/commit/29368/dcd7991bb6f78cca77715e3038dec1d...

attiks’s picture

attiks commented
Status: Needs review » Needs work

You're branches are messed up

git checkout -b 6.x-1.x
git push -u origin 6.x-1.x
raynimmo’s picture

raynimmo commented

right, lets try that again then....

raynimmo’s picture

raynimmo commented
Status: Needs work » Needs review

sorted, possibly?

6.x-1.x branch at http://drupalcode.org/sandbox/raynimmo/1326130.git/shortlog/refs/heads/6...

and I even managed to clean up the other branches by deleting the other tags, slowly getting the hang of Git, slowly :)

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Needs work

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

  • Run coder to check your style, some issues were found (please check the Drupal coding standards):
    Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards

sites/all/modules/pareview_temp/test_candidate/picon.module:
 +14: [minor] Comment should be read "Implements hook_foo()."
 +33: [minor] There should be no trailing spaces
 +46: [minor] There should be no trailing spaces
 +79: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
 +168: [minor] There should be no trailing spaces
 +198: [minor] There should be no trailing spaces
 +207: [minor] Comment should be read "Implements hook_foo()."
 +209: [minor] There should be no trailing spaces
 +210: [minor] There should be no trailing spaces
 +215: [minor] Comment should be read "Implements hook_foo()."
 +261: [minor] There should be no trailing spaces
 +262: [minor] There should be no trailing spaces
 +264: [minor] There should be no trailing spaces
 +292: [minor] There should be no trailing spaces
 +298: [minor] There should be no trailing spaces
 +304: [minor] There should be no trailing spaces
 +305: [minor] There should be no trailing spaces

sites/all/modules/pareview_temp/test_candidate/picon.install:
 +7: [minor] Comment should be read "Implements hook_foo()."
 +15: [minor] Comment should be read "Implements hook_foo()."
 +23: [minor] Comment should be read "Implements hook_foo()."
 +75: [minor] There should be no trailing spaces

Status Messages:
 Coder found 1 projects, 2 files, 1 normal warnings, 20 minor warnings, 0 warnings were flagged to be ignored
  • Comments should be on a separate line before the code line, see http://drupal.org/node/1354#inline
    ./picon.module:157:      t('Yes'), // 0 for yes.
./picon.module:178:      t('Replace standard <strong>Powered By</strong> block icon'), // 0 for default.
./picon.module:193:      t('Drupal\'s <strong>misc</strong> folder'), // 0 for default.
  • ./picon.module: The description for the @param/@return documentation is either missing or not formatted correctly. See http://drupal.org/node/1354#functions
    32- *  Image directory path from the site root.
35- *  An array containing image paths and names.
59- *  Image location stored as integer value.

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

manual review:

  • picon.css: indentation errors, always use 2 spaces per level, see also http://drupal.org/node/302199
  • picon.css: "@CHARSET "ISO-8859-1";": why do you need that?
  • picon_uninstall(): where is $theme_registry coming from? it is not defined.
  • picon_schema(): do not run DB description texts through t(), this will only create overhead for translators.
  • "define('PICON_IMAGE_DEFAULT', 'powered-black-80x15.png');": indentation error, should be moved to the left completely.
  • PICON_IMAGE_DEFAULT is not used anywhere, so remove it or use it.
  • "$theme_path = drupal_get_path('theme', $theme_name);": you can use "global $theme_path" instead
  • "if ($current_image == '') {": why do you need this? you have ran that insert query on installation already?
  • "$current_image = db_result(db_query('SELECT image from {picon} WHERE pid=%d', '1'));": there should be a space before and after "=", see http://drupal.org/node/2497
  • "// 0 for yes.": that is not ideal, you can use array keys to specify 1 => t('Yes').
  • picon_theme_registry_alter(): indentation error in the body of the if statement
  • picon_theme_registry_alter(): why do you invoke the theme function in there? $variables is undefined.
  • phptemplate_system_powered_by(): why this strange drupal_add_js() call with the HTML comments? what am I missing?
  • "_image.setAttribute('alt','" . $alt_text . "');": be careful when embedding user generated data like that. I have the feeling that if I use something like a'); alert('XSS as alt text I might get funny pop ups ;-P You should sanitize this, see also http://drupalscout.com/knowledge-base/drupal-text-filtering-cheat-sheet-...
raynimmo’s picture

raynimmo commented
Status: Needs work » Needs review

Hiya Klausi, thanks for taking the time to review this, much appreciated.

It certainly seems like your PAReview.sh is far more thorough than the standard Coder module, my local copy was still reporting 'Coder found 1 projects, 2 files, 0 warnings were flagged to be ignored' but using your listing I was able to find the offending lines, again, much appreciated.

I have removed or edited the trailing spaces and commenting errors from the files in question.

The @param errors were incorrectly formatted with a comma after the value, my bad.

RE:CSS file, my CSS editor seems to prefer tabs to spaces, have used a different editor and removed the offending indentations. Also, the @charset declaration, I saw that on a few other modules CSS files when researching the best way of formatting my CSS, I thought it may have been a prerequisite, I have removed it.

The line, unset($theme_registry['system_powered_by']['preprocess functions']['theme_system_powered_by']); was used primarily after I had problems disabling and uninstalling the module but I have since realised it only happened when the 'Themer' module was enabled, it was throwing an error that $theme_registry['system_powered_by'] was undefined. I should maybe re-create that and enter it as an issue in Themer's queue.

The static variable definition again was legacy code from a method I was using previously to ensure a default was set, as you said its not used anymore, removed.

Again, the $current_image check and set was legacy code to ensure a default was set and if not then it is re-inserted, removed.

With regards to yor comment

"// 0 for yes.": that is not ideal, you can use array keys to specify 1 => t('Yes').

I was using the literal database value to set the state of the radio buttons, that a good tip using the array keys, I have used that in all my radio selections now.

With regards to picon_theme_registry_alter(), I have sorted the indentation error. The call to the theme function, again was legacy code that was in there as I was initally having trouble getting the theme function to fire, have removed it and tested, seems fine without it.

The line drupal_add_js('--></script><div class="picon-el">' . $image_as_link . '</div><script><!--', 'inline', 'footer'); was required to insert the <div class="picon-el"> </div> into the $closure. Unless I escaped the <script> block that was already in there then it was just outputting the html code. The HTML comments where required for Internet Explorer as it would not display unless I escaped the comment before the block and then re-instate it at the end. Yes, I know it is a bit of a hack, any suggestions on a cleaner method would be most appreciated.

With regards to the line "_image.setAttribute('alt','" . $alt_text . "');":, I didnt think that would really be an issue as site admins are the only ones able to access the settings form, however taking into account your comments I have used filter_xss() within the JavaScript sections that output the code for the title, src and alt attributes.

I have again ran through the module with Coder which reported;

Coder found 1 projects, 2 files, 0 warnings were flagged to be ignored

but I am starting to trust that less with regards to spacing at the end of lines, so have done a visual check and have found nothing to note.

I have commited to http://drupal.org/commitlog/commit/29368/d7b0ea572d67e6bb9dbcb4cff24ad82... and set as 'needs review'

Again, thanks for the thorough review, looking forward to any replies :)

raynimmo’s picture

raynimmo commented

I have been thinking about those calls to filter_xss() with regards to the lines in picon.module , primarily;
Line 272 _image.setAttribute('src','" . filter_xss($full_image_path) . "');

I have used the filter for the alt and title attributes as they can be set by the admin on the settings page but the src attribute cannot be manipulated by the user as it is set within the function picon_get_img_path().

Does anyone think this is an extra overhead using it for the src attribute and should I remove that filter?

raynimmo’s picture

raynimmo commented

Realised I had a crazy long line in the README.txt , have reduced it to 80 characters and recommitted that file to http://drupal.org/commitlog/commit/29368/592c00a654a35e15a2515d43d54b359...

raynimmo’s picture

raynimmo commented

While checking if XSS was possible with the alt and title attributes I realised that I wasnt including the title attribute within the HTML anchor tags output.

Have re-committed picon.module to http://drupal.org/commitlog/commit/29368/740f22ea88e588ead478ad13605cd45...

raynimmo’s picture

raynimmo commented

With regards to the HTML anchor tag, I currently have it opening any hits to drupal.org in a new tab using target="_blank". I realise the usability issues behind this but generally, as a site owner, I would prefer those type of links to open in a new tab as they are external.

I could enforce an external link icon next to the image or set the link title to display 'This link opens in a new tab' but then I would lose the admin functionality to set the title attribute.

I could possibly also include another setting in the admin settings to switch between opening in a new tab or opening in the same tab and if set to 'in a new tab' then remove the ability to set the title attribute and have it set as 'This opens in a new tab'.

I would love to hear anyone's opinions on this.

raynimmo’s picture

raynimmo commented

Today I learned that if you force a new line using 'enter' into your .info file description then it breaks the modules compatibility with your Drupal version, oops.

Updated picon.info committed to http://drupal.org/commitlog/commit/29368/8ac3f627cbff9136796a6f8b3df9bfa...

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Needs work

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

  • Run coder to check your style, some issues were found (please check the Drupal coding standards):
    Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards

sites/all/modules/pareview_temp/test_candidate/picon.module:
 +145: [minor] There should be no trailing spaces

Status Messages:
 Coder found 1 projects, 2 files, 1 minor warnings, 0 warnings were flagged to be ignored
  • Comments: there should be a space after "//", see http://drupal.org/node/1354#inline
    picon.module:207:  //function picon_theme_registry_alter(&$variables) {
picon.module:211:      //phptemplate_system_powered_by(&$variables);
  • There should be no space after the opening "(" of an array, see http://drupal.org/node/318#array
    picon.module:145:    '#options' => array(
  • Do not use t() in hook_schema(), this will only generate overhead for translators.
        'description' => t('Stores the icon reference used'),
        'description' => t('The icon image used.'),
      'description' => t('The image used in place of the default one'),
        'description' => t('The alternate text for the icon image'),
        'description' => t('Flag denoting if an anchor tag is used.'),
        'description' => t('The title attribute that is associated with the anchor tag'),
        'description' => t('The location to search for images'),
        'description' => t('The location within the template to render the icon'),

This automated report was generated with PAReview.sh, your friendly project application review script. Please report any bugs to klausi.

manual review:

  • "'#title' => 'Images available',": all user facing text should run through t() for translation
  • "filter_xss($full_image_path)": I guess this is a URL, so use check_url() here.
  • I think title and alt attributes should be plain text, so use check_plain() instead of filter_xss()
raynimmo’s picture

raynimmo commented
Status: Needs work » Needs review

Thanks for the review and the advice Klausi, much appreciated.

I have taken the appropriate action and reworked the code where it was required, and once more ran it through Coder :)

I have recommitted the updated files to http://drupal.org/commitlog/commit/29368/e1663d5b3ae190be853a327b3eda253...

raynimmo’s picture

raynimmo commented

Just uploaded an updated version, have committed it to http://drupal.org/commitlog/commit/29368/01db2aa7352df206ad80e4e5a4c7fb7...

Mainly dealing with an issue that has been bugging me for a while and one that Klausi picked up on in comment #11 with regards to;

phptemplate_system_powered_by(): why this strange drupal_add_js() call with the HTML comments?

Initially was having trouble writing the image and the link to the $closure when that option wasnt selected. I have implemented a check for the $closure now which is called from phptemplate_preprocess_page() and depending upon the output echoes the result to the $closure.

I have also managed to combine some if-else statements that ended up with similar return values after these changes.

Updated files ran through Coder which reported 'Coder found 1 projects, 2 files, 0 warnings were flagged to be ignored'

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Needs work

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

  • Assignments should have a space before and after the operator, see http://drupal.org/node/318#operators
    ./picon.module:218:function phptemplate_system_powered_by($image_path, $is_closure=0) {

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so I can get back to yours sooner.

manual review:

  • "'#title' => 'Image as a link',": all user facing text should run through t() for translation
  • "'type' => 'MENU_NORMAL_ITEM'": this should be a constant, so remove the quotation marks
  • picon_schema(): you create your own schema, but you are only using one row in that table, no? In that case you could just save that 7 settings in the variable table with variable_set() and variable_get().
raynimmo’s picture

raynimmo commented
Status: Needs work » Needs review

Thanks (again) Klausi for your thorough review and advice.

I have fixed the spacing issue and the minor textual errors.

With regards to your comment;

picon_schema(): you create your own schema, but you are only using one row in that table, no? In that case you could just save that 7 settings in the variable table with variable_set() and variable_get().

I was previously using the variable table for this module before I actually released it. I felt that maybe I was committing a faux-pas by using the variable table for so much data. I understand there can be issues when the variable table gets swollen causing performance issues so I then switched over to storing the data in its own table.

I have taken your comments on board and switched the data read/writes over to the variable table and edited the install file with appropriate variable_get() and variable_del() calls. I have also changed the module file and used variable_get() where previously I was using sql lookups and variable_set() within the modules hook_submit().

I also upgraded my Coder to 6.x-2.x-dev and installed coder_tough_love, both of which pointed out another 2 small textual errors within my module file.

Updated files committed to http://drupal.org/commitlog/commit/29368/3f972b078622c344278d47e0cb80269...

jthorson’s picture

jthorson commented

From your commit diff, it looks like you added a new typo:

@@ -173,7 +168,7 @@ function picon_admin_settings() {
   );
   $form['directory_options'] = array(
     '#type' => 'fieldset',
-    '#title' => 'Folder To Scan For Images',
+    '#title' => 'Folder to scan ror images',
   );
   $form['directory_options']['picon_location'] = array(
     '#type' => 'radios',
raynimmo’s picture

raynimmo commented

@jthorsen, thanks for spotting that, I actually added that in my last commit, doh!

coder_tough_love pulled me for using title case rather than sentence case on that line which is why I changed it. The 'F' and the 'R' are quite close on the keyboard so I'll call that a fat-finger error :)

Fixed and recommitted to http://drupal.org/commitlog/commit/29368/fe93e988807d835d7371f07a328315f...

raynimmo’s picture

raynimmo commented
Issue summary: View changes

added Drupal version

raynimmo’s picture

raynimmo commented
Status: Needs review » Needs work

refactoring code and changing some central aspects of the module so I have changed its status to 'needs work' until I have them resolved.

raynimmo’s picture

raynimmo commented
Status: Needs work » Needs review

With regards to my own comment above (#16) with regards to the usability issues surrounding opening the link in a new tab as opposed to the parent, I have now added an extra option to allow for the site admin to set this attribute of the link. I have however implemented some restrictions on this functionality.

If the admin sets it to open in a new tab then the anchor 'title' attribute is changed to 'This will open in a new tab'.

The option to create an image icon or an image within a link now also has some restrictions. When selected to only display an image and not a link, the 'open in a new tab' and 'title' setting functionality are disabled.

I have rewrote the drupal_add_js() calls within the module file to keep them more in line with the use of jQuery whereas previously I was using DOM methods. I have also employed the use of the Drupal.theme() JavaScript function to allow the HTML produced to be themed rather than the static tags I was previously using.

The changes to the JavaScript have now allowed me to address the cross-browser compatibility issues that were troubling me as the 'append to body' function now works for Internet Explorer.

Latest commit to http://drupal.org/commitlog/commit/29368/1c647e6e012e441f5524c552ddebdd9...

raynimmo’s picture

raynimmo commented

Noticed a few other module reviewers commenting upon the use of general administrator access permissions in module applications so I decided to implement a permission setting for access to the modules settings page.

I established a function based on hook_perm() that fills the role of access arguments in my implementation of hook_menu().

Latest commit to http://drupal.org/commitlog/commit/29368/b5414f66e1ba46899721457c00405eb...

raynimmo’s picture

raynimmo commented

Refined some of the module code and checked the formatting, spacing, line endings for nefarious spacing issues.

Added a new line at the end of text files (picon.info, README.txt) as I have seen a few others being pulled for that from reviewers.

Added to and expanded upon the README.txt, the content of which I have also posted to this projects sandbox.

Also, tidied up the master branch and left only a README.txt in there explaining about the module and where to get it.

Hopefully everythings in order, all I need now is for someone to RTBC this :)

Latest commit to http://drupal.org/commitlog/commit/29368/91178e1558fff8553a1f7f6bf89bb50...

raynimmo’s picture

raynimmo commented

The page that outlines the Drupal coding standards does seem somewhat confusing in their use of words.

All text files should end in a single newline (\n). This avoids the verbose "\ No newline at end of file" patch warning and makes patches easier to read since it's clearer what is being changed when lines are added to the end of a file.

Now this would suggest to me that it is text files only that this applies to. Now this may be argued over, but in my book code files are not really text files, they are code files, module files, install files, include files, etc etc.

Text files to me, are those that have a file type of .txt.

However, I am increasingly seeing people being chastised for not having a new line at the end of module, install, css and javascript files.

So in order to avoid any holdups in my project application I have added newlines to end of each file.

Latest commit is at http://drupal.org/commitlog/commit/29368/305379dd6cf94218ce013267e1ac455...

Now I am waiting for a reviewer to come along and tell me I have too much whitespace at the end of my files <scream>:-o</scream>

doitDave’s picture

doitDave commented
Status: Needs review » Needs work

Hey,

now with a role reversal* ;P

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

  • Run coder to check your style, some issues were found (please check the Drupal coding standards):
    Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards

sites/all/modules/pareview_temp/test_candidate/picon.module:
 +285: [normal] String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms

Status Messages:
 Coder found 1 projects, 1 files, 1 normal warnings, 0 warnings were flagged to be ignored
  • ./picon.module: all functions should be prefixed with your module/theme name to avoid name clashes. See http://drupal.org/node/318#naming
    function phptemplate_system_powered_by($image_path, $is_closure = 0) {
function phptemplate_preprocess_page(&$vars) {
  • All text files should end in a single newline (\n). See http://drupal.org/node/318#indenting

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

To be honest, although I work with theming functions myself, this phptemplate_ stuff confuses me ever and ever again. Although it seems logic. However: Isn't modulename_preprocess_page the correct function name? In John VanDyk's book, the phptemplate_preprocess(_breadcrumb) is explained as theme specific, but not for modules (where the corresponding function is named as above). Which, in my understanding, makes sense, as in one Drupal call, only one theme is invoked, but many modules are, so that function names could clash. The same should apply to phptemplate_preprocess_page() most obvious!?!
Theming workflow in modules, AFAIR, is that you first define themeable functions and their variables in hook_theme(), then you catch your self-defined theme functions with default implementations in either in functions like theme_modulename_themeablefunction($arg1, $arg2, ...) OR in the themeablefunction.tpl.php in your module directory. Whatever variable values you want to pass to the themed HTML, is provided when you call theme('modulename_themeablefunction',$arg1, $arg2, ...);
(I primarily explain this to help myself remember it and if there's any mistake on my side please correct me!)

As for string concatenation warning, my editor warns about some " or ' missing in line 285 and after, in your Drupal.theme() call. And, speaking of the Javascript thing: You use drupal_add_js to insert $(document).ready syntax. After all I have read, the suggested way is

  • putting that stuff in a modulename.js file,
  • adding the file with drupal_add_js() just where you now add plain code,
  • not using $(document).ready directly but the Drupal JS API and Drupal.behaviors instead,
  • Passing your variables (such as $full_image_path) using
    drupal_add_js(
  array(
    "modulename" => array(
      "fullImagePath" => $full_image_path,
    ),
  ),
  "setting"
);

    and then referring to it within your scriptfile using Drupal.settings.modulename.fullImagePath.

And don't forget to fix the newline issue at the end of all files listed above (ha, ha ;))

HTH, dave

*feel re-invited at any time ;)

raynimmo’s picture

raynimmo commented

Thanks for that Dave, it always helps to have another persons view to flesh out ideas, it can be so hard working alone sometimes when encountered with new ideas and techniques to get your head around.

RE: the phptemplate_preprocess_hook() , I often use John VanDyk's book for reference also, cant do without it! The breadcrumb example is rather thin though and doesnt really cover all aspects of the usage I feel.

The call to phptemplate_system_powered_by() is meant to overwrite the core impementation of theme_system_powered_by() that exists in modules/system/system.module , I tried a few naming conventions and that is the only way I could get it to work. I used the picon_theme_registry_alter() as an implementation of hook_theme_registry_alter() method to unset the default function call from the registry so that my own implementation is invoked rather than the default core one.

Again, the call to phptemplate_preprocess_page() was created after a few attempts at modifying the theme output, I figured I didnt want to unset this function as it was used often by other developers and I only wanted to alter the $closure variable with it anyway.

RE:unix line breaks, yeah, working on a windows machine is a bummer for stuff like that. I have been meaning to stick a copy of PAReview.sh on my server for a while now, still having trouble getting Drush running on my Windows development machine.

Found an easy fix at http://serverfault.com/questions/6134/how-to-convert-text-file-from-wind...

Using Git Bash I opened each file and changed the filetype and then resaved them, each originally had windows linebreaks at the end, hopefully they have now been converted to unix style linebreaks.

vim <filename>
:set fileformat=unix
:wq

I will probably try and get PAReview.sh running and check this myself before I next recommit.

Also, found a few problems myself. I recently started setting up a new testing environment, set the theme as Bluemarine and instantly it encountered a few errors.

warning: Missing argument 2 for variable_get(), called in C:\xampp\htdocs\dev\drupal-6-themedev\sites\default\modules\custom\picon\picon.module on line 240 and defined in C:\xampp\htdocs\dev\drupal-6-themedev\includes\bootstrap.inc on line 583.

(there was one of these errors for each variable_get() call in the module.)

Found similar problems and a solution at http://drupal.org/node/113946 ,I suppose this make sense although I never encountered a warning before about it. The Drupal 6 version of variable_get() doesnt have a variable default assignment although the Drupal 7 version does. Most examples (even VanDyk's book) only use single arguments.

Previous syntax I was using;
variable_get('picon_image');
changed to;
variable_get('picon_image', NULL);
and the error warnings go away :)

I also encountered a warning error for;

warning: count() expects at least 1 parameter, 0 given in C:\xampp\htdocs\dev\drupal-6-themedev\sites\default\modules\custom\picon\picon.module on line 113.

Previously the syntax I was using;
if ($current > $options . count()) {
changed to
if ($current > $count($options)) {
sorted :)

RE:Drupal.behaviors; I was only reading about that last night, lol, I wasnt happy myself with those $(document).ready calls. I have been thinking about moving the whole lot into a function in my existing picon.js file anyway and then calling that function when required. I have two use cases for the function, one that requires 4 arguments and another that requires 2 arguments, possibly set up default arguments for the 2 that are only called for one use case. Your idea seems like a more efficient implementation though, maybe give that a try.

I havent recommitted anything yet but I will have a play with the phptemplate_ calls and the JavaScript before I do. Will set this to 'needs review' once I have had a play with stuff.

Once again, thanks for your review, looking forward to the next one :)

If anyone has any other ideas of pointers about anything mentioned here then I would appreciate any feedback you may have.

doitDave’s picture

doitDave commented

Hey, just a brief re on OS's: I also work with Windows. In my editor (I am happy since many years with Textpad as it is tiny and very powerful though). I can preset the line endings and many more there by filetype and have set "Unix style" for simply any text file type related with dev. That makes it very easy.

For the shell environment: I prefer emulating an entire Linux machine rather than setting up a unix style environment in a DOS world (which will always have too many caveats IMO). With some few tweaks you can easily run e.g. Ubuntu server even within a Windows Virtual PC machine, that's what I do when working and developing with my desktop. (Servers of course are native *x machines.)

Regarding the phptemplate:

The call to phptemplate_system_powered_by() is meant to overwrite the core impementation of theme_system_powered_by() that exists in modules/system/system.module

That won't work. Trying to install:

Fatal error: Cannot redeclare phptemplate_preprocess_page() (previously declared in X:\drupal-6.22\sites\all\modules\1326130-6.x-1.x-305379d\picon.module:324) in X:\Internet\Server\test\drupal-6.22\themes\garland\template.php on line 49

Just for me to get it: You want to replace the value of $image instead of the HTML around it. Right? As of my understanding this means that the module defining this themeable item (i.e. system.module) would have to invoke a hook_preprocess_powered_by() that you could respond to with picon_preprocess_powered_by(). IMO that would be the only way (but there is no such hook, I have tried). As the module doesn't do this, it seems to be kind of not willing to allow other modules an interaction with the variable it passes. The only place the result is intended to be overwritten would be in a theme.

Looks like there is some more in-depth investigation to be done. Let me know once you find a solution.

Just another thing I found looking into your module: variable_get("picon_foo"); - you miss out the second argument (default value) at many places.

HTH, dave

raynimmo’s picture

raynimmo commented

lol, just installed Textpad this afternoon after finding it in this list of text editors and their newline support.

For a shell I tend to jump between Putty for working on the server and Git Bash for localhost, works for me :)

Earlier today I managed to get Drush running on my Windows dev box and PAReveiw.sh also. Not too sure if PAReview.sh has a bug as it outputs:

<ul>
</li>
<li>All text files should end in a single newline (\n).
< code >
< /code >
</li>
</ul>

* notice the empty < code > < /code > section.

...and then when I remove a line at the end of the file it outputs:

<ul>
</li>
<li>All text files should end in a single newline (\n).
< code >
 ./picon.module
< /code >
</li>
</ul>

As expected as there is now no newline at the end of the module file.

Not too sure but it seems like because the < code > section is empty that there are no files found to meet that criteria but it still outputs the header for that warning. Klausi's sandbox issues dont have any mention of this so maybe its more of a feature and not a bug?

RE:the variable_get() single argument, yeah, I mentioned that in my previous comment, havent committed any changes today though so still the previous codebase thats in there.

Still investigating the phptemplate_ section, todays has been a client work day, no time for personal projects :(

doitDave’s picture

doitDave commented

Yes, although klausi was not happy at first with my statement I stick to it: *Something* is weird at that point. ;)

Although I already tried to figure out or at least get closer to it, I did not succeed yet. Let's see who will.

There is a German saying which states that good things need their time. (I don't know the english counterpart but sure one exists.)

Don't forget to share your findings on the preprocess ting. I am really interested too.

raynimmo’s picture

raynimmo commented

Right then, where do I start.

First of all, those pesky phptemplate_preprocess functions where driving me nuts. It worked up until I changed anything that called any of the functions I was using then it all came crashing down.

First was to rename phptemplate_system_powered_by() to picon_system_powered_by() and also to rename phptemplate_preprocess_page() to picon_preprocess_page() as these were the main culprits causing the crash when the theme was changed.

Once changed, everything stopped working!

So I know I have to somehow get a call to picon_powered_by() , but how?

After some extensive reading of chapters 3 and 8 of John VanDyks book and a hefty peruse through the Drupal api pages for theme, hook_theme(), phptemplate_theme() and the documentation pages for setting up variables for use in a template, About overriding themable output, the beginners guide to overriding themable output and a whole host of other snippets and comments too numerous to mention. These are the key ones I found handy for anyone else that is interested.

So, to create a call to picon_system_powered_by() I had to build an implementation of hook_theme and then pass it my function name.

function picon_theme($existing, $type, $theme, $path) {
  $templates = drupal_find_theme_functions($existing, array('picon_system_powered_by', $theme));
  return $templates;
}

As soon as that was in there, everything started working again, wasnt that easy!

My next step was to rework my JavaScript as doitDave had suggested previously in the thread by the use of Drupal.behaviors

Previously I was building up the javascript and then executing it once I had all my variables together as such;

drupal_add_js("
        Drupal.behaviors.picon = function(context) {
          var pImage = Drupal.theme('piconImage', '" . check_url($full_image_path) . "', '" . check_plain($alt_text) . "');
          var url = 'http://www.drupal.org';
          var pLink = Drupal.theme('piconLink', url, '" . check_plain($title_text) . "', '" . $link_behaviour . "' );
          var pDiv = Drupal.theme('piconDiv', 'picon-el');
          $('body').append($(pDiv).append($(pLink).append($(pImage))));
        };
      ", 'inline', 'footer');

But using Drupal.behaviours I was able to assign each one of my variables to an array key and pass the full array to my javascript function.
In my main module file I put;

$picon_settings = array(
        'image_path' => check_url(url($image_path)),
        'alt_text' => check_plain($alt_text),
        'title_text' => check_plain($title_text),
        'link_behaviour' => $link_behaviour
      );
      drupal_add_js(array('picon' => $picon_settings), 'setting');

for one use case when a link is created, and then for when it is only the image I put;

$picon_settings = array(
        'image_path' => check_url(url($image_path)),
        'alt_text' => check_plain($alt_text)
      );
      drupal_add_js(array('picon' => $picon_settings), 'setting');

and then within my main javascript file picon.js I put;

var image_path = Drupal.settings.picon.image_path;
  var alt_text = Drupal.settings.picon.alt_text;
  var title_text = Drupal.settings.picon.title_text;
  var link_behaviour = Drupal.settings.picon.link_behaviour;
  var url = 'http://www.drupal.org';
  var pImage = Drupal.theme('piconImage', image_path, alt_text);  
  var pLink = Drupal.theme('piconLink', url, title_text, link_behaviour );
  var pDiv = Drupal.theme('piconDiv', 'picon-el');
  (link_behaviour) ? $('body').append($(pDiv).append($(pLink).append($(pImage)))) : $('body').append($(pDiv).append($(pImage)));

The end result works like a charm and I have to say the code is so much lighter and now that I have a bit of a grasp of Drupal.behaviors it makes so much sense to do it this way.

I would elaborate more but its steadily approaching 5am and I really need to get some sleep :)

BTW: thanks Klausi for fixing the bug in PAReview.sh and now I can claim no errors found by that or Coder, yay!

Latest commit is at http://drupal.org/commitlog/commit/29368/25e99ff198080de4432f2a2848f4f89...

Looking forward to your reviews but hopefully its an RTBC :)

raynimmo’s picture

raynimmo commented
Status: Needs work » Needs review
raynimmo’s picture

raynimmo commented

I have re-written a few small parts to ensure that any CSS class names or Id's used are unique by appending 'picon-' to them. Also done the same with JavaScript variables throughout to avoid any potential clashes.

I have also implemented a few checks for if the user selects to read available images from a theme folder where there are no images that meet the criteria. When this is encountered various areas on the UI indicate that no images are available.

Latest commit to http://drupal.org/commitlog/commit/29368/803a523d6b010ebcb655bb0b3cda028...

Also added a screenshot of the admin settings page to the project page at http://drupal.org/sandbox/raynimmo/1326130

raynimmo’s picture

raynimmo commented

Whilst perusing the code review groups I came across a script by attiks that automates the review process much the same way the PAReview.sh script does. Attiks has already reviewed my projects and I came across the entry he runs for reviews with a handy link that states 'Retest Project'. When I ran it against this project it highlighted a few 'All comments should end with a ".".' erros, which surprised me as my own local copies of Coder and PAReview.sh didnt flag either of them.

The lines in question are not actually contravening Drupal's coding standards. The @file block was one that was flagged for not having a period at the end and even the examples shown on the comment formatting conventions for files does not have one. Other lines that were flagged were those containing @param declarations and again the examples on the commenting and formatting conventions for functions sections also uses examples without periods at the end of the lines.

Anyway, in an attempt to allow this project to progress as soon as one of the senior community members reviews it I have added the offending periods.

Latest commit is at http://drupal.org/commitlog/commit/29368/250d7e02a967b1a51b22cf18ad959cc...

raynimmo’s picture

raynimmo commented

I have been reading through the Drupal CSS coding standards and found that a few lines in my CSS file dont adhere to some of the set rules.

Namely the rules I was flouting were;

  • A space should be between the selector and the opening curly brace.
  • Multiple properties should be listed in alphabetical order.
  • As a rule of thumb, add a /* LTR */ comment in your style, when using unequal margins in a box.
  • A blank line should be placed between each group of selectors.

Re-committed, then noticed a typo in the header comment block, doh!, so 2 commits, latest one at http://drupal.org/commitlog/commit/29368/e4a76566de353c0336b86f31af64c84...

Full 6.x-1.x branch at http://drupalcode.org/sandbox/raynimmo/1326130.git/tree/refs/heads/6.x-1.x

raynimmo’s picture

raynimmo commented

Further testing revealed problems with my implementation of t() when there are dynamic strings involved. With regards to the documentation page for dynamic strings with placeholders I have reworked part of the main module file.

My previous implementation took the $alt_text and $title_text attributes straight from the database and then passed them into the translation function;

$image = theme('image', $image_path, t($alt_text), t($title_text));

I have now upgraded that line to use placeholders that the dynamic strings are then passed to.

$image = theme(
                   'image', 
                   $image_path, 
                   t('@alt_text', array('@alt_text' => $alt_text)), 
                   t('@title_text', array('@title_text' => $title_text))
              );

* broken up for readability - this should all be on one line.

Latest commit to http://drupal.org/commitlog/commit/29368/1f43fa8f4f79cd7a069e09e58a323f5...

raynimmo’s picture

raynimmo commented

Updated all of my calls to t() to ensure that none of them included any HTML tags and that I wasnt escaping any apostrophes within the translatable string.

Also, attempted an implementation of hook_help() but it seems that due to a known bug it clashes with my implementation of hook_theme(), so thats 2 commits, first one is junked, the latest one at http://drupal.org/commitlog/commit/29368/90b37e68fa2d0316abe08270bdeed81...

raynimmo’s picture

raynimmo commented

There were a few areas within picon.module and picon.js that I was employing the use of ternary operators where the output differed depending upon the conditions.

After reading through the Programming Best Practices I came across the following;

Programming pitfalls to avoid .... The dichotomy of the ternary operator

So I have replaced all occurences of ternary operators within my module and JavaScript code with standard if-else control structures, nice, clean and readable :)

Latest commit is at http://drupal.org/commitlog/commit/29368/5f320e223b92b001cce0c987432ddae...

raynimmo’s picture

raynimmo commented

Have combined some if-else statements that were sharing common goals after recently removing ternary operators from the module.

Also spotted a stray HTML tag within a t() function, have removed and separated them.

Latest commit at http://drupalcode.org/sandbox/raynimmo/1326130.git/commit/a7479b0edaf7d5...

Full 6.x-1.x branch is at http://drupalcode.org/sandbox/raynimmo/1326130.git/tree/refs/heads/6.x-1.x

Checked with PAReview.sh, all clear.

Checked with Coder
Coder settings

  • Coder Tough Love
  • Drupal Coding Standards
  • Drupal Commenting Standards
  • Drupal SQL Standards
  • Drupal Security Checks
  • Interface text translatability
  • Internationalization
  • User interface text
  • set at minor (most)

All clear :)

RTBC ? anyone :)

doitDave’s picture

doitDave commented
Status: Needs review » Needs work

Hi Ray,

I just managed to install drupalcs. Thought that you'd be interested in the results:

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

  • Run coder to check your style, some issues were found (please check the Drupal coding standards):
    Severity minor, Drupal Commenting Standards, Internationalization, Drupal Security Checks, Drupal SQL Standards, Drupal Coding Standards

sites/all/modules/pareview_temp/test_candidate/picon.module:
 +129: [minor] There should be no trailing spaces

Status Messages:
 Coder found 1 projects, 1 files, 1 minor warnings, 0 warnings were flagged to be ignored
  • Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards):
    
FILE: .../web/dp709/sites/all/modules/pareview_temp/test_candidate/picon.install
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 3 | ERROR | The second line in the file doc comment must be " * @file"
--------------------------------------------------------------------------------


FILE: ...d/web/dp709/sites/all/modules/pareview_temp/test_candidate/picon.module
--------------------------------------------------------------------------------
FOUND 5 ERROR(S) AND 5 WARNING(S) AFFECTING 10 LINE(S)
--------------------------------------------------------------------------------
   4 | ERROR   | The second line in the file doc comment must be " * @file"
  11 | ERROR   | You must use "/**" style comments for a function comment
  18 | ERROR   | You must use "/**" style comments for a function comment
 129 | ERROR   | Whitespace found at end of line
 198 | WARNING | A comma should follow the last multiline array item. Found: )
 215 | WARNING | A comma should follow the last multiline array item. Found: )
 267 | WARNING | A comma should follow the last multiline array item. Found: )
 352 | WARNING | A comma should follow the last multiline array item. Found:
     |         | $link_behaviour
 358 | ERROR   | Expected "}\nelse {\n"; found "}\n  // Flat image without
     |         | link.\n\nelse{\n"
 365 | WARNING | A comma should follow the last multiline array item. Found: )
--------------------------------------------------------------------------------

Cheers, d.

raynimmo’s picture

raynimmo commented
Status: Needs work » Needs review

Thanks for that Dave, I havent managed to get drupalcs running here yet, still messing with the PEAR package for it, although I thought it may highlight some points.

The @file block warnings have got me puzzled though as all files have @file doc blocks in them already? Previously the scanner that attiks uses had warned me about those lines not having a period at the end; which I thought was odd as the documentation doesnt mention those lines needing periods, so I ended up putting periods on the ends of every line in the comments, including the @file declarations. Coder thought nothing of it, obviously drupalcs was having none of it and flagged them as incorrect. Have removed them now.

With regards to the 'A comma should follow the last multiline array item' warnings, I didnt think they were required for the last check/radio button values, obviously drupalcs thinks different. Someone should tell John VanDyk as his book doesnt use that convention either, see page 30 [annotate_deletion], now that I scan through it though, there are loads of examples where he doesnt use that syntax. Anyway, commas added.

I should have addressed all the issues brought up in that last scan, latest commit is at http://drupalcode.org/sandbox/raynimmo/1326130.git/commit/2d28ca4

If anyone else is passing by and runs this through drupalcs can you please post if it passes, it would set my mind at ease :)

raynimmo’s picture

raynimmo commented

I just ran it through the scanner that attiks uses and now that I have removed the periods after the @file declarations it is telling me they are missing!!

Its results state..

/picon.install:
 +3: [minor] All comments should end with a ".".

/picon.module:
 +4: [minor] All comments should end with a ".".
 +340: [minor] All comments should end with a ".".
 +357: [minor] All comments should end with a ".".

Install, line 3 , thats an @file block without a period.
module, line 4 thats an @file block without a period.
module, line 340 , that is // Image as a link. * notice the period.
module, line 357, again that is // Flat image without link. * again with a period.

Not totally sure of the integrity of that scan if these are false positives or not.

doitDave’s picture

doitDave commented

It is a really merciless script. I hate it ;) [Just kidding! Fine tool. But who is without mistakes.]

If you're in a debian world, did you try apt-get install php-pear already?

"@file": You are right. What is this?! Strange. Probably raise an issue in the drupalcs project queue? (It also harassed me when complaining about a legal hook implementation docblock.)

Well, what would we want to discuss on the always-current always-involving standards. John's book is printed on paper, it just cannot keep on with the evolution ;) But anyhow I recently read an argument for why adding the last comma in arrays: "To make clearer that this is an array and that something may be added right here." (or similar). Discussable, but not totally wrong. However.

Funny as it is:

Automated review (Please keep in mind that this is primarily a high level check that does not replace but, after all, eases the review process. There is no guarantee that no other issues could show up in a more in-depth manual follow-up review.)

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

  • Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards):
    
FILE: ...d/web/dp709/sites/all/modules/pareview_temp/test_candidate/picon.module
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 357 | ERROR | Line indented incorrectly; expected at least 4 spaces, found 2
--------------------------------------------------------------------------------

This automated report was generated with PAReview.sh, your friendly project application review script. Go and review some other project applications, so we can get back to yours sooner.

No more @file warning. What did you change now?

(I'll leave it to "needs review" as for your last closure.)

raynimmo’s picture

raynimmo commented

Thanks for that Dave, much appreciated.

RE:book, yeah, suppose the only way to keep up with a moving standard is constant reading and learning. The fact that most of Drupal core wont pass through Coder is a testament to that, must be driving them nuts if they intend D8 to pass Coder.

RE: @file blocks, yeah, I removed the periods at the end of the lines

RE: arrays, it works both ways, if its a yes/no answer then there are no more possibles unless you are going to included a 'maybe' option sometime, lol.

RE:comment indentation warning.
previously it didnt like the comment being before the else statement as such..

  }
  // Flat image without link.
  else {

So I copied and pasted the comment to inside the else as such...

  }
  else {  
  // Flat image without link.

... and there was my mistake, trapped by the copy/paste. It should have had the extra 2 spaces indented as it was now inside the else statment, so now it is...

  }
  else {  
    // Flat image without link.

RE:OS's, I have a box sitting in the corner that I have intended sticking Linux on for sooooo long and using it as a dev server, just never got round to it. Previously had a dual boot with Fedora but after putting a fatter hard disk in last year havent got round to re-installing it yet.

Latest commit to http://drupalcode.org/sandbox/raynimmo/1326130.git/commit/d3bea01

raynimmo’s picture

raynimmo commented

Right, newlines.

There is a bit of a discussion all around the place with regards to the exact newline characters at the end of the files. It seems that drupalcs and PAReview are *possibly* incorrect on their implementation of the newline-character-at-end-of-file check as elaborated on by TR in this thread comment.

As he has said, check the core files, which I have; and they are the same line ending as what I now have on my files. :)

If you are running drupalcs or PAReview against my files and it doesnt pass the newline test then please dont bother posting the results, I am aware that they will not pass, I have tested it myself with the most recent versions.

Awaiting clarification on this issue but in the meantime, latest commit is at http://drupalcode.org/sandbox/raynimmo/1326130.git/commit/3a1aa28

raynimmo’s picture

raynimmo commented
Issue summary: View changes

clarifying the git clone path

raynimmo’s picture

raynimmo commented

Right, regardless of the outcome of the 'is PAReview correct?' discussion, I have reverted my latest commit to having the so called 'double newline' end of file.

latest push at http://drupalcode.org/sandbox/raynimmo/1326130.git/commit/6673f56

doitDave’s picture

doitDave commented
Status: Needs review » Needs work

Literally no more style issues from the scripts. So back to real reviewing FTW!

Code:

  • .module, line 154: This results in my german interface as follows:

    Aktuell powered by icon used

    As you, AFAIR, do have some touchpoints to german, you will most likely notice there is something odd. Thus, translation should not be like

    t('Current') . ' <strong>' . t('powered by') . '</strong> ' . t('icon used');

    but instead like

    t('Current <strong>powered by</strong> icon used');

    Or, to speak generally, translatable strings should not be splitted, but preserve as much of the context as possible. (See http://drupal.org/node/322774 for more.)

  • jQuery.fn.enable, jQuery.fn.disable: Sure you want to do this in such a global namespace? Aside of that: Glad to see you are now deeply inside the Drupal JS API :-)

Usability/UI:

  • In admin/settings/picon, you present a list called "images available". Some images are listed there and they all are clickable (by source code, they appear to be an input (type=image)). If I click them, nothing happens while I would expect clicking means selecting. Why is that?
  • When I change Set the image as a clickable link to no, the The TITLE text attribute of the link becomes disabled. So far, so good. But changing back to yes actually changes nothing. Most funny at all: Saving with option no and THEN changing back to "yes" works and enables the text field again. Try it out.
  • Why do you override the default fieldset title css? Probably not everyone will like that (especially not themers ;)).

Your way of dealing with the theme registry and related stuff is definitely worth being dissected in some theme( functions) related documents. Nice stuff, I will keep that as a cheat-sheet.

doitDave’s picture

doitDave commented

Oh, one more thing: Is there a deeper sense in initializing your variables like

variable_set('picon_title', 'Powered by Drupal, an open source content management system');

in your install file and then, when retrieving the value (only at one place, as far as I see), doing it like

$title_text = variable_get('picon_title', NULL);

in your module? Why not just change your module e.g. at line 310 to

$title_text = variable_get('picon_title', 'Powered by Drupal, an open source content management system');

and drop the initialization? I know (and strongly support) the revised and stricter variable handling as intended in D8, but currently your only achievement is more code (unless I miss something).

raynimmo’s picture

raynimmo commented
Status: Needs work » Needs review

Thanks for that Dave, much appreciated that someone actually took the time to review my code and offer an opinion (even if it is d.o's latest authorized contributor :) well done, congrats).

I saw your rather thorough review last night but it was quite late and I really didnt have time to get into it then. Had a 'client work' day yesterday and then spent the evening re-tooling my machine, updated PHP to cope with D8, ended up doing the lot, Apache, MySQL etc also. Then ripped loads of handy packages along with PHP_CodeSniffer from the PEAR repo's which took a while as we live in the jungle and there aint no telephone cables in the jungle, my internet is an EDGE connection so as you can image it took a while!. Anyway, all that just to get drupalcs running, yay! no more surprises after uploading.

Right on to business...

You brought up some really good points and made me think about a few other things, your comments actually prompted me to rewrite my javascript and hopefully it has streamlined it a bit more.

RE: translation
I have no doubt read a bit too much into the 'remove html tags from translatable strings' ethos. I actually think I saw someone last week getting told by a reviewer to remove them and thus it prompted me to remove them from mine. After having a read of http://drupal.org/node/322774 I now see that I was rather over zealous with my implementation. Have reworked the translation strings, now peppered with a slight flavour of html.

RE: jQuery extended functions
Those are a few snippets I have used now and then on different projects, I suppose if you are in control there is not much of a problem, but as you say I wouldnt want to pollute the namespaces of the users of this module. Their removal has allowed me to simplify it anyway and thus reduce the lines of code significantly for that area, previously where I was using..

/*
 * Extend jQuery to include a disable() function.
 */
jQuery.fn.disable = function() {
  return this.each(function() {
    if (typeof this.disabled != "undefined"){
      this.disabled = true;
    }
  });
}

/*
 * Extend jQuery to include an enable() function.
 */
jQuery.fn.enable = function() {
  return this.each(function() {
    if (typeof this.disabled != "undefined") {
      this.disabled = false;
    }
  });
}

I have now implemented a simple toggle function that takes an elements ID attribute and a TRUE/FALSE flag, as such..

/*
 * Function to toggle disabled attribute of html elements.
 *
 * @param el.
 *   Contains ID attribute of HTML element.
 *
 * @param task.
 *   Contains boolean value for switching toggle.
 */
function disable_toggle(el, task) {
  if (task){
    $(el).attr('disabled', false);
  }
  else {
    $(el).attr('disabled', true);
  }
}

and this is now called by the use of disable_toggle(title_el, false);.
simple yet effective.

I have also reworked the radio buttons enable/disable routine and abstracted the variables to cut down on repitition and make the code a bit more legible. In doing so I have managed to get to the cause of the incorrect selections.

Whilst reworking my JavScript I noticed that the console was reporting an undefined error for Drupal.settings.picon but only when the 'append to body' function was not in use. It didnt actually crash anything and the functionality remained stable but I have now implemented a check for this variable being defined to avoid the error warning.

if (typeof Drupal.settings.picon != 'undefined') {
  //do magic here
}

seems to have done the trick.

RE: 'images available' image-buttons
I am struggling to find a form element that I can use to display an image, not for selection (although that may be in a future version) but just to allow the admin to view the actual images available. The only element within the Forms API that I could work with was the Image Button, all of which were disabled anyway so clicking has no effect. I have now also added a CSS rule to override the default hover icon so that it doesnt appear 'clickable'. I was also using this for the 'current image used' image at the top also, have changed both.

If anyone has any advice on a more suitable element to use within this area then I would appreciate the advice.

Radio buttons enable/disable
As I said previously, have rewrote that section entirely, it now performs as it should.

default fieldset CSS overrides
I would firstly describe myself as a frontend guy rather than backend (no pun intended) and usually the fieldset legends are one of the first things that I head for, I just cant stand that text floating in the middle of the line with no defined separation. Old habits die hard, have removed the CSS formatting and leaving it up to the default site style.

RE: variable initialization
Thats a good point you raised there, not really much point in the picon_install() if they can be set as defaults if no value exists, have taken your advice on board and made some appropriate changes. Although they do get initialized in 2 separate places, lines 108-114 in picon_admin_settings() and at lines 307-313 in picon_system_powered_by(). Not too sure if I should separate them out into a function of their own such as picon_define_variables() as it wouldn't really cut down on the code amounts, if anything it would increase it as I would still have to assign each variable at those points in the functions. If anyone has any thoughts or opinions on this it would be most appreciated.

As you may have assumed, Coder reports nothing and drupalcs just smiled and said commit already will you :)

If anyone has any thoughts or opinions on anything here I would really appreciate the feedback.

Latest commit to http://drupalcode.org/sandbox/raynimmo/1326130.git/commit/39b2f2e

raynimmo’s picture

raynimmo commented
Status: Needs review » Needs work

Got a bit of a bug with regards to the theme folder image detection. Changing status until I have time to have a look at it and sort it out.

raynimmo’s picture

raynimmo commented
Status: Needs work » Needs review

Managed to find some time today to have a delve into recent problems. The other day I discovered that I had lost the functionality to read the available images contained within the active themes /images/icons/ folder.

I have re-written a few sections relating to where there are filenames used to hep differentiate when the sites /misc/ folder is used against the active themes images.

All working nice now :)

Checked with Coder (D6 & D7) and drupalcs, all passed OK.

Latest commit is at http://drupalcode.org/sandbox/raynimmo/1326130.git/commit/a26adaf

raynimmo’s picture

raynimmo commented
Issue summary: View changes

temporary message added to stop people from wasting their time running code reviews as I know it wont pass but I believe the code is good.

raynimmo’s picture

raynimmo commented
Priority: Normal » Major

Its been a long 2 weeks since this was last reviewed by anyone, have set the priority to 'major' in line with the guidelines for project reviews

6.x-1.x repo is at http://drupalcode.org/sandbox/raynimmo/1326130.git/shortlog/refs/heads/6...

raynimmo’s picture

raynimmo commented

The latest version of Drupal Code Sniffer has found some code style issues that were previously undetected.

--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
136 | ERROR | AS keyword must be lowercase; expected "as" but found "AS"
--------------------------------------------------------------------------------

Have changed and committed. Latest commit is at http://drupalcode.org/sandbox/raynimmo/1326130.git/commit/a476127

I would really appreciate if someone could have an in depth look at this, its now been over 3 weeks since anyone actually commented and since it seems to have already had manual reviews by Klausi, jthorson and doitDave I see no reason why someone cant bump this at least to an RTBC and then hopefully the git admins will at least have a look at it.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Priority: Major » Normal
Status: Needs review » Needs work

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

  • Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards).
    
FILE: ...ce/drupal-7/sites/all/modules/pareview_temp/test_candidate/picon.module
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
  65 | ERROR | @return data type must not contain "$"
 389 | ERROR | @return data type must not contain "$"
--------------------------------------------------------------------------------

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. Go and review some other project applications, so we can get back to yours sooner.

manual review:

  • README.txt should not contain \feff as first character, see http://drupalcode.org/sandbox/raynimmo/1326130.git/blob/refs/heads/6.x-1...
  • Same for picon.info and picon.js and picon.css. Please check all your files.
  • picon_admin_settings_submit(): this is not a hook, it is a form submission handler. See http://drupal.org/node/1354#forms
  • "Implements theme_system_powered_by().": what does that mean? This is not a hook?
  • picon_theme(): $templates? This function should return the name of your themable item and the argument list. See http://api.drupal.org/api/drupal/developer--hooks--core.php/function/hoo... . Why can't you replace the system default theme function with your own in picon_theme_registry_alter()? Maybe I miss something here.
  • picon_admin_settings(): the definition of $i and the usage is far away from each other. Also: is this variable still needed? And $i is a bad name, use self explaining names like $option_count etc.
raynimmo’s picture

raynimmo commented
Status: Needs work » Needs review

Thanks for that Klausi, much appreciated to have somebody run the rule over my module after all this time.

That Drupal Code Sniffer warning was an odd one? my own version (which I updated today) didnt catch that and neither did patrick's online version. Anyway, I have removed the offending $ symbols from the comment declaration.

Not quite sure what was going on with the zero width whitespace marker(\feff) although I have been messing with filetypes (unix/windows) and different text editors recently when combating the newline at the end of files saga. Have re-saved the files and it doesnt show up in the most recent commit though.

RE: picon_admin_settings_submit() documentation, my mistake, have reworded it.

RE: theme_system_powered_by(): you would be correct there, it is not a hook, its a function, again, my mistake. Have re-worded the documentation inline with its original form as found in system.module.

RE: picon_admin_settings(): the definition of $i : originally when I was using a simple for-loop construct this was employed but since changing that to a foreach loop this variable is no longer required, have removed all instances of it.

RE: picon_theme() and picon_theme_registry_alter() : no matter what I do when I attempt to set the registry value within the _alter() function then it breaks a few things within the layout and doesnt actually place the icon as it should. As far as I am aware the only way to set a theme_registry value is using the theme() function. I have searched for alternate methods to do this but to no avail.

If ANYONE has any advice on this then please leave a comment.

Setting to needs review to allow for further reviewers to offer their comments on this but I do intend to hunt down an alternate method.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

Regarding @return: you should place the return data type there, not the internal name of the returned variable. E.g. string, int, bool, array ...

raynimmo’s picture

raynimmo commented

yeah, was a late night last night, I read incorrectly into your comments and the correct return documentation.

Have fixed and recommitted, latest commit is at http://drupalcode.org/sandbox/raynimmo/1326130.git/commit/974b1f0a65929e...

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Reviewed & tested by the community
  • picon_admin_settings(): you could use system_settings_form(), so you would not need picon_admin_settings_submit().
  • picon_admin_settings_submit(): $opento is not defined.
  • "t('@alt_text', array('@alt_text' => $alt_text))": the placeholder does not make much sense here. Also, alt text and title are run through check_plain() anyway in theme_image(), so you are double escaping here.
  • Not sure about the theme registry alter problem, I don't have much experience in that area so I can't help you here.

But that are minor issues, otherwise this looks RTBC to me.

raynimmo’s picture

raynimmo commented

Santa Claus was quite good to me this year, but Santa Klausi was even better :) Thanks for the RTBC, much appreciated, lets hope the Git Admin's thinks so too.

RE: picon_admin_settings()
I have been reading up on system_settings_form() and it does seem like it will do what I need. Will try and have a look over that in the next few days.

RE: $opento is not defined
Yeah, thats a slight slip on my part there, I have now defined the variable further up the function and used that variable to then populate the variable_set() call later also.

RE: double escaping
Thats a good part of the 'learning process' from in depth reviews, it forces you to read a few pages from the API and learn something new. the API function theme_image() does in fact use check_plain() as Klausi so kindly pointed out. Thanks for the advice, have reworked that line.

RE: theme registry alter problem
I wouldnt say it was a problem, possibly a slight misuse of the APi but no more :)

I have been reading up on different methods and I think I may be able to cut a few corners in my processing if I use hook_init() to set up my javascript and css files and also set up some of the page elements. I still have to read up a bit more and try a few things out before I change anything. Hopefully I can slot this in as a feature request for version 2.x and the Git Admins will push it through in its current working state :)

Have pushed a commit that deals with the 'double escape' and undefined variable errors pointed out by Klausi. I should hopefully have time to look into his other comments over the next few days.

Latest commit is at http://drupalcode.org/sandbox/raynimmo/1326130.git/commit/0e9de680541786...

raynimmo’s picture

raynimmo commented

So it seems that last commit contained a few errors. It had a single 'whitespace-at-end-of-line' and a 'space-before-closing-parenthesis' errors.

Both have been corrected and the latest commit is at http://drupalcode.org/sandbox/raynimmo/1326130.git/commit/736e1e3

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Reviewed & tested by the community » Fixed
raynimmo’s picture

raynimmo commented

Initially I was a bit perplexed by the change of tag to 'fixed' although thought nothing of it and waited for someone to inform me of being granted Git access or for someone from the security team to rip my code apart and set it back to 'needs work'.

Then a few days later when I visited my project sandbox page, I noticed a link that wasnt there before that said 'promote to full project', yay!, then I realised that this had passed the RTBC status and could now be promoted and that I had earned Git access.

Thanks loads for that kiamlaluno, much appreciated, although you could maybe be a bit more clear about your actions to the uninitiated, lol.

I have now promoted this project and created a 6.x-1.0 release which can be found at http://drupal.org/project/picon

I would also like to take this opportunity to heartfully thank everyone who helped to guide me through this process, especially klausi and doitDave for your guidance, suggestions and valuable comments, to jthorson for being the only person who ever actually answered my questions in IRC and to attiks for your initial offerings when I started out on this path.

Think I will go for a beer now to celebrate :)

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

(not verified) commented
Issue summary: View changes

Remove test warning as it now passes those tests