Ensure a parameter mapping entry exists for InstanceOf DQL expressions

[craigmarvelley]

Hi,

This is a possible fix for http://www.doctrine-project.org/jira/browse/DDC-1995, in that it resolves the issue for me but I'm afraid I haven't had time to test it extensively with more complex queries than the use case I gave in that ticket.

Cheers,
Craig

[travisbot]

This pull request passes (merged daa58f6 into ece6a00).

[stof]

Please add some testcase to avoid regressions

[craigmarvelley]

I'd planned on it, but unable to install Doctrine's dependencies through Composer ATM. Getting 'The requested package doctrine/dbal >=2.3-dev,<2.5-dev could not be found.'. Any ideas? :)

[stof]

The doctrine testsuite is not setup using composer currently but submodules, as you can see by looking at the way the travis config file does it

[craigmarvelley]

Ah, of course. Right. I added a test case, and my 'fix' caused an SQLite exception so I removed it. I'll take another look when I've time, if nobody else gets chance first.

[travisbot]

This pull request fails (merged 7138248 into ece6a00).

[stof]

in fact, I'm simply not sure this is possible. INSTANCE OF is handled by using the discriminator map. It cannot be done if the type uses a placeholder bound only in SQL.

[craigmarvelley]

I see. If it's mapped then presumably I don't have to worry about escaping it through a parameter anyway, but this (https://groups.google.com/forum/#!msg/doctrine-user/IomnY_RhPz4/elBFbwVgEzAJ) seems to indicate that it used to work; if it's a regression it may cause problems for people upgrading?

[stof]

I think a better fix would be to remove the parameter which is not used anymore

[beberlei]

I assigned the issue to @guilhermeblanco - I don't know what the procedure here should be.

[guilhermeblanco]

I can only see a test case here. So... just added test coverage?
InstanceOf expressions always work at the top of ClassMetadata. It never worked any other way.
That was a mechanism enforced to avoid magic and unexpected behavior (like giving a string and actually testing a ClassMetadata).
Can someone tell me what I should do here? It doesn't seem a bug, just a missing coverage.

[stof]

@guilhermeblanco this PR is about adding a failing testcase showing the issue. there is no fix for the issue here (which is why @beberlei assigned it to you)

[stof]

btw, the latest travis comment shows that it does not work as expected by this test

[craigmarvelley]

@guilhermeblanco I would just have expected to be able to provide the fully qualified class name as a parameter, so queries can be reused, and to be consistent. The test case I added illustrates that it doesn't work at the moment - I did originally commit a fix which worked on MySQL but caused an exception on SQLite so I reverted it.

[guilhermeblanco]

I'm having issues to fix this issue. Attempted 2 different ways without any luck.
Need a different approach to solve the issue correctly. I should come back ot this issue in a week.

[asm89]

ping @guilhermeblanco

[FabioBatSilva]

close in favor of #689