Skip to content
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.

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

Return case insensitive check #8591

Merged

Conversation

DmitriiBezborodnikov
Copy link
Contributor

@DmitriiBezborodnikov DmitriiBezborodnikov commented Apr 5, 2021

Return insensitive check after #8453

Problem: ->andWhere("u.name = ?1 or u.username = ?1"); did not wrap part in parenthesis when or or and was written in lowercase anymore. It still worked for uppercase OR and AND.

Fixes #8595

greg0ire
greg0ire previously approved these changes Apr 5, 2021
@greg0ire greg0ire added this to the 2.8.4 milestone Apr 5, 2021
@greg0ire
Copy link
Member

greg0ire commented Apr 5, 2021

@Warxcell please review

@Warxcell
Copy link
Contributor

Warxcell commented Apr 5, 2021

Yeah, seems we missed the case with lower case keywords. And there were no tests that could catch it. :(

@greg0ire
Copy link
Member

greg0ire commented Apr 5, 2021

I tried restoring the source code and running the test suite, and the test suite did not fail, because the test class, is not suffixed with Test 🙈

@Warxcell
Copy link
Contributor

Warxcell commented Apr 5, 2021

I tried restoring the source code and running the test suite, and the test suite did not fail, because the test class, is not suffixed with Test

./vendor/bin/phpunit tests/Doctrine/Tests/ORM/Functional/QueryBuilderParenthesis.php
PHPUnit 9.5.2 by Sebastian Bergmann and contributors.

.. 2 / 2 (100%)

Time: 00:00.063, Memory: 12.00 MB

OK (2 tests, 4 assertions)

So phpunit have different behaviour depending on how you run it? That's bad :( It should tell me that this is not valid test IMO.

@greg0ire
Copy link
Member

greg0ire commented Apr 5, 2021

Yup… kinda weird that my last commit does not result in an increase in coverage.

I have to go for now but I've started working on a find expression to find more test cases like this: find tests -type f -name '*.php' -not \( -name '*Test.php' -o -wholename 'tests/Doctrine/Tests/Models/*' -o -wholename 'tests/Doctrine/Tests/Proxies/__CG__*' -o -wholename 'tests/Doctrine/Tests/ORM/Tools/Export/*' \)

UPD: here is a legible input with a legible output:

find tests -type f -name '*.php' -not \( -name '*Test.php' \
 -o -wholename 'tests/Doctrine/Tests/Models/*' \
 -o -wholename 'tests/Doctrine/Tests/Proxies/__CG__*' \
 -o -wholename 'tests/Doctrine/Tests/ORM/Tools/Export/*' \
 -o -wholename 'tests/Doctrine/Tests/ORM/Mapping/*.php' \
 -o -wholename 'tests/Doctrine/Performance/*'\
 -o -wholename 'tests/Doctrine/Tests/Mocks/*'\
 -o -name '*TestCase.php'\
 -o -name '*Type.php' \)
tests/Doctrine/Tests/VerifyDeprecations.php
tests/Doctrine/Tests/TestUtil.php
tests/Doctrine/Tests/IterableTester.php
tests/Doctrine/Tests/TestInit.php
tests/Doctrine/Tests/ORM/Functional/ClassTableInheritanceTest2.php
tests/Doctrine/Tests/ORM/Functional/Ticket/Ticket69.php
tests/Doctrine/Tests/ORM/Functional/Locking/LockAgentWorker.php
tests/Doctrine/Tests/EventListener/CacheMetadataListener.php
tests/Doctrine/Tests/DbalTypes/CustomIdObject.php

DmitriiBezborodnikov and others added 2 commits April 5, 2021 14:03
It makes tests more isolated from each other: another test relying on
some tables including some of the ones created here may fail creating
the tables it needs because they already exist.
@greg0ire
Copy link
Member

greg0ire commented Apr 5, 2021

I don't reproduce the test failure locally 😬

They will not be taken into account when running vendor/bin/phpunit
otherwise.
@greg0ire
Copy link
Member

greg0ire commented Apr 5, 2021

Let's treat that one separately, I'll open another PR for it.

@greg0ire greg0ire merged commit 7de8453 into doctrine:2.8.x Apr 5, 2021
@greg0ire
Copy link
Member

greg0ire commented Apr 5, 2021

Thanks @DmitriiBezborodnikov !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Statement in Where-Clause are not wrapped in brackets anymore
3 participants