Skip to content

[RFC] Allow hooks for backed readonly properties #18757

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

NickSdot
Copy link
Contributor

@NickSdot NickSdot commented Jun 4, 2025

@TimWolla TimWolla added the RFC label Jun 4, 2025
@NickSdot NickSdot force-pushed the allow-readonly-hooks branch 3 times, most recently from 1a21998 to 6840873 Compare June 6, 2025 02:22
@NickSdot NickSdot force-pushed the allow-readonly-hooks branch from da64264 to 24f2687 Compare June 7, 2025 16:23
@NickSdot NickSdot force-pushed the allow-readonly-hooks branch from c06c915 to 12b986a Compare June 7, 2025 16:28
@NickSdot NickSdot changed the title Allow hooks for backed readonly properties [RFC] Allow hooks for backed readonly properties Jun 7, 2025
int(42)
Cannot modify readonly property Test::$prop
Cannot modify protected(set) readonly property Test::$prop from global scope
int(42)
Copy link
Member

Choose a reason for hiding this comment

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

Please make sure to include the trailing newline at EOF (also applies to other tests).

Copy link
Member

Choose a reason for hiding this comment

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

This test does not appear to belong to this PR.

Copy link
Member

Choose a reason for hiding this comment

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

ditto

Cannot modify readonly property Test::$prop
Cannot modify protected(set) readonly property Test::$prop from global scope
int(42)

Copy link
Member

Choose a reason for hiding this comment

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

this appears to be a newline too many.

try {
$t->set(43);
} catch (Error $e) {
echo $e->getMessage(), "\n";
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
echo $e->getMessage(), "\n";
echo $e::class, ": ", $e->getMessage(), PHP_EOL;

For consistency with other tests (applies to the entire PR).

Copy link
Member

Choose a reason for hiding this comment

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

Unrelated test.

}
?>
--EXPECTF--
Fatal error: Hooked virtual properties cannot be readonly in %s on line %d
Copy link
Member

Choose a reason for hiding this comment

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

This error message is misleading.

}
?>
--EXPECTF--
Fatal error: Hooked virtual properties cannot be readonly in %s on line %d
Copy link
Member

Choose a reason for hiding this comment

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

This error message is misleading.

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.

3 participants