Coder_Review does not recognize @file docblock in .js files

All of the comment rules were being excluded from .js files, so I added a flag to the review array to allow it to add js files to the default allowed extensions.

See http://drupalcode.org/project/coder.git/commit/da2c9e9

I've just encountered this same issue with the JS file I'm writing today.
The version 7.x-2.x-dev of coder doesn't seem to detect @file docblock in .js files although it is there.

I can confirm this error; @file docblock is present, but Coder is still complaining.

I am having the same issue.

It seems that Coder is looking for the opening <?php tag to exist before it will spot the @file block. If you temporarily add a php tag to the .js file this then the @file block is detected ok.

Maybe there needs to be a slight change in the processing rules for .js files?

Jonathan

I'm having the same issue

Thanks for commenting. I know it is an annoying problem, but it is not 'critical'. That priority should only really be used when the code is completely broken and fails to run at all.

Maybe everyone who has reported this since the original fix in #1 could go back and look at the change which Douggreen implemented and try to understand the code and see if we can work out a solution. I'm happy to test and make a patch, but currently I have no background knowledge of Coder Review so it would mean reading the code base and interpreting from scratch.

Jonathan

I am facing the same issue . I could not able to solve this yet.

Thanks

I am experiencing this issue with version 7.x-2.2. The workaround I came up with is to add an opening PHP tag in a comment at the beginning of the file. After adding this line, I got a style_camel_case warning despite the Drupal standard of lowerCamelCase in Javascript. I found that a commented closing PHP tag after the file docblock solved the problem, so I think some part of Coder Review was processing the entire file as PHP, which makes sense. I hope it is appropriate to post a workaround here.

// <?php
/**
 * @file
 * My Javascript file.
 */
// ?>

Yes, that matches my observation in #6 but thanks for posting the explicit code. That does imply that some flag or option is being set when coder detects an opening php tag, even when it is processing a non-php file. I am sure this info will be useful in helping to debug coder.

I found the problem. It was that the working extension $ext was being set to 'php' which then meant that the comments were not being detected (because a <?php was being searched for, which was not going to be found). The solution is quite simple, once I figured it out. The 'js' should be temporarily excluded from the array of allowed extensions when checking if the file is php. So in coder_review.common.inc in function _coder_review_read_and_parse_file() change

$ext = in_array($pathinfo['extension'], $allowed_extensions) ? 'php' : $pathinfo['extension'];

to

$ext = in_array($pathinfo['extension'], array_diff($allowed_extensions, array('js'))) ? 'php' : $pathinfo['extension'];

This results in $ext having the correct value of 'js' for javascript files, then the comments are detected and the test passes.

Removed trailing spaces. We can't have new coder_review lines failing any coder_review tests.

this seems to fix the problem for .js files. just a minor suggestion for the comment...

1. +++ b/coder_review/coder_review.common.inc
   @@ -383,7 +383,10 @@ function _coder_review_read_and_parse_file(array &$coder_args) {
   +  // If the file extension is any of the allowed extensions (other that 'js')
   

   could maybe read better as "extensions (other than 'js')"

Thanks for testing, and well done for spotting that typo! Fixed in the attached patch (which has no other changes).
If you are happy with it, please feel free to mark it RTBC, but would be good for another of the 16 other followers to test it too ;-)
Thanks
Jonathan

this also looks good to me, thanks @jonathan1055.

After applying the patch I get

Notice: Uninitialized string offset: -1 in _coder_review_read_and_parse_file() (line 630 of "PROJECT DIR"/sites/all/modules/develop/coder/coder_review/coder_review.common.inc).

I'm running coder review on a single module, with default coder configuration.

Edit: Changed actual project dir name to "PROJECT DIR"

Can you edit your file coder_review.common.inc and check that line 630 is the line with IF test below:

case ':':
  // Look for separators which force a new quote string.
  if ($this_quote_index < 0 || !empty($this_quote_lines[$this_quote_index])) {
  $this_quote_sep = TRUE;
}
break;

I'm using the stand-alone /coder/ module, ie my path is sites/all/modules/coder/coder_review/ but the code should be the same.

Having looked at this further, I'm not sure it is directly related to the patch, but I'm happy to help anyway.

Before we launch into discussing it, please can you revert the code to the original without the patch and see if you still get the error. If you do, then it would be better to start a new issue, to avoid this one getting side-tracked. Two other people in addition to me are happy that this patch fixes the original problem.

