Skip to content

Fix query tokenizing on windows #205

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Dec 20, 2021
Merged

Fix query tokenizing on windows #205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
edb1707
Add CI for windows tests
alexander-schranz Dec 15, 2021
67456bc
Try to fix SqlScanner on windows for multiline select statements
alexander-schranz Dec 15, 2021
6ce7d0f
Update test-application.yaml
alexander-schranz Dec 15, 2021
e690036
Update .github/workflows/test-application.yaml
alexander-schranz Dec 17, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 33 additions & 0 deletions .github/workflows/test-application.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,3 +50,36 @@ jobs:

- name: Execute test cases
run: vendor/bin/phpunit

php-windows:
name: "PHP Windows ${{ matrix.php-version }} ${{ matrix.dependencies }} ${{ matrix.dev-dependencies && 'dev' }}"
runs-on: windows-latest

strategy:
fail-fast: false
matrix:
include:
- php-version: '8.1'

steps:
- name: Checkout project
uses: actions/checkout@v2

- name: Install and configure PHP
uses: shivammathur/setup-php@v2
with:
php-version: ${{ matrix.php-version }}
tools: 'composer:v2'

- name: Allow unstable dependencies
if: matrix.dev-dependencies == true
run: composer config minimum-stability dev

- name: Install dependencies with Composer
uses: ramsey/composer-install@v1
with:
dependency-versions: ${{ matrix.dependencies }}
composer-options: --prefer-dist

- name: Execute test cases
run: vendor/bin/phpunit
2 changes: 1 addition & 1 deletion src/PHPCR/Util/QOM/Sql2Scanner.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@alexander-schranz alexander-schranz Dec 17, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if ($currentToken !== '') {
$tokens[] = $currentToken;
}
Expand Down