rfc:hook_improvements

PHP RFC: Property hook improvements

  • Version: 0.9
  • Date: 2024-06-28 (use today's date here)
  • Author: Ilija Tovilo (tovilo.ilija@gmail.com), Larry Garfield (larry@garfieldtech.com)
  • Status: Under Discussion

Introduction

While finalizing the implementation of the property hooks RFC patch, we have found a few small areas for improvement. These are nominally changes from the behavior defined in the original RFC, and thus we want to refer them through the RFC process.

Proposal

There are two improvements we propose.

Remove the proactive guard against recursive backing value access

As defined in the original RFC, the following code would have been detected and throw a dedicated error.

class C
{
    public string $val = 'a' {
        get {
          return $this->val . $this->suffix();
        }
    }
 
    public function suffix(): string
    {
        return $this->val;
    }
}
 
$c = new C();
print $c->val;

While looking for optimizations, Ilija found he was able to refactor the hook logic to make it more than twice as fast: While there is still some overhead to a hook compared to a traditional get method, it's been reduced from “close to the cost of __get” to “almost negligible compared to a method.” Enough that it would almost certainly be swamped by the cost of the hook body itself. This is a massive improvement.

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 stack overflow.

We consider the slightly worse error handling fully justified by the performance improvement, and by the fact that the code above is a bug no matter what (it's just a question of when the bug gets reported). But as it is a change from the behavior stated in the previous RFC, we are putting it out for consideration.

Partial support for readonly properties

Support for hooks on readonly properties was omitted from the original RFC, primarily to minimize complexity as there were questions around when it was safe to do. On further consideration, we believe that hooks on backed properties are sufficiently safe to support readonly, but not on virtual properties.

We therefore propose to allow both get and set hooks on readonly properties, if and only if it is a backed property.

The main concern is that readonly implies a property is immutable and idempotent. However, a get hook supports arbitrary code, so technically a developer could do something silly like:

class Dumb
{
    public readonly int $value { get => $this->value * random_int(1, 100); }
}

On the other hand, there is no shortage of dumb things that people can do with PHP already. The exact same silliness could be implemented using __get, for instance. However, there are valid uses for a readonly get hook, especially for ORMs and proxies. For example:

readonly class ProductFromDB
{
    public string $id;
 
    public function __construct(
        public string $name,
        public float $price,
        public Category $category,
    ) {}
}
 
// Generated code.
readonly class LazyProduct
{
    // Assigned via reflection or a closure.
    private DbConnection $dbApi;
 
    // Assigned via reflection or a closure.
    private string $categoryId;
 
    public Category $category {
        get {
            $this->category ??= $this->dbApi->loadCategory($this->categoryId);
        }
    }
}

That is, we feel, an entirely reasonable use of hooks, and would allow for lazy-load behavior per-property on readonly classes.

This is subtly different from the Lazy Proxy RFC, which operates on the whole object at once. We believe both use cases are valuable and should be supported.

A set hook, meanwhile, offers no issue for a backed readonly property. As long as it is backed we are able to determine if it is still uninitialized, and so a second set call would correctly fail as it should.

At this time, Ilija is still investigating whether it is more sensible to put the uninitialized check on the wrapping property, when visibility is checked, or only on the backing property, so the error would be thrown from within the hook body. In practice it will not make much difference to the developer, so we consider either case acceptable depending on what ends up being easier to do.

On balance, we believe the advantages and use cases for a lazy readonly property outweigh the potential for developers to do silly things. For that reason, we propose to allow both get and set hooks on backed readonly properties.

Backward Incompatible Changes

None.

Proposed PHP Version(s)

PHP 8.4.

RFC Impact

Proposed Voting Choices

There will be two independent primary votes, one for each of the improvements suggested above. Please vote for each separately.

Remove the extra hook recursion guard?
Real name Yes No
Final result: 0 0
This poll has been closed.
Permit hooks on backed readonly properties?
Real name Yes No
Final result: 0 0
This poll has been closed.

Patches and Tests

Implementation

After the project is implemented, this section should contain

  1. the version(s) it was merged into
  2. a link to the git commit(s)
  3. a link to the PHP manual entry for the feature
  4. a link to the language specification section (if any)

References

Links to external references, discussions or RFCs

Rejected Features

Keep this updated with features that were discussed on the mail lists.

rfc/hook_improvements.txt · Last modified: 2024/07/01 20:55 by crell