Actually the notice does go away after reverting to original code.

To answer your post in #19:
Line 630 in coder_review/coder_review.common.inc starts the following IF statement:

if ($content[$pos - 1] != '\\') {
    $this_php_lines .= $char;
    $in_quote = $char;
    if ($this_quote_sep) {
      $this_quote_lines[++$this_quote_index] = '';
      if ($char == '"') {
        $this_doublequote_lines[++$this_doublequote_index] = '';
      }
    }
    $this_quote_sep = FALSE;
  }

Using:
Coder version 7.x-2.2
PHP Version 5.5.7
Patch 1834598_15.@file_in_js.patch

Ah, that was worth me asking, as my lines are not exactly the same. OK so it may be relevent to this patch.

Can you isolate what file is being processed when the error happens, and what lines in the file are causing it? Then I can try to replicate the problem.

I've haven't had the time to isolate the file being processed when the notice happens, but perhaps the following details would already give more insight.

The code review is done on an omega based theme (not a module). That seems important, because when running on modules, this notice doesn't appear.
Also, it only occurs after a "drush cc all" (clear all cache).

I've retried reverting to the original code of 7.x-2.2 + clearing cache, after that the notice doesn't occur. So this really does seem to be related to the patch. What strikes me as strange, is that the patch covers a completely different part of coder_review/coder_review.common.inc.

I've narrowed down the notice to a JS file called Gruntfile.js which starts with a ' character. For a JS file this seems valid.
On $pos = 0, this causes a problem because the rest of the code reads $pos - 1. I've attached a patch with an extra check in the IF statement to only continue is $pos !=0.

I'm not 100% sure about the impact of the extra check. What do you guys think?

Changing status to "Needs review" because of altered patch.

That does make sense, because the un-patched code was treating .js files as .php (and hence not finding the docblock). With the patch they are correctly treated as not php. That error existed all the while, but would never occur because anything before the open <?php is ignored.

I have slightly changed your logic from

if ($pos != 0 && $content[$pos - 1] != '\\') {

to

if ($pos == 0 || $content[$pos - 1] != '\\') {

so that we do not ignore quotes at the very start of the .js file

@jonathan1055 You're absolutely right. Good catch!

You're patch however gave a warning while applying: 1834598_26.%40file_in_js.patch:22: trailing whitespace.
That should be fixed in the attached patch.

Sorry about that. I did not run Coder on my code change!

However, it looks like you have not only removed the trailing space but my entire two lines of comment. Here is my patch with the comment, but without the trailing space.

If you and I both agree that this change does the right thing when avoiding the error, and the other folks above have already agreed the original change works, then the issue can be put back to RTBC?

@jonathan1055 Sorry about removing the comments. I meant to fix the patch with the correct code and forgot to copy in your comments.
The patch applies just fine now. For me this is RTBC.

Excellent, thanks very much.

I can see the same problem in the D8 version so that needs to be fixed first. The code looks identical so it should be a simple thing to check.

I have a working D8 site and have enabled the latest Coder module to test my patch, but I cannot find where to run any Coder Reviews. I have no link in the Development block within configuration, as the urls admin/config/development/coder and admin/config/development/coder/review are not found.

What else do I need to do?

Do we actually have a working coder review for Drupal8? I've just read up about how the D7 hook_menu() is being replaced in D8 by .routing.yml, menu_links.yml etc, and we do not appear to have any of this yet in D8. I will do some more research and try to build the new files. Or has anyone done that already and I'm just not looking in the right place?

@fryer
Thanks!
#10 works for me.

Added the following code to the top of the page.

// <?php
/**
 * @file
 * My Javascript file.
 */
// ?>

Coder Review has been removed in 8.x-2.x, see #2354071: Remove coder_review until a new maintainer is found. You can run Coder Sniffer standalone from the command line or in your IDE, see https://www.drupal.org/node/1419988

Thx @klausi for updating this issue.

The patch in #28 still applies cleanly to HEAD for me.
So I guess this is now actually is RTBC and can be committed.

So I guess this is now actually RTBC and can be committed

Yes.
Just checked and the patch in #28 still applies ok to 7.x-2.3 and to the latest 7.x dev of 29th Nov.

Committed, thanks!

Great. Thank you.
Thanks for the commit credit too.