eloquent / phony

Mocks, stubs, and spies for PHP.
http://eloquent-software.com/phony
MIT License
194 stars 7 forks source link

PHP 8.1 internal method return type deprecation notices #255

Closed keksa closed 1 year ago

keksa commented 1 year ago

I've got a bunch of deprecation notices when mocking exceptions, or classes implementing Countable, IteratorAggregate or ArrayAccess.

Return type of PhonyMock_ConnectionException_89::__wakeup() should either be compatible with Exception::__wakeup(): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice
Return type of PhonyMock_TemplateRenderException_90::__wakeup() should either be compatible with Exception::__wakeup(): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice
Return type of PhonyMock_Exception_93::__wakeup() should either be compatible with Exception::__wakeup(): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice
Return type of PhonyMock_Collection_119::count() should either be compatible with Countable::count(): int, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice
Return type of PhonyMock_Collection_119::getIterator() should either be compatible with IteratorAggregate::getIterator(): Traversable, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice
Return type of PhonyMock_Collection_119::offsetExists($a0) should either be compatible with ArrayAccess::offsetExists(mixed $offset): bool, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice
Return type of PhonyMock_Collection_119::offsetGet($a0) should either be compatible with ArrayAccess::offsetGet(mixed $offset): mixed, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice
Return type of PhonyMock_Collection_119::offsetSet($a0, $a1) should either be compatible with ArrayAccess::offsetSet(mixed $offset, mixed $value): void, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice

I've managed to track the cause to this RFC - https://wiki.php.net/rfc/internal_method_return_types

The return types will be fully in the reflection in PHP 9, but PHP 8.1 only includes them as "tentative return types" and the FunctionSignatureInspector::signature() method can't handle it.

For example the \Exception::__wakeup() $functionString looks like this:

Method [  public method __wakeup ] {

  - Parameters [0] {
  }
  - Tentative return [ void ]
}

The tentative return type is not handled by the FunctionSignatureInspector::RETURN_PATTERN regex, so the code for mock class is generated without return type and the deprecation notice gets triggered.

If I understand the RFC correctly, this will stop being a problem in PHP 9.0 as the reflection will just show it as straight up return type, but PHP 9.0 is still ways ahead.

ezzatron commented 1 year ago

Thanks for the report. By the looks of it, it should be fairly trivial to modify FunctionSignatureInspector to ignore any return type that's prefixed with Tentative. Although by just looking at it, I'm not entirely sure how it's possible that RETURN_PATTERN is matching lowercase return when there's no case-insensitive i modifier on the pattern 🤔

I might be able to take a look this weekend. If you're interested in submitting a PR, that would be welcome. But fair warning, I haven't touched this project in quite a while, so I don't know what other PHP 8 problems are lying in wait.

keksa commented 1 year ago

I might have explained it incorrectly, the problem is that it's not matching the tentative return. To prevent the issue, it should match it, or maybe just match it as a fallback.

For example, the Exception::__wakeup() is currently generated like this. (Writing from memory, so consider this as pseudo code.)

function __wakeup() {...}

Which triggers the deprecation notice.

To prevent the notice, it should be generated like this.

function __wakeup(): void {...}
keksa commented 1 year ago

@ezzatron I've opened a work-in-progress PR fixing this and couple more PHP 8.1 issues. There are still some failing tests due to PHP 8.1 changes, but I don't have the time to look into it now. Feel free to use the code however you want, it might at least help you a bit.

ezzatron commented 1 year ago

These particular problems are now fixed on main. There are a bunch of other issues still to tackle in #265 before PHP 8.1 & 8.2 are properly supported, but at least the test suite finishes and CI is back up and running.

Thanks for your work on #256, it was really helpful 👍