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.

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

Jump to bottom

Do not generate FROM clause in QueryBuilder if no tables specified #2292

Merged
merged 1 commit into from
Jul 15, 2016
Merged

Do not generate FROM clause in QueryBuilder if no tables specified #2292

merged 1 commit into from
Jul 15, 2016

Conversation

deeky666
Copy link
Member

fixes #2291

SELECT statements without FROM clause can be valid and legit in some scenarios. See issue.

@deeky666 deeky666 added this to the 2.5.5 milestone Jan 11, 2016
@deeky666 deeky666 mentioned this pull request Jan 11, 2016
Closed
@Ocramius
Copy link
Member

@deeky666 👍

But doesn't this count as improvement?

@Ocramius Ocramius modified the milestones: 2.6, 2.5.5 Jan 11, 2016
@deeky666
Copy link
Member Author

@Ocramius not sure. I'm fine having it as improvement. Guess it depends on what was to expect before, whether the QueryBuilder was intended to work without FROM specified. I guess nobody can answer that. But currently it just creates borked SQL if FROM is not specified, even though this can be valid SQL. We are doing conditions on other clauses like WHERE and ORDER BY, too so it's basically the same thing IMO. If we didn't generate those clauses conditonally, the resulting SQL would be borked just as it is now with FROM. I'll let you decide :)

@Ocramius
Copy link
Member

@deeky666 ok, then I'll take it as bugfix. Restoring milestone

@Ocramius Ocramius modified the milestones: 2.5.5, 2.6 Jan 11, 2016
@deeky666
Copy link
Member Author

@Ocramius fine. You can merge then as soon as you want.

billschaller
billschaller reviewed Jan 11, 2016
View reviewed changes
lib/Doctrine/DBAL/Query/QueryBuilder.php

$query .= implode(', ', $this->getFromClauses())
$query .= ($this->sqlParts['from'] ? ' FROM ' . implode(', ', $this->getFromClauses()) : '')
Copy link
Member

Choose a reason for hiding this comment

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

Consider that on Oracle you can't select without FROM. Needs select from dual in that case.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah it is neither meant to be platform aware nor is it meant to be able to validate SQL..

Copy link
Member

Choose a reason for hiding this comment

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

Good call

@deeky666
Copy link
Member Author

@Ocramius poke

@Ocramius Ocramius self-assigned this Jul 15, 2016
@Ocramius Ocramius merged commit 272b872 into doctrine:master Jul 15, 2016
Ocramius added a commit that referenced this pull request Jul 15, 2016
@Ocramius
Merge branch 'fix/#2292-do-not-generate-sql-from-with-no-tables' into…
e69d1cf 
… 2.5

Close #2292
@Ocramius
Copy link
Member

Backported to 2.5 via e69d1cf

@Ocramius Ocramius mentioned this pull request Oct 1, 2016
Merged
1 task
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees

@Ocramius Ocramius

Labels
Bug QueryBuilder
Projects
None yet
Milestone
2.5.5
Development

Successfully merging this pull request may close these issues.

The QueryBuilder::getSQLForSelect() always appends a FROM clause for select queries
3 participants
@deeky666 @Ocramius @billschaller