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

Update documentation to recommend DQL over QueryBuilder when possible #7880

Merged
merged 1 commit into from Oct 29, 2019

Conversation

kuraobi
Copy link

@kuraobi kuraobi commented Oct 28, 2019

Fixes #7520

As explained in the related issue, the idea here is to promote usage of DQL over QueryBuilder when writing static queries. While the QueryBuilder helps dynamically building DQL, it brings complexity to the code, so it should only be used when it actually brings value compared to DQL.
While the QueryBuilder could be argued to bring abstraction, the real abstraction is DQL itself, and the QueryBuilder does not bring more abstraction to writing queries.

I tried to clarify the intent of both DQL and the QueryBuilder, so please tell me if you think my update is clear enough or not, and I'll update the PR accordingly.

Copy link
Contributor

@Pierstoval Pierstoval left a comment

Some small wording reviews, nothing fundamental, but I hope it helps :)

docs/en/reference/query-builder.rst Outdated Show resolved Hide resolved
docs/en/reference/working-with-objects.rst Outdated Show resolved Hide resolved
Copy link
Member

@greg0ire greg0ire left a comment

Great work!

docs/en/reference/faq.rst Outdated Show resolved Hide resolved
docs/en/reference/query-builder.rst Show resolved Hide resolved
SenseException
SenseException previously requested changes Oct 29, 2019
docs/en/reference/query-builder.rst Show resolved Hide resolved
docs/en/reference/query-builder.rst Outdated Show resolved Hide resolved
docs/en/reference/query-builder.rst Show resolved Hide resolved
@Ocramius Ocramius added this to the 2.6.5 milestone Oct 29, 2019
@Ocramius Ocramius self-assigned this Oct 29, 2019
@Ocramius
Copy link
Member

@Ocramius Ocramius commented Oct 29, 2019

LGTM, good work, @kuraobi!

@Ocramius Ocramius dismissed SenseException’s stale review Oct 29, 2019

Review comments handled

@Ocramius Ocramius merged commit e9e012a into doctrine:2.6 Oct 29, 2019
2 checks passed

But the ``QueryBuilder`` is not an alternative to DQL, it actually generates DQL
queries at runtime, which are then interpreted by Doctrine. This means that
using the ``QueryBuilder`` to build and run a query is actually always slower
Copy link
Contributor

@Majkl578 Majkl578 Oct 30, 2019

Choose a reason for hiding this comment

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

While I understand you want to convince people to not use QueryBuilder, using performance as a reason sounds funny in the context of an ORM. A few method calls is totally negligible and not measurable.

Copy link
Member

@greg0ire greg0ire Oct 30, 2019

Choose a reason for hiding this comment

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

Totally, but I think some people might be using the QB because they think it's more performant.

Copy link
Member

@greg0ire greg0ire Oct 30, 2019

Choose a reason for hiding this comment

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

That paragraph was my idea, but I'll make a PR to amend it if more people think it's irrelevant. I I still like it because it teaches people about the internals.

Copy link
Author

@kuraobi kuraobi Oct 30, 2019

Choose a reason for hiding this comment

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

The point here is to explain that the QB is an extra layer that comes at a cost. Coding choices are based on trade offs, and I'm just emphasizing that performance wouldn't be in the "+" column. A lot of people think the QB is preferable because Doctrine doesn't need to parse DQL, but it's not true.

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

6 participants