Plans and regions to look to setup/options api first #39
Conversation
…class because no existing methods made sense to use in the context of the need
src/Model/Resource.php
Outdated
| * @return array The resource object, or false if the resource is | ||
| * not found. | ||
| */ | ||
| public static function post(array $body, $collectionUrl = null, ClientInterface $client) |
There was a problem hiding this comment.
this is mimicking get() but I think you want to mimic getCollection(), as it's a list of results you're after
you could add a parameter to getCollection to override the request method, getCollection( ... , $method = 'get')
There was a problem hiding this comment.
Would getCollection work? Given that is also processes the returned data with wrapCollection.
I thought we might need to implement more posts in future if we use the APIs more so it seems reasonable to create a new post method. If you don't think that's the case we can alter an existing method.
There was a problem hiding this comment.
It takes a $collectionUrl so it seems like you're trying to fetch a collection; list of things. But it's ignoring 404s, which isn't appropriate for a POST nor for a collection.
src/Model/SetupOptions.php
Outdated
| * Represents Platform.sh setup options return data. | ||
| * | ||
| */ | ||
| class SetupOptions extends Resource |
There was a problem hiding this comment.
SetupOptions is an odd "resource" name, it just generally doesn't fit the RESTful "resource"/"collection" model because it isn't a thing that I can fetch with a GET, create with a POST (containing its properties), and update with a PUT/PATCH. I wonder if it just shouldn't use Resource at all. As in we might want a SetupOptions class that wraps the API data with appropriate methods and properties.
The other related problem here is that the class doesn't have/document any properties. Other Resource classes use these @property-read annotations because you can fetch from properties via __get(), so you can get a list of the properties within your object (if you have an IDE or have read the docblock). But the data here is a list so it doesn't have properties...
... so tl;dr I think extending Resource is getting in the way.
There was a problem hiding this comment.
I'm still learning about the API but I'm wondering if there are other API endpoints we're going to need to use in future that behave in the same way? If that's the case we probably shouldn't call it SetupOptions.
…o handle a template and initialize flag
…class because no existing methods made sense to use in the context of the need
|
@pjcdawkins Just wanted to follow up on this, are there any other changes to make to get this branch in order? |
|
Incorporated with some changes into d13eb8c |
2 participants
This branch updates the
createproject command to look to thesetup/optionsapi before falling back into the general plans/regions lists so that if a user has auser_project_optionsfile specified it is respected. This is dependent on PR #37 and will need to be rebased once that branch is finalized.