Detect non-unix newlines
Arancaytar - November 11, 2008 - 15:49
| Project: | Project Issue File Review |
| Version: | 6.x-2.x-dev |
| Component: | Code |
| Category: | task |
| Priority: | critical |
| Assigned: | boombatower |
| Status: | closed |
Jump to:
Description
The TestBot has marked #207475: Add a script to run cron on multisite installations as CNW for failing PHP syntax - but the attached patch changes a bash script. Perhaps the syntax checker should be filtered to .module, .php and .inc extensions?
#1
This was debated, but it seems necessary now.
#2
#3
Interestingly if I apply the patch locally and run
php -l ./scripts/cron-lynx.shI get the expected:
No syntax errors detected in ./scripts/cron-lynx.sh#4
I had this problem while developing the system.
I can run the commands in command line or from drupal index.php and get the $output and $status to come out great, but not from inside my code. Instead I get $output always equall to nothing and the status apparently is being 1.
<?phpexec('php -l ./scripts/cron-lynx.sh', $output, $status);
print_r($output);
var_dump($status);
$output = shell_exec('php -l ./scripts/cron-lynx.sh');
echo $output;
?>
#5
#6
Another example: #343426: Getting "PHP syntax error" on valid patch with files added and removed
#7
I had the same problem and converting the patch newline delimiter from CR+LF to LF did the trick (see #338630: Locale is unable to rebuild lost Javascript translation files and #347288: Locale uninstall hook broken).
#8
Both jsomers and I confirmed with some CR+LF -> LF conversions that the same patch then passes the syntax check.
#9
So this is unix vs. windows newline standard?
If so do we have an official standard? If not we either need one of the bots need to be able to convert (handle) both. I would vote for creating a standard if we don't.
#10
Pretty sure this is a windows line endings issue as well. I'd be in favour of adding something to patch/create to document this.
#11
@catch: please do. I'll try and detect this in the next version.
#12
#13
#14
Added client confirmation test patch for this.
#15
This is now included in the review.
#16
Sorry, just clarify, is it already implemented? I have few patches (1, 2) which blocked by this error and it's not gone even now (Feb 24).
#17
This is implemented in PIFR 2.x which I hope to deploy in the next few weeks. That is to say that it is not deployed on the active testing.drupal.org, but the patches are incorrect if they contain non-unix newlines, per http://drupal.org/patch.
#18
Okay, thanks.
#19
Automatically closed -- issue fixed for 2 weeks with no activity.