Closed NoelDeMartin closed 6 years ago
Well I just spent 5 minutes for the fixes and 1 hour for the tests, and it's still not accepting the changes so I don't know, let me know what you think -.-.
Hey, just letting you know I will be reviewing this but am migrating my dev environment so it may take a few days
After real life got in way, this one's now rolled up - thanks @NoelDeMartin !
I have tried using this library and there are a couple of things that could be improved, in particular the following:
PODATA_LARAVEL_APP_ROOT_NAMESPACE
constant is not necessary. Other than this, Laravel's convention is to use config files and env variables instead of php constants, so that could also be improved in the future.Other than those improvements above, which you are free to accept or not, there are a couple of fixes as well that I think are critical because they break the integration. Those two are:
setStatusCode
method incorrectly with a string, breaking the code at runtime since that method only accepts integers. In fact that same status code is already being passed correctly on the previous line when calling the constructor, so that line is entirely unecessary. Maybe in previous versions of Laravel this was working, but on the current version of Laravel 5.6 that code breaks.