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

Add additional detail and clarifications on SELECT #6572

Merged
merged 3 commits into from
Aug 18, 2017
Merged

Add additional detail and clarifications on SELECT #6572

merged 3 commits into from
Aug 18, 2017

Conversation

bitwombat
Copy link

  • Also the effect of WHERE on result array.

As per #6563

- Also the effect of WHERE on result array.
The SELECT clause of a DQL query specifies what gets hydrated in
the query result. You are always returned usable objects, but any
associated objects not included in the SELECT clause will be
proxies (ie. unhydrated). They get hydrated by Doctrine when
Copy link
Member

Choose a reason for hiding this comment

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

Too much information here - this should be detailed somewhere else

Copy link
Author

@bitwombat bitwombat Jul 22, 2017

Choose a reason for hiding this comment

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

Somewhere else in the same doc, or somewhere else on the internet :)

This PR is sort of an inclusion of the things that took me too long to figure out... and accepted SO answers weren't very good.

Copy link
Member

Choose a reason for hiding this comment

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

Possibly, but remember that you are approaching readers that may have no clue about doctrine, so this stuff can be plain confusing until the basics are covered :-)

Copy link
Member

Choose a reason for hiding this comment

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

Basically, ELI5

Copy link
Author

Choose a reason for hiding this comment

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

Actually, near line 181 you already have a description of the lazy-loading and walking the object tree... so you've either changed your mind or I'm duplicating things. :)

Either way I should delete this part. Except I'm not sure it's clear that including things in the SELECT clause is what hydrates them. The discussion of fetch joins at the top is attempting to do that.

What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Either way I should delete this part. Except I'm not sure it's clear that including things in the SELECT clause is what hydrates them. The discussion of fetch joins at the top is attempting to do that.

As long as it's documented somewhere else, heh.

In this case, the result array will be made up of objects of
the type in the FROM clause. In the example above, the query
will return an array of User objects, with associated classes
identify else where in the query as 'p' and 'n' hydrated.
Copy link
Member

Choose a reason for hiding this comment

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

The wording is a bit confusing here - can you rephrase it so that it is clear that p and n are children of u, and are not going to be selected as separate columns in the result?

Copy link
Member

Choose a reason for hiding this comment

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

Use double backticks around anything that is code-like, such as u, p, n

Copy link
Author

Choose a reason for hiding this comment

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

Wow, typo on my part. Bad news. OK, will fix, rephrase and add backticks.

element being an array made up of a User object and the scalar
value p.quantity.

Multiple FROM clauses are allowed, which would cause the result
Copy link
Member

Choose a reason for hiding this comment

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

Use double backticks around anything that is code-like, such as FROM

Copy link
Author

Choose a reason for hiding this comment

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

I'm following convention. Elsewhere in the doc, the DQL keywords (SELECT, FROM, WITH) are not surrounded by backticks (though entire DQL commands are). Maybe Previous You thought that would clutter the output? Tell me what you want me to do here.

Copy link
Member

Choose a reason for hiding this comment

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

I wrote almost none of the docs, but the separate rendering format of code as code aids readability

Copy link
Author

@bitwombat bitwombat Aug 18, 2017

Choose a reason for hiding this comment

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

So you'd like me to surround all the existing SELECT, FROM, WITH, etc. statements with backticks as part of this PR? No prob, just clarifying.

``SELECT u, p.quantity FROM Users u...``
Here, the result will again be an array of arrays, with each
element being an array made up of a User object and the scalar
value p.quantity.
Copy link
Member

Choose a reason for hiding this comment

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

Use double backticks around anything that is code-like, such as p.quantity


.. note::

You cannot select other entities unless you also select the
Copy link
Member

Choose a reason for hiding this comment

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

This case only applies to the scenario where entities are being selected, not the scalars scenario

Copy link
Author

Choose a reason for hiding this comment

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

It's a note, not coupled to the scalars scenario. Unless I'm not seeing something.

Copy link
Member

Choose a reason for hiding this comment

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

Alright then 👍

root of the selection (the first entity in FROM).

Doctrine tells you you have violated this constraint with the
exception "Cannot select entity through identification
Copy link
Member

Choose a reason for hiding this comment

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

"with an exception" - the exception message should be improved in the library if it isn't clear enough there.

I would also add an example of when this would fail (example query)

Copy link
Author

Choose a reason for hiding this comment

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

The message does call the same thing an "identification variable" and an "alias" in the same message. As do the docs. I'm wondering if one term should be picked and used throughout.

Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if one term should be picked and used throughout.

Possibly - it would surely reduce impedance between ORM internals and their documentation (as they are called "identification variable" internally too)

exception "Cannot select entity through identification
variables without choosing at least one root entity alias."

It is possible to wrap both fields and identification values into
Copy link
Member

Choose a reason for hiding this comment

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

This block is confusing - why is it in this section?

Copy link
Author

Choose a reason for hiding this comment

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

I think you might have selected the wrong lines there, as that's not a block... ??

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, miswording on my side - I mean paragraph. The information about functions being applicable to the selection seems weird if it's redundant with a later section. Maybe move this information directly there?

Copy link
Author

Choose a reason for hiding this comment

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

Line 127 above? That was in the original (unchanged by me).

Copy link
Author

Choose a reason for hiding this comment

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

Didn't hear back from you on this. Leaving it in there since it's yours originally.

Copy link
Member

Choose a reason for hiding this comment

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

Alright 👍

- A per Ocramius' explanation in #6563
@Ocramius
Copy link
Member

One note on positioning knowledge in the document: after editing, try to scan the entire document again and see if it makes sense or it actually asks the user to jump around "goto" style :-)

@Ocramius Ocramius added this to the 2.6.0 milestone Aug 18, 2017
@Ocramius
Copy link
Member

🚢

Thanks @bitwombat!

@Ocramius Ocramius merged commit 671fd50 into doctrine:master Aug 18, 2017
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.

2 participants