Fixed CND parsing under Windows OS #48

dinamic · 2013-04-11T15:43:07Z

CND data was not correctly parsed while using Windows OS.
The issue was caused by the different line endings between the operating systems.

I came up with a fix and updated the unit tests to test both Windows and Unix line endings.

I've found some discrepancies inside the FileReader class, so it is updated as well.

This commit fixes: https://github.com/symfony-cmf/symfony-cmf-standard/issues/21

…acters

…Path ~ updated unit test

lsmith77 · 2013-04-12T07:07:01Z

src/PHPCR/Util/CND/Scanner/GenericScanner.php

            $reader->rewind();

            return true;
+        } else {


else is not needed here

Thanks. Just removed it.

lsmith77 · 2013-04-12T07:09:35Z

thank you! can you have a look at the failing test on Travis? @dbu should have a look at you method rename

dinamic · 2013-04-12T07:24:14Z

Sorry, didn't noticed the failing test.

dinamic · 2013-04-12T08:11:17Z

While checking the failed tests I realized there's an issue with my current implementation.
It has to use look-ahead to determine the new line sequence, rather than look-behind.

… of the EOL character

dbu · 2013-04-12T12:07:48Z

src/PHPCR/Util/CND/Reader/BufferReader.php

+    public function isEol()
+    {
+        $current = $this->current();
+        $marker = $this->getEolMarker();


i don't think we need a field and getter for the eol, rather use the PHP_EOL directly below. end-of-line is handled by php for us, i think that is good enough.

the issue is that PHP_EOL is different depending on which OS you are running the code.

ah ok, you wanted the test to test both. then lets just have a member variable but not use a getter method each time.

and lets have the constructor accept an optional $eolMarker that defaults to PHP_EOL. it could be useful for somebody in edge cases.

dbu · 2013-04-12T12:31:48Z

thanks for this PR. i commented on some naming things.
can you solve the look-ahead issue you have or do you need input on that?

is the reason you ran into this issue in the first place that your git client converted the line endings? i fear that we still can see problems after this fix. what would happen if we always check for both types of line endings, and maybe add the apple OS endings as well? might be the most flexible approach.

btw, impressive that after 30 years of PC we still see this stupid problem :-(

+ unix and windows eol tests

dinamic · 2013-04-12T13:05:22Z

The look-ahead is solved.

The issue I came upon was invoked because of the hardcoded EOL. It was set like "\n" and for Windows is "\r\n". Later on the process of parsing there was the assumption that the EOL is a single character, which obviously is not true, so the code failed to parse the string.

I'm struggling at the GenericScanner now. There are some naming wtfs that are pretty confusing.

Once I'm done with the GenericScanner, I believe we would be able to handle whatever EOL we like. LF, CR, CR+LF, LF+CR, you name it. IMHO, it is a design fail that caused the issue.

I had the idea of converting the EOL within the reader constructor. Any thoughts on that?

dbu · 2013-04-12T13:52:20Z

maybe we could simply string replace all possible formats to \n and then do the char comparison on \n rather than the php constant. the LF char is not valid anywhere else in the cnd document anyway than as end of line character... this could make the code as simple as it was before.
we load the whole string anyways, and cnd files are not expected to be huge, so doing the replace upfront sounds save to me from a memory perspective.

dbu · 2013-04-12T13:54:22Z

the constructor then would be

$buffer = str_replace("\r\n", "\n", $buffer);
$buffer = str_replace("\r", "\n", $buffer);

dinamic · 2013-04-15T07:25:30Z

Exactly my point. I was not aware whether the CRLF would be used anywhere inside the document and couldn't decide by myself to do such conversion.

dbu · 2013-04-15T08:04:41Z

tests/PHPCR/Tests/Util/CND/Reader/FileReaderUnixTest.php

@@ -54,7 +88,7 @@ public function testGetNextChar()
                break;
            }

-            //var_dump('Expected:' . $this->chars[$i] . ', found: ' . $peek);
+//            var_dump('Expected:' . $this->chars[$i] . ', found: ' . $peek);


lets remove commented out code

dbu · 2013-04-15T08:07:09Z

i don't expect any problems, lets convert everything to the one-char \n in the constructor of BufferReader, and simplify the code again (we won't need peek and multi-char EOL anymore then)

dbu · 2013-04-19T07:58:29Z

@dinamic can you do the cleanup and convert all cnd files to unix line endings in the constructor, before further handling them?

relo-san · 2013-04-20T07:29:55Z

@dbu In Windows (at least Windows 7) in PHP global constant PHP_EOL by default equal CRLF, not LF. Accordingly, any comparison between currentChar() and PHP_EOL not work, because PHP_EOL contains 2 chars. In this case, it does not matter what kind of line breaks in the cnd files.

relo-san · 2013-04-20T07:34:02Z

I see the most reasonable solution to convert all line breaks in cnd to the LF and replace PHP_EOL in all comparisons also to LF.
P.S. Sorry, I casually read discussion, it's an already known.

dbu · 2013-04-20T07:47:14Z

yes, thats exactly what we should do. i hope dinamic can do it - if you don't want to wait you could fork his repo and finish it and create a new PR

lsmith77 · 2013-04-20T12:16:16Z

superseded by #51

dinamic · 2013-04-22T07:19:01Z

Thanks for contributing @relo-san. I didn't had the time to finish my fix.

dinamic added 4 commits April 11, 2013 16:27

~ unit testing different platforms for problems with line ending char…

90ba062

…acters

~ unit testing different platforms for problems with line ending char…

113920a

…acters

~ bug fix for windows environment line endings

6697102

~ refactored FileReader to use proper variable name; fileName -> file…

6167cef

…Path ~ updated unit test

lsmith77 reviewed Apr 12, 2013
View reviewed changes

- unnecessary else statement

1c423fd

~ using look-ahead; the user no longer needs to know the exact length…

84d9fb2

… of the EOL character

dbu reviewed Apr 12, 2013
View reviewed changes

dinamic added 2 commits April 12, 2013 15:46

~ removed filename and filepath renamed to path, as discussed

d4925ae

+ unix and windows eol tests

~ renamed method BufferReaderTest::test__construct -> doTestReader()

9836c70

~ updated file fixtures

e2b9689

dbu reviewed Apr 15, 2013
View reviewed changes

relo-san mentioned this pull request Apr 20, 2013

Add support for Windows line endings in cnd #51

Closed

lsmith77 closed this Apr 20, 2013

Fixed CND parsing under Windows OS #48

Fixed CND parsing under Windows OS #48

Uh oh!

Conversation

dinamic commented Apr 11, 2013

Uh oh!

lsmith77 Apr 12, 2013

Choose a reason for hiding this comment

Uh oh!

dinamic Apr 12, 2013

Choose a reason for hiding this comment

Uh oh!

lsmith77 commented Apr 12, 2013

Uh oh!

dinamic commented Apr 12, 2013

Uh oh!

dinamic commented Apr 12, 2013

Uh oh!

dbu Apr 12, 2013

Choose a reason for hiding this comment

Uh oh!

lsmith77 Apr 12, 2013

Choose a reason for hiding this comment

Uh oh!

dbu Apr 12, 2013

Choose a reason for hiding this comment

Uh oh!

dbu commented Apr 12, 2013

Uh oh!

dinamic commented Apr 12, 2013

Uh oh!

dbu commented Apr 12, 2013

Uh oh!

dbu commented Apr 12, 2013

Uh oh!

dinamic commented Apr 15, 2013

Uh oh!

dbu Apr 15, 2013

Choose a reason for hiding this comment

Uh oh!

dbu commented Apr 15, 2013

Uh oh!

dbu commented Apr 19, 2013

Uh oh!

relo-san commented Apr 20, 2013

Uh oh!

relo-san commented Apr 20, 2013

Uh oh!

dbu commented Apr 20, 2013

Uh oh!

lsmith77 commented Apr 20, 2013

Uh oh!

dinamic commented Apr 22, 2013

Uh oh!

Uh oh!