Skip to content

Conversation

@kocsismate
Copy link
Member

@kocsismate kocsismate commented May 31, 2025

  • Property hooks may now throw exceptions, that seem to be forgotten to be handled (?)
  • The $previous and $trace properties are private, and they were not accessible from the constructor of a child class

- Property hooks may now throw exceptions, that seem to be forgotten to be handled
- The $previous and $trace properties are private, and they were not accessible from the constructor of a child class
@kocsismate kocsismate force-pushed the fix-exception-constructor branch from 06357db to a259d88 Compare May 31, 2025 10:30
?>
--EXPECTF--
string(3) "bar"
int(1)
Copy link
Member Author

@kocsismate kocsismate May 31, 2025

Choose a reason for hiding this comment

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

The tests may be a little complicated, but I tried to make it visible that properties are now not modified after an exception is thrown (the $code property remains 1, instead of 2 etc.)

@nielsdos
Copy link
Member

nielsdos commented May 31, 2025

Good find. I don't really like the fact that we need to check for exceptions, I don't think it would be a big deal to not do this. (but probably best to do this anyway for consistency)
Anyway: the exceptions checks probably also need to be done on 8.4, no?

@kocsismate
Copy link
Member Author

kocsismate commented May 31, 2025

I don't think it would be a big deal to not do this.

Yes, not a huge issue, but I agree with the consistency stuff. Also, the if (code) calls were already a little shady when Nikita added them, now it may be even more unexpected that a hook doesn't run when a parameter is not passed.

Anyway: the exceptions checks probably also need to be done on 8.4, no?

Oh, I wasn't aware that the change was already added for 8.4. Yes, good idea, I'll take care of adding the missing exception checks for PHP 8.4.

@nielsdos
Copy link
Member

I'll pull and play with this in a bit

@kocsismate
Copy link
Member Author

I'll pull and play with this in a bit

Yes, feel free to push directly to this branch if something needs to be changed.

@kocsismate kocsismate merged commit c045ba3 into php:master May 31, 2025
8 of 9 checks passed
kocsismate added a commit that referenced this pull request May 31, 2025
These property writes may now throw exceptions because of property hooks, and this was not handled previously.
kocsismate added a commit that referenced this pull request May 31, 2025
* PHP-8.4:
  Backport relevant changes of #18719
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants