Skip to content

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

Open
antecedent wants to merge 5 commits into
masterfrom
213-php-86-returning-a-value-from-a-constructor-is-deprecated
Open

Treat constructors as implicitly void-typed for the purposes of prependCodeToFunctions()#214
antecedent wants to merge 5 commits into
masterfrom
213-php-86-returning-a-value-from-a-constructor-is-deprecated

Conversation

@antecedent

Copy link
Copy Markdown
Owner

See #213.

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

@antecedent antecedent linked an issue Jul 27, 2026 that may be closed by this pull request

@jrfnl jrfnl left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@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.

@jrfnl
jrfnl force-pushed the 213-php-86-returning-a-value-from-a-constructor-is-deprecated branch from 273d773 to 6aee010 Compare August 15, 2026 22:40
@antecedent

antecedent commented Aug 16, 2026

Copy link
Copy Markdown
Owner Author

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

jrfnl commented Aug 16, 2026

Copy link
Copy Markdown
Collaborator

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).

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.

PHP 8.6 | Returning a value from a constructor is deprecated

2 participants