explicit __construct call for \DateTimeImmutable object would break immutable

[sasezaki]

Description

The following code:

<?php
$date = new \DateTimeImmutable('1970-01-01 00:00:00.000000');
$date->__construct('2000-01-01 00:00:00.000000');

var_dump($date);

Resulted in this output:

object(DateTimeImmutable)#1 (3) {
  ["date"]=>
  string(26) "2000-01-01 00:00:00.000000"
  ["timezone_type"]=>
  int(3)
  ["timezone"]=>
  string(16) "Europe/Amsterdam"
}

https://3v4l.org/hnnIE

But I expected this output instead:

Fatal error: Uncaught Error: Cannot modify readonly property DateTimeImmutable::$date

I did learn this issue when reading this slide.
https://speakerdeck.com/twada/growing-reliable-code-phperkaigi-2022?slide=84

According to the author of this slide, twada, he noticed this problem himself. (after reading https://www.simonholywell.com/post/2017/03/php-and-immutability/ )

I propose to give readonly to each of the properties of \DateTimeImmutable.

PHP Version

PHP 8.1.4

Operating System

No response

[cmb69]

I propose to give readonly to each of the properties of \DateTimeImmutable.

That would require to actually have these properties, but so far their state is tracked internally. Something like the following might solve the issue, though:

 ext/date/php_date.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/ext/date/php_date.c b/ext/date/php_date.c
index d607a07420..db21ba21a5 100644
--- a/ext/date/php_date.c
+++ b/ext/date/php_date.c
@@ -2208,6 +2208,9 @@ PHPAPI bool php_date_initialize(php_date_obj *dateobj, const char *time_str, siz
 	int options = 0;
 
 	if (dateobj->time) {
+		if (dateobj->std.ce == date_ce_immutable && flags & PHP_DATE_INIT_CTOR) {
+			zend_throw_exception_ex(NULL, 0, "nonono");
+		}
 		timelib_time_dtor(dateobj->time);
 	}
 	if (format) {

@derickr, what do you think?

[TimWolla]

That would require to actually have these properties,

That does not appear to help with internal classes. The SensitiveParameterValue::$value property is readonly, but calling the __construct method allows to change it:

$ sapi/cli/php test2.php
<?php

readfile(__FILE__);

echo "\n\n=================\n\n";

$v = new SensitiveParameterValue('secret');
var_dump($v->getValue());
$v->__construct('changed');
var_dump($v->getValue());


=================

string(6) "secret"
string(7) "changed"

[bwoebi]

I am wondering of how much of a problem this actually is. Calling __construct directly is basically openly saying "I know what I am doing".

In my opinion it's more of an academical problem than actually a problem in practice. There is, in my opinion, nothing wrong with giving users powerful tooling, allowing them to re-initialize state, as long as it's not part of the default contract.
For me it's comparable to changing a private value via reflection.

I do not really consider this a bug, rather a feature.

[cmb69]

That does not appear to help with internal classes.

The constructor's implementation circumvents the readonly protection. Instead of just replacing the zval, the write_property handler of the object would need to be called.

diff

 Zend/zend_attributes.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Zend/zend_attributes.c b/Zend/zend_attributes.c
index 9f7b8f0144..f060e9d23c 100644
--- a/Zend/zend_attributes.c
+++ b/Zend/zend_attributes.c
@@ -109,7 +109,9 @@ ZEND_METHOD(SensitiveParameterValue, __construct)
 		Z_PARAM_ZVAL(value)
 	ZEND_PARSE_PARAMETERS_END();
 
-	ZVAL_COPY(OBJ_PROP_NUM(Z_OBJ_P(ZEND_THIS), 0), value);
+	zend_string *name = zend_string_init("value", sizeof("value")-1, 0);
+	Z_OBJ_P(ZEND_THIS)->handlers->write_property(Z_OBJ_P(ZEND_THIS), name, value, NULL);
+	zend_string_release(name);
 }
 
 ZEND_METHOD(SensitiveParameterValue, getValue)

[sasezaki]

Hi, all.

The problem with this issue is that a class named Immutable is not immutable using standard PHP functionality.
Of course, there is no way to prevent this, since it can be rewritten by extensions such as Reflection.
But the barrier to change is high.

With PHP 8.1, I thought that an immutable class could be coded as follows Therefore, I was proposing a change to grant readonly.

<?php

final class OtherDateTimeImmutable
{
    public function __construct(
        private readonly string $date    
    ) {}
}

$date = new OtherDateTimeImmutable('now');
$date->__construct('yesterday'); // Fatal error: Uncaught Error: Cannot modify readonly property OtherDateTimeImmutable::$date

However, in retrospect, I think this is an encapsulation violation, since the internal structure of the DateTimeImmutable class is known to the user.
(As derick comments on #8152 (comment), It is not intended that each property of DateTimeImmutable be public.)

Therefore, as per cmb69's first comment, it would be preferable to throw an exception if it is initialized.

[derickr]

internal structure of the DateTimeImmutable class is known to the user.

That is not true. It encapsulates a lot more things then the three properties that you see when doing serialisation or var_dump().

These properties should indeed be read only, but that wasn't functionality that didn't exist until PHP 8.1.

I am also unsure as to whether we should do anything here. I don't really want them as "real properties" (they're more like SQL's materialised views), and marking them "readonly" is only possible from 8.2 onwards, as we can't really break existing code (calling __construct() itself) either.

As such, I agree with @bwoebi that this is similar to using reflection overriding a private property, and that we shouldn't really do anything here, even though it isn't technically 100% encapsulated.

[sasezaki]

internal structure of the DateTimeImmutable class is known to the user.

That is not true.

!!!
Excuse me. It was my sentence mistakes. really sorry.

What I wanted to say is that "I think this is an encapsulation violation, since the internal structure of the DateTimeImmutable class is should not known to the user." (lack of "should not" when posting)

[nicolas-grekas]

I agree with @bwoebi and @derickr personally. "Encapsulation" relates to the public API of some software design. A constructor doesn't belong to what "encapsulation" refers to. Yes, you can mutate the object. So does reflection with private properties. Calling the constructor is like using reflection to me: do it if you know why you shouldn't 🤷 ...

[sasezaki]

Thank you feedbacks, all.

I agree with @bwoebi 's point that I do not really consider this a bug, rather a feature.

I do not really consider this a bug that should be addressed in 8.0, 8.1.

Therefore, I hope it will be addressed in 8.2 or 9 if possible.

[hormus]

The bug exists in the value by reference while with clone by value the constructor does not change the reference with the exception of the object variable used for the construct.

<?php

$tz = new \DateTimeZone('Europe/Rome');
$dateoriginal = new \DateTimeImmutable('1970-01-01 00:00:00.000000', $tz);
$old = $dateoriginal;
$date = (clone $dateoriginal); // DateImmutable::clone from php >= 5.5.0
var_dump($date);
$date->__construct('2000-01-01 00:00:00.000000', $tz);
var_dump($date, $dateoriginal, $old);

Correct:

object(DateTimeImmutable)#3 (3) { ["date"]=> string(26) "1970-01-01 00:00:00.000000" ["timezone_type"]=> int(3) ["timezone"]=> string(11) "Europe/Rome" } object(DateTimeImmutable)#3 (3) { ["date"]=> string(26) "2000-01-01 00:00:00.000000" ["timezone_type"]=> int(3) ["timezone"]=> string(11) "Europe/Rome" } object(DateTimeImmutable)#2 (3) { ["date"]=> string(26) "1970-01-01 00:00:00.000000" ["timezone_type"]=> int(3) ["timezone"]=> string(11) "Europe/Rome" } object(DateTimeImmutable)#2 (3) { ["date"]=> string(26) "1970-01-01 00:00:00.000000" ["timezone_type"]=> int(3) ["timezone"]=> string(11) "Europe/Rome" }

<?php

$tz = new \DateTimeZone('Europe/Rome');
$dateoriginal = new \DateTimeImmutable('1970-01-01 00:00:00.000000', $tz);
$old = $dateoriginal;
$date = $dateoriginal;
var_dump($date);
$date->__construct('2000-01-01 00:00:00.000000', $tz);
var_dump($date, $dateoriginal, $old);

incorrect:

object(DateTimeImmutable)#2 (3) { ["date"]=> string(26) "1970-01-01 00:00:00.000000" ["timezone_type"]=> int(3) ["timezone"]=> string(11) "Europe/Rome" } object(DateTimeImmutable)#2 (3) { ["date"]=> string(26) "2000-01-01 00:00:00.000000" ["timezone_type"]=> int(3) ["timezone"]=> string(11) "Europe/Rome" } object(DateTimeImmutable)#2 (3) { ["date"]=> string(26) "2000-01-01 00:00:00.000000" ["timezone_type"]=> int(3) ["timezone"]=> string(11) "Europe/Rome" } object(DateTimeImmutable)#2 (3) { ["date"]=> string(26) "2000-01-01 00:00:00.000000" ["timezone_type"]=> int(3) ["timezone"]=> string(11) "Europe/Rome" }

Regardless of the read-only property, it must change the reference inside the variable container and not any one even if of the same value but different variable container.

[derickr]

@hormus You're just changing the same object in your last example, so of course they update "both".

[derickr]

I'm closing this as we've come to the conclusion that this is the same as working around encapsulation through Reflection, and that calling __construct() is not a normal thing to do.