"Docblock should be immediately above" - Carriage Return Line Feed problems

This bug still exists in 2.x.

This patch is part of the #1day1patch initiative.

Confirming patch works.

Hi Guys

I have spent two to three hours tracing this issue because i was considering it is my module code formatting issue then i debug coder module there i found bug in coder module, here is few important info.

OS: Window 7
PHP: 5.4.3
drupal : 7.23
coder: 7.x-2.0
coder file :sites\all\modules\coder\coder_review\includes\coder_review_comment.inc and line 271
SNIP:

if (!isset($alllines[$lineno - 1]) || substr($alllines[$lineno - 1][0], -2) != '*/') {

The following portion of the code * substr($alllines[$lineno - 1][0], -2)* returning only back slash "/" but actually there is "*/" then i printed the last two characters of the ASCII code then it returned two ascii code values
0 => /, ASCII ===>47
1 => , ASCII ===>13
It means that substr also considering enter value as character which we cannot see as normal routine so that at that stage our condition statement goes to false and we see warning messages on coder review page.

Solution
we can solve this issue with rtrim or trim functions.
rtirm

if (!isset($alllines[$lineno - 1]) || substr(rtrim($alllines[$lineno - 1][0]), -2) != '*/') {

or
only trim

if (!isset($alllines[$lineno - 1]) || substr(rtrim($alllines[$lineno - 1][0]), -2) != '*/') {

Your patch is identical to the one in #2.

yes you are right.

Just to confirm coder-docblock-immediately-1984908-5.patch is working great for me.

@stephenhendry thanks for confirmation.

I discovered this error when I switched to using Git for some of my modules. Previously they did not have this problem, so it was caused by the CRLF setting which Git introduced. If I change a file's CRLF setting from Windows back to Unix then it fixes the problem, and I also discovered the unprintable character being counted in the substring. Hence I agree with the diagnosis above.

I also found other coder_review warnings incorrectly displayed, due to the same cause (that CRLF are considered to be real characters at the end of the line). In addition, when a file has this problem then the tests for trailing spaces do not work, as the blanks are not 'at the end' of the line.

Even though the patch files in #2 and #6 fix this individual error I think the problem is bigger and should be solved in a way which fixes it for all these issues.

Jonathan

The same cause also creates the following incorrect result:

@see references should be separated by "," followed by a single space and with no trailing punctuation (Drupal Docs) [comment_comment_see_sep]

when the @see reference is correct.

The code which detects a newline and pre-processes the content currently only detects '\n' but to cater for both type of line end, we also need to look ahead to the next character and test for '\r\n'. This covers both of the errors and also future-proofs it for new checks which might use the same logic.

With the new code, the following test function was used to create the debug below:

/**
 * Test.
 */
function coder_testing_function1() {
  return;
}

Linefeed set to UNIX LF

$pos = 28 $lineno = 8 $char = ascii 47 = / $next_char = ascii 42 = *
$pos = 30 $lineno = 8 $char = ascii 42 = * $next_char = ascii 10 = \n (LF)
$pos = 31 $lineno = 8 $char = ascii 10 = \n (LF) $next_char = ascii 32 =  
found new line
$pos = 32 $lineno = 9 $char = ascii 32 =   $next_char = ascii 42 = *
$pos = 33 $lineno = 9 $char = ascii 42 = * $next_char = ascii 32 =  
$pos = 34 $lineno = 9 $char = ascii 32 =   $next_char = ascii 84 = T
$pos = 35 $lineno = 9 $char = ascii 84 = T $next_char = ascii 101 = e
$pos = 36 $lineno = 9 $char = ascii 101 = e $next_char = ascii 115 = s
$pos = 37 $lineno = 9 $char = ascii 115 = s $next_char = ascii 116 = t
$pos = 38 $lineno = 9 $char = ascii 116 = t $next_char = ascii 46 = .
$pos = 39 $lineno = 9 $char = ascii 46 = . $next_char = ascii 10 = \n (LF)
$pos = 40 $lineno = 9 $char = ascii 10 = \n (LF) $next_char = ascii 32 =  
found new line
$pos = 41 $lineno = 10 $char = ascii 32 =   $next_char = ascii 42 = *
$pos = 42 $lineno = 10 $char = ascii 42 = * $next_char = ascii 47 = /
moved to $pos: 43
ended $this_comment_lines:  */
$pos = 44 $lineno = 10 $char = ascii 10 = \n (LF) $next_char = ascii 102 = f
found new line
$pos = 45 $lineno = 11 $char = ascii 102 = f $next_char = ascii 117 = u
$pos = 46 $lineno = 11 $char = ascii 117 = u $next_char = ascii 110 = n

Linefeed set to Windows CRLF

$pos = 35 $lineno = 8 $char = ascii 47 = / $next_char = ascii 42 = *
$pos = 37 $lineno = 8 $char = ascii 42 = * $next_char = ascii 13 = \r (CR)
$pos = 38 $lineno = 8 $char = ascii 13 = \r (CR) $next_char = ascii 10 = \n (LF)
found new line
$pos = 40 $lineno = 9 $char = ascii 32 =   $next_char = ascii 42 = *
$pos = 41 $lineno = 9 $char = ascii 42 = * $next_char = ascii 32 =  
$pos = 42 $lineno = 9 $char = ascii 32 =   $next_char = ascii 84 = T
$pos = 43 $lineno = 9 $char = ascii 84 = T $next_char = ascii 101 = e
$pos = 44 $lineno = 9 $char = ascii 101 = e $next_char = ascii 115 = s
$pos = 45 $lineno = 9 $char = ascii 115 = s $next_char = ascii 116 = t
$pos = 46 $lineno = 9 $char = ascii 116 = t $next_char = ascii 46 = .
$pos = 47 $lineno = 9 $char = ascii 46 = . $next_char = ascii 13 = \r (CR)
$pos = 48 $lineno = 9 $char = ascii 13 = \r (CR) $next_char = ascii 10 = \n (LF)
found new line
$pos = 50 $lineno = 10 $char = ascii 32 =   $next_char = ascii 42 = *
$pos = 51 $lineno = 10 $char = ascii 42 = * $next_char = ascii 47 = /
moved to $pos: 52
ended $this_comment_lines:  */
$pos = 53 $lineno = 10 $char = ascii 13 = \r (CR) $next_char = ascii 10 = \n (LF)
found new line
$pos = 55 $lineno = 11 $char = ascii 102 = f $next_char = ascii 117 = u
$pos = 56 $lineno = 11 $char = ascii 117 = u $next_char = ascii 110 = n

The attached patch was against 7.x-2.2 but it should also apply ok to the dev version.

Jonathan

Has anyone tried my patch in #14? I am finding it so much more helpful with code reviews now, with all the incorrect messages gone I can see the real problems.

Also has anyone checked D8? I do not know if this problem exists there. If so I guess I should download the 8.x dev and make the changes there first.

Jonathan

FYI, this is still an open issue in Drupal 7.29 using coder 7.x-2.x-dev.
None of these patches succeed with 7.x-2.x-dev (or 7.x-2.2).
Executing patch < 1984908_-14.coder_crlf.patch produces the following output:
Hunk #1 FAILED at 447.
Hunk #2 FAILED at 507.
Hunk #3 FAILED at 522.

Thanks for testing this, and sorry to hear it does not work for you.

I guess the dev module has moved on, so I will re-cut the patch.

I have just checked the patch in #14 and it applies cleanly to the latest dev, 7.x-2.x-dev, dated 2014-06-01

mac2012:~ jonathan$ cd /Library/webserver/documents/drupal7/sites/all/modules/coder
mac2012:coder jonathan$ 
mac2012:coder jonathan$ patch  -p1 -Vt < 1984908_-14.coder_crlf.patch
patching file coder_review/coder_review.common.inc
mac2012:coder jonathan$

Your 'failed at' line numbers are slightly different to what they should be, so I think you must have an altered version of the code.

Jonathan

I've been trying to get a D8 version of Coder Review working, specifically to test and create a patch for this issue, as per my patch in 14. However the module is missing all of the menu and routing .yml files, so is not currently useable. Is anyone else working on this?

I presume that any issue patches should be made to the D8 code-base first, but as it's not currently operational that is a bit hard to do. Just wondering what the plan was, ie are we going to re-cut the entire D8 code base at some point, based on the latest D7 at that time?

Jonathan

Just checked and the patch in #14 still applies ok to 7.x-2.3 and to the latest 7.x dev of 29th Nov. Please can someone confirm it solves the CR/LF 'Docblock should be immediately above ...' problem, then the maintainers can commit it. Thanks very much.

My patch in #14 still applies ok at 7.x-2.4

Confirming that patch #14 still applies and fixes the issue.

Thanks for testing Ozmodiar. If you are happy this fixes the problem please could you mark it RTBC.
The patch is #14 still applies at the new release 7.x-2.5. Would be nice to get it committed :-)

Committed, thanks!

Great to have this fixed. Thank you for committing my patch and for giving me the authorship credit. Cheers!
Jonathan