Feature/rest crud #60
Conversation
* feature/api-transformers: Added failing tests to postman. Added test entity model, migration, seeder and routes
FF to master
…t-crud * remotes/origin/master: (53 commits) Implements collection() and item() base methods to handle response transformation Declares the basecontroller as abstract Adds @todo entry to the relevant docblocks Removes the trait as it is now available in Lumen framework Removes teardown to close the Mockery, as that is now handled by Lumen Implements temporary fix for Mockery bug in Lumen 5.1.0 Adds back the lock file Update bootstrap for 5.1 Update dependencies for Lumen framework 5.1 Restores the method names Adds initial tests for the Test Entity Repository Adds mockery as a development dependency Removes hard coded entity_id from the test Inject the Application instead over using Facade to prevent command line problems Fixed integration test expectation Switched over to using angular material Dropped unneeded zombiejs dependency, updated dependencies Add the traits in the base test class Implements a CrawlerTrait Implements additional assertions. ... # Conflicts: # api/postman/spira.json.postman_collection
…response structure
| use Illuminate\Support\MessageBag; | ||
| use Illuminate\Http\Exception\HttpResponseException; | ||
|
|
||
| class ValidationException extends HttpResponseException |
There was a problem hiding this comment.
The ValidationException should probably implemenent Symfony\Component\HttpKernel\Exception\HttpExceptionInterface as the error handler (spira/api/app/Http/Controllers/BaseController.php:renderException) method checks if $e instanceof HttpExceptionInterface and sets the response code with $e->getStatusCode(). We are using the Symfony\Component\HttpKernel\Exception elsewhere so we should probably implement their interface for consistency
There was a problem hiding this comment.
Ah, I see. I've been checking that out now. So if I understand correctly, we'll need to add handling in BaseController.php:renderException that injects the invalid object into the response. Right?
There was a problem hiding this comment.
@xiphiaz 95bfdc0 adds this change. I added a call to ValidationException from BaseController.php:renderException that modifies the response so we get the invalid key.
There was a problem hiding this comment.
@xiphiaz I made a small tweak in dd31da5 to not hardcode it against ValidationException but to allow any custom exception class to override the response if needing to add more keys.
There was a problem hiding this comment.
@artstorm i think it is better practice to make the check if ($e instanceof HttpExceptionInterface) if you want to check for the presence of the getResponse method. As the ValidationException needs the getResponse method and the getStatusCode method of HttpExceptionInterface I think the ValidationException should be defined as
use Illuminate\Http\Exception\HttpResponseException;
use Symfony\Component\HttpKernel\Exception\HttpExceptionInterface;
ValidationException extends HttpResponseException implements HttpExceptionInterfaceso that both methods are available.
There was a problem hiding this comment.
@xiphiaz By having ValidationException extending HttpResponseException it actually already inherits the HttpExceptionInterface from the parent class, so it makes no practical difference to duplicate the implements again in the extending class. I'll add it to the code base though.
And I'll move the check to be inside the if ($e instanceof HttpExceptionInterface).
There was a problem hiding this comment.
@xiphiaz Done in 0095401
There was a problem hiding this comment.
@artstorm ah I think you misunderstood - see in the use commands in the example above - there is two different namespaces - one from Illuminate the other from Symfony. It is the Illuminate\Http\Exception\HttpResponseException that has the getResponse method.
There was a problem hiding this comment.
@xiphiaz I'm not sure I completely follow along now? Haha. But I think we have it implemented correctly now, or do I need to adjust anything?
…tpException instead of Illuminate\Http\Exception\HttpResponseException
…eck for getResponse() method
|
@artstorm comment #60 (comment) is being swallowed by the outdated diff in case you miss it. |
…o model factory for the TestEntity
|
@artstorm ok on closer inspection it looks like laravel catches and returns the responses with 200OK when a |
Fix/entity id validation
|
@xiphiaz I just reviewed and merged #61, looked super to me. |
|
@artstorm awesome, I'll wait for the travis builds to pass then merge. |
No description provided.