Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for "decorator" pattern #5168

Open
wants to merge 1 commit into
base: master
from
Open

Add support for "decorator" pattern #5168

wants to merge 1 commit into from

Conversation

@nikic
Copy link
Member

nikic commented Feb 11, 2020

This is an incomplete prototype for better "decorator" support. The basic problem this solves is this: We generally encourage the use of composition over inheritance, but currently using inheritance is simply much more convenient...

interface Foo {
    public function method1(int $a): void;
    public function method2(): int;
    public function method3(string $b): string;
    public function method4(): stdClass;
    // ...
}

A basic decoration pattern for such an interface would look something like this:

class ComposedFoo implements Foo {
    private Foo $inner;
    public function __construct(Foo $inner) { $this->inner = $inner; }
    public function method1(int $a): void {
        $this->inner->method1($a);
    }
    public function method2(): int {
        return $this->inner->method2($a);
    }
    public function method3(string $b): string {
        return $this->inner->method3($b);
    }
    public function method4(): stdClass {
        return $DO_SOMETHING_DIFFERENT_HERE;
    }
}

That is, we need to proxy through a large number of methods to the decorated object. Using __call() is not an alternative, because it loses the method signatures, and does not allow implementing the interface.

This proposal reduces the above example to:

class ComposedFoo implements Foo {
    private decorated Foo $inner;
    public function __construct(Foo $inner) { $this->inner = $inner; }
    public function method4(): stdClass {
        return $DO_SOMETHING_DIFFERENT_HERE;
    }
}

The property is marked as decorated, which means that we will automatically add forwarding methods for all public methods of Foo, unless they are explicitly overridden in the class. These methods will have full signatures, so they still satisfy the interface.

Some notes:

  • Only one decorated property is allowed, at least for now. I could be convinced that multiple make sense, but that comes with issues on how conflicts are handled.
  • The decorated property must have a single class/interface type (no union types).
  • Only the public method interface is forwarded. Forwarding public properties would in principle also make sense, but would only be possible once we have a first-class "property accessor" feature.
  • The "override" rules are similar to traits: That is, method declared in the class wins over method from decorated class, and method from decoared class wins over inherited parent class.
@nikic nikic changed the title Add support fore "decorator" pattern Add support for "decorator" pattern Feb 11, 2020
@fruitl00p

This comment has been minimized.

Copy link

fruitl00p commented Feb 11, 2020

This assumes only the public interface is decorated / exposed to ComposedFoo ?

@nikic

This comment has been minimized.

Copy link
Member Author

nikic commented Feb 11, 2020

Yes, only the public (method) interface is decorated. It would be good if public properties were forwarded as well, but we would have to add support for 1st class "property accessors" first.

@nikic nikic added the RFC label Feb 11, 2020
@pcrov

This comment has been minimized.

Copy link
Contributor

pcrov commented Feb 11, 2020

Will methods returning $this still break out of these decorators or are they (or can they be) special-cased to automatically retain them?

@nikic

This comment has been minimized.

Copy link
Member Author

nikic commented Feb 11, 2020

Will methods returning $this still break out of these decorators or are they (or can they be) special-cased to automatically retain them?

Interesting case. As-is, if you decorate a fluent interface, the inner $this is going to leak through.

