Ignore non-function callback arguments #8
Conversation
|
I'm not a fan of ignoring invalid arguments. This type of thing should fail. For public APIs checking the args and failing early and explicitly is usually a good idea. IMO we should either keep it as is or add exceptions if the arg is not null and not callable. |
| public function typesDataProvider() | ||
| { | ||
| return array( | ||
| array('', 'empty string'), |
There was a problem hiding this comment.
side note: you can use an assoc array where the key is the desc.
|
Do you know what the promises/a+ motivation is for ignoring invalid args? JS is careless about args anyway, so it's a slightly different story for PHP imho. |
|
@briancavalier hi there. :) can you perhaps clarify this? |
|
I was just following Promiss/A+. At the moment its just wrong, even worse, passing a truthy value as progress handler produces an infinite loop. Throwing exceptions inside then() is synchronous behavior and not a good idea. See also promises-aplus/promises-spec#25 for a lengthy discussion about it. |
|
In PHP 5.4 we will have a I suggest we check the args and then trigger a We should do that in addition to ignoring non-callable args. EDIT: Thanks for the research btw! |
|
Hey guys, yeah, you can read a lot of backstory over at promises-aplus/promises-spec#25. Some of us felt that this type of error should be caught as early as possible and reported as loudly as possible, but the majority felt that all non-functions should be ignored. So, we went with the majority vote for the Promises/A+ spec. In when.js, our plan is to have @jsor For sure, progress is a bit of a mess right now. It's actually outside of the Promises/A+ spec, and there's not yet a clear de facto standard. It's being discussed and worked on over at promises-aplus/progress-spec, so please do jump into the conversation there if you have ideas. There's also some good background reading cited in promises-aplus/progress-spec#3 about the trickiness with progress handling. |
|
@briancavalier thanks a lot for the clarifications. Btw, my comment about about being "wrong" and the infinite loop was about my code (react/promise), not when.js :-) |
|
@jsor no problem! And it wouldn't surprise me if when.js's progress handling still has some wonky edge cases, especially related to progress propagation through a promise chain :) (runs off to test progress handler infinite loops in when.js) |
| @@ -27,6 +27,10 @@ public function then($fulfilledHandler = null, $errorHandler = null, $progressHa | |||
| } | |||
| }; | |||
| } else { | |||
| if (null !== $progressHandler) { | |||
| trigger_error('Invalid $progressHandler argument passed to then(), must be null or callable.', \E_USER_NOTICE); | |||
There was a problem hiding this comment.
\E_USER_NOTICE can be just E_USER_NOTICE. Applies to the other instances as well.
Make it compliant with Promises/A+