more semantic

[yozman]

https://github.com/guzzle/promises/blob/master/src/Promise.php#L35

$p = new static(null, [$this, 'cancel']);

wish u improve it

[sagikazarmark]

Can you please explain? I don't see any added value apart from making hacks which rely on inheritance possible.

[jeskew]

It's unsafe to assume a child class has a constructor compatible with that of its parent. If anyone is using subclasses of promise in their code, this could easily be a breaking change.

[sagikazarmark]

Putting this onto my close in a few days list.

[yozman]

@sagikazarmark
I want to add some feature for my own project
So I make Process extends Promise
but when I use my addon function there is error call no function on Promise
so I think this should be new static not new Promise
@jeskew
as you see that is not in constructor it's just call constructor
when a child class extends parent the breaking change should be care by child not parent

[sagikazarmark]

What you could do is overriding then and otherwise to always wrap the returned Promise with yours. The other option is that you always store the new promise in an internal variable and just return $this. For this it's probably better to implement PromiseInterface instead of extending promise, but I am not sure this latter solution would work with your logic.

Ultimately: you can just copy the Promise class and make your changes. The Fulfilled/Rejected promises are created that way. IMO that's the cleanest solution.

when a child class extends parent the breaking change should be care by child not parent

Based on what do you say that? The child's constructor signature does not have to be compatible with the parent's, so relying on it is not safe. There is no such thing as the "child needs to take care of it", because there is no such restriction.

[mtdowling]

What's your use case for extending Promise?

[yozman]

@sagikazarmark
about extends parent.
I mean if there will be a new breaking change,
when child extends parent,
should let child fix it, not parent.
so there should be new static I think
thanks for your answer

@mtdowling
make new useful function
like next and fail
now then is only for callable
but callable is not clear for read and no arguments.
I want to use like $promise->next('Modal@action', $arg1, $arg2)

[sagikazarmark]

I'm sorry, I don't understand your arguments (on the technical level).

[mtdowling]

I actually think it was a miss that I didn't mark Promise as final.

Instead of refactoring code to allow subclasses to override the constructor, I'd rather see a release that makes Promise final. I'm not sure what impact that would imply for semver, maybe it could be argued as a breaking change warranting a 2.0 at some point. If so, we can add it to the 2.0 wishlist along with possibly some other helper functions baked into Promise perhaps (like map, filter, etc.).

[yozman]

I don't want to override the constructor,
I just want to add some useful function to Promise
for my own logic.
but because of then is return only Promise,
not my subclass,
so there can't find addon function on Promise

[elyobo]