Not sure how that could be addressed best (and whether it needs to be addressed at all...) We could remap the inner $this to the outer one in return values automatically, but that seems like altogether too much magic. A way to make it more explicit is the : $this return type that has recently been discussed, and only remap for those (as it's strictly required there).

@pcrov

This comment has been minimized.

Copy link
Contributor

pcrov commented Feb 11, 2020

A way to make it more explicit is the : $this return type that has recently been discussed, and only remap for those (as it's strictly required there).

That would be ideal, yes. Though in its absence I think making fluent interfaces suck less is worth the extra bit of magic here anyway.

/* Forwards to the generated method. Note that unlike CALL_TRAMPOLINE, this is going to insert
* a proper stack frame. */
ZEND_VM_HANDLER(195, ZEND_CALL_DECORATED, ANY, ANY)
{

This comment has been minimized.

Copy link
@cmb69

cmb69 Feb 11, 2020

Contributor
Suggested change
{
{
USE_OPLINE
@javiereguiluz

This comment has been minimized.

Copy link
Contributor

javiereguiluz commented Feb 11, 2020

Personally I'd find the following syntax easier to understand in this case:

class ComposedFoo decorates Foo {
    private Foo $inner;
    public function __construct(Foo $inner) { $this->inner = $inner; }
    public function method4(): stdClass {
        return $DO_SOMETHING_DIFFERENT_HERE;
    }
}

But I don't know if it's feasible or desired to do that. Thanks.

@nikic

This comment has been minimized.

Copy link
Member Author

nikic commented Feb 11, 2020

@javiereguiluz Decoration must be tied to a property, because it has to forward to that property.

@brzuchal

This comment has been minimized.

Copy link
Contributor

brzuchal commented Feb 11, 2020

Personally I find it uncommon to add property modifier which can be used only once.
How about magic methods? for eg. self::__decorate($decorated) call in constructor and decorated modifier in front of a class declaration?
With this approach, if $decorator is needed, it can be assigned by a user to any class property if not then it'll just be used for method proxying purposes.
For eg.:

interface Foo {
    public function method1(int $a): void;
    public function method2(): int;
}

decorated class ComposedFoo implements Foo {
    public function __construct(Foo $decorated) {
        self::__decorate($decorated);
    }
    public function method2(): int {
        return 5;
    }
}

Then a call to (new ComposedFoo($foo))->method2() will execute ComposedFoo::method2() method but (new ComposedFoo($foo))->method1() will invoke $foo->method1() under the hood.


With above in mind we could introduce ReflectionDecoratedClass with method like getDecorated() which provide decorated instance without traversing through all properties to check where is decorated one instance saved.

@jackbentley

This comment has been minimized.

Copy link

jackbentley commented Feb 11, 2020

As with others, I think the syntax is a bit off. I think the reason is that adding the decorated attribute to the property makes the property responsible for how other parts/methods of the class works - which is a little weird.

Similar to @brzuchal comment. Have we considered adding a new type of object? i.e. interface/trait/class/decorator. This makes the concept easier to understand - given that it could magically pull in the properties.

Alternatively, a magic class which can be extended might be another option.

Could we consider creating an RFC? Seems like this functionality has interest.

@HallofFamer

This comment has been minimized.

Copy link

HallofFamer commented Feb 11, 2020

I think the delegation idea from Kotlin is a good alternative.
https://kotlinlang.org/docs/reference/delegation.html

@brzuchal

This comment has been minimized.

Copy link
Contributor

brzuchal commented Feb 11, 2020

@HallofFamer looks interesting and it can work with multiple interfaces where methods are derived from multiple constructor args.
Worth noting is what I've found about Kotlin delegates:

A class can implement multiple interfaces and delegate its functionality to one or more objects
A class with multiple interfaces that extend a common parent can delegate each interfaces’ implementation but must override functions defined in the parent
A class that implements a child interface can delegate to its parent and only needs to add functions defined in the child.

@Crell

This comment has been minimized.

Copy link

Crell commented Feb 11, 2020

I am also very much in support of this functionality, although I agree the syntax could use improvement.

The single-object limitation seems possibly problematic. Is it that there's only one decorated object allowed, or only one per interface? Eg, would this be legal:

Eg:

class ComposedFooBar implements Foo, Bar {
    private decorated Foo $aFoo;
    private decorated Bar $aBar;
    public function __construct(Foo $aFoo, Bar $aBar) { 
        $this->aFoo = $aFoo;
        $this->aBar = $aBar;
    }
    public function method4(): stdClass {
        return $DO_SOMETHING_DIFFERENT_HERE;
    }
}

Regarding methods that return $this, I'm not sure of the correct way to handle it; however, I will note that if you want to return the wrapping object it's not that hard to do it the same as now:

class ComposedFooBar implements Foo, Bar {
    // ...
    public function method4(): stdClass {
        $this->aFoo->method4();
        return $this;
    }
}

So it's not fatal; and it's easier to work around than the other way around, I think.

Nitpick: Is decorated really the best keyword here? As a native English speaker the subject/verb doesn't feel like it's lining up nicely, but I don't have an obvious alternative readily at hand.

@HallofFamer

This comment has been minimized.

Copy link

HallofFamer commented Feb 11, 2020

@brzuchal yup the fact that a class can implement multiple delegates is a brilliant solution that easily solves the problem with decorator which seem to be able to only 'decorate' one object. Also this feature has been battle-tested in Kotlin and is well loved by the community.

@nikic

This comment has been minimized.

Copy link
Member Author

nikic commented Feb 11, 2020

@Crell Yeah, I came to the same conclusion regarding multiple decorated properties. Should be supported, and any conflicts should require that method to be explicitly declared in the class, in which case it can choose to forward to one or the other, or do something else entirely.

@jdpanderson

This comment has been minimized.

Copy link

jdpanderson commented Feb 11, 2020

This idea is excellent, and this is functionality I would love to see in PHP almost exactly as-is. However, IMHO, it really should use a keyword other than decorated.

As some of the comments above allude to, I don't think the proposed syntax is directly related to the decorator pattern. Using the term delegate would be closer. My understanding is that the decorator pattern is adding functionality on top of existing functionality, thus decorating the existing functionality. In this implementation either the function call is delegated to the inner object, or the outer object overrides the inner's function call; Neither case is decoration. You have to explicitly call $this->inner->someMethod() to make it a decorator.

I.e. it's much closer to Kotlin's delegate pattern. This would be a slam dunk if the syntax were changed to:

class ComposedFoo implements Foo {
   private delegated Foo $inner; // or delegate
   // The rest identical to the original proposal
}
@brzuchal

This comment has been minimized.

Copy link
Contributor

brzuchal commented Feb 11, 2020

@nikic would it be possible in future to lazy load decorated property instance and compute it before first use?
The case I think of is when derived class gets only some input or even not then decorated property should be computed before first method call. Except if method call is implemented in derived class.

@marandall

This comment has been minimized.

Copy link

marandall commented Feb 11, 2020

I wonder if we couldn't get a lot more bang for the buck by using a mystery magical method:

class Foo decorates Bar { 
   private Bar $bar;

   private function __getDecorator(string $class_id) { 
       if ($class_id === Bar::class) { 
          return $this->bar;
       }
    }
}

Or for an insta lazy proxy for a public interface:

class Foo decorates Bar { 
   private Bar $bar;
   private BarFactory $bar_factory;

   private function __construct(BarFactory $factory) { 
       $this->bar_factory = $factory;
   }

   private function __getDecorator(string $class_id) { 
       if ($class_id === Bar::class) { 
          // this return value would be cached
          return $this->bar = $this->bar_factory->build();
       }
    }
}

An alternative would be setting the decorators as part of the constructor but this doesn't have the benefit of the magic unless it allowed passing a callback in...

class Foo decorates Bar { 
   public function __construct(Bar $bar) { 
     $this->__decorate(Bar::class, $bar);
   }
}

Just a few ideas anyway.

@weirdan

This comment has been minimized.

Copy link
Contributor

weirdan commented Feb 12, 2020

  • What will happen if at the time of call the property used as a delegate target is unset (or not set yet)? Will __call() or __get() get invoked?
  • Can the delegated interface be narrowed? Say, property is typed as ArrayObject, but only Countable::count() is forwarded to it?
  • How does it behave with union types?
@brzuchal

This comment has been minimized.

Copy link
Contributor

brzuchal commented Feb 12, 2020

@marandall your example with lazy load made me thinking about __get(string $name) to be used internally in case $bar property is uninitialized, but then realized it's not called on uninitialized properties as of 7.4.1 (maybe a new magic method like __init(string $name) for uninitialized properties could work).
I don't find it a good solution to work with class names in __getDecorator().


Speaking of multiple interfaces maybe sort of conflict resolution mechanism like in traits could help?!

decorate class DecoratedFooBar implements Foo, Bar {
  private decorate Foo $foo;
  private decorate Bar $bar {
    Foo::foo insteadof Bar;
    Bar::foo as fooBar;
  }
  public function __construct(Foo $foo) {
    $this->foo = $foo;
  }
  public function __init(string $name) {
    if ($name == 'bar') {
      $this->bar = new BarImpl();
    }
  }
}
@nikic

This comment has been minimized.

Copy link
Member Author

nikic commented Feb 12, 2020

What will happen if at the time of call the property used as a delegate target is unset (or not set yet)? Will __call() or __get() get invoked?

It will behave exactly as if you wrote $this->prop->foo() to delegate. That is, you will get an uninitialized property Error if you don't initialize, or __get() will be invoked if you explicit unset the property (the standard magic lazy initialization pattern).

The latter part doesn't work in the current implementation, but that's how it should work.

Can the delegated interface be narrowed? Say, property is typed as ArrayObject, but only Countable::count() is forwarded to it?

It can be narrowed by specifying the correct interface. You would write public decorated Countable $obj. Note that you can (of course) store an ArrayObject inside a Countable property. The type specifies which part of the interface you consider relevant.

How does it behave with union types?

It must be a single type, union types are forbidden in this position. In the future, it would be possible to support intersection types to forward multiple interfaces to the same object.

@Ocramius

This comment has been minimized.

Copy link
Contributor

Ocramius commented Feb 14, 2020

I was initially interested in this feature, but quickly came to realise that it is only useful with classes/interfaces with many, many methods.

If you have up to 4 methods, writing the forwarding logic yourself is trivial and a no-brainer, and doesn't need this kind of syntactic sugar.

If you have a dozen methods, then the feature starts to become useful, but I'd also add: why do you have a dozen methods? What's going on?

So the principal feature of this patch is very much unnecessary.

What's interesting though is:

  • added reflection/semantic information about decoration: decoration is no longer an implicit/inferred concept that is to be interpreted by a programmer, but rather explicitly declared
  • removed stack frames when calling decorated methods

I'd probably rather declare the decoration at method level then:

class Foo implements Bar
{
    private Bar $bar;
    // ...
    function decorates $bar baz;
    function decorates $bar taz;
}

This would also solve the forwarding when multiple internal decorated references exist:

class Foo implements Bar
{
    private Bar $bar;
    private Waz $waz;
    // ...
    function decorates $bar baz;
    function decorates $waz taz;
}
@IMSoP

This comment has been minimized.

Copy link
Contributor

IMSoP commented Feb 14, 2020

@Ocramius Even if there are only a small number of methods, the sugar would be useful if there are a large number of parameters, because the signature is automatically copied across.

It's easy enough to write this:

public function getFoo() {
    return $this->delegated->getFoo();
}

But rather annoying to write this:

public function takePayment(Card $card, Money $amount, string $reference, bool $async=false) {
    return $this->delegated->takePayment($card, $amount, $reference, $async);
}

The decorator needs to copy across and maintain the full signature, including types and defaults. Taking your per-method syntax suggestion, this reduces to one line which automatically maintains that for you:

public function decorates $delegated takePayment;

As for whether to delegate each method individually or automatically based on an interface, I'll quote a comment you made on a previous thread:

One problem with delegating calls this way, is that any new method implemented by an ancestor will not be delegated by default.
...
If you don't do it this way, you end up with a BC break any time an ancestor defines new public API.
...
Still, even if you do it this way, implicit delegation of any newly added API may also not be wished, so it is an open-ended question.

I think that's a very good point, and there are probably use cases that favour both approaches.

@Ocramius

This comment has been minimized.

Copy link
Contributor

Ocramius commented Feb 14, 2020

It's easy enough to write this:

public function getFoo() {
    return $this->delegated->getFoo();
}

But rather annoying to write this:

public function takePayment(Card $card, Money $amount, string $reference, bool $async=false) {
    return $this->delegated->takePayment($card, $amount, $reference, $async);
}

The decorator needs to copy across and maintain the full signature, including types and defaults. Taking your per-method syntax suggestion, this reduces to one line which automatically maintains that for you:

public function decorates $delegated takePayment;

Done that a gazillion times: it's really trivial to do, and adding a language feature for that is not useful. I've adjusted the snippets above to remove any function signature, since it makes sense to inherit everything.

Delegation becomes very dangerous with BC compliance and implicit method forwarding (where no implicit method forwarding is wished), so a per-method delegation is indeed preferable.

@IMSoP

This comment has been minimized.

Copy link
Contributor

IMSoP commented Feb 14, 2020

Done that a gazillion times: it's really trivial to do, and adding a language feature for that is not useful.

You might think it's not useful enough to warrant an extra language feature, but I don't understand the absoluteness of saying it's not useful at all.

It basically has all the same benefits as traits: the compiler does the copy-and-paste for you, so you don't have to. The usefulness of that is relative to the complexity of the thing you're copying and pasting, and what kind of changes might happen to it in future.

For instance, imagine if the default value for $async in the above example changed from false to true. If you've copied the signature by hand, you will receive no notification that your decorator now uses a different default from the original. If the compiler generates the signature for you, it will copy in the new default, retaining the intended behaviour that the decorator forwards the call unchanged.

Delegation becomes very dangerous with BC compliance and implicit method forwarding (where no implicit method forwarding is wished), so a per-method delegation is indeed preferable.

You are perfectly entitled to change your mind, but I just want to reiterate that this is basically the opposite of your response on the previous thread.

@Ocramius

This comment has been minimized.

Copy link
Contributor

Ocramius commented Feb 14, 2020

You might think it's not useful enough to warrant an extra language feature, but I don't understand the absoluteness of saying it's not useful at all.

Added language features come with:

  • parser complexity
  • AST complexity
  • reflection complexity
  • Static analysis complexity
  • Added BC implications

Adding features should always be weighed with the advantages they bring. If a feature is marginally useful (Pareto 20%), then it's probably not worth adding.

I just want to reiterate that this is basically the opposite of your response on the previous thread.

Implicit delegation still leads to BC breaks: you probably didn't click through to the link @ Roave/BackwardCompatibilityCheck#111

In that example, you have two behavioral breakages:

  1. logger is no longer invoked (decorator is no longer doing its work - not fixed by this potential feature)
  2. state is broken (this would be fixed by this potential feature)
@IMSoP

This comment has been minimized.

Copy link
Contributor

IMSoP commented Feb 14, 2020

Adding features should always be weighed with the advantages they bring.

Absolutely. I was just calling you out on the distinction between "not useful" (which is rather absolute, and a bit of a discussion-killer) and "not useful enough to outweigh the cost" (which acknowledges there is both value and cost, and expresses an opinion on their relative values).

Implicit delegation still leads to BC breaks: you probably didn't click through to the link @ Roave/BackwardCompatibilityCheck#111

I may have done at the time, but it was a long time ago :)

It's an interesting example, although a bit confusing because it both extends and delegates the same class. If we change Counter to an interface being implemented, it's more inline with the examples being discussed:

class LoggingCounter implements Counter
{
    private Logger $logger;
    private Counter $originalCounter;
    public function __construct(Logger $logger, Counter $originalCounter)
    {
         $this->logger = $logger;
         $this->originalCounter = $originalCounter;
    }
    public function increment() : void {
        $this->logger->log('increment');
        $this->originalCounter->increment();
    }
    public function count() : int {
        return $this->originalCounter->count();
    }
}

Then the change would propagate as follows:

  • If you delegate all functions not explicitly over-ridden (as in Nikita's proposal), the counter will be incremented correctly, but no logging will take place. Whether this is a problem depends on the use case.
  • If you delegate only named functions (as in your proposal), you will get an error at compile time that the class doesn't fully implement the interface, forcing you to decide whether you want to delegate the new method or not.

I suggested on the mailing list that the feature could be extended with an AOP-style syntax for decorating the delegated functions. If you allowed a default for that, you could implement LoggingCounter something like this:

class LoggingCounter implements Counter
{
    private Logger $logger;
    private delegate Counter $originalCounter;
    public function __construct(Logger $logger, Counter $originalCounter)
    {
         $this->logger = $logger;
         $this->originalCounter = $originalCounter;
    }
    // default decoration for methods from interface
    after delegate from Counter {
        $this->logger->log(__METHOD__);
    }
    // opt out for specific method
    after delegate count {}
}

On the other hand, you may be right that explicitly listing methods is safer, in which case it might look more like this:

class LoggingCounter implements Counter
{
    private Logger $logger;
    private Counter $originalCounter;
    public function __construct(Logger $logger, Counter $originalCounter)
    {
         $this->logger = $logger;
         $this->originalCounter = $originalCounter;
    }

    // forward calls, then run block
    delegate $originalCounter increment, increment2 {
        $this->logger->log(__METHOD__);
    }

    // forward calls unchanged
    delegate $originalCounter count;
}
@Crell

This comment has been minimized.

Copy link

Crell commented Feb 15, 2020

Inquiry: Would this just forward methods, or properties, too? (I'm thinking of the way Go does "inheritance", which is essentially an implicit version of this, for properties and for implicit interface fulfillment.)

@brzuchal

This comment has been minimized.

Copy link
Contributor

brzuchal commented Feb 15, 2020

I suggested on the mailing list that the feature could be extended with an AOP-style syntax for decorating the delegated functions.

IMHO proposed after method is completely different feature and therefore would be much more useful with no relation to delegate and probably you would also want to know the input of originally invoked method when doing something after or before unless you wanna limit the functionality to reporting which uses object state and some magic constants.

@mikeschinkel

This comment has been minimized.

Copy link

mikeschinkel commented Feb 16, 2020

@nikic — I am so excited about your work on PHP, and especially this PR.

I started to write a comment but realized I was writing an epic so decided to write a blog post instead. You can read the summary as well as the rest of the post here.

But to summarize here for those who don't click the link, if we look at how Go handles these challenges and look at how close PHP's trait is to delegation we'll find that Go has solved the syntax and semantics very elegantly and that like traits we should seriously consider leveraging the use statement but with a class modifier. If we do the latter then we get to reuse the concepts for trait method disambiguation that were already agreed upon for PHP.

To provide just one code example to illustrate what this might look like; nice, clean and simple:

<?php 
class Bar {
    function say_hello() {
        echo "Hello";
    }
}
class Foo {
    use class Bar;
    function __construct(Bar $bar) {
        $this->Bar = $bar
    }
}
$foo = new Foo( new Bar() );
$foo->say_hello();       // echos "hello"
$foo->Bar->say_hello();  // also echos "hello"

My blog post also deals with:

  1. Instantiation of delegated instances,
  2. Accessing delegated instances as distinct instances,
  3. Renaming the delegation properties
  4. Dealing with method naming conflicts
  5. Including and excluding class methods from delegation, and
  6. A few more bits and bobbles.

Hopefully this parallel between traits and delegation makes sense to you and this might be a way forward for delegation in PHP?

Thank you in advance for considering.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.