Skip to content

Conversation

TimWolla
Copy link
Member

@TimWolla TimWolla commented Sep 8, 2025

This will also fix a memory leak, since the constructor did not free any pre-existing ->uri.

Copy link
Member

@nielsdos nielsdos left a comment

Choose a reason for hiding this comment

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

Classic reinitialization problem that pops up every now and then.
The "uri" property does not actually exist, i.e. the error message leads users to believe there is such a property. They may try accessing it. Even if it were private and a user tries accessing it a user would get Cannot access private property . However, they'd get a different error which seems inconsistent to me.

@TimWolla
Copy link
Member Author

TimWolla commented Sep 8, 2025

Even if it were private and a user tries accessing it a user would get Cannot access private property . However, they'd get a different error which seems inconsistent to me.

I believe a little bit of a leaky abstraction is fine in this case, since this is only triggerable by specially crafted code that a regular user won't come across.

@nielsdos
Copy link
Member

nielsdos commented Sep 8, 2025

For other extensions I've seen a wording used something along the lines of "cannot call constructor twice" or "cannot reconstruct object".

@TimWolla
Copy link
Member Author

TimWolla commented Sep 9, 2025

For other extensions I've seen a wording used something along the lines of "cannot call constructor twice" or "cannot reconstruct object".

That would not really be applicable to the Exception during unserialization, though. Treating the internal URI as a “super private property” sounds least confusing to me. But let's see what Mate thinks.

@nielsdos
Copy link
Member

nielsdos commented Sep 9, 2025

Well, I find the "super private property" confusing ;) I still prefer a (simple) custom exception messag.


?>
--EXPECTF--
Error: Cannot modify readonly property Uri\WhatWg\Url::$uri
Copy link
Member

Choose a reason for hiding this comment

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

FWIW I agree with the change, but I'd also prefer to somehow eliminate the property term from URIs (even the existing ones). People would rightfully scratch their head if they were trying to find these properties.

This will also fix a memory leak, since the constructor did not free any
pre-existing `->uri`.
@TimWolla TimWolla force-pushed the uri-construct-leak-modify branch from 268ae6f to 2168b54 Compare September 11, 2025 21:51
@TimWolla
Copy link
Member Author

I've adjusted the message to:

Cannot modify readonly object of class %s

The URI classes are actual readonly classes in the stub. The “Cannot modify readonly” matches the phrasing of the readonly property modification error and the “object of class” phrasing has precedent in other error messages. This should be sufficiently descriptive without being misleading and without inventing anything entirely new. WDYT?

Copy link
Member

@kocsismate kocsismate left a comment

Choose a reason for hiding this comment

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

I'm fine with the message

@TimWolla TimWolla merged commit d4f5c84 into php:master Sep 12, 2025
9 checks passed
@TimWolla TimWolla deleted the uri-construct-leak-modify branch September 12, 2025 19:00
TimWolla added a commit that referenced this pull request Sep 12, 2025
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