Perhaps you could replace new Promise with a function call that does exactly the same (e.g. as below); then extenders could change the class created appropriately if they need to (even if they've changed the signature of the constructor), but the default behaviour remains the same.

// replace $p = new Promise(null, [$this, 'cancel']);
$p = $this->nextPromise(null, [$this, 'cancel']);
// and elsewhere in the class
protected function nextPromise(callable $waitFn = null, callable $cancelFn = null) {
    return new Promise($waitFn, $cancelFn);
}

Obviously not useful if you change Promise to be final though, which I'm generally not a fan of due to the limitations it imposes.

[yozman]

@elyobo
I can't override then method,
because of some attribute for Promise is private
that means I should overrider them too.

[elyobo]

Yes, my suggestion would remove the need to extend `.then`, it would add a new protected method that you could override to change the class of the promise that it creates instead.

…

On Sat, 7 Jan 2017, 13:24 木由子 ***@***.***> wrote: @elyobo <https://github.com/elyobo> I can't override then method, because of some attribute for Promise is private that means I should overrider them too. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#58 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAY4_pKfQ6tg-e8YZ5oObn-wX1aRQCqWks5rPvdcgaJpZM4LaroC> .

[yozman]

@elyobo
did u read the source file?
there is too many private attributes to override.
so your suggestion isn't a good idea

[elyobo]

I don't know what your use case is, all I'm suggesting is a way that the code can be changed to allow you to instantiate your child promise in `.then` without changing the current default behavior so that it's backwards compatible, as in the original issue description. If the private attributes are also difficult then that's a different problem.

…

On Sat, 7 Jan 2017, 14:21 木由子 ***@***.***> wrote: @elyobo <https://github.com/elyobo> did u read the source file <https://github.com/guzzle/promises/blob/master/src/Promise.php#L11,L16>? there is too many private attributes to override. so your suggestion isn't a good idea — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#58 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAY4_kSkB8BR6Fnhl5nJf76RKQBWNPqeks5rPwSdgaJpZM4LaroC> .

[sagikazarmark]

@mtdowling Opened an issue for making promise final

@elyobo what limitations? That you can't misuse inheritance? You have an interface, so you can do whatever you want (except inheriting from Promise)

[elyobo]

@sagikazarmark I see that you have some strong opinions about what the proper way to use inheritance is, and that any extension of Promise is not proper.

Without commenting specifically on this library, I think that marking things final and private is to assume that you have imagined all possible uses for extension and that they are all invalid. I don't think I'm clever enough to do that, so my preference in general is to leave things more open and if people shoot themselves in the foot because it, that's on their heads :D There is useful code in Promise that people may want to reuse and the privates make it difficult and the final more or less impossible without cut and paste.

[sagikazarmark]

about what the proper way to use

rather about the improper way which is usually the case

you have imagined all possible uses for extension and that they are all invalid

Not quite. Promise is still open for extension via the interface and composition patterns. It's closed for inheritance though, which people tend to misuse as "the only way" for extension.

[elyobo]

Yes, composition could probably do what the OP was after; however marking it final prevents any valid use cases too.

…

On Sat, 7 Jan 2017, 18:10 Márk Sági-Kazár ***@***.***> wrote: about what the proper way to use rather about the improper way which is usually the case you have imagined all possible uses for extension and that they are all invalid Not quite. Promise is still open for extension via the interface and composition patterns. It's closed for inheritance though, which people tend to misuse as "the only way" for extension. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#58 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAY4_hvgCQRyT_evG1lBFoTruaY6xAfAks5rPzpzgaJpZM4LaroC> .

[sagikazarmark]

I am sorry, I don't see what "valid use cases" mean here. What are the cases which can ONLY be solved via inheritance?

[elyobo]

No idea, but I don't know that they don't exist so lean towards allowing the possibility of extension. Perhaps if you or mtdowling could explain what you see the benefits of final are? To me it seems like there's no upside but potential downsides. OP, can you solve your problem by composition instead? E.g. implement a new class that implements the promise interface that also can be given a Promise from this library. In each case your methods would just call the methods of the underlying Promise, but in the case of `.then` you wrap the Promise returned with an instance of *your* wrapper before returning it. Hopefully that's clear, writing code on my phone is too painful to give a clearer example!

…

On Sat, 7 Jan 2017, 18:17 Márk Sági-Kazár ***@***.***> wrote: I am sorry, I don't see what "valid use cases" mean here. What are the cases which can ONLY be solved via inheritance? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#58 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAY4_tejDX1a-_kNnNRD8PC4QLF0ZVTzks5rPzv5gaJpZM4LaroC> .

[sagikazarmark]

extension

The thing is that "extension" is a bit misleading. By extension you mean extending the class. For that I say inheritance. Extension in a wider term however means functional extension of a module/software component. This is what O is from SOLID: Open/Closed principle.

Open for extension in our case means that we have an abstraction which we can use to extend the existing software components. For example write a decorator which adds your logic to the existing one.

People regularly (mis)use inheritance for extension, because it's easy to do. But in many cases it is just wrong, because that way you might actually modify the behaviour from the inside instead of extending it. Classes not meant to be extended (inherited) should not be extended (inherited). That's where the final keyword helps.

If you want to read more about this topic, there are awesome posts by awesome people:

http://verraes.net/2014/05/final-classes-in-php/
http://ocramius.github.io/blog/when-to-declare-classes-final/
https://codeblog.jonskeet.uk/2013/03/15/the-open-closed-principle-in-review/
https://8thlight.com/blog/uncle-bob/2014/05/12/TheOpenClosedPrinciple.html

[elyobo]

Thanks, I'll have a read over those, perhaps they'll change my mind :)

…

On Sat, 7 Jan 2017, 20:09 Márk Sági-Kazár ***@***.***> wrote: extension The thing is that "extension" is a bit misleading. By extension you mean extending the class. For that I say inheritance. Extension in a wider term however means functional extension of a module/software component. This is what O is from SOLID: Open/Closed principle. Open for extension in our case means that we have an abstraction which we can use to extend the existing software components. For example write a decorator which adds your logic to the existing one. People regularly (mis)use inheritance for extension, because it's easy to do. But in many cases it is just wrong, because that way you might actually modify the behaviour from the inside instead of *extending* it. Classes not meant to be extended (inherited) should not be extended (inherited). That's where the final keyword helps. If you want to read more about this topic, there are awesome posts by awesome people: http://verraes.net/2014/05/final-classes-in-php/ http://ocramius.github.io/blog/when-to-declare-classes-final/ https://codeblog.jonskeet.uk/2013/03/15/the-open-closed-principle-in-review/ https://8thlight.com/blog/uncle-bob/2014/05/12/TheOpenClosedPrinciple.html — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#58 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAY4_mHbNbvAP8TlTuoh2kAnRI62Uu-Kks5rP1ZlgaJpZM4LaroC> .

[elyobo]

Hahaha, although ironically Ocramius' (mis)use of final in some Doctrine code was one of the things which helped me form my current opinion against final :D

…

On Sat, 7 Jan 2017, 23:15 Liam O'Boyle ***@***.***> wrote: Thanks, I'll have a read over those, perhaps they'll change my mind :) On Sat, 7 Jan 2017, 20:09 Márk Sági-Kazár ***@***.***> wrote: extension The thing is that "extension" is a bit misleading. By extension you mean extending the class. For that I say inheritance. Extension in a wider term however means functional extension of a module/software component. This is what O is from SOLID: Open/Closed principle. Open for extension in our case means that we have an abstraction which we can use to extend the existing software components. For example write a decorator which adds your logic to the existing one. People regularly (mis)use inheritance for extension, because it's easy to do. But in many cases it is just wrong, because that way you might actually modify the behaviour from the inside instead of *extending* it. Classes not meant to be extended (inherited) should not be extended (inherited). That's where the final keyword helps. If you want to read more about this topic, there are awesome posts by awesome people: http://verraes.net/2014/05/final-classes-in-php/ http://ocramius.github.io/blog/when-to-declare-classes-final/ https://codeblog.jonskeet.uk/2013/03/15/the-open-closed-principle-in-review/ https://8thlight.com/blog/uncle-bob/2014/05/12/TheOpenClosedPrinciple.html — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#58 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAY4_mHbNbvAP8TlTuoh2kAnRI62Uu-Kks5rP1ZlgaJpZM4LaroC> .

[elyobo]

Thanks, they were good reading.

I don't think that allowing inheritance really violates the open/closed principal, but I can see that forcing users not to use inheritance is likely to encourage better design, that using final limits your exposed public API, and that Ocramius' rule that they can be made final "if they implement an interface, and no other public methods are defined" limits the downsides to it. Presumably you have to exclude constructors from the "no other public methods" list though (as in the case of Promise here). IIRC my problem with Doctrine and final would not have been a problem had that rule been followed then :D

Anyway, good food for thought, much appreciated.

For @yozman, perhaps something like this would help you do what you need to? Instead of extending Promise you can wrap it an implement PromiseInterface. Add modifications as you need to in order to add the functionality you need, but this allows you to reuse the Promise in a way that is acceptable anywhere a PromiseInterface is accepted and still lets you return your own wrapper from .then.

<?php

use GuzzleHttp\Promise\Promise;
use GuzzleHttp\Promise\PromiseInterface;

class WrappedPromise implements PromiseInterface {
    protected $promise;

    public function __construct(Promise $promise)
    {
        $this->promise = $promise;
    }

    public function then(callable $onFulfilled = null, callable $onRejected = null)
    {
        return new static($this->promise->then($onFulfilled, $onRejected));
    }

    public function otherwise(callable $onRejected)
    {
        return $this->promise->otherwise($onRejected);
    }

    public function getState()
    {
        return $this->promise->getState();
    }

    public function resolve($value)
    {
        return $this->promise->resolve($value);
    }

    public function reject($reason)
    {
        return $this->promise->reject($value);
    }

    public function cancel()
    {
        return $this->promise->cancel();
    }

    public function wait($unwrap = true)
    {
        return $this->promise->wait($unwrap);
    }
}

// Example usage
$noop = function () {};
$promise = (new WrappedPromise(new Promise($noop, $noop)))->then($noop);
assert(
    $promise instanceof WrappedPromise,
    '.then() returns a WrappedPromise'
);

Edit: or perhaps that should be

final class WrappedPromise implements PromiseInterface {
    protecteed $promise;

[yozman]

@elyobo
thanks for your answer.
maybe your solution is the best way right now.

could you please help me for apply
https://github.com/sinoci/sinoci/blob/master/app/Services/Process.php

[elyobo]

@yozman see https://github.com/sinoci/sinoci/pull/5

I don't know enough about your application (and there are no tests!) so I have no idea whether this actually works for you, but it demonstrates the idea at least.

[yozman]

@elyobo
thanks for your help.
then is work, otherwise doesn't
but I think I got that point. : )