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

Fix Lexer query error #519

Merged
merged 2 commits into from
Mar 24, 2020
Merged

Conversation

core23
Copy link
Member

@core23 core23 commented Mar 20, 2020

Subject

There was an lexer error in my previous PR:

[Syntax Error] line 0, col 9: Error: Expected Doctrine\ORM\Query\Lexer::T_FROM, got 'WHERE'

This results in an invalid query:

SELECT c WHERE c.slug = :slug AND c.context = :context AND c.enabled = :enabled

I am targeting this branch, because {reason}.

Fixes #517

Changelog

### Fixed
- Fix Lexer query error in managers

@core23 core23 requested a review from a team March 20, 2020 18:13
@VincentLanglet
Copy link
Member

Can you add a test for the bug @core23 ?

@core23
Copy link
Member Author

core23 commented Mar 20, 2020

I'm not sure if this makes much sense, because these things are some kind of doctrine builder

@VincentLanglet
Copy link
Member

The SonataDoctrineOrmAdminBundle use is own mock and check the query
https://github.com/sonata-project/SonataDoctrineORMAdminBundle/blob/3.x/tests/Filter/QueryBuilder.php

It's possible to use a database and check for the results.

In this case, the query executed was failing and we couldn't detect this. If someone modify this code, it can be broken again. There should be a way to avoid regression...

@core23
Copy link
Member Author

core23 commented Mar 20, 2020

Done @VincentLanglet

VincentLanglet
VincentLanglet previously approved these changes Mar 20, 2020
@VincentLanglet
Copy link
Member

Woops, tests are failing @core23

@VincentLanglet
Copy link
Member

I think you should use the syntax

$qb->expects($self->exactly(...))->method('andWhere')->withConsecutive(
   [...]	                    
);

When there is multiple calls

tests/Entity/CollectionManagerTest.php Outdated Show resolved Hide resolved
tests/Entity/TagManagerTest.php Outdated Show resolved Hide resolved
tests/Entity/CategoryManagerTest.php Outdated Show resolved Hide resolved
tests/Entity/CollectionManagerTest.php Outdated Show resolved Hide resolved
tests/Entity/CollectionManagerTest.php Outdated Show resolved Hide resolved
tests/Entity/TagManagerTest.php Outdated Show resolved Hide resolved
tests/Entity/TagManagerTest.php Outdated Show resolved Hide resolved
tests/Entity/TagManagerTest.php Outdated Show resolved Hide resolved
VincentLanglet
VincentLanglet previously approved these changes Mar 23, 2020
@core23
Copy link
Member Author

core23 commented Mar 23, 2020

We need this one to fix the tests :/

sonata-project/sonata-doctrine-extensions#186

greg0ire
greg0ire previously approved these changes Mar 23, 2020
@core23
Copy link
Member Author

core23 commented Mar 24, 2020

[Composer\Downloader\TransportException]

Could not authenticate against github.com

This is strange, even after a fresh rebase

@greg0ire
Copy link
Contributor

Yeah very. I restarted the job, and it passes now… weird.

@VincentLanglet VincentLanglet merged commit 85e5f1e into sonata-project:3.x Mar 24, 2020
@core23 core23 deleted the fix-manager branch March 24, 2020 08:55
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.

None yet

4 participants