-
Notifications
You must be signed in to change notification settings - Fork 46
Allow for a candidate to have a Closure as a class parameter #66
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,6 +2,7 @@ | |
|
||
namespace Http\Discovery; | ||
|
||
use Http\Discovery\Exception\ClassinstantiationFailedException; | ||
use Http\Discovery\Exception\DiscoveryFailedException; | ||
use Http\Discovery\Exception\StrategyUnavailableException; | ||
|
||
|
@@ -36,7 +37,7 @@ abstract class ClassDiscovery | |
* | ||
* @param string $type | ||
* | ||
* @return string | ||
* @return string|\Closure | ||
* | ||
* @throws DiscoveryFailedException | ||
*/ | ||
|
@@ -177,4 +178,28 @@ protected static function evaluateCondition($condition) | |
|
||
return false; | ||
} | ||
|
||
/** | ||
* Get an instance of the $class. | ||
* | ||
* @param string|\Closure $class A FQN of a class or a closure that instantiate the class. | ||
* | ||
* @return object | ||
*/ | ||
protected static function instantiateClass($class) | ||
{ | ||
try { | ||
if (is_string($class)) { | ||
return new $class(); | ||
} | ||
|
||
if (is_callable($class)) { | ||
return $class(); | ||
} | ||
} catch (\Exception $e) { | ||
throw new ClassinstantiationFailedException('Unexcepced exception when instantiating class.', 0, $e); | ||
} | ||
|
||
throw new ClassinstantiationFailedException('Could not instantiate class becuase parameter is neitehr a callable or a string'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. s/neitehr/neither There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. neither ... nor ... |
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
<?php | ||
|
||
namespace Http\Discovery\Exception; | ||
|
||
use Http\Discovery\Exception; | ||
|
||
/** | ||
* Thrown when a class fails to instantiate. | ||
* | ||
* @author Tobias Nyholm <[email protected]> | ||
*/ | ||
class ClassinstantiationFailedException extends \RuntimeException implements Exception | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should this be final? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, and also a capital "I" in Instantiation |
||
{ | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -24,7 +24,7 @@ public static function find() | |
try { | ||
$asyncClient = static::findOneByType(HttpAsyncClient::class); | ||
|
||
return new $asyncClient(); | ||
return static::instantiateClass($asyncClient); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we need the instantiation within the try? Is it possible that a closure throws a DiscoveryFailedException? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, we do not need a try-catch here. We know that instantiateClass could only throw a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then maybe we should move the instantiation out of the try-catch block. Makes the code easier to read IMO. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry, I think I miss-read your comment. I meant that we do not need an additional try-catch here (at this line).
Yes, we need the instantiation within a try-catch. Any weird exception could happen both with a FQCN (Class not found) and with the closure. The closure may throw a "VeryRandomException" that is not part of our code. Our strategies does not behave weirdly (of course) but the application developer may inject a strategy of their own. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, we need the instantiation within a try-catch. Any weird exception
could happen both with a FQCN (Class not found) and with the closure.
The closure may throw a "VeryRandomException" that is not part of our code.
makes sense to me.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Correct me if I am wrong, but this try-catch only handles DiscoveryFailedExceptions, which mean we cannot (and we don't want to) catch VeryRandomExceptions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you propose that we should rewrite it like: try {
$asyncClient = static::findOneByType(HttpAsyncClient::class);
} catch (DiscoveryFailedException $e) {
throw new NotFoundException(
'No HTTPlug async clients found. Make sure to install a package providing "php-http/async-client-implementation". Example: "php-http/guzzle6-adapter".',
0,
$e
);
}
return static::instantiateClass($asyncClient); There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't understand how the linked method is relevant: the exceptions there would still not be caught by this catch block. Yes, I propose exactly that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now I understand you. You got a PR. |
||
} catch (DiscoveryFailedException $e) { | ||
throw new NotFoundException( | ||
'No HTTPlug async clients found. Make sure to install a package providing "php-http/async-client-implementation". Example: "php-http/guzzle6-adapter".', | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FQCN