[D6] ImageLink

Greetings, sorry it has taken so long for someone to get to your application. There is a bit of a backlog.

Running module through Coder

Please run code through Coder set on minor and address any issues. It's mostly just spaces and comment blocks.

README.txt

needs to be fleshed out to explain about the module a bit more
See Best practices for creating and maintaining projects

.install file

• missing any information in @file block
• Function comments should be in PHPDoc/Doxygen style
• Weight should be based off something rather than an arbitrarily large number. Are there any modules you know of that use hook_nodeapi('view',..) to alter links like this that you have to be after?
• During uninstall, why are you changing the weight of the module? It should no longer be used at all and makes no difference to the running of the site.
• no EOL character at end of file
• should inform user that it has been installed/enabled since there is no other outwards indication that the module is enabled and working except for hunting down a previously unlinked image in their posts.

.module file

• missing @file block
• Why are you adding 'absolute' => TRUE to the URLs? This will interfere with sites which have already taken the time to deal with this themselves. Relative should work just fine.

git branching

Everything in master branch. This is will not allow releases to be created. When you believe you have a release ready; please branch to allow the creation of a release as per Release naming conventions

coder

you have to set it to "minor" to catch all the foibles .. it's in the settings once the page has come up telling you there are no errors.

git branching

master branch no longer in use? Don't forget to follow the cleanup step in step 5 of Moving from a master to a major version branch. I recommend against the "removing remote master" path, but instead just cleaning out the master and pointing people to the correct location in the README.txt file.

git branch naming

oops .. imagelink-6.x-1.0 isn't a release branch name! 6.x-1.x is, 6.x-1.0 is a release tag, "imagelink-6.x-1.0" is just branch with an unparseable name :(

haven't looked at the rest..

• "// find all img tags in content": All comments should start capitalized and end with a "."
• "foreach ($imgs as $key => $img) {": if you don't use $key remove it.
• $imagelink = '<a href="' . $href . '" title="' . $node->title . '">' . $img . '</a>';: you need to sanitize the node title before you output it in the markup. Otherwise you are vulnerable to XSS, see http://drupal.org/node/28984

README.txt

In the master's version of README.txt, you would put some text telling people who download it what branches are available. It's only for people who pull your project via git instead of the releases.

I usually put something like ..

+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
+ Where is the module?                                                        +
+=============================================================================+

Since branching to support D6 and D7, the master branch that you are looking at
has been cleared of content to avoid old code being used.

If you did a full git clone, then you should have all the code hidden away in
two additional branches:

- 6.x-1.x
- 7.x.1.x

While "git branch" will only show locally tracked branches, in this case
master, the other branches are hidden in the .git repository.

Running "git branch -a" will show something like this:
* master
  remotes/origin/6.x-1.x
  remotes/origin/7.x-1.x
  remotes/origin/HEAD -> origin/master
  remotes/origin/master

To checkout one of those branches so you can use the code, type:
git checkout -b 7.x-1.x remotes/origin/7.x-1.x

This created a local tracking branch called 7.x-1.x which tracks the remote
remotes/origin/7.x-1.x branch. You will also now have all the files to use the
git version of the module.

Have fun!

The README.txt file doesn't have an EOL character at the end of it.

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.

(not a problem)

I had to go check what a plain_text() function was in the code - check_plain() is the actual function that you used (and the right one).

git tag

Also, now that you've done a few more commits, don't forget to move your tag (since no one has really used it) to the actual release planned release, or tag it with a new one. This is often why people do RC versions before finally committing to a release.

git commit comments

For the standard format, please see
Commit messages - providing history and credit.

I think we're at the end of finding any issues with this project. Since this is quite a simple module and you have a few other projects, I'd like to look through any appropriate ones too as this application queue is not only about making a single project good, but making all future full projects you wish to create in the future as good as they can be.

Are any of the following projects that you are thinking of releasing, or just sandbox play things that should be ignored?

