HHVM compatibility: implement declared interfaces #373
Conversation
HHVM does not allow declaration of interface implementation without real implementation of methods which parameters differs from parent class.
|
Hello, thank you for creating this pull request. I have automatically opened an issue http://www.doctrine-project.org/jira/browse/DBAL-609 We use Jira to track the state of pull requests and the versions they got |
|
@javer are there any breakages in PDO versions that may cause trouble with this? |
| */ | ||
| public function query() | ||
| { | ||
| $args = func_get_args(); |
There was a problem hiding this comment.
Shouldn't you just declare a first param on the method and pass it to the parent call?
There was a problem hiding this comment.
@staabm, no, because query method in implemented interface Connection declared without parameters and HHVM throws fatal error:
HipHop Fatal error: Declaration of Doctrine\\DBAL\\Driver\\PDOConnection::query() must be compatible with that of Doctrine\\DBAL\\Driver\\Connection::query() in /path/to/doctrine/dbal/lib/Doctrine/DBAL/Driver/PDOConnection.php on line 30
There was a problem hiding this comment.
So the interface should be adjusted?
There was a problem hiding this comment.
@staabm interface changes in 2.x are denied
|
Connection interface is not compatible with declaration of native PDO class, and I don't know, why is this works in PHP. But HHVM checks implementation more strict and we should find a solution compatible both with PHP and HHVM. Same issues are with interfaces Statement and ResultStatement combining with class PDOStatement. |
| /** | ||
| * {@inheritdoc} | ||
| */ | ||
| public function quote($input, $type=\PDO::PARAM_STR) |
|
this seems to break the tests |
|
Yes, I am trying to find a solution which compatible with both PHP and HHVM. |
|
Done. |
|
Would there be a way to have a |
|
It is only implementation of declared interface, nothing more. As I wrote above, it is a bit strange that PHP allows when methods declarations differ in PDO and Connection without explicit implementation of this methods. It is possible only due to weak checking of inheritance, so it can be treated as "hack" and can be broken in any new version of PHP. HHVM does not have another PDO implementation, it is compatible with PHP PDO. So creating separated class PDOConnection only for HHVM will lead to a lot of changes in source code, because there are many classes which inherit PDOConnection and PDOStatement. |
|
unfortunately the creator of PDO thought it would be wise for PDO to not follow the standard PHP OO model. another solution could be to try and clean that up for PHP 5.6 |
|
With a quick test under HHVM this PR appears to work. Can't speak for normal php at this time, but I assume most others are able to comment on that, and that Travis reporting success also means that the tests run well with normal php. +1 @javer :) |
|
Any timeframe for when this pull request will be merged? cc: @guilhermeblanco |
|
@JoelMarcey I'm getting close to look into pending HHVM support in a few days. |
|
Sorry to pester you @guilhermeblanco , but did you ever take a look at this? Really love doctrine, would love to get it to run under HHVM. :) |
|
I think I know what the issue is on HHVM and it will impact all projects relying on the PDO methods to fulfill their interface applied on a child class: the HHVM definition for PDO uses scalar typehints for its methods (which is possible in HHVM), which means it has a different signature than the official PDO. |
|
@stof I'm not sure what you base that on. Yes, HHVM allows use of scalar type hints, but I don't know how that's relevant to this issue. Doctrine alters the signature of a function, as pointed out by @lsmith77 above. HHVM is strict about this, and refuses to guess which signature is correct, whereas Zend allows multiple signatures, and probably uses some heuristic to determine which one to use. |
|
Hi all, What can we do to get this pull request accepted? I would love to have our HHVM framework test script not to have to rely on a fork to have doctrine2 to test correctly on HHVM. I want to be able to git clone from doctrine itself. |
|
I'm actually 👍 to this - at least for us, the suite seems to pass all tests. @javer can you complete coverage? |
|
cc: @ptarjan |
|
@JoelMarcey I really don't like that it makes the code so ugly. Is there no way to provide two implementations of these classes and have a working one being picked up by HHVM? |
|
@beberlei Assuming you are talking about 7aa02d0 While I am not quite sure what you mean by "ugly" --- all this code is doing is explicitly implementing the functions of the interface that the class is declared to implement ---- an option I guess is to have two classes, one of which is named, let's say: class PDOConnection_HHVM extends PDO implements Connection and a "require" statement based on an "if this is HHVM". Not sure if that is any prettier or not. Beauty is in the eye of the beholder, I suppose. To be honest, I don't care if we have to do that workaround for now, as long as we can reference the doctrine2 github repository "master" branch directly in our testing without a bunch of forks and pull requests. |
|
@JoelMarcey Would that work not to instantiate this classes, if we are on HHVM and it wouldnt complain about them not being compatible? Rephrased, does HHVM care about non compatible code, when its never require'ed? |
|
@JoelMarcey if yes, how do i check for "if this is HHVM". |
|
@JoelMarcey just realized we keep extending from it, so we have to use this approach. |
HHVM compatibility: implement declared interfaces
|
@JoelMarcey We are seeing failures with the default DateTime zone on the travis hhvm builds: https://travis-ci.org/doctrine/dbal |
|
@beberlei Awesome! Thanks for the merge! :) Let me see about the DateTimeZone issues you are having. |
|
@beberlei Are you still seeing failures on travis? Maybe I am missing them... |
|
Travis needs to set a default datetime. You can try hacking around it @JoelMarcey by making a in it |
|
@beberlei already reported: travis-ci/travis-cookbooks#248 |
|
Thanks and very cool. Seems to work in our project now You may add a comment or addition here: https://github.com/facebook/hhvm/wiki/OSS-PHP-Frameworks-Unit-Testing:-Doctrine |
|
@mmicael1 that code was in place because of performance optimizations... I don't mind to see it go, but we should first verify how much is being eaten up. |
|
@mmicael1 IIRC, |
|
@stof I found the thread they talk about: facebook/hhvm#1027 Commit updated af9fbe1 |
|
Could that get merged into the 2.3 branch? |
|
@hardchor 2.3 won't get any new features merged in. |
|
@Ocramius do you have an idea, why i still get this error? https://travis-ci.org/saxulum/saxulum-doctrine-orm-manager-registry-provider/jobs/37031287 |
|
@dominikzogg do you actually use DBAL |
|
@deeky666 oh, ok thx, i missunderstood it, meant its fixed in 2.4 |
HHVM does not allow declaration of interface implementation without real implementation of methods which parameters differs from parent class.