From 83dd8cdbf30086677d7b9b90100cda6c9b5c4a4e Mon Sep 17 00:00:00 2001 From: Taylor Otwell Date: Fri, 2 Mar 2018 11:21:29 -0600 Subject: [PATCH] Conditional load fixes (#23369) * fix conditional loading logic for numeric keyed arrays * Apply fixes from StyleCI (#23368) * more tests * Apply fixes from StyleCI (#23370) * another test * Apply fixes from StyleCI (#23371) * complicate test more --- .../ConditionallyLoadsAttributes.php | 51 ++++--- tests/Integration/Http/ResourceTest.php | 137 ++++++++++++++++++ 2 files changed, 170 insertions(+), 18 deletions(-) diff --git a/src/Illuminate/Http/Resources/ConditionallyLoadsAttributes.php b/src/Illuminate/Http/Resources/ConditionallyLoadsAttributes.php index 300305564ebe..e6f70a098251 100644 --- a/src/Illuminate/Http/Resources/ConditionallyLoadsAttributes.php +++ b/src/Illuminate/Http/Resources/ConditionallyLoadsAttributes.php @@ -16,6 +16,8 @@ protected function filter($data) { $index = -1; + $numericKeys = array_values($data) === $data; + foreach ($data as $key => $value) { $index++; @@ -26,16 +28,7 @@ protected function filter($data) } if (is_numeric($key) && $value instanceof MergeValue) { - return $this->merge($data, $index, $this->filter($value->data)); - } - - if (($value instanceof PotentiallyMissing && $value->isMissing()) || - ($value instanceof self && - $value->resource instanceof PotentiallyMissing && - $value->isMissing())) { - unset($data[$key]); - - $index--; + return $this->merge($data, $index, $this->filter($value->data), $numericKeys); } if ($value instanceof self && is_null($value->resource)) { @@ -43,7 +36,7 @@ protected function filter($data) } } - return $data; + return $this->removeMissingValues($data, $numericKeys); } /** @@ -52,20 +45,42 @@ protected function filter($data) * @param array $data * @param int $index * @param array $merge + * @param bool $numericKeys * @return array */ - protected function merge($data, $index, $merge) + protected function merge($data, $index, $merge, $numericKeys) { - if (array_values($data) === $data) { - return array_merge( + if ($numericKeys) { + return $this->removeMissingValues(array_merge( array_merge(array_slice($data, 0, $index, true), $merge), - $this->filter(array_slice($data, $index + 1, null, true)) - ); + $this->filter(array_values(array_slice($data, $index + 1, null, true))) + ), $numericKeys); } - return array_slice($data, 0, $index, true) + + return $this->removeMissingValues(array_slice($data, 0, $index, true) + $merge + - $this->filter(array_slice($data, $index + 1, null, true)); + $this->filter(array_slice($data, $index + 1, null, true))); + } + + /** + * Remove the missing values from the filtered data. + * + * @param array $data + * @param bool $numericKeys + * @return array + */ + protected function removeMissingValues($data, $numericKeys = false) + { + foreach ($data as $key => $value) { + if (($value instanceof PotentiallyMissing && $value->isMissing()) || + ($value instanceof self && + $value->resource instanceof PotentiallyMissing && + $value->isMissing())) { + unset($data[$key]); + } + } + + return $numericKeys ? array_values($data) : $data; } /** diff --git a/tests/Integration/Http/ResourceTest.php b/tests/Integration/Http/ResourceTest.php index f8d2f1c0d26d..5b52520ac61d 100644 --- a/tests/Integration/Http/ResourceTest.php +++ b/tests/Integration/Http/ResourceTest.php @@ -4,9 +4,11 @@ use Orchestra\Testbench\TestCase; use Illuminate\Support\Facades\Route; +use Illuminate\Http\Resources\MergeValue; use Illuminate\Pagination\LengthAwarePaginator; use Illuminate\Tests\Integration\Http\Fixtures\Post; use Illuminate\Tests\Integration\Http\Fixtures\Author; +use Illuminate\Http\Resources\ConditionallyLoadsAttributes; use Illuminate\Tests\Integration\Http\Fixtures\PostResource; use Illuminate\Tests\Integration\Http\Fixtures\Subscription; use Illuminate\Tests\Integration\Http\Fixtures\PostCollectionResource; @@ -521,4 +523,139 @@ public function test_original_on_response_is_collection_of_model_when_collection $this->assertTrue($response->getOriginalContent()->contains($post)); }); } + + public function test_leading_merge_value_is_merged_correctly() + { + $filter = new class { + use ConditionallyLoadsAttributes; + + public function work() + { + return $this->filter([ + new MergeValue(['First', 'Second']), + 'Taylor', + 'Mohamed', + new MergeValue(['Adam', 'Matt']), + 'Jeffrey', + new MergeValue(['Abigail', 'Lydia']), + ]); + } + }; + + $results = $filter->work(); + + $this->assertEquals([ + 'First', 'Second', 'Taylor', 'Mohamed', 'Adam', 'Matt', 'Jeffrey', 'Abigail', 'Lydia', + ], $results); + } + + public function test_merge_values_may_be_missing() + { + $filter = new class { + use ConditionallyLoadsAttributes; + + public function work() + { + return $this->filter([ + new MergeValue(['First', 'Second']), + 'Taylor', + 'Mohamed', + $this->mergeWhen(false, ['Adam', 'Matt']), + 'Jeffrey', + new MergeValue(['Abigail', 'Lydia']), + ]); + } + }; + + $results = $filter->work(); + + $this->assertEquals([ + 'First', 'Second', 'Taylor', 'Mohamed', 'Jeffrey', 'Abigail', 'Lydia', + ], $results); + } + + public function test_initial_merge_values_may_be_missing() + { + $filter = new class { + use ConditionallyLoadsAttributes; + + public function work() + { + return $this->filter([ + $this->mergeWhen(false, ['First', 'Second']), + 'Taylor', + 'Mohamed', + $this->mergeWhen(true, ['Adam', 'Matt']), + 'Jeffrey', + new MergeValue(['Abigail', 'Lydia']), + ]); + } + }; + + $results = $filter->work(); + + $this->assertEquals([ + 'Taylor', 'Mohamed', 'Adam', 'Matt', 'Jeffrey', 'Abigail', 'Lydia', + ], $results); + } + + public function test_all_merge_values_may_be_missing() + { + $filter = new class { + use ConditionallyLoadsAttributes; + + public function work() + { + return $this->filter([ + $this->mergeWhen(false, ['First', 'Second']), + 'Taylor', + 'Mohamed', + $this->mergeWhen(false, ['Adam', 'Matt']), + 'Jeffrey', + $this->mergeWhen(false, (['Abigail', 'Lydia'])), + ]); + } + }; + + $results = $filter->work(); + + $this->assertEquals([ + 'Taylor', 'Mohamed', 'Jeffrey', + ], $results); + } + + public function test_nested_merges() + { + $filter = new class { + use ConditionallyLoadsAttributes; + + public function work() + { + return $this->filter([ + $this->mergeWhen(true, [['Something']]), + [ + $this->mergeWhen(true, ['First', $this->mergeWhen(true, ['Second'])]), + 'Third', + ], + [ + 'Fourth', + ], + ]); + } + }; + + $results = $filter->work(); + + $this->assertEquals([ + [ + 'Something', + ], + [ + 'First', 'Second', 'Third', + ], + [ + 'Fourth', + ], + ], $results); + } }