• git clone http://git.drupal.org/sandbox/lugir/1247188.git cinjector
• git clone http://git.drupal.org/sandbox/lugir/1242666.git comment_name
• git clone http://git.drupal.org/sandbox/lugir/1166898.git publisher_check
• git clone http://git.drupal.org/sandbox/lugir/1170452.git taobaoke

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/imagelink.module:
   +11: [minor] Comment should be read "Implements hook_foo()."
  
  Status Messages:
   Coder found 1 projects, 1 files, 1 minor warnings, 0 warnings were flagged to be ignored
  

• ./imagelink.module: comment lines should break at 80 characters, see http://drupal.org/node/1354#general
  

        // If paging module is enabled, then working on $node->pages than $node->body.
          // Add absolute link to image (people will get a break link in some rss reader if use a relative link here).
            // else if 'paging' module is enable, add links to images in $node->pages
  

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

manual review:

• "$imagelink = '<a href="' . $href . '" title="' . check_plain($node->title) . '">' . $img . '</a>';": you should use the l() function to create links.
• "$replacement = '<a href="' . $href . '" title="' . check_plain($node->title) . '">$0</a>';": same here

But that are minor details, otherwise RTBC for me.

• why did you switch to react on node load and not on view? I think "view" is more correct as you only want to alter the data when it is displayed, not when it is loaded for some different purpose.
• "'title' => check_plain($node->title),": no need to use check_plain() here, the l() function will use drupal_attributes() which will check_plain() all attributes anyway.
• "'attribute' => array(" should be "'attributes' => array("
• "$replacement = '<a href="' . $href . '" title="' . check_plain($node->title) . '">$0</a>';": why can't you use the l() function and pass $0 as the first text parameter?

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:

• 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/imagelink.module:
   +11: [minor] Comment should be read "Implements hook_foo()."
  
  Status Messages:
   Coder found 1 projects, 1 files, 1 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
  

  ./imagelink.module:51:    "(" .                     // Group 1 - Linked Images.
  ./imagelink.module:52:      "<a[^>]+>" .            // Start with <a>,
  ./imagelink.module:53:        "(?:(?!<\/a>).)*?" .  // Not </a> until ...
  ./imagelink.module:54:          "<img[^>]+>" .      // <img> tag,
  ./imagelink.module:55:        ".*?" .               // Any thing until ...
  ./imagelink.module:56:      "<\/a>" .               // </a>
  ./imagelink.module:57:    ")" .                     // Group 1 END.
  ./imagelink.module:59:    "|" .                     // OR
  ./imagelink.module:61:    "(" .                     // Group 2 - Unlinked Images.
  ./imagelink.module:62:      "<img[^>]+>" .          // <img> tag
  ./imagelink.module:63:    ")" .                     // Group 2 END.
  

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

HTH, dave

Manual review:

• Yes, you are right, there is no file. Overlooked that. There are weird results concerning newlines sometimes, no one yet figured out why. I propose that you carefully check by re-downloading a snapshot of your latest commit from the repo viewer and if there are single \n then it's ok (as for me).
• Regarding your repo: There are several tags and D6 branches, I suggest cleaning all that up to avoid confusion (you will create tags later when releasing in the full project).
• imagelink.install: I understand that you want your module to act later than some unspecified other module. What if, for any reason, some module installs itself with a weight of e.g. 6 and adds further images? The module weight thing is somehow tricky, so I personally would never use absolute values just out of guessing like "seems high enough", you know what I mean?

hth, dave

Hi, I just updated my review environment and re-run it over your file. Only some small things showed:

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

• Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards):
  

  
  FILE: ...b/dp709/sites/all/modules/pareview_temp/test_candidate/imagelink.module
  --------------------------------------------------------------------------------
  FOUND 12 ERROR(S) AFFECTING 12 LINE(S)
  --------------------------------------------------------------------------------
   64 | ERROR | String concat is not required here; use a single string instead
   65 | ERROR | String concat is not required here; use a single string instead
   66 | ERROR | String concat is not required here; use a single string instead
   67 | ERROR | String concat is not required here; use a single string instead
   68 | ERROR | String concat is not required here; use a single string instead
   69 | ERROR | String concat is not required here; use a single string instead
   70 | ERROR | String concat is not required here; use a single string instead
   71 | ERROR | String concat is not required here; use a single string instead
   73 | ERROR | String concat is not required here; use a single string instead
   75 | ERROR | String concat is not required here; use a single string instead
   76 | ERROR | String concat is not required here; use a single string instead
   77 | ERROR | String concat is not required here; use a single string instead
  --------------------------------------------------------------------------------
  

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.

