-
Notifications
You must be signed in to change notification settings - Fork 48
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
set endpoint_aliases to specify fixed aliases for exposed dataobjects #80
Conversation
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.
I've suggested PHP 7.1 typing as you've used the null coalesce operator. I also think that "$endpoint
" isn't really an appropriate parameter name. The method is resolving the endpoint from the class name. I would propose a method signature like:
resolveEndpoint(string $className): string
Also I think that ->unsanitiseClassName
should only be called on the class name and not the "aliased endpoint" if that is being used.
Thanks for this contribution! It's a great idea.
Co-Authored-By: Guy Marriott <guy@scopey.co.nz>
Co-Authored-By: Guy Marriott <guy@scopey.co.nz>
Co-Authored-By: Guy Marriott <guy@scopey.co.nz>
@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 |
@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 public function resolveClassName(HTTPRequest $request): string All you would have to add is Sorry again for past me 😅 |
Ah, yeah using simply $request as the param makes sense. |
Ah, nope. Just habit I guess. It should probably be protected. |
Updated it again :-) |
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.
Thanks! Sorry for all the crazy back on forth - that's my fault 😧
Just one last little thing but then it's ready to merge!
Also please be sure to run silverstripe-restfulserver/composer.json Line 22 in e0f4e56
https://travis-ci.org/silverstripe/silverstripe-restfulserver/jobs/557159932#L626 |
Co-Authored-By: Guy Marriott <guy@scopey.co.nz>
…stripe-restfulserver into endpoint-aliases
@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. |
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.
I see you've introduced PHP 7 only syntax. This module still supports PHP 5.6. We can bump this to require SilverStripe 4.5 which is PHP 7 only, but I don't see the benefit. Would you mind reverting those changes please?
That was my suggestion 😅 . |
#82 has come up now asking to revert the PHP 7 syntax |
No description provided.