Hi folks. As Ilija's been polishing off hooks to get the PR merged, we've run into two small revisions that should make life better for all involved. One is a performance improvement that requires a very slight error handling behavior change, and the other is enabling readonly in selected (but probably all of the relevant) circumstances.
I'd say we expect these to be uncontroversial, but this is PHP. :-) So I will instead just note that it's a short RFC and open the discussion accordingly.
https://wiki.php.net/rfc/hook_improvements
--
Larry Garfield
larry@garfieldtech.com
On Mon, Jul 1, 2024 at 7:05 PM Larry Garfield larry@garfieldtech.com
wrote:
Hi folks. As Ilija's been polishing off hooks to get the PR merged, we've
run into two small revisions that should make life better for all
involved. One is a performance improvement that requires a very slight
error handling behavior change, and the other is enabling readonly in
selected (but probably all of the relevant) circumstances.I'd say we expect these to be uncontroversial, but this is PHP. :-) So I
will instead just note that it's a short RFC and open the discussion
accordingly.https://wiki.php.net/rfc/hook_improvements
--
Larry Garfield
larry@garfieldtech.com
"A side effect of that optimization, however, is that we cannot proactively
detect the bug above. Instead, it would result in an infinite loop, which
would eventually trigger a a stack overflow."
This got a small typo ("a a" at the end). This reads no different to me
than infinite recursion between 2 methods. It honestly does not bother me,
and any static analysis tool will be able to pick this anyway. Sounds like
a free performance gain to me!
"A side effect of that optimization, however, is that we cannot
proactively detect the bug above. Instead, it would result in an
infinite loop, which would eventually trigger a a stack overflow."This got a small typo ("a a" at the end).
Fixed, thanks.
This reads no different to me
than infinite recursion between 2 methods. It honestly does not bother
me, and any static analysis tool will be able to pick this anyway.
Sounds like a free performance gain to me!
Yep, it would be exactly 2 method recursion, and the engine would naturally treat it as such.
--Larry Garfield
Hi
I'd say we expect these to be uncontroversial, but this is PHP. :-) So I will instead just note that it's a short RFC and open the discussion accordingly.
Big fan of the performance improvement, given that I convinced Ilija to
look into that once more. Really happy with what he found there.
For the second part about supporting readonly: I don't want to rush this
in the last few weeks before feature freeze, especially since the word
'investigating' appears in the RFC text. Given that readonly is
currently entirely unsupported, this could very well be added with PHP
8.5, allowing folks to really think about the implications and how it
interacts with stuff. That's a "no" from my side.
Best regards
Tim Düsterhus
PS: Small typo: '$this-dbApi->loadCategory($this->categoryId);', '>'
missing there after 'this-'.
Hi
I'd say we expect these to be uncontroversial, but this is PHP. :-) So I will instead just note that it's a short RFC and open the discussion accordingly.
Big fan of the performance improvement, given that I convinced Ilija to
look into that once more. Really happy with what he found there.For the second part about supporting readonly: I don't want to rush this
in the last few weeks before feature freeze, especially since the word
'investigating' appears in the RFC text. Given that readonly is
currently entirely unsupported, this could very well be added with PHP
8.5, allowing folks to really think about the implications and how it
interacts with stuff. That's a "no" from my side.
"Investigating" here is a small term, honestly. Ilija just hadn't had a chance to poke at it to see where is more feasible. I expect it would be a day or so at most to sort that part out; we just wanted to get the discussion going given the timing, and he didn't have time over the weekend. :-)
Are there specific concerns you have about implications, or just a general "dude, it's July" sense?
Best regards
Tim DüsterhusPS: Small typo: '$this-dbApi->loadCategory($this->categoryId);', '>'
missing there after 'this-'.
Fixed, thanks.
--Larry Garfield
Le 1 juil. 2024 à 19:02, Larry Garfield larry@garfieldtech.com a écrit :
Hi folks. As Ilija's been polishing off hooks to get the PR merged, we've run into two small revisions that should make life better for all involved. One is a performance improvement that requires a very slight error handling behavior change, and the other is enabling readonly in selected (but probably all of the relevant) circumstances.
I'd say we expect these to be uncontroversial, but this is PHP. :-) So I will instead just note that it's a short RFC and open the discussion accordingly.
https://wiki.php.net/rfc/hook_improvements
--
Larry Garfield
larry@garfieldtech.com
Hi,
-
Removing the guard against recursion is sensible to me, as the sort of bug it is supposed to prevent is not specific to property accessors. IMO, a better solution to the issue is to implement a global upper limit on the call stack size. Currently, I am able to generate a call stack of more than 10 millions items before an OOM error occurs; PHP should have thrown a stack overflow error long before that. But this is entirely orthogonal to property hooks.
-
As for readonly, I think that the invariant it is supposed to provide should be enforced as strictly as possible. It means that
readonlyis only acceptable if there is nogethook.
The primary reason I would want readonly with get hook, is lazy initialisation, and I think that there is better design than to trust the user for not implementing bugs. For that use case, I think it is preferable to implement dedicated hook for lazy initialisation, orthogonal to the readonly feature. Here is what I would suggest (in a separate RFC, of course):
- An additional hook, called
init, may be added to backed properties. When attempting to read the value of the backing store, if it is uninitialised, then theinithook is triggered, which is supposed to initialise it.
In general, a hooked property can be marked as readonly when it is backed and has no get hook. In particular, it can have an init hook, which is all we need in order to have a lazy readonly property that is robust against bugs by construction.
—Claude
Le 3 juil. 2024 à 11:54, Claude Pache claude.pache@gmail.com a écrit :
- As for readonly, I think that the invariant it is supposed to provide should be enforced as strictly as possible. It means that
readonlyis only acceptable if there is nogethook.
Hi,
One more thing, why I think that we should be strict here. It is not just for preventing the user to write dumb code, it is also for preventing them to write creative code, e.g.
class doc {
public readonly int page {
get => $this->page + $this->offset;
}
private int $offset = 0;
public function __construct(int $page) {
$this->page = $page;
}
public function foo() {
// $this->offset may be adjusted here
}
}
—Claude
Le 3 juil. 2024 à 11:54, Claude Pache claude.pache@gmail.com a écrit :
- As for readonly, I think that the invariant it is supposed to provide should be enforced as strictly as possible. It means that
readonlyis only acceptable if there is nogethook.Hi,
One more thing, why I think that we should be strict here. It is not just for preventing the user to write dumb code, it is also for preventing them to write creative code, e.g.
class doc { public readonly int page { get => $this->page + $this->offset; } private int $offset = 0; public function __construct(int $page) { $this->page = $page; } public function foo() { // $this->offset may be adjusted here } }—Claude
I don't see anything wrong with that code. $page doesn't say "immutable" it says "readonly", as in "cannot write to".
— Rob
Le 3 juil. 2024 à 14:42, Rob Landers rob@bottled.codes a écrit :
Le 3 juil. 2024 à 11:54, Claude Pache claude.pache@gmail.com a écrit :
- As for readonly, I think that the invariant it is supposed to provide should be enforced as strictly as possible. It means that
readonlyis only acceptable if there is nogethook.Hi,
One more thing, why I think that we should be strict here. It is not just for preventing the user to write dumb code, it is also for preventing them to write creative code, e.g.
class doc { public readonly int page { get => $this->page + $this->offset; } private int $offset = 0; public function __construct(int $page) { $this->page = $page; } public function foo() { // $this->offset may be adjusted here } }—Claude
I don't see anything wrong with that code. $page doesn't say "immutable" it says "readonly", as in "cannot write to".
— Rob
The fact you don’t see anything wrong is precisely the issue. According to the original RFC 1, the intention of readonly properties was immutability, not just non-writability. For the weaker case of non-writability, it was explicitly mentioned the possibility of (future) asymmetric property visibility.
That said, I agree that “immutable” would have been a better word choice than “readonly” for conveying the original intention. We could also ignore the original intention and reinterpret the feature in case everyone agrees (personally, I prefer the original intention, but I won’t fight for that).
—Claude
Le 3 juil. 2024 à 14:42, Rob Landers rob@bottled.codes a écrit :
Le 3 juil. 2024 à 11:54, Claude Pache claude.pache@gmail.com a écrit :
- As for readonly, I think that the invariant it is supposed to provide should be enforced as strictly as possible. It means that
readonlyis only acceptable if there is nogethook.Hi,
One more thing, why I think that we should be strict here. It is not just for preventing the user to write dumb code, it is also for preventing them to write creative code, e.g.
class doc { public readonly int page { get => $this->page + $this->offset; } private int $offset = 0; public function __construct(int $page) { $this->page = $page; } public function foo() { // $this->offset may be adjusted here } }—Claude
I don't see anything wrong with that code. $page doesn't say "immutable" it says "readonly", as in "cannot write to".
— Rob
The fact you don’t see anything wrong is precisely the issue. According to the original RFC [1], the intention of readonly properties was immutability, not just non-writability. For the weaker case of non-writability, it was explicitly mentioned the possibility of (future) asymmetric property visibility.
That was before properties could be functions? Now I suppose we have to decide if the definition of read-only doesn't mean read-only, but immutability; despite the original intention.
That said, I agree that “immutable” would have been a better word choice than “readonly” for conveying the original intention. We could also ignore the original intention and reinterpret the feature in case everyone agrees (personally, I prefer the original intention, but I won’t fight for that).
Agree. I just wanted to point out that there were two interpretations: the English definition, and the original intention of the RFC.
— Rob
Le 3 juil. 2024 à 14:42, Rob Landers rob@bottled.codes a écrit :
Le 3 juil. 2024 à 11:54, Claude Pache claude.pache@gmail.com a écrit :
- As for readonly, I think that the invariant it is supposed to provide should be enforced as strictly as possible. It means that
readonlyis only acceptable if there is nogethook.Hi,
One more thing, why I think that we should be strict here. It is not just for preventing the user to write dumb code, it is also for preventing them to write creative code, e.g.
class doc { public readonly int page { get => $this->page + $this->offset; } private int $offset = 0; public function __construct(int $page) { $this->page = $page; } public function foo() { // $this->offset may be adjusted here } }—Claude
I don't see anything wrong with that code. $page doesn't say "immutable" it says "readonly", as in "cannot write to".
— Rob
The fact you don’t see anything wrong is precisely the issue. According to the original RFC [1], the intention of readonly properties was immutability, not just non-writability. For the weaker case of non-writability, it was explicitly mentioned the possibility of (future) asymmetric property visibility.
That was before properties could be functions? Now I suppose we have to decide if the definition of read-only doesn't mean read-only, but immutability; despite the original intention.
That said, I agree that “immutable” would have been a better word choice than “readonly” for conveying the original intention. We could also ignore the original intention and reinterpret the feature in case everyone agrees (personally, I prefer the original intention, but I won’t fight for that).
Agree. I just wanted to point out that there were two interpretations: the English definition, and the original intention of the RFC.
— Rob
In better words, as a dev, I don't care which interpretation is chosen, as long as it is consistent.
— Rob