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

INSTANCE OF now behaves correctly by including all possible child classes in IN() filtering conditions #1441

Closed
wants to merge 1 commit into from

Conversation

taueres
Copy link
Contributor

@taueres taueres commented Jun 29, 2015

There was a bug in the "INSTANCE OF" operator as described in
https://groups.google.com/forum/#!topic/doctrine-user/B8raq8CNMgg

"INSTANCE OF" was not taking into account subclasses.
It was merely translating the class to its discriminator.
This is not correct since the class can have subtypes and those
are, indeed, still instance of the superclass.
Also, classes may not have a discriminator (e.g. abstract classes).

This commit also provides useful tests to avoid regression.

@doctrinebot
Copy link

Hello,

thank you for creating this pull request. I have automatically opened an issue
on our Jira Bug Tracker for you. See the issue link:

http://www.doctrine-project.org/jira/browse/DDC-3800

We use Jira to track the state of pull requests and the versions they got
included in.

There was a bug in the "INSTANCE OF" operator as described in
https://groups.google.com/forum/#!topic/doctrine-user/B8raq8CNMgg

"INSTANCE OF" was not taking into account subclasses.
It was merely translating the class to its discriminator.
This is not correct since the class can have subtypes and those
are, indeed, still instance of the superclass.
Also, classes may not have a discriminator (e.g. abstract classes).

This commit also provides useful tests to avoid regression.
@stof
Copy link
Member

stof commented Jun 30, 2015

IIRC, the doc mentions that inheritance is not taken into account for this operator.
This change is technically a BC break for anyone relying on the current behavior.

@taueres
Copy link
Contributor Author

taueres commented Jun 30, 2015

There's also an additional problem. If classes are parametrized they won't be converted to slugs since they are passed directly to connection level statement.

@taueres
Copy link
Contributor Author

taueres commented Jun 30, 2015

@stof Docs do not say much about "instance of" operator. I found only this reference: http://doctrine-orm.readthedocs.org/en/latest/reference/dql-doctrine-query-language.html

@peelandsee
Copy link

quoting @Ocramius in https://groups.google.com/d/msg/doctrine-user/B8raq8CNMgg/kYeSR7gC1Y4J
i think the same: it's a bug!

this fix could simplify a lot of existing code,
where you have to manually build an external hierarchical discrimination map from entity classes,
to prepare your DQL..

if current behavior it's not mentioned in the docs (so the bug was also undocumented),
IMHO this should not be considered a BC..

@Jean85
Copy link
Contributor

Jean85 commented Jun 30, 2015

It shouldn't also be considered a BC because if you try to do an INSTANCEOF now on (for example) an abstract class, it does not provide a "current behavior"; it simply throws an exception, because it can't find any discriminator associated to the abstract class itself.

@peelandsee
Copy link

any chance to merge this?

PR was motivated by https://groups.google.com/d/msg/doctrine-user/B8raq8CNMgg/kYeSR7gC1Y4J

thanks!

@Jean85
Copy link
Contributor

Jean85 commented Jan 8, 2016

Hi there... no news here?
In the meantime, DoctrineBot migrated the issue into #4646

@taueres
Copy link
Contributor Author

taueres commented Jun 24, 2017

I'm closing this PR in favor of the newer #6392

@taueres taueres closed this Jun 24, 2017
@Ocramius Ocramius self-assigned this Jun 24, 2017
@Ocramius Ocramius added this to the 2.6.0 milestone Aug 18, 2017
@Ocramius
Copy link
Member

Handled in #6392

@Ocramius Ocramius changed the title [QUERY] "INSTANCE OF" now behaves correctly with subclasses "INSTANCE OF" now behaves correctly by including all possible child classes in IN() filtering conditions Aug 18, 2017
@Ocramius Ocramius changed the title "INSTANCE OF" now behaves correctly by including all possible child classes in IN() filtering conditions INSTANCE OF now behaves correctly by including all possible child classes in IN() filtering conditions 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.

6 participants