Allow dynamic property creation in macroable classes

[kokoshneta]

I don’t know how common this is overall, but I personally have several macros that add additional dynamic properties on the class instance the macro is called on, and I imagine others probably do as well.

Up until PHP 8.1, that worked fine, but dynamic creation of class properties is deprecated as of PHP 8.2 and will result in a fatal error from PHP 9.0.

I would think that anything that’s macroable should still be allowed to create dynamic properties, so I would update the Macroable trait accordingly. Unfortunately, the easiest solution, to add the AllowDynamicProperties attribute, won’t work since that can’t be applied to traits, but explicitly defining a __set() method works:

trait Macroable {
  …

  public function __set($key, $value) {
    parent::__set($key, $value);
  }
}

(In my testing, the __set() method always seems to be inherited from Illuminate\Container\Container::__set() – can this be relied upon?)

Edit:
Nope, some further testing shows that the parent cannot always be relied upon; in some macros, the current class scope has no parent. That makes this more difficult.

[ju5t]

This is definitely an issue. We're seeing this in some third party packages. Our deprecation log channel was not happy.

But, I wouldn't like to see __set as part of the Macroable trait. I would prefer a type hinted attribute. This is not the greatest example to illustrate it, as it's based on the array helper, but it would allow us to move away from $this:

Arr::macro('some-magic-method', function (Arr $arr, array $array) {
    $arr->exists();
    ....

If the Macroable class has a __set() method, dynamic properties would work as well.

[kokoshneta]

Just having a __set() method in the class unfortunately won’t make dynamic properties work. There are a lot of places that give __set() as one of the possible ways to get around this change, but what they really mean is that you can use __set() and __get() to emulate dynamic properties, by saving and retrieving properties from an array that is then explicitly declared in the class (the same way Laravel models already do). Trying to dynamically set a property inside __set() still causes a deprecation notice.

I don’t really understand what you mean by “a type hinted attribute” as an alternative to dynamic properties – how would your example solve the issue? And why would you want to move away from $this, which (to me) is one of the major strengths of macros?

I do agree that having a __set() method in a trait is not really something to aim for, as it may override the class’s own __set() method, which is obviously bad. But I can’t think of a different way to do it either.

[ju5t]

I don’t really understand what you mean by “a type hinted attribute” as an alternative to dynamic properties – how would your example solve the issue? And why would you want to move away from $this, which (to me) is one of the major strengths of macros?

When you define a macro in a service provider, $this would be the service provider. PHP would not know that it's the class where you're applying the macro to. Type hinting is great for your editor, but PHP has no clue.

I would like the Macroable trait to work similarly to the Conditionable trait. I wouldn't want to drop $this, I want it to be part of the function call in your callable. This would make it easier to type hint in your function and for PHP to parse it.

Something along the lines of:

MyClass::macro('magic', fn (MyClass $myClass) => $myClass->foo = 'bar');

In this case, we're not making $this magically available, we're passing it to the closure as a parameter, where it's now available as $myClass or any other variable name you'd like. If MyClass has a __set-method, this example should just work. This would be a breaking change though.

For clarity, this is how the same macro would look at the moment:

MyClass::macro('magic', fn () => $this->foo = 'bar');

It's all untested though, I'm brainstorming and typing out what I would like :-)

[kokoshneta]

Oh, I see what you mean now! That would be quite a breaking change, though – it would essentially invalidate all existing macros, and apart from making the reference to the object being manipulated more transparent, I don’t really see any significant advantage to it.

And I still don’t see how it would solve the problem with dynamic properties. It would work if MyClass in your example either explicitly declares the property $foo or is built like this:

class MyClass {
  private $dynamicProperties = [];

  public function __set($key, $value) {
    $this->dynamicProperties[$key] = $value;
  }

  public function __get($key) {
    return $this->dynamicProperties[$key];
  }
}

– but otherwise, the macro will still trigger the deprecation warning. Since the Macroable trait is used by lots of Laravel’s own classes, many of which do not have magic getters and setters akin to the ones above, macros would fail or succeed depending on which class you’re macroing. This, for example, would trigger the warning still:

Collection::macro('magic', fn (Collection $object) => $object->foo = 'bar');

– since the \Illuminate\Support\Collection class doesn’t have a magic setter.

If you’re macroing one of your own classes, there’s no problem, because you can always make sure to address the issue yourself in the class being macroed (just apply the AllowsDynamicProperties attribute and you won’t even need setters). The problem is when macroing classes that you don’t control, because you can’t address the issue in userland then – it needs to be addressed in Laravel itself.

An alternative, stop-gap solution would be to apply the AllowDynamicProperties attribute manually to all classes that use the Macroable trait. That would do it, but it would still mean that adding macros that make use of dynamic properties is a two-step process: (1) make sure the targeted class allows dynamic properties, (2) make the macro.

Not sure if there’s any way around that since PHP 8.2, though.

[ju5t]

If it's up to me, Macroable would only allow dynamic properties if the class that uses it supports it through __set(). And yes, that means a pretty hefty change, but one that's needed moving forward. If there are classes that require it, support for dynamic properties should be enabled for that class in the framework itself by implementing __set() on for example the Collection.

They are dropping dynamic properties for a reason after all.

And yes, passing the class to the callable instead of binding it as $this, is a breaking change but one that I have no problems with. It would not only make things more transparent, it would support dynamic properties for those classes that support it, contrary to the current situation. As PHP will natively understand your code. You can always keep some backwards compatibility by binding $this in the callable, but I would drop it myself as it's a breaking change already.

This is not up to me though, just sharing my thoughts.

[henzeb]

If your macro is the only one ever accessing that property, you could also use a static property inside the function.

function () {
    static $property;

    return $property ??='something heavy';
} 

Be aware that this is globally though.

Not a big fan of using __get and __set as a solution as that would break potentially a lot of stuff as Laravel is leaning on them a lot.

[kokoshneta]

In my case, once the properties are set in the macro, they are frequently retrieved from various other places in the code; and the same property may be set on different instances of the same targeted class (e.g., different Collection instances).