JustBlackBird / gulp-phpcs

Gulp plugin for running PHP Code Sniffer
MIT License
47 stars 12 forks source link

Newlines introduced? #4

Closed mackensen closed 9 years ago

mackensen commented 9 years ago

I'm using this module with the Drupal coding standard (available as part of the coder module). Drupal standards require that all files end a single new line. This is sample output from such a failure:

FILE: STDIN
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 32 | ERROR | Files must end in a single new line character
--------------------------------------------------------------------------------

If I manually check the same file by running phpcs from the command line it validates correctly. I've verified in vim that the file ends with a single newline. My suspicion (caveat, I'm still new to gulp) is that a spurious newline is being introduced somewhere in the stream.

JustBlackBird commented 9 years ago

gulp-phpcs does not modify files in stream, so you need to check plugins that transforms the stream before gulp-phpcs. Also you could try to run only gulp-phpcs to make sure it is the source of the problem.

mackensen commented 9 years ago

I don't believe any other plugin is touching the stream. This is the relevant part of my gulpfile:

// File groups.
var paths = { 
  scripts: ['./js/*.js', '!./js/*min.js'],
  php: ['./*.php', './templates/**/*.php']
};

// Drupal coding standards.
gulp.task('standards', function() {
  return gulp.src(paths.php)
    .pipe(phpcs({
      standard: 'Drupal'
    })) 
    .pipe(phpcs.reporter('log'));
});
JustBlackBird commented 9 years ago

Could you please provide a console command for phpcs that does not reports a error on your code?

mackensen commented 9 years ago

Sure, there's nothing special: phpcs --standard=Drupal page.tpl.php. Here's the file itself: https://gist.github.com/mackensen/22d9aeec172d7f1f218e.

JustBlackBird commented 9 years ago

Actually there is no empty line at the end of the file in the provided gist

mackensen commented 9 years ago

There's been a lot of discussion in the Drupal community and the standard (as I understand it) is that there should be a single new line character, which is present at the end of the line. If you do :set list in vim you see this in line 32: print $feed_icons;$. Adding a blank line means there are actually two newline characters, not one. If you add a new line gulp-phpcs reports the same error, except on line 33 instead of line 32, as does phpcs.

JustBlackBird commented 9 years ago

Frankly, I know almost nothing about real drupal coding standards but traditionally "a blank line" means double \n at the end of the file.

As for the problem, I can reproduce it now. Could you please add the second \n and run the following cat page.tpl.php | phpcs --standard=Drupal?

mackensen commented 9 years ago

Well, I don't much about them myself but I'm getting a quick education!

Very interesting result. Using cat with and without the second \n produced the error both times, albeit with a different line number (as expected). Running the command directly on the file itself only provoked the standards error with the second \n.

JustBlackBird commented 9 years ago

Finally I've traced the problem. It's in the approach used by the drupal's coder module.

It seems that PHP cannot read from stdin more than once. Here the custom phpcs sniff tries to read from stdin, but the later is already emptied by phpcs itself. The sniff gets an empty string instead of a real file contents and fails because an empty string has no \n char the end.

As a result, you cannot use custom Drupal standard and stdin stream together. You've already seen it when ran cat page.tpl.php | phpcs --standard=Drupal. This behavior is definitely a bug of the coder module.

I'm going to close the current issue because the bug isn't in gulp-phpcs. Also I suggest you to open an issue in drupal's coder module queue.

mackensen commented 9 years ago

Thank you for taking the time to run down this issue; I really do appreciate it. Filed bug against drupal_coder here: https://www.drupal.org/node/2365271.