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

PHP 5 doesn't need these, and later versions warn #8

Closed
wants to merge 1 commit into from

Conversation

mal
Copy link
Contributor

@mal mal commented Jul 27, 2015

Managed to commit without these when throwing together the previous PR! >_>;;

@jakoch
Copy link
Member

jakoch commented Jul 27, 2015

Some isReference() related tests fail after this change.
If you have the time, it would be nice, if you commit some follow-up fixes for the broken tests.

@mal
Copy link
Contributor Author

mal commented Jul 28, 2015

Hmmm, seems those fails are valid. Looking at them I also suspect that that last PR has introduced some potential pit falls. I think perhaps the original PR was the way to go after all.

Problem is those ampersands are needed (objects are implicitly byref in PHP5, but scalars and arrays are not). Unfortunately that means it can't inherit (warnings to do with differing method signatures) so the separate class and just tweaking the isExpectation method was probably the safest change afterall!

Recommend reopening #5 and reverting #7 merging #11.

Sorry for the confusion 😞

@mal mal closed this Jul 28, 2015
@mal mal deleted the fix-ampersands branch July 28, 2015 00:51
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

2 participants