Closed sanderha closed 5 years ago
@ScopeyNZ Please have another look when you have the time :-)
Thanks @sanderha . Looks like past me gave some bad feedback.
Looks like you were correct in the first place where resolveEndpoint
was actually resolving a class name 🤦♂ . I guess I got confused that the URL param is labelled ClassName
. I'm sorry for messing you around a bit here but perhaps we should actually have it as
public function resolveClassName(HTTPRequest $request): string
All you would have to add is $request->param('ClassName')
in that method and update where it's called. I think that's probably more helpful for a developer looking to extend as they can inspect other parts of the request to determine what classname should be returned (if they want).
Sorry again for past me 😅
Ah, yeah using simply $request as the param makes sense.
Any specific reason why you want it to be public
method?
Ah, nope. Just habit I guess. It should probably be protected.
Updated it again :-)
Also please be sure to run phpcs
over the code before committing! :)
If you don't have this locally, you can get it on the project you're testing your changes on with composer require --dev squizlabs/php_codesniffer ^3
and run ../../bin/phpcs
from within this module's install dir (vendor/silverstripe/restfulserver
)
https://travis-ci.org/silverstripe/silverstripe-restfulserver/jobs/557159932#L626
@NightJar Alright changed some docblocks to satisfy code sniffer! :)
I've just pushed an update to drop PHP 5.6 and 7.0 from builds. That should get travis happy.
That was my suggestion 😅 .
@ScopeyNZ Thanks for your reply!
Indeed what you've wrote makes sense. Would it be best to also change the null coalesce operator for backwards compatability?
Also I'll get that method and param name renamed asap