HTH, dave

This module used to be almost RTBC. Somehow it got complicated and very inefficient.

If you want to know if something as simple as "<img" is in the body, use strpos or stripos. These are much faster and use much less memory than preg_match_all for something so simple.

The pattern used for the second call should not be split over multiple lines and using double quotes. That's a double whammy and just insanely inefficient for something that will be called for every single node to be viewed. Think of views.

You're using url and l() together when a single call to l() would suffice. l() actually calls url itself. Using alias = TRUE means that it will only return "node/N" style links which isn't very good for SEO either (which seems to be a goal of the module).

Wowser. The number string replacements, regular expressions and string building used here is very large for this task.

I went a little nuts; here's your module re-written into a single function, with only one expensive function call. O(n) instead of O(5n). I'm sure the regex can be tweaked to make it only return the desired unlinked tags which would simplify the internal function to be without the condition.

function imagelink_nodeapi(&$node, $op, $teaser = NULL, $page = NULL) {
  if ($op == 'view') {
    if (stripos($node->content['body']['#value'], '<img ') !== FALSE) {
      $node->content['body']['#value'] = preg_replace_callback(
        '/(<a[^>]+>(?:(?!<\/a>).)*?<img[^>]+>.*?<\/a>)|(<img[^>]+>)/si',
        create_function(
          '$matches',
          'if (isset($matches[2])) {
             return l($matches[2], \'node/' . $node->nid . '\', array(
               \'attributes\' => array(
                 \'title\' => \'' . $node->title . '\',
               ),
               \'html\' => TRUE,
               \'absolute\' => TRUE,
             ));
           }
           else {
             return $matches[0];
           }'
        ),
        $node->content['body']['#value']
      );
    }
  }
}

Hmm... I think I might have just negated the ability to review this module.

Update: didn't check stripos against !== FALSE.

The code I posted was actually a complete replacement for all the code in the module. It wasn't meant to be split into two different functions as it does all the checking in one go.

Also, in posting that I kinda removed anything I can review. I guess I recuse myself from being able to review this application. This module is also pretty small now which doesn't give much for anyone else to review in combination with me rewriting it.

How about changing this over to the http://drupal.org/sandbox/lugir/1170452 project? It looks like you've been following all the suggestions for this module over there (except for version branching .. there's code in the master and the 6.x-1.x branch is called 6.x-1.x-dev which is not a valid branch name for releases .. and having old code in the module commented out .. and setting variables in install instead of as default values, and not deleting variables the correct way .. and INSTALL file is missing .txt extension .. and the admin form should just use drupal_get_form instead of a wrapper function .. and .. well it would be easy to fix up ;) It's a much more substantial module.

There is a single project promote available which we're trying to use for useful projects which are quite small, or are features modules. That process would make this project a full project, but not provide the ability to promote other projects from sandbox to full projects. By the sounds of it, this seems very much like it would suit this module. You also then get the experience running a full project for the next app.

If I get a few spare moments, I could run through Taobaoke with a bit more of a fine tooth comb. I'm sure that one could be made ready to breeze through here with a few changes.

It appears that there have been multiple project applications opened under your username:

Taobaoke: http://drupal.org/node/1176740
ImageLink: http://drupal.org/node/1275104
Comment Name: http://drupal.org/node/1242806

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue.

With this in mind, I have marked your secondary applications as 'closed(duplicate)', and left one application open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one I that I have left open, then please feel free to close the 'open' application as a duplicate, and re-open one of the other project applications which I had closed.

Thanks in advance for your patience and understanding!

Update description