Skip to content

Fix compatibility with DBAL 2.8 (doctrine/dbal#3157)#7290

Merged
Majkl578 merged 1 commit intodoctrine:2.6from
Majkl578:dbal-2.8-tests-compat
Jul 3, 2018
Merged

Fix compatibility with DBAL 2.8 (doctrine/dbal#3157)#7290
Majkl578 merged 1 commit intodoctrine:2.6from
Majkl578:dbal-2.8-tests-compat

Conversation

@Majkl578
Copy link
Copy Markdown
Contributor

@Majkl578 Majkl578 commented Jul 2, 2018

Forward-compat for doctrine/dbal#3157.

@Majkl578 Majkl578 added this to the 2.6.2 milestone Jul 2, 2018
@Majkl578 Majkl578 requested review from lcobucci and morozov July 2, 2018 23:59
lcobucci
lcobucci previously approved these changes Jul 3, 2018
Copy link
Copy Markdown
Member

@lcobucci lcobucci left a comment

Choose a reason for hiding this comment

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

Code LGTM, I'd suggest to add more details to the commit message or add a comment in the code. It might be complicated to understand this after some time.


$this->assertEquals('SELECT c0_.id AS id_0, c0_.status AS status_1, c0_.username AS username_2, c0_.name AS name_3, c0_.email_id AS email_id_4 FROM cms_users c0_ LIMIT 10 OFFSET 0', $q->getSql());
self::assertThat(
$q->getSql(),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does it use DBAL\QueryBuilder internally? If yes, maybe the test could be improved by only making sure that the values passed to Query::setMaxResults() and Query::setFirstResult() are passed down to the QueryBuilder as is.

E.g. if instead of getSql() we could get the internal QueryBuilder and check its state, we could get rid of the dependency on SQL.

If it's not possible, then I'm fine with this solution.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's not using DBAL builder, but SQL walker.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@morozov it doesn't using the query builder internally but rather a compiles a DQL and translates it into SQL using a Query object.

morozov
morozov previously approved these changes Jul 3, 2018
@Majkl578 Majkl578 dismissed stale reviews from morozov and lcobucci via ac1e1c7 July 3, 2018 00:14
@Majkl578 Majkl578 force-pushed the dbal-2.8-tests-compat branch from c85b547 to ac1e1c7 Compare July 3, 2018 00:14
@Majkl578
Copy link
Copy Markdown
Contributor Author

Majkl578 commented Jul 3, 2018

@lcobucci Added comment + reworded.

@Majkl578
Copy link
Copy Markdown
Contributor Author

Majkl578 commented Jul 3, 2018

Meh, Scrutinizer is not working properly... :S

@Majkl578 Majkl578 merged commit 4192c3a into doctrine:2.6 Jul 3, 2018
@Majkl578 Majkl578 deleted the dbal-2.8-tests-compat branch July 3, 2018 00:58
@Majkl578 Majkl578 self-assigned this Jul 3, 2018
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.

3 participants