Feature/json api output#24
Conversation
There was a problem hiding this comment.
I agree with the approach to have each resource be a separate object, but I feel like the current ResourceObject (in the commented out functions) is gaining a lot of responsibilities that feel like they belong to the renderer. Furthermore, we might not need things like a relationship object, as we already have Association.
Let's discuss how to move forward tomorrow.
| # to type-hint and annotate than it's worth. | ||
| - src/Apitizer/Support/ClassFilter.php | ||
|
|
||
| checkMissingIterableValueType: false |
There was a problem hiding this comment.
Let's not disable this, as it forces us to think about the content of our arrays/iterables, rather than having undocumented arrays with "something" in them. If the array is too complex to easily document, make it an object (or array of objects).
| if ($this->isSingleRowOfData($data)) { | ||
| $data = collect([$data]); | ||
| } |
There was a problem hiding this comment.
This seems incorrect, as it makes it impossible to return a single object from the API, while that is supported by the JSON-API spec:
Primary data MUST be either:
- a single resource object, a single resource identifier object, or null, for requests that target single resources
- an array of resource objects, an array of resource identifier objects, or an empty array ([]), for requests that target resource collections
| $data = [ | ||
| 'data' => [] | ||
| ]; | ||
|
|
||
| foreach ($this->resources as $resource) { | ||
| $data['data'][] = $resource->toArray(); | ||
| } | ||
|
|
||
| return $data; | ||
| } |
There was a problem hiding this comment.
I would probably simplify this to something like:
return [
'data' => collect($this->resources)->map->toArray()->all(),
];
If we add the Illuminate\Contracts\Support\Arrayable interface to the ResourceObject (which is already implemented with toArray), then we can simplify it even further to:
return [
'data' => collect($this->resources)->toArray(),
];
Because the Collection class will then take care of calling toArray for us. This advice holds for all container objects we introduce by the way.
| // need to do this because DB unreliable since it doesn't always delete entire DB before test | ||
| $user = User::updateOrCreate(['id' => 1], [ | ||
| 'name' => 'Daan Hage', | ||
| 'email' => 'daan@atabix.nl', | ||
| ]); |
There was a problem hiding this comment.
Is there a reason a factory wasn't used for this?
Also, the database isn't deleted because the feature tests uses DatabaseTransactions to rollback changes, rather than completely rebuilding the database for all the tests.
There was a problem hiding this comment.
not really possible when I return an ID. Need a way to keep ID the same every time
jorisatabix
left a comment
jorisatabix
left a comment
There was a problem hiding this comment.
Why is there so much uncommented code?
Haven't tested the code yet, just looked at it.
| $request = $this->request()->fields('name, email')->make(); | ||
| $result = UserBuilder::make($request)->setRenderer(new JsonApiRenderer)->render($collection); | ||
|
|
||
| $this->assertJsonStringEqualsJsonFile( |
There was a problem hiding this comment.
No need to assert on json? And maybe put it inline so we can directly see what the result should be.
There was a problem hiding this comment.
but assertJsonStringEqualsJsonFile looks so hot!
| // need to do this because DB unreliable since it doesn't always delete entire DB before test | ||
| $user = User::updateOrCreate(['id' => 1], [ | ||
| 'name' => 'Daan Hage', | ||
| 'email' => 'daan@atabix.nl', | ||
| ]); |
| class JsonApiTest extends TestCase | ||
| { | ||
| /** @test */ | ||
| public function it_renders_existing_eloquent_model_to_jsonapi_format() |
There was a problem hiding this comment.
I would suggest
public function we_can_use_the_json_api_renderer_to_format_the_builder_response_in_json_api_format()
Mainly because I am looking at what this PR does from the tests. Same goes for the one below.
| return [ | ||
| 'name' => $faker->name, | ||
| 'email' => $faker->email, | ||
| 'should_reset_password' => rand(0, 1), |
There was a problem hiding this comment.
Please don't use random in the factories. In another project, after using it for a while, it can lead to failures in x % of the time for certain tests.
It's probably not needed to be set here.
There was a problem hiding this comment.
Why? this value is always true or false so this should work. If a test is dependant on this value then it should be set inside the test and never be hidden inside factory
There was a problem hiding this comment.
Thus it can be removed from here.
| */ | ||
| public static function factory(array $attributes, string $type, string $id) | ||
| { | ||
| $resourceObject = new self($type, $id); |
There was a problem hiding this comment.
Use the contstructor? Or factory(ResourceObject::class)->create($attributes);
There was a problem hiding this comment.
je can use the factory pattern also just for initing your object if you have some specific create logic you dont always want inside your constructor. I remember we had this discussion before ;)
There was a problem hiding this comment.
We didn't finish that discussion
Because I'm not done :). ANd its inspiration code |
Codecov Report
@@ Coverage Diff @@
## master #24 +/- ##
============================================
- Coverage 81.94% 81.71% -0.24%
- Complexity 851 875 +24
============================================
Files 105 108 +3
Lines 1983 2040 +57
============================================
+ Hits 1625 1667 +42
- Misses 358 373 +15
Continue to review full report at Codecov.
|
4 participants
@drtheuns het aantal commits is nu onduidelijk terug te zien door php cs fixer. Ik adviseer dat we die snel mergen ivm leesbaarheid. Benieuwd wat je voor de rest van me initiële opzet vindt!
@jorisatabix