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

HHVM compatibility #580

Merged
merged 2 commits into from
Jul 27, 2016
Merged

HHVM compatibility #580

merged 2 commits into from
Jul 27, 2016

Conversation

goetas
Copy link
Collaborator

@goetas goetas commented May 9, 2016

  • reflection incompatibility between hhvm & php

@goetas goetas changed the title HHVM compatibility and PHPUnit [WIP] HHVM compatibility and PHPUnit May 9, 2016
@schmittjoh
Copy link
Owner

Please also check the Scrutinizer issue, we might need to adjust the exception it looks like.

@stof
Copy link
Contributor

stof commented May 9, 2016

tests are not green on HHVM

@stof
Copy link
Contributor

stof commented May 9, 2016

allow to use symfony validator 2.2 and newer

Would be great to also allow the new Validator API available in Symfony 2.5+ (which is the only one in 3.0+)

@goetas goetas force-pushed the hhvm branch 2 times, most recently from 2600a3d to a503b0a Compare May 9, 2016 15:40
@goetas
Copy link
Collaborator Author

goetas commented May 9, 2016

@schmittjoh issues on scrutinizer errors are because HHVM introduces a new public property on the reflection object

@goetas
Copy link
Collaborator Author

goetas commented May 9, 2016

@stof sorry for the mess, moved the validator feature to a separate PR #583

@goetas
Copy link
Collaborator Author

goetas commented May 9, 2016

Currently making the serializer fully compatible with HHVM 3.6 is not easy.
With HHVM 3.7, tests are green.

It looks that 3.6 has a buggy implementation of the SplStack class

@schmittjoh
Copy link
Owner

@goetas, yes aware of that, but I was referring to the one about the exception. Did you check that, too?

@schmittjoh
Copy link
Owner

3.6 seems quite outdated, I think we should not try to make that compatible if it has problems.

@goetas
Copy link
Collaborator Author

goetas commented May 9, 2016

@goetas
Copy link
Collaborator Author

goetas commented May 9, 2016

3.6 seems quite outdated, I think we should not try to make that compatible if it has problems.

totally agree. but travis allows only HHVM 3.6. I'm not aware of a way to install a different version

@schmittjoh
Copy link
Owner

I was referring to this one here: https://scrutinizer-ci.com/g/schmittjoh/serializer/inspections/f09be7fe-fd8f-4b2b-b833-45fd17ee7358/issues/files/src/JMS/Serializer/EventDispatcher/Subscriber/SymfonyValidatorSubscriber.php?status=new&orderField=path&order=asc&honorSelectedPaths=0

Maybe we need to keep the allow_failures for now and wait for an image update. It's unfortunate, but working around bugs of outdated HHVM versions sounds not so good.

@goetas
Copy link
Collaborator Author

goetas commented May 9, 2016

Ok, then will keep this PR alive till travis do not update HHVM

@goetas
Copy link
Collaborator Author

goetas commented Jul 27, 2016

blocked by facebook/hhvm#7193 (but 0d6997b#diff-e8213d9df27c4ee083659ea8fadc9423R350 is a workaround)


group: edge
sudo: required
dist: trusty
Copy link
Contributor

Choose a reason for hiding this comment

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

this should be done only for the HHVM build rather than for PHP builds too, as the docker-based infra is faster to boot. See how it is done in Symfony

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cool! will do

@goetas goetas changed the title [WIP] HHVM compatibility and PHPUnit HHVM compatibility and PHPUnit Jul 27, 2016
@goetas goetas changed the title HHVM compatibility and PHPUnit HHVM compatibility Jul 27, 2016
@goetas
Copy link
Collaborator Author

goetas commented Jul 27, 2016

Finally is green! (3.7 and later)

@schmittjoh schmittjoh merged commit e0704d3 into schmittjoh:master Jul 27, 2016
@schmittjoh
Copy link
Owner

Great, Thanks!

@goetas goetas deleted the hhvm branch July 27, 2016 12:47
@goetas goetas added this to the v1.2 milestone Aug 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants