Fix query tokenizing on windows #205
Conversation
alexander-schranz
changed the title
| include: | ||
| - php-version: '7.4' |
There was a problem hiding this comment.
@dbu should we add other php versions also for windows test matrix?
There was a problem hiding this comment.
i would not run the full matrix, but maybe the latest version (8.1 at the moment)?
| include: | ||
| - php-version: '7.4' |
There was a problem hiding this comment.
i would not run the full matrix, but maybe the latest version (8.1 at the moment)?
| @@ -152,7 +152,7 @@ protected function scan($sql2) | |||
| $splitString = \str_split($sql2); | |||
| for ($index = 0; $index < count($splitString); $index++) { | |||
| $character = $splitString[$index]; | |||
| if (!$stringStartCharacter && in_array($character, [' ', "\t", "\n"], true)) { | |||
| if (!$stringStartCharacter && in_array($character, [' ', "\t", "\n", "\r"], true)) { | |||
There was a problem hiding this comment.
if you do not have this line, do the tests actually fail? do we have a test that builds a query with PHP_EOL? if that is the case, lets have a windows build.
if not, i suggest that we craft queries with explicit \n and \r newlines to cover all cases (macos, linux and windows) instead of building on a windows system.
There was a problem hiding this comment.
oh, found the reproducer commit and build you linked. so it does only fail on windows. but why is that? shouldn't that test also fail on linux?
There was a problem hiding this comment.
@dbu I think git converts line breaks on windows from "\n" to the windows "\n\r" format. And that's why its only failing there. I would keep atleast one windows run to make sure in future it works. But also don't think the whole matrix does add any value.
There was a problem hiding this comment.
so if we would add a test that has a \n\r in it, it would also fail on linux (without the fix)?
i think we should have such a test as well then, it seems a bit indirect and obscure that the windows test happens the way it does.
|
thanks for the fix! https://github.com/phpcr/phpcr-utils/releases/tag/1.6.1 |
|
@dbu Thank you! |
Fixes #204
Reproducer Commit: edb1707 CI: https://github.com/phpcr/phpcr-utils/runs/4533239583?check_suite_focus=true
Fix Commit: 67456bc CI: https://github.com/phpcr/phpcr-utils/pull/205/checks