Member

I think this is a good idea, and the keyword final is quite appropriate here. Java has final class/method/property, while C# doesnt have the final keyword and instead use other keywords like 'sealed' and 'readonly'. From the aspect of consistency, the keyword final is very suitable for immutable properties too.

Member Author

@HallofFamer Unfortunately, I won't propose final as a keyword in the RFC :(

In PHP, final currently prevents a class or a method to be overridden. In the sense of this, using final for "immutable" properties would be a mistake.

Member

This doesn't implement the "references are not allowed" part yet, right?

Maybe I missed it in RFC, but what happens when you call next($this->c); ? Will it move the internal pointer (of an array) or throw an exception?

Member Author

@mart-k I've just done a quick test, and actually, next() throws an exception. :( I think it should work since it's needed for reading, so I'll add a test case for this and then try to fix the issue. Also I'll modify the RFC to mention the expected behaviour. Thank you for pointing this out!

Member

@kocsismate Err, why would next() work? It modifies the array, so it should not be permitted.

Note that foreach(), ArrayIterator etc all work independently of the internal array pointer and can work fine with a readonly property.

Member Author

Note that foreach(), ArrayIterator etc all work independently of the internal array pointer and can work fine with a readonly property.

Oh, I thought that the internal pointer is used during iteration, so my reasoning was that then next() should be allowed. Sorry for this. But the testcase + another example come in handy indeed.

One of the reasons why I pointed next() out is that there are whole family of similar functions that read/write array's "internal pointer". For example what happens if you call current()? It is read only and doesn't change the pointer, but if next(), each() etc. give error, will allowing current() even make sense?

Member Author

@mart-k Calling current() currently works. My opinion is that it doesn't make sense to disallow those functions that only read the pointer... But Nikita should be more informed.

Member

@kocsismate Right, I don't see why these need any special consideration. They are normal by-value or by-reference functions, depending on whether they perform modification or not.

What about clonable objects? There is a popular #withX() pattern which first clones an object, and then assigns the prop of cloned object new value, therefore preserving immutability. It is used e.g. in PSR-7 interfaces (methods like withHeader()).

Like this:

<?php
class C
{
    writeonce public int $prop;
    public function __construct(int $prop)
    {
        $this->prop = $prop;
    }
    public function withProp(int $prop): self
    {
         $that = clone $this;
         $that->prop = $prop; // will fail?

         return $that;
    }
}
$instance = new C(1);
$newInstance = $instance->withProp(2);

Member Author

@someniatko Cloning is currently under debate. My point of view is that it's not a prerequisite for writeonce/readonly properties since you can create a completely new instance instead of using clone. It's not that comfortable though, but I'd like to come up with a way to support mutability of immutable properties. :)

IMO, this suggestion is extremely unsettling. Probably on the level of singletons - it produces a sudden behavior change that can't be explained by surrounding code logic.

If you want special actions taken for writing data to the property, a more generic approach can be taken.

Something like

[static] property [typehint] $name [write methodName];

where methodName is a class method with signature identical to __set() except for matching typehints. F.e. function(string $name, ?typehint $value).
If no setter method declared, it will be public read-only property. If a setter is declared, an attempt to write to the property outside the object/class will be redirected to the setter.

It is arguable if a setter requires $name in its arguments, but I think this can increase usability of the feature by allowing for setter reuse without writing a lot of proxy methods.

What would happen with property reflection? I haven't read about reflection in the RFC, I guess it should be taken into account and add a method to identify whether a property is immutable or not.

Member Author

@Aaronidas yeah, I'll definitely add a new ReflectionProperty::isReadonly() method. It's also worth a mention in the RFC. Thanks for the reminder.

@kocsismate i think you should be consistent and name it isWriteonce(), please.

Member Author

@someniatko sorry, I was a bit unclear: the actual method name depends on the outcome of the secondary vote which is about the keyword choice.

Member

Choose a reason for hiding this comment

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

I'm missing tests for the "lazy initialization" pattern, that is unset the uninit property and use __get() to perform initialization.

I also saw that you do allow acquiring references via foreach-by-ref -- is that intentional? Basically, we should either be allowing all references (and preventing assignments to them) or forbidding all references. Allowing some references combines the disadvantages of both :)

Member Author

@nikic My newest commit addresses some of your concerns, like adding the missing tests and inheritance checks. Thanks to your help I think I managed to make the "foreach-by-ref"-related tests work.

However, lazy initialization didn't yet work because zend_verify_prop_assignable_by_ref() is called when calling getters and I implemented the "Cannot modify final properties by reference" check there. I am not sure if I did the right thing, but I disabled this validation in this very case, and now it's all good... I guess the magic_unset_success.phpt test doesn't work because of a similar reason (and that's why it says reference while in fact it isn't involved).

Member

I'm missing some tests for $obj->prop += 1 or similar. I think it might not be handled right now.

These tests might not be very useful because all of them boil down to simple assignments from the perspective of "write-once" properties

Member

One more thing to test would be by-ref passing. For example sort($obj->prop).

This is still a work in progress part

Contributor

Choose a reason for hiding this comment

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

Could add tests involving unserialize() and final properties - For example, setting final properties with or without defaults in __unserialize() or __wakeup(), or no magic methods at all, or after unserialization (should throw if defined, and not throw if undefined).
Unserializers have been problematic, e.g. for typed properties https://bugs.php.net/bug.php?id=79002 was changed in 7.4.2. I'd guess the implementation probably works, though

Also, are there tests of references to fields of properties (e.g. what is the intended behavior for preg_match('/abc/', 'abcdef', $x->prop['field'])) - I skimmed the tests and didn't see any

Contributor

Other edge cases that might need extra code in the unserializer, or warrant tests (I'd definitely think they'd be fixable if they have bugs):

• Malformed data which sets the final property twice should throw (probably already handled?) (O:7:"MyClass":2:{s:9:"finalProp";i:2;s:9:"finalProp";i:3;})

  An annoying edge case would be unserializing properties that already have defaults - maybe those objects shouldn't throw at all (e.g. in __wakeup or __unserialize) until unserialization is over.

• The unserializer creating final properties as references (e.g. due to being serialized with an older class definition which didn't use final)
  'O:7:"MyClass":2:{s:9:"finalProp";i:2;s:7:"refProp";R:2;}' for $x->refProp = &$x->finalProp;

I can imagine serialize() being used with final properties (e.g. to load from the database once and cache in memcache).

I'd naively guess the below snippet might throw an exception the way this is currently implemented, and if it doesn't, it'd still be useful to have the test to ensure behavior doesn't accidentally change in the future.

<?php
class X {
    final public $hasDefault = ['v7'];
}
$x = new X();
$s = serialize($x);
// Desired: This should not throw.
var_dump(unserialize($x));

(I maintain https://github.com/igbinary/igbinary , a compact binary serializer for php)

Member Author

@TysonAndre I've started to add tests for serialization. I can say that

• Malformed data which sets the final property doesn't throw an exception (yet)
• Final properties that have a default value can be unserialized

I'll continue tomorrow :)

Member Author

@TysonAndre Today's findings:

The unserializer creating final properties as references (e.g. due to being serialized with an older class definition which didn't use final)

A serialized data like that is currently possible to unserialize. However, these properties can be currently modified via reference. :/.

Also, are there tests of references to fields of properties (e.g. what is the intended behavior for preg_match('/abc/', 'abcdef', $x->prop['field'])) - I skimmed the tests and didn't see any

Currently, an exception is thrown in this case with a message of "Cannot modify final property Foo::$property1 after initialization". I added a test for this case.

Contributor

A serialized data like that is currently possible to unserialize. However, these properties can be currently modified via reference. :/.

ext/standard/var_unserializer.re has a check for whether it's possible to assign a value to a typed property while unserializing - I assume a similar error should be thrown if it was write-once. Not completely familiar with this part of the code.

		if (UNEXPECTED(info)) {
			if (!zend_verify_prop_assignable_by_ref(info, data, /* strict */ 1)) {
				zval_ptr_dtor(data);
				ZVAL_UNDEF(data);
				zval_dtor(&key);
				goto failure;
			}
			if (Z_ISREF_P(data)) {
				ZEND_REF_ADD_TYPE_SOURCE(Z_REF_P(data), info);
			}
		}

Contributor

@kocsismate The RFC seems to be not accepted, do you know why and do you plan a revision/revote?

Member Author

@mvorisek I believe these were the main reasons against: some people would prefer property accessors, and some people disliked the fact that cloning doesn't play well with readonly properties.

I'll certainly try to revisit this for PHP 8.1. :)

Old school style

<?php
class Point {
    protected int $x;
    protected int $y;
    public function __construct(int $x, int $y = 0) {
        $this->x = $x;
        $this->y = $y;
    }
}

PHP 8.0.0 style

<?php
class Point {
    public function __construct(protected int $x, protected int $y = 0) {
    }
}

This is all good but I still need to write getters. And I'm disappointed that I don't see properties in the class description

<?php
class Point {
    public function __construct(protected int $x, protected int $y = 0) {
    }
    public function getX(): int
    {
        return $this->x;
    }
    public function getY(): int
    {
        return $this->y;
    }
}

That would be a great option for example if I want to use a class for DTO

<?php
class Point {
    readonly public int $x;
    readonly public int $y;
}

Example

<?php
class Point {
    readonly public int $x;
    readonly public int $y;
}
$point = new Point();
$point->x = 1;
$point->y = 1;
$arr = (array)$point;
echo json_encode($arr);

@kocsismate I will be glad to see this RFC in 8.1