Treat constructors as implicitly void-typed for the purposes of prependCodeToFunctions()

[antecedent]

See #213.

The title assumes use function Patchwork\CodeManipulation\Actions\Generic\prependCodeToFunctions.

[jrfnl]

@antecedent I've been running the BrainMonkey tests with this branch to see whether it would fix the test failures BrainMonkey was seeing on PHP 8.6.

As things were, half the test failures were fixed. Looking at what remained showed me the oversight, namely that the same PHP RFC also forbids returning from a __destruct() method.

I've tried to fix that with the additional two commits (sorry, didn't mean to push to this branch, but as I did, the commits are here now), but while the fix works *, the test I added does not.

* The reason I'm saying the fix works is that the BrainMonkey tests pass on PHP 8.6 with the additional fix in place.

Hope this helps.

[antecedent]

Thank you @jrfnl! It looks like the missing link was NamedObject having no __destruct() defined in the first place.

I found it striking that Patchwork did not throw anything from the redefine('NamedObject::__destruct', ...), and I think you will find it that way too. The class already exists, and the method does not, so it would make sense to assume that it never will, and to throw a relevant exception. I think we should change that in the next minor release. What do you think?

[jrfnl]

Thank you @jrfnl! It looks like the missing link was NamedObject having no __destruct() defined in the first place.

Yes, that would explain it. Sorry I missed that.

Should we also add some tests with an anonymous class with a __construct() and __destruct() method to safeguard that the fix works correctly for such code too ?

I found it striking that Patchwork did not throw anything from the redefine('NamedObject::__destruct', ...), and I think you will find it that way too. The class already exists, and the method does not, so it would make sense to assume that it never will, and to throw a relevant exception. I think we should change that in the next minor release. What do you think?

If you mean that Patchwork should throw an exception when redefine() is called with a function/method name which doesn't exist on the original, then yes, I think that would be helpful for users (though outside the scope of this PR as this is fixing a specific PHP 8.6 issue).

[antecedent]

I added the extra assertions with an anonymous class.

I also discovered that PHP 7.4 changed the behavior of anonymous classes with respect to get_declared_classes(). It seems that since 7.4.0, there are more conditions on an anonymous class being added to get_declared_classes() (i.e. to the list returned by this function). Specifically, it might not be added if it is never instantiated, even if it is in a file that was compiled. Therefore Patchwork's wildcards (like redefine('*::__construct', ...)) theoretically allow capturing anonymous classes across all supported PHP versions, but since PHP 7.4 this works unreliably.

The preceding is also something that we might want to address later, but I think we are good to go with this particular PR?

[jrfnl]

I added the extra assertions with an anonymous class.

👍🏻

I also discovered that PHP 7.4 changed the behavior of anonymous classes with respect to get_declared_classes(). It seems that since 7.4.0, there are more conditions on an anonymous class being added to get_declared_classes() (i.e. to the list returned by this function). Specifically, it might not be added if it is never instantiated, even if it is in a file that was compiled.

Correct. This is a documented backward compatibility break in PHP 7.4: https://www.php.net/manual/en/migration74.incompatible.php#migration74.incompatible.core.get-declared-classes

I'm also now wondering, now you've mentioned get_declared_classes(), whether we need to take PHP 8.1+ enums into account... ?
The get_declared_classes() function includes enums into its return value, but enums cannot declare __construct() or __destruct() methods.

I can see the get_declared_classes() function is called in the ‎getUserDefinedClasses() function and does not filter out enums* and that there is no similar getUserDefinedEnums() function.
* which would be trivial to do with function_exists('is_enum') && is_enum()

I haven't looked into the Patchwork wiring deeply enough to know whether there is an issue here or not, but consider get_declared_classes() is used and returns enums, should we include a test to safeguard that an enum (without __construct() or __destruct() as those methods are not allowed) is not affected by this fix ?

Also: with the above in mind - get_declared_classes() = classes + enums - should more tests be added for other parts of the framework ? (outside the scope of this PR)

Therefore Patchwork's wildcards (like redefine('*::__construct', ...)) theoretically allow capturing anonymous classes across all supported PHP versions, but since PHP 7.4 this works unreliably.

The preceding is also something that we might want to address later, but I think we are good to go with this particular PR?

Agreed. Let's open issues as a reminder for those things moved to "later" and in the mean time, let's get this PR merged and released as it will unblock other projects using Patchwork for testing their PHP 8.6 readiness.