php.net |  support |  documentation |  report a bug |  advanced search |  search howto |  statistics |  random bug |  login
Bug #78904 Uninitialized property triggers __get()
Submitted: 2019-12-03 22:24 UTC Modified: 2020-01-20 19:13 UTC
Votes:1
Avg. Score:5.0 ± 0.0
Reproduced:0 of 0 (0.0%)
From: public at grik dot net Assigned: nikic (profile)
Status: Closed Package: *General Issues
PHP Version: 7.4.0 OS:
Private report: No CVE-ID: None
View Add Comment Developer Edit
Welcome! If you don't have a Git account, you can't do anything here.
You can add a comment by following this link or if you reported this bug, you can edit this bug over here.
(description)
Block user comment
Status: Assign to:
Package:
Bug Type:
Summary:
From: public at grik dot net
New email:
PHP Version: OS:

 

 [2019-12-03 22:24 UTC] public at grik dot net
Description:
------------
Unitialized properties make unpredictabe side effects.

In fact, unitialized property is a Schrödinger property - it both exists and does not exist at the same time.

All popular frameworks use magic getters in base classes.

Having __get() called for uninitialized properties makes initialization in declaration to be obligatory, or face unpredictable side effects, as it was in Pascal 30 years ago.

Scripting languages were inteneded to help avoid obligatory initialization in declaration.

Test script:
---------------
<?php

abstract class Base{
    public function __get($name){echo 'magic';}
}
class A extends Base{
    public string $x;
    public ?string $y;
    public $z;
}
$A = new A;
var_dump(get_object_vars($A)); //array(0) {}
var_dump(property_exists($A,'x'));//true
$A->z; // __get is not executed
$A->y; // magic - __get() is executed
$A->x; //Fatal error

Expected result:
----------------
array(3) {
  ["x"]=>
  NULL
  ["y"]=>
  NULL
  ["z"]=>
  NULL
}
bool(true)

Actual result:
--------------
array(1) {
  ["z"]=>
  NULL
}
bool(true)
magicmagic
Fatal error: Uncaught TypeError: Typed property A::$x must be string, null used

Patches

Add a Patch

Pull Requests

Pull requests:

Add a Pull Request

History

AllCommentsChangesGit/SVN commitsRelated reports
 [2019-12-03 22:40 UTC] requinix@php.net
-Status: Open +Status: Not a bug
 [2019-12-03 22:40 UTC] requinix@php.net
Works as intended. https://wiki.php.net/rfc/typed_properties_v2

Typed properties are never initialized to a default null, unlike untyped properties. That is why $y and $z do not show up through get_object_vars: they are uninitialized. No, it cannot simply return null because (a) the properties do not have the value of null and (b) null may not even be a valid value for the property.

And as happens in some other languages, accessing an uninitialized property is not allowed.

As for the fatal error,
* ->y triggers __get, which returns null, which is valid for the "?string" type. No error.
* ->x triggers __get, which returns null, which is not valid for the "string" type. Error.
 [2019-12-04 06:08 UTC] nikic@php.net
Do you have any particular (not quite so reduced) examples where this causes problems in practice? We had some issues with __set interaction that were addressed, so possibly there is some tweak in behavior possibly here as well, it's just not clear from the bug report what the actual issue is.

But generally, yes, initializing typed properties is obligatory, that's part of the point of having them. __get is called to allow some lazy initialization patterns, but shouldn't be relevant for most purposes.

Is the issue here that __get is called instead of immediately throwing a "trying to access uninitialized property" error?
 [2019-12-06 13:34 UTC] public at grik dot net
@requinix
Restricting access to uninitialised typed variable is OK.
The problem is having the __get() called.


@nikic Yes, I do have. 

1. Consider a Request class from Laravel: https://github.com/laravel/framework/blob/master/src/Illuminate/Http/Request.php#L695

I extend it in an application to add custom behaviour, add a property. When the property is typed, I have a __get() call executed from the framework class, which is unexpected, because it differs from behavoir when the property has no type restriction.

2. __get() is extensively based in ORM implementations to map database data to the "virtual" properties.
https://github.com/illuminate/database/blob/master/Eloquent/Model.php#L1552
When we define a userland class property named after the table field name, we expect the __get() method not to be called. 

Virtial public properties may be a bad practice, but it is promoted extensively in several frameworks.

Having __get() called for uninitialized variables introduces a new variable state, and as such it needs a way to be processed. Same as isset(), property_exists(), we need some property_initialized() function.
 [2019-12-06 14:01 UTC] nikic@php.net
So, sounds like we should have given __get() the same treatment as __set() in how it interacts with uninitialized typed properties. I'm not sure if we can still make this change at this point.
 [2019-12-06 14:19 UTC] public at grik dot net
It would make sense to show a compile-time warning/notice for typed property declarations without initial value. Consider it for a later version, please.
If it is not possible, consider adding the way to check if the property is initialized, so framework authors could process this state in magic getters properly.
 [2019-12-06 14:35 UTC] nikic@php.net
-Status: Not a bug +Status: Re-Opened
 [2019-12-06 14:35 UTC] nikic@php.net
> It would make sense to show a compile-time warning/notice for typed property declarations without initial value. Consider it for a later version, please.

Certainly not. There is nothing wrong with a property that does not have an initial value. In fact nearly all properties shouldn't have one, as most properties get initialized based on constructor arguments.

Based on preliminary discussion we might change __get() behavior, so I'm reopening this. I'll prepare a patch asap.
 [2019-12-06 14:47 UTC] nikic@php.net
The following pull request has been associated:

Patch Name: Also don't call other magic for uninitialized typed properties
On GitHub:  https://github.com/php/php-src/pull/4974
Patch:      https://github.com/php/php-src/pull/4974.patch
 [2019-12-06 15:32 UTC] public at grik dot net
Thank you. Sorry for filling this ticket up improperly, I did not expect attention.
 [2019-12-09 07:38 UTC] nikic@php.net
-Summary: Side-effects for uninitialised typed property +Summary: Uninitialized property triggers __get()
 [2019-12-09 07:43 UTC] nikic@php.net
Automatic comment on behalf of nikita.ppv@gmail.com
Revision: http://git.php.net/?p=php-src.git;a=commit;h=84354c62b37a56816a695b18ebd898f9703a9ad2
Log: Fixed bug #78904: Don't call any magic for uninitialized typed properties
 [2019-12-09 07:43 UTC] nikic@php.net
-Status: Re-Opened +Status: Closed
 [2020-01-17 09:57 UTC] andrzej dot heczko at gmail dot com
I was experimenting with this behavior to allow lazy property initialization. For instance I can lazy load services in controllers. Now there is no other option, how to allow lazy initialize property. I have to stick back to annotations and dynamic properties.

You could add another magic method to allow that e.g. "__init", "__initProperty", "__getStrict", "__getTyped", etc. which would respect access modifiers.

You can see all my efforts in composer package bigbit/oddin. 
Version 1.* is using annotations and dynamic properties, version 2.* is using typed properties and __get.
 [2020-01-17 10:04 UTC] nikic@php.net
-Assigned To: +Assigned To: nikic
 [2020-01-17 10:04 UTC] nikic@php.net
@andrzej: Lazy property initialization is still supported and used by ProxyManager for example. To lazily initialize both typed and untyped properties, you must unset() them in the constructor.
 [2020-01-20 13:19 UTC] andrzej dot heczko at gmail dot com
Thank you for reply.

By doing unset first I have to explicitly put all the unset thing in constructor. In my opinion, this comes with time and resource penalty and makes code less readable.

Before fix (7.4.0):
class A {
  use GetMagicTrait;

  private Foo $a;
  private Bar $b;
  private Baz $z;
}

After fix (7.4.1+):
class A {
  use GetMagicTrait;

  private Foo $a;
  private Bar $b;
  private Baz $z;

  function __construct()
  {
    unset($this->$a, $this->b, $this->z);
  }
}

Anyway I have read the rfc for typed properties briefly and if I understand it correctly, this fix breaks it starting with section https://wiki.php.net/rfc/typed_properties_v2#uninitialized_and_unset_properties.

I would like to explain, what I want to achieve.
As of php7.4 not nullable typed properties can have 3 different states:

1. Not Initialized
2. Not Set (Initialized)
3. Set (Initialized)

So whenever I use unset I'm making its state unset (2) and __get is working.
Problem is, we cannot define typed property as unset, only "Not Initialized" or "Initialized"

I would like to be able to define unset state at the property instead of "Not Initialized". This would avoid using explicit unset in constructors.

I like typescript ! operator so final property definition could use one of:

class A {
  use GetMagicTrait;

  private Foo $a!;
  private Bar! $b;
  private ! Baz $z;
  private unset Bay $f;
}

where "private unset Foo $a" could be equivalent to shorter use with !.

Can I write RFC for this?
Are there any guidelines for RFCs?
What timeline could be expected for implementation?
 [2020-01-20 19:13 UTC] public at grik dot net
Andrzej, dependency injection based on property types is contract programming, while annotation-based injection (you name it lazy loading) is a declarative programming.

These approaches should not be compared cause they follow ideologically different approach to system design. A sample of approaches is Yii vs Symfony.

I don't know they purpose of the code, but in general case I would advice against utilizing undocumented side effects in core code used in production, such as service locator and dependency injection. Low-level hacks are OK for debugging, mocking and testing utilites.
 
PHP Copyright © 2001-2024 The PHP Group
All rights reserved.
Last updated: Fri Apr 19 16:01:27 2024 UTC