-
Notifications
You must be signed in to change notification settings - Fork 3
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
Docblock Updates and General Improvements #4
base: master
Are you sure you want to change the base?
Changes from 1 commit
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 |
---|---|---|
@@ -0,0 +1,2 @@ | ||
/vendor/ | ||
composer.lock |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,15 +2,36 @@ | |
|
||
namespace Cubex\ApiFoundation\Exceptions; | ||
|
||
use Throwable; | ||
|
||
class InvalidPayloadException extends \Exception | ||
{ | ||
/** @var string */ | ||
protected $message = 'Invalid payload'; | ||
|
||
/** @var int */ | ||
protected $code = 400; | ||
|
||
public static function withType($type): static | ||
/** | ||
* @param string $type | ||
* @param string $message | ||
* @param int $code | ||
* @param Throwable|null $previous | ||
*/ | ||
public function __construct(string $type, string $message = "", int $code = 0, ?Throwable $previous = null) | ||
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. This doesn't follow the constructor for 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. As we are looking to add a type I don't see why we want it to follow the exception constructor. |
||
{ | ||
parent::__construct($message, $code, $previous); | ||
$this->message .= ' ' . $type; | ||
} | ||
|
||
/** | ||
* @deprecated Use the constructor instead | ||
* @param string $type | ||
* | ||
* @return static | ||
*/ | ||
public static function withType(string $type) | ||
{ | ||
$ex = new static(); | ||
$ex->message .= ' ' . $type; | ||
return $ex; | ||
return new static($type); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,27 +5,29 @@ | |
use Cubex\ApiFoundation\Exceptions\InvalidPayloadException; | ||
use Cubex\ApiTransport\Endpoints\ApiEndpoint; | ||
use Cubex\ApiTransport\Payloads\AbstractPayload; | ||
use Exception; | ||
use Packaged\Context\Context; | ||
use Packaged\Context\ContextAware; | ||
use Packaged\Http\Responses\JsonResponse; | ||
use Packaged\Routing\Handler\Handler; | ||
use Packaged\Routing\RequestCondition; | ||
use Packaged\Routing\Route; | ||
use ReflectionClass; | ||
use ReflectionException; | ||
use Symfony\Component\HttpFoundation\Response; | ||
|
||
class ProcedureRoute extends Route implements Handler | ||
{ | ||
/** @var */ | ||
protected $_endpoint; | ||
/** @var string */ | ||
protected $_procedureClass; | ||
|
||
public function __construct(ApiEndpoint $endpoint, string $procedureClass) | ||
/** | ||
* @param ApiEndpoint $_endpoint | ||
* @param class-string<AbstractProcedure> $_procedureClass | ||
*/ | ||
public function __construct(protected ApiEndpoint $_endpoint, protected string $_procedureClass) | ||
{ | ||
$this->_endpoint = $endpoint; | ||
$this->_procedureClass = $procedureClass; | ||
$this->add( | ||
RequestCondition::i()->path($endpoint->getPath(), RequestCondition::TYPE_EXACT)->method($endpoint->getVerb()) | ||
RequestCondition::i() | ||
->path($this->_endpoint->getPath(), RequestCondition::TYPE_EXACT) | ||
->method($this->_endpoint->getVerb()) | ||
); | ||
} | ||
|
||
|
@@ -45,6 +47,11 @@ public function setAuthenticator(ApiAuthenticator $authenticator) | |
return $this; | ||
} | ||
|
||
/** | ||
* @param Context $context | ||
* | ||
* @return bool | ||
*/ | ||
public function match(Context $context): bool | ||
{ | ||
if(parent::match($context)) | ||
|
@@ -68,53 +75,52 @@ public function match(Context $context): bool | |
return false; | ||
} | ||
|
||
/** @return $this */ | ||
public function getHandler() | ||
{ | ||
return $this; | ||
} | ||
|
||
/** | ||
* @throws InvalidPayloadException | ||
* @throws InvalidPayloadException|ReflectionException|Exception | ||
*/ | ||
public function handle(Context $c): Response | ||
{ | ||
$procedure = new $this->_procedureClass(); | ||
if(!($procedure instanceof Procedure)) | ||
{ | ||
throw new \Exception("Invalid procedure class given"); | ||
throw new Exception("Invalid procedure class given"); | ||
} | ||
|
||
if($procedure instanceof ContextAware) | ||
{ | ||
$procedure->setContext($c); | ||
} | ||
|
||
if(method_exists($procedure, 'execute')) | ||
if (!method_exists($procedure, 'execute')) | ||
bajb marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
$plClass = $this->_endpoint->getPayloadClass(); | ||
if($plClass !== null) | ||
{ | ||
$payload = new $plClass(); | ||
if($payload instanceof AbstractPayload) | ||
{ | ||
$payload->fromContext($c); | ||
} | ||
throw new Exception("Unable to handle procedure: execute missing"); | ||
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. The response code is lost here 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. Fixed |
||
} | ||
|
||
if(!$payload->isValid()) | ||
{ | ||
throw InvalidPayloadException::withType((new \ReflectionClass($payload))->getShortName()); | ||
} | ||
$plClass = $this->_endpoint->getPayloadClass(); | ||
|
||
//Execute with payload | ||
$response = $procedure->execute($payload); | ||
} | ||
else | ||
{ | ||
//Execute without payload | ||
$response = $procedure->execute(); | ||
} | ||
return JsonResponse::create($response); | ||
if (!$plClass) | ||
{ | ||
return JsonResponse::create($procedure->execute()); | ||
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. We should avoid creating the JsonResponse twice here. Its preferred to create the final response in a single line for improved refactoring or modification in the future. Another option to would be to have a method to convert the response to the JsonResponse. This would give us the option to return as text/xml for different clients, or endpoints. 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. Having an else statement increases code complexity. An early return does look like the better option here. If we need to modify any of this logic we can override the method. |
||
} | ||
return \Packaged\Http\Response::create("Unable to handle procedure: execute missing", 404); | ||
|
||
$payload = new $plClass(); | ||
if($payload instanceof AbstractPayload) | ||
{ | ||
$payload->fromContext($c); | ||
} | ||
|
||
if(!$payload->isValid()) | ||
{ | ||
throw new InvalidPayloadException((new ReflectionClass($payload))->getShortName()); | ||
} | ||
|
||
//Execute with payload | ||
return JsonResponse::create($procedure->execute($payload)); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,19 +7,33 @@ | |
|
||
class ModuleCondition extends RequestCondition | ||
{ | ||
private $_module; | ||
/** | ||
* @param class-string<Module> $_moduleClassName | ||
* @param string $moduleBasePath | ||
*/ | ||
public function __construct(private string $_moduleClassName, string $moduleBasePath) | ||
{ | ||
$this->path($moduleBasePath); | ||
} | ||
|
||
/** | ||
* @deprecated Use the constructor instead | ||
* @param Module $module | ||
* | ||
* @return static | ||
*/ | ||
public static function withModule(Module $module) | ||
{ | ||
$condition = new static(); | ||
$condition->_module = $module; | ||
$condition->path($module::getBasePath()); | ||
return $condition; | ||
return new static($module::class, $module::getBasePath()); | ||
} | ||
|
||
/** | ||
* @param Context $context | ||
* | ||
* @return void | ||
*/ | ||
public function complete(Context $context) | ||
{ | ||
$context->meta()->set('api.module', get_class($this->_module)); | ||
$context->meta()->set('api.module', $this->_moduleClassName); | ||
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. As class names are not case sensitive, sticking with get_class here is safer 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. Fixed |
||
} | ||
|
||
} |
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.
This should stay as module, otherwise its a breaking change.
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.
Looking at the code base this looks like an object that is used internally and I've already updated the reference(https://github.com/cubex/api-foundation/pull/4/files#diff-d08ab29dc7d96cd6c5d5f15f1c75366b9f4167b82a94951330adda2d7bac1ec6R30)?