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

Fix incompatibility w/ PHP7.2+ #6142

Merged
merged 1 commit into from
Nov 26, 2016
Merged

Conversation

mbeccati
Copy link
Contributor

Mock_ParserResult_*::getParameterMappings() was returning null, which was then passed to count() on Query.php:308, causing a "Parameter must be an array or an object that implements Countable" error.

Mock_ParserResult_*::getParameterMappings() was returning null, which
was then passed to count() on Query.php:308, causing a "Parameter must
be an array or an object that implements Countable" error.
@lcobucci
Copy link
Member

@mbeccati that sounds like a phpunit-mock issue, since when you call createMock() with a concrete class it should stub the public interface of the object...

@mbeccati
Copy link
Contributor Author

By default, all methods of the original class are replaced with a dummy implementation that just returns null

Which is not what you'd want there, @lcobucci

@Ocramius
Copy link
Member

Ocramius commented Nov 22, 2016

Got a link to the failure? I don't understand the reason for this change.

Specifically, I don't understand why it fails on 7.2 only.

@lcobucci
Copy link
Member

lcobucci commented Nov 22, 2016

Which is not what you'd want there, @lcobucci

@mbeccati We just don't care here if we stub everything or not, you might right when saying that we're just need to do that for the method getSqlExecutor() but there's no reason to change doctrine's test suite if it's passing for the other builds.

Got a link to the failure? I don't understand the reason for this change.

@Ocramius It's failing for nightly builds (like https://travis-ci.org/doctrine/doctrine2/jobs/177333526)

@mbeccati
Copy link
Contributor Author

@Ocramius https://revive.beccati.com/bamboo/browse/PHP-DOCTR-MAS-919/test/case/11810654 w/ php-src master, since count() has been restricted to work only on array|Countable.

@Ocramius Ocramius added the Bug label Nov 22, 2016
@Ocramius Ocramius self-assigned this Nov 26, 2016
@Ocramius Ocramius added this to the 2.6.0 milestone Nov 26, 2016
@Ocramius
Copy link
Member

Merging, since I feel like digging further into this is just going to be more pain.

The fix is simple/easy: let's roll with it, since it's a test-suite change only.

@Ocramius Ocramius merged commit 899393d into doctrine:master Nov 26, 2016
@mbeccati mbeccati deleted the php72-count-fix branch November 26, 2016 07:25
@mbeccati
Copy link
Contributor Author

Thanks @Ocramius. Since I've did that digging already, I'll try to write it down for posterity:

  1. \Doctrine\ORM\Query\ParserResult has a private property $_parameterMappings, which is initialised as an empty array;
  2. such property is never overwritten with non-array values;
  3. \Doctrine\ORM\Query\ParserResult::getParameterMappings() returns its value, which means it is always returning an array;
  4. Doctrine/ORM/Query.php on line 306 calls getParameterMappings() and on line 308 calls count() on its return value, which is guaranteed to be an array;
  5. Doctrine/Tests/ORM/Functional/QueryCacheTest.php was injecting a stub of ParserResult that returned null for all methods but getSqlExecutor();
  6. that means that getParameterMappings() was unexpectedly returning null and causing the above count() to fail on PHP 7.2 since the introduction of this RFC;
  7. the test was fixed by using a mock that only overrides getSqlExecutor().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants