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

Improve docs: INSTANCE OF needs class metadata as parameter value #7963

Merged
merged 1 commit into from
Dec 12, 2023

Conversation

flaushi
Copy link
Contributor

@flaushi flaushi commented Dec 23, 2019

…r value for the INSTANCE OF DQL expression

As discussed on slack and in relation to #6483, one could clarify in the docs that it's not enough to just bind a class name by its FQCN.

$query->setParameter(1, $em->getClassMetadata(CompanyEmployee::class);

.. note::
To use a class as parameter, you have to bind it's class metadata: ``$query->setParameter(0, $em->getClassMetadata(CompanyEmployee::class);``.
Copy link
Member

Choose a reason for hiding this comment

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

it should be its as it's a possessive

@greg0ire
Copy link
Member

greg0ire commented Jan 1, 2020

Please squash your commits and please improve your commit message according to the contributing guide.

Also, please associate your email address with your Github account, or change the
email in your commits to an address already associated with it. If you do not
want to expose your personal email address, you may use
flaushi@users.noreply.github.com, that way we can still reach you through Github.
Additionally, you will get credit for this contribution on your Github profile.

@greg0ire
Copy link
Member

greg0ire commented Jan 1, 2020

Also, this should probably target 2.7 or 2.8.x otherwise your changes will not appear in the docs people actually read until 3.0 is released.

@flaushi
Copy link
Contributor Author

flaushi commented Jan 2, 2020

@greg0ire Yes, that should be done of course. So should I create a new fork of the branch '2.7'?

@flaushi
Copy link
Contributor Author

flaushi commented Jan 2, 2020

I applied my note to a branch basing on 2.7, and pushed it here https://github.com/flaushi/orm/tree/note-instance-of (commits squashed, email address changed)

Can you change the merge request or should I start a new one?

@greg0ire
Copy link
Member

greg0ire commented Jan 3, 2020

You can reset your master branch to note-instance-of and force push. It will be confusing, but I guess this is why you should not use master when making a PR. Once it is done, click the Edit button and change the target branch. Once this is merged, I recommend you delete your master branch, that way it will be hard for you to commit on it.

@flaushi
Copy link
Contributor Author

flaushi commented Jan 3, 2020

I am too unexperienced with this git s**t. Argh, now I have all my messages twice 😩.
Can't I just open a new PR for the note-instance-of branch? I guess it's also better because it already targets 2.7?

@greg0ire greg0ire changed the base branch from master to 2.7 January 3, 2020 13:40
@greg0ire
Copy link
Member

greg0ire commented Jan 3, 2020

Please just git checkout master && git reset --hard note-instance-of && git push --force

@flaushi
Copy link
Contributor Author

flaushi commented Jan 3, 2020

I think that's it. One commit, with email address. Anything else required from my side? You spoke about merging...🤷‍♂️

@greg0ire
Copy link
Member

greg0ire commented Jan 3, 2020

Your commit does not seem to start from 2.7

@greg0ire
Copy link
Member

greg0ire commented Jan 3, 2020

If you can find your commit in the git history, try git reset --hard 2785cde && git cherry-pick d3209633abcd31a1f50346ec58065493130d1f68 && git push --force

@greg0ire
Copy link
Member

greg0ire commented Jan 3, 2020

I'm on the symfony-dev slack if you want to chat

@greg0ire
Copy link
Member

greg0ire commented Jan 3, 2020

It looks sane now :)

ostrolucky
ostrolucky previously approved these changes Jan 3, 2020
@cs278
Copy link

cs278 commented Dec 12, 2023

What's needed to get this merged?

@derrabus derrabus changed the base branch from 2.10.x to 2.17.x December 12, 2023 10:21
@greg0ire
Copy link
Member

One more approval I'd say.

@derrabus derrabus added this to the 2.17.2 milestone Dec 12, 2023
@derrabus derrabus merged commit 46cb9a9 into doctrine:2.17.x Dec 12, 2023
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