search

colymba / silverstripe-restfulapi

SilverStripe RESTful API with a default JSON serializer.
BSD 3-Clause "New" or "Revised" License
64 stars 33 forks source link

SS4 Compatibility #93

Closed robingram closed 5 years ago

robingram commented 5 years ago

This builds on the work done in #92 by DNA Design. @colymba, there are a few things to note here:

I have added a mechanism for mapping the URL segment following /api/ to a class name. This is to allow it to work for namespaced classes. So now, for example, you can map /api/member to SilverStripe\Security\Member. This is done in the configuration of the DefaultQueryHandler and will fall back to the original mechanism if no mapping is defined. I've updated the documentation file for the class but you might like to review it and see if you think the approach is OK.

You might also want to review the changes in the context of Ember Data. There were some points around the initial caps of the class name that I didn't fully understand and I'm also not sure how that will work more generally in the context of namespaced classes.

I had to exclude the tests for CORS in order to get the test suite to run. Anything that used the CORS pre-flight would hit an exit in the code and halt the test suite execution but since it had an exit code of 0 Travis was seeing it as a test pass. I've added a @group annotation to those tests and excluded that group in the Travis config.

I dropped the Framework requirement down to 4.1 because we need it in CWP 2.0. :)

Validation errors now come back as an array rather than an single error so I decided to concatenate them.

robingram commented 5 years ago

Also @colymba, if you're happy with this it would make sense to create a branch to maintain 3.x compatibility before merging.

colymba commented 5 years ago

sorry it's taking long to review all this... I'll do my best to go through it all asap. one thing for sure, let's delete the whole Ember Data part of it. It's out of date, and was made way back when Ember was in beta.... so let's delete and simplify the whole thing.

So with that in mind let's remove "Basic" out of BasicSerializer and BasicDeSerializer. And with simplification in mind, shall we remove Default out of DefaultPermissionManager and DefaultQueryHandler? Just trying to streamline all the naming...

So maybe switching from Basic to Default... is a better choice, sounds better and give the idea that this can be extended. What do you think?

robingram commented 5 years ago

I like the idea of switching to Default rather than simply removing Basic. As you say, makes it clearer that it can be overridden.

I'll take a look at that and removing Ember data.

robingram commented 5 years ago

Sorry @colymba, this is getting messy now. The Travis build failed because the password in the TokenAuthenticator test was too weak - not sure why it didn't before. I pushed a couple of changes but now the build is erroring, probably due to GitHub API limits, and I don't have permission to restart the build.

Would you be able to restart it when you get a chance and see if it passes this time?

Don't worry, I think I can sort it.

robingram commented 5 years ago

Yay, fixed. :)

colymba commented 5 years ago

thank you @robingram for the huge work and thanks @adrexia for kicking this off. Merged in just for xmas :christmas_tree: