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

Compatible with facebook HHVM 3.2 #52

Merged
merged 1 commit into from
Jun 26, 2014
Merged

Compatible with facebook HHVM 3.2 #52

merged 1 commit into from
Jun 26, 2014

Conversation

goetas
Copy link
Member

@goetas goetas commented Jun 17, 2014

This patch to make HTML5 compatible with HHVM contains also workaround to avoid this facebook/hhvm#2962

@goetas goetas changed the title [WIP] Compatible with facebook HHVM 3.1 Compatible with facebook HHVM 3.1 Jun 17, 2014
@goetas
Copy link
Member Author

goetas commented Jun 18, 2014

@mattfarina @technosophos What you think about this?
Except the facebook workaround, there is no big changes...

@@ -7,6 +7,7 @@
</testsuites>
<filter>
<blacklist>
<file>systemlib.phpreflection_hni</file>
Copy link
Member

Choose a reason for hiding this comment

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

I'm not all that familiar with HHVM. What is systemlib.phpreflection_hni and why is it blacklisted but not part of the repo?

Copy link
Member Author

Choose a reason for hiding this comment

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

it is a phpunit/codecoverage limitation

I have commented it here https://github.com/goetas/html5-php/commit/f902b05cc252721e5440f3437d0c5de3e0a8d789#comments

It could blacklisted by default, but it is a @sebastianbergmann issue

Choose a reason for hiding this comment

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

Of course I could take care of this in PHP_CodeCoverage. But IMHO it should be taken care of in HHVM. HHVM's code coverage API should not expose data for systemlib*.

@ptarjan @sgolemon

Copy link

Choose a reason for hiding this comment

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

Yeah, we can (and should) fix it. Can you cook up a small example and submit an issue please?

Choose a reason for hiding this comment

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

@sebastianbergmann @ptarjan I've opened an issue here: facebook/hhvm#2992

@mattfarina
Copy link
Member

HHVM support will be a nice thing. I'm glad to see you're working on it.

@goetas goetas changed the title Compatible with facebook HHVM 3.1 Compatible with facebook HHVM 3.2 Jun 26, 2014
@goetas
Copy link
Member Author

goetas commented Jun 26, 2014

Squashed all commits into one.
@mattfarina @technosophos I consider this PR as ready to be merged

@technosophos
Copy link
Member

It looks good to me. This patch has all the new error reporting calls, so I'm happy to get that in.

@goetas goetas merged commit 45e0f80 into Masterminds:master Jun 26, 2014
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.

None yet

6 participants