PHP 8 support

[ezzatron]

Hey, if you're here about PHP 8 support, please leave a 🚀.

I haven't had a lot of time to work on Phony lately, so PHP 8 support is not available yet. Sorry about that. I'm working on PHP 8 support over on the php-8 branch. The first step is to get the existing feature set running under PHP 8, then release a new version ASAP. After that, I'll look into new features to compliment the new stuff in PHP 8.

Due to the complex nature of this project, I don't really expect any PRs to help out too much, but if you're really keen to contribute, by all means please do.

Thanks for your patience 🙏

[jmalloc]

Would you consider tagging a version that works under PHP v8 before adding version 8 specific features? Let me know if there's anything I can to to help out.

[ezzatron]

I was trying to just "get it working" first, but unfortunately I think all of the built-in PHP classes and functions were updated to use union types. Anything with union types breaks the mock generation code. Even if I could release a version that worked under PHP 8 more quickly that way, I feel like people would run into issues with union types almost immediately.

The FunctionSignatureInspector either needs a full rewrite, or just to be replaced with "normal" reflection. Then anything that depends on it, like mock generation and function hook generation needs updating also.

[ezzatron]

I've made some decent progress here. The FunctionSignatureInspector got a rewrite to support union types, and I managed to ditch some old baggage from previous PHP versions / HHVM at the same time. I wouldn't be surprised if it's a bit faster in some cases. As a result of that rewrite, the test suite now runs to completion (no more fatal errors, but still regular errors and failures).

There are still some issues to tackle:

1. Some of the functions I chose for testing function hooks (which drive stubGlobal() and spyGlobal()) seem to have received new argument type and return type definitions in PHP 8, which is breaking some tests. Unfortunately the "expected" results now need to differ when running the tests under PHP 7 vs. PHP 8. That sucks, so it might be better to find some other built-in functions whose types are stable across both versions as better examples to use in the test suites.
2. The EmptyValueFactory needs to be updated to support union types. It currently just assumes that the ReflectionType object it gets passed is an instance of ReflectionNamedType, but as of PHP 8 it can now also be an instance of ReflectionUnionType. Choosing the "best" empty value for a union type could be moderately complicated. It should probably prefer returning an empty string when the type is object|string, for example. Annoyingly, neither of these ReflectionType subclasses are present in the PHP docs.
3. PHP 8 no longer seems to allow setting dynamic properties on generators. This breaks generator spies, which set a property named _phonySubject on the spy "wrapper" generator itself, allowing the pair of generators to be associated to one another without keeping some kind of global register of generator spies and their inner "original" generators. I... don't know what to do about this one yet. Keeping a global register would basically be a memory leak waiting to happen.

Issue no. 1 is probably the easiest to tackle, followed closely by no. 2. Issue no. 3 I'm open to suggestions for.

[jmalloc]

3. Keeping a global register would basically be a memory leak waiting to happen.

Perhaps PHP 8's WeakMap could be used here? Assuming you can have a weak reference to a generator. Obviously that would mean a different implementation for PHP 7 vs PHP 8 :/

[ezzatron]

Yeah, that's probably the way to go, if it works. Might roll with a simple abstraction to handle the PHP version differences.

[ezzatron]

I've implemented support for union types into EmptyValueFactory. The implementation feels... too simple, but it's working for now.

Basically, for built-in types, PHP 8 reflection seems to partially canonicalize union types, and it just so happens to canonicalize the types in a way that puts the simpler, better choices for empty values at the end of the canonical version of the type declaration. Meaning that I can just look at the last type in a union and use that to decide which empty value to return.

I say "partially canonicalize" because PHP 8 doesn't seem to exhibit the same behaviour for regular old classname-based union types. That means that an empty type for a type definition of ClassA|ClassB would be a mock of ClassB, but an empty type for ClassB|ClassA would be a mock of ClassA instead, since I just look at the last type. I think I'm okay with not guaranteeing anything about which class would be returned in this instance.

The Union Types 2.0 RFC does actually state that:

The getTypes() method returns an array of ReflectionTypes that are part of the union. The types may be returned in an arbitrary order that does not match the original type declaration. The types may also be subject to equivalence transformations.

For example, the type int|string may return types in the order [“string”, “int”] instead. The type iterable|array|string might be canonicalized to iterable|string or Traversable|array|string. The only requirement on the Reflection API is that the ultimately represented type is equivalent.

Meaning that the behaviour I'm relying upon could change, but I have tests in place that should catch any important changes. If that happens, I can always implement an explicit priority system that matches the current behaviour, at the cost of some performance.

So yay, more progress! But also I found a bunch more stuff to fix along the way:

• Return types are currently not parsed by the FunctionSignatureInspector. I don't know why, because it makes perfect sense to do it at the same time since return type information is right there in the same string used to parse the parameters. It's either shift responsibility into FunctionSignatureInspector and fix it there, or fix it in multiple places throughout MockGenerator. Additionally, this will save a bunch of method calls, further improving performance.
• This isn't strictly PHP 8 related, but In addition to the documented types there's also a super-secret, super-stupid parent type declaration which Phony probably doesn't support right now. The self and parent types are a pain to handle because PHP doesn't resolve them to actual types for you. Using reflection "properly" doesn't change anything either (proof). It might be better to supply the class name and parent class name directly from MockGenerator into SignatureInspector->signature(), since MockGenerator already has these available.
• The docs also now state that static is a possible return type in PHP 8. Need to add support for that, but should be simple.
• Need to check the impact of named arguments. It's unlikely to affect the compatibility of generated mocks / hooks, but if you call a mocked method expecting the argument name to be foo, when Phony has actually used a1 as the name, it might result in a bad experience.

That's all I can remember right now.

[jmalloc]

Meaning that the behaviour I'm relying upon could change, but I have tests in place that should catch any important changes. If that happens, I can always implement an explicit priority system that matches the current behaviour, at the cost of some performance.

I would suggest documenting this behavior as totally undefined within Phony's API for now. That is, don't make any guarantees unless you actually implement that priority system since it seems like this behavior could change even in a patch release of PHP that doesn't happen to be in your test matrix.

[ezzatron]

a patch release of PHP that doesn't happen to be in your test matrix.

I agree with your overall point, but my tests run on a cron with no composer.lock file, and currently include a pre-release build too. It's pretty unlikely I'll miss a whole patch release.

[ezzatron]

Everything except named arguments seems to be working as intended now:

• Union types are supported in parameters and return types.
• Generator-to-generator-spy association now uses a WeakMap under PHP 8, and dynamic properties under PHP 7.
• New types like false and static are working.
• The parent type is also supported now.

Unfortunately named arguments support looks like it's going to be a can of worms. Some code assumes that arguments collected by a variadic parameter always present like a sequential array, when they can now have string keys in addition to the usual integer keys. I'm sure there are other dragons to slay too.

I will probably make a release with the current changes, since it should be enough to get users' current test suites working in most cases. But I don't really know how to "frame" the release. It's definitely not "full support for PHP 8", it's only "partial" support at best. And users may quickly be disappointed to discover that named arguments are not yet working. I'm extra worried about managing expectations because it will have to be a major release since I'm dropping support for PHP 7.2 at the same time.

[typhonius]

But I don't really know how to "frame" the release. It's definitely not "full support for PHP 8", it's only "partial" support at best. And users may quickly be disappointed to discover that named arguments are not yet working. I'm extra worried about managing expectations because it will have to be a major release since I'm dropping support for PHP 7.2 at the same time.

Thank you for your work on this issue and this library. Have you considered cutting a alpha/beta/rc tag for a new major version with PHP 8 support? I haven’t checked to see how much it may clash with minimum stability config, but it would set expectations clearly that it’s not 100% complete whilst also getting a working release out.

[jmalloc]

Thanks Erin!

What exactly does it mean for named arguments to not be supported? Is it that the generated code does not use the same parameter names and therefore can not be called using named arguments?

FWIW, I don't think it's problematic to release a new major version without "full PHP 8 support"; especially if you could be somewhat confident that support for named parameters is possible within future minor release. Though, even if that's not possible the "upgrade burden" for this release is low (unless you're actually using PHP 7.2, I suppose), so I wouldn't be concerned about making an additional major release in the near future.

As far as managing expectations, I would say you can frame the release solely in terms of the individual features that Phony has gained support for, and if need be list the lack of support for named parameters as a "known issue" in the changelog.

Such a release is still a win for existing users, but of course I am biased because I am one who can benefit from having my current test suites pass under PHP 8. If i'm being pragmatic, a pre-release tag would certainly get me by, too. 😄

[ezzatron]

Thank you for your work on this issue and this library. Have you considered cutting a alpha/beta/rc tag for a new major version with PHP 8 support? I haven’t checked to see how much it may clash with minimum stability config, but it would set expectations clearly that it’s not 100% complete whilst also getting a working release out.

Thanks for the feedback! That's not a bad idea. After consideration though, as @jmalloc points out, the named arguments support would probably be added in a future minor release. Even if it requires more BC breaks and another major version bump, well, so be it I guess. That's what SemVer is all about after all.

What exactly does it mean for named arguments to not be supported? Is it that the generated code does not use the same parameter names and therefore can not be called using named arguments?

Spies seem to "just work", but stubs throw an undefined index exception as soon as they're called with named arguments. I didn't get as far as testing actual mocks with regards to whether they need to start using "real" parameter names, but they use stubs so they're already basically guaranteed to be broken.

[ezzatron]

Basic PHP 8 support is now available in:

• eloquent/phony 5.0.0
• eloquent/phony-phpunit 7.1.0

Something's not working under CI with the Kahlan version, even though it's working locally. Gonna have to tackle that later.

[ezzatron]

Kahlan support available as of eloquent/phony-kahlan 4.0.0.

Closing this issue as all maintained versions of Phony now support PHP 8, with the exception of support for named arguments. If you're interested in that, you can follow #251.