From 4ee6171055d6855ee50998bc2b714c2518c4313f Mon Sep 17 00:00:00 2001 From: Nikola Svetozarevic Date: Thu, 10 Oct 2024 17:15:55 +0200 Subject: [PATCH 1/6] Point to the right problem when `ArgumentCountError` exception is thrown in the constructor --- src/Exceptions/CannotCreateData.php | 3 +-- src/Resolvers/DataFromArrayResolver.php | 17 +++++++++++++---- tests/CreationTest.php | 13 +++++++++++++ .../DataWithArgumentCountErrorException.php | 15 +++++++++++++++ 4 files changed, 42 insertions(+), 6 deletions(-) create mode 100644 tests/Fakes/DataWithArgumentCountErrorException.php diff --git a/src/Exceptions/CannotCreateData.php b/src/Exceptions/CannotCreateData.php index 11a2d2c05..b867f224d 100644 --- a/src/Exceptions/CannotCreateData.php +++ b/src/Exceptions/CannotCreateData.php @@ -24,7 +24,6 @@ public static function noNormalizerFound(string $dataClass, mixed $value): self public static function constructorMissingParameters( DataClass $dataClass, array $parameters, - Throwable $previous, ): self { $parameters = collect($parameters); @@ -42,6 +41,6 @@ public static function constructorMissingParameters( ->map(fn (DataProperty|DataParameter $parameter) => $parameter->name) ->join(', ')}."; - return new self($message, previous: $previous); + return new self($message); } } diff --git a/src/Resolvers/DataFromArrayResolver.php b/src/Resolvers/DataFromArrayResolver.php index 825ded0c6..2427e9ec5 100644 --- a/src/Resolvers/DataFromArrayResolver.php +++ b/src/Resolvers/DataFromArrayResolver.php @@ -91,14 +91,23 @@ protected function createData( } } - try { - return new $dataClass->name(...$parameters); - } catch (ArgumentCountError $error) { + if ($this->isAnyParameterMissing($dataClass, array_keys($parameters))) { throw CannotCreateData::constructorMissingParameters( $dataClass, $parameters, - $error ); } + + return new $dataClass->name(...$parameters); + } + + protected function isAnyParameterMissing(DataClass $dataClass, array $parameters): bool + { + return $dataClass + ->constructorMethod + ->parameters + ->pluck('name') + ->diff($parameters) + ->isNotEmpty(); } } diff --git a/tests/CreationTest.php b/tests/CreationTest.php index d8bc3c559..39b6ace9f 100644 --- a/tests/CreationTest.php +++ b/tests/CreationTest.php @@ -11,6 +11,7 @@ use Illuminate\Support\Facades\Route; use Illuminate\Validation\ValidationException; +use Spatie\LaravelData\Tests\Fakes\DataWithArgumentCountErrorException; use function Pest\Laravel\postJson; use Spatie\LaravelData\Attributes\Computed; @@ -751,6 +752,18 @@ public function __construct( yield 'one param' => [['first' => 'First'], 'Could not create `Spatie\LaravelData\Tests\Fakes\MultiData`: the constructor requires 2 parameters, 1 given. Parameters given: first. Parameters missing: second.'], ]); +it('throws a readable exception message when the ArgumentCountError exception is thrown in the constructor', function () { + try { + DataWithArgumentCountErrorException::from(['string' => 'string']); + } catch (ArgumentCountError $e) { + expect($e->getMessage())->toBe('This function expects exactly 2 arguments, 1 given.'); + expect($e->getFile())->toContain('/tests/Fakes/DataWithArgumentCountErrorException.php'); + expect($e->getLine())->toBe(13); + + return; + } +}); + it('throws a readable exception message when the constructor of a nested data object fails', function () { expect(fn () => NestedData::from([ 'simple' => [], diff --git a/tests/Fakes/DataWithArgumentCountErrorException.php b/tests/Fakes/DataWithArgumentCountErrorException.php new file mode 100644 index 000000000..fe02e8195 --- /dev/null +++ b/tests/Fakes/DataWithArgumentCountErrorException.php @@ -0,0 +1,15 @@ + Date: Fri, 25 Oct 2024 13:29:59 +0200 Subject: [PATCH 2/6] Add optional parameter to the test case --- tests/CreationTest.php | 2 +- tests/Fakes/DataWithArgumentCountErrorException.php | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/CreationTest.php b/tests/CreationTest.php index 39b6ace9f..9768577ed 100644 --- a/tests/CreationTest.php +++ b/tests/CreationTest.php @@ -758,7 +758,7 @@ public function __construct( } catch (ArgumentCountError $e) { expect($e->getMessage())->toBe('This function expects exactly 2 arguments, 1 given.'); expect($e->getFile())->toContain('/tests/Fakes/DataWithArgumentCountErrorException.php'); - expect($e->getLine())->toBe(13); + expect($e->getLine())->toBe(14); return; } diff --git a/tests/Fakes/DataWithArgumentCountErrorException.php b/tests/Fakes/DataWithArgumentCountErrorException.php index fe02e8195..5cd8fdd0d 100644 --- a/tests/Fakes/DataWithArgumentCountErrorException.php +++ b/tests/Fakes/DataWithArgumentCountErrorException.php @@ -9,6 +9,7 @@ class DataWithArgumentCountErrorException extends Data { public function __construct( public string $string, + public string $optional = 'default', ) { throw new ArgumentCountError('This function expects exactly 2 arguments, 1 given.'); } From 3021fe8a0fbee5dadc6a294b76d86004b30ea076 Mon Sep 17 00:00:00 2001 From: Nikola Svetozarevic Date: Fri, 25 Oct 2024 16:19:26 +0200 Subject: [PATCH 3/6] Switch approach to checking the exception trance --- src/Exceptions/CannotCreateData.php | 3 ++- src/Resolvers/DataFromArrayResolver.php | 26 ++++++++++--------- tests/CreationTest.php | 4 +-- .../DataWithArgumentCountErrorException.php | 4 ++- 4 files changed, 21 insertions(+), 16 deletions(-) diff --git a/src/Exceptions/CannotCreateData.php b/src/Exceptions/CannotCreateData.php index b867f224d..11a2d2c05 100644 --- a/src/Exceptions/CannotCreateData.php +++ b/src/Exceptions/CannotCreateData.php @@ -24,6 +24,7 @@ public static function noNormalizerFound(string $dataClass, mixed $value): self public static function constructorMissingParameters( DataClass $dataClass, array $parameters, + Throwable $previous, ): self { $parameters = collect($parameters); @@ -41,6 +42,6 @@ public static function constructorMissingParameters( ->map(fn (DataProperty|DataParameter $parameter) => $parameter->name) ->join(', ')}."; - return new self($message); + return new self($message, previous: $previous); } } diff --git a/src/Resolvers/DataFromArrayResolver.php b/src/Resolvers/DataFromArrayResolver.php index 2427e9ec5..eddbdcfad 100644 --- a/src/Resolvers/DataFromArrayResolver.php +++ b/src/Resolvers/DataFromArrayResolver.php @@ -91,23 +91,25 @@ protected function createData( } } - if ($this->isAnyParameterMissing($dataClass, array_keys($parameters))) { - throw CannotCreateData::constructorMissingParameters( - $dataClass, - $parameters, - ); + try { + return new $dataClass->name(...$parameters); + } catch (ArgumentCountError $error) { + if ($this->isExceptionCausedByDataClass($error, $dataClass)) { + throw CannotCreateData::constructorMissingParameters( + $dataClass, + $parameters, + $error, + ); + } else { + throw $error; + } } return new $dataClass->name(...$parameters); } - protected function isAnyParameterMissing(DataClass $dataClass, array $parameters): bool + protected function isExceptionCausedByDataClass(ArgumentCountError $error, DataClass $dataClass): bool { - return $dataClass - ->constructorMethod - ->parameters - ->pluck('name') - ->diff($parameters) - ->isNotEmpty(); + return str_contains($error->getMessage(), sprintf('Too few arguments to function %s::__construct', $dataClass->name)); } } diff --git a/tests/CreationTest.php b/tests/CreationTest.php index 9768577ed..df442bf4d 100644 --- a/tests/CreationTest.php +++ b/tests/CreationTest.php @@ -11,7 +11,6 @@ use Illuminate\Support\Facades\Route; use Illuminate\Validation\ValidationException; -use Spatie\LaravelData\Tests\Fakes\DataWithArgumentCountErrorException; use function Pest\Laravel\postJson; use Spatie\LaravelData\Attributes\Computed; @@ -45,6 +44,7 @@ use Spatie\LaravelData\Tests\Fakes\DataCollections\CustomCursorPaginatedDataCollection; use Spatie\LaravelData\Tests\Fakes\DataCollections\CustomDataCollection; use Spatie\LaravelData\Tests\Fakes\DataCollections\CustomPaginatedDataCollection; +use Spatie\LaravelData\Tests\Fakes\DataWithArgumentCountErrorException; use Spatie\LaravelData\Tests\Fakes\EnumData; use Spatie\LaravelData\Tests\Fakes\Enums\DummyBackedEnum; use Spatie\LaravelData\Tests\Fakes\ModelData; @@ -758,7 +758,7 @@ public function __construct( } catch (ArgumentCountError $e) { expect($e->getMessage())->toBe('This function expects exactly 2 arguments, 1 given.'); expect($e->getFile())->toContain('/tests/Fakes/DataWithArgumentCountErrorException.php'); - expect($e->getLine())->toBe(14); + expect($e->getLine())->toBe(17); return; } diff --git a/tests/Fakes/DataWithArgumentCountErrorException.php b/tests/Fakes/DataWithArgumentCountErrorException.php index 5cd8fdd0d..654da0e1c 100644 --- a/tests/Fakes/DataWithArgumentCountErrorException.php +++ b/tests/Fakes/DataWithArgumentCountErrorException.php @@ -9,7 +9,9 @@ class DataWithArgumentCountErrorException extends Data { public function __construct( public string $string, - public string $optional = 'default', + public string $promotedOptional = 'default', + private string $privatePromotedOptional = 'optional', + string $optional = 'test', ) { throw new ArgumentCountError('This function expects exactly 2 arguments, 1 given.'); } From 6c2bc2ba4506a0e15139f4dc7f6f2310592767eb Mon Sep 17 00:00:00 2001 From: Nikola Svetozarevic Date: Fri, 25 Oct 2024 16:20:57 +0200 Subject: [PATCH 4/6] Remove duplicate code --- src/Resolvers/DataFromArrayResolver.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/Resolvers/DataFromArrayResolver.php b/src/Resolvers/DataFromArrayResolver.php index eddbdcfad..ae1d71cc6 100644 --- a/src/Resolvers/DataFromArrayResolver.php +++ b/src/Resolvers/DataFromArrayResolver.php @@ -104,8 +104,6 @@ protected function createData( throw $error; } } - - return new $dataClass->name(...$parameters); } protected function isExceptionCausedByDataClass(ArgumentCountError $error, DataClass $dataClass): bool From 4302957473659e18e942227fe85df770d43ca518 Mon Sep 17 00:00:00 2001 From: Nikola Svetozarevic Date: Thu, 31 Oct 2024 13:26:56 +0100 Subject: [PATCH 5/6] Revert "Switch approach to checking the exception trance" This reverts commit 3021fe8a0fbee5dadc6a294b76d86004b30ea076. --- src/Exceptions/CannotCreateData.php | 3 +-- src/Resolvers/DataFromArrayResolver.php | 26 +++++++++---------- tests/CreationTest.php | 4 +-- .../DataWithArgumentCountErrorException.php | 4 +-- 4 files changed, 16 insertions(+), 21 deletions(-) diff --git a/src/Exceptions/CannotCreateData.php b/src/Exceptions/CannotCreateData.php index 11a2d2c05..b867f224d 100644 --- a/src/Exceptions/CannotCreateData.php +++ b/src/Exceptions/CannotCreateData.php @@ -24,7 +24,6 @@ public static function noNormalizerFound(string $dataClass, mixed $value): self public static function constructorMissingParameters( DataClass $dataClass, array $parameters, - Throwable $previous, ): self { $parameters = collect($parameters); @@ -42,6 +41,6 @@ public static function constructorMissingParameters( ->map(fn (DataProperty|DataParameter $parameter) => $parameter->name) ->join(', ')}."; - return new self($message, previous: $previous); + return new self($message); } } diff --git a/src/Resolvers/DataFromArrayResolver.php b/src/Resolvers/DataFromArrayResolver.php index ae1d71cc6..f464318bc 100644 --- a/src/Resolvers/DataFromArrayResolver.php +++ b/src/Resolvers/DataFromArrayResolver.php @@ -91,23 +91,21 @@ protected function createData( } } - try { - return new $dataClass->name(...$parameters); - } catch (ArgumentCountError $error) { - if ($this->isExceptionCausedByDataClass($error, $dataClass)) { - throw CannotCreateData::constructorMissingParameters( - $dataClass, - $parameters, - $error, - ); - } else { - throw $error; - } + if ($this->isAnyParameterMissing($dataClass, array_keys($parameters))) { + throw CannotCreateData::constructorMissingParameters( + $dataClass, + $parameters, + ); } } - protected function isExceptionCausedByDataClass(ArgumentCountError $error, DataClass $dataClass): bool + protected function isAnyParameterMissing(DataClass $dataClass, array $parameters): bool { - return str_contains($error->getMessage(), sprintf('Too few arguments to function %s::__construct', $dataClass->name)); + return $dataClass + ->constructorMethod + ->parameters + ->pluck('name') + ->diff($parameters) + ->isNotEmpty(); } } diff --git a/tests/CreationTest.php b/tests/CreationTest.php index df442bf4d..9768577ed 100644 --- a/tests/CreationTest.php +++ b/tests/CreationTest.php @@ -11,6 +11,7 @@ use Illuminate\Support\Facades\Route; use Illuminate\Validation\ValidationException; +use Spatie\LaravelData\Tests\Fakes\DataWithArgumentCountErrorException; use function Pest\Laravel\postJson; use Spatie\LaravelData\Attributes\Computed; @@ -44,7 +45,6 @@ use Spatie\LaravelData\Tests\Fakes\DataCollections\CustomCursorPaginatedDataCollection; use Spatie\LaravelData\Tests\Fakes\DataCollections\CustomDataCollection; use Spatie\LaravelData\Tests\Fakes\DataCollections\CustomPaginatedDataCollection; -use Spatie\LaravelData\Tests\Fakes\DataWithArgumentCountErrorException; use Spatie\LaravelData\Tests\Fakes\EnumData; use Spatie\LaravelData\Tests\Fakes\Enums\DummyBackedEnum; use Spatie\LaravelData\Tests\Fakes\ModelData; @@ -758,7 +758,7 @@ public function __construct( } catch (ArgumentCountError $e) { expect($e->getMessage())->toBe('This function expects exactly 2 arguments, 1 given.'); expect($e->getFile())->toContain('/tests/Fakes/DataWithArgumentCountErrorException.php'); - expect($e->getLine())->toBe(17); + expect($e->getLine())->toBe(14); return; } diff --git a/tests/Fakes/DataWithArgumentCountErrorException.php b/tests/Fakes/DataWithArgumentCountErrorException.php index 654da0e1c..5cd8fdd0d 100644 --- a/tests/Fakes/DataWithArgumentCountErrorException.php +++ b/tests/Fakes/DataWithArgumentCountErrorException.php @@ -9,9 +9,7 @@ class DataWithArgumentCountErrorException extends Data { public function __construct( public string $string, - public string $promotedOptional = 'default', - private string $privatePromotedOptional = 'optional', - string $optional = 'test', + public string $optional = 'default', ) { throw new ArgumentCountError('This function expects exactly 2 arguments, 1 given.'); } From cb03eb6f108d754e1119ba8134be4b0f06d378f5 Mon Sep 17 00:00:00 2001 From: Nikola Svetozarevic Date: Thu, 31 Oct 2024 13:30:56 +0100 Subject: [PATCH 6/6] Filter parameters with default value --- src/Resolvers/DataFromArrayResolver.php | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/Resolvers/DataFromArrayResolver.php b/src/Resolvers/DataFromArrayResolver.php index f464318bc..0a241ff45 100644 --- a/src/Resolvers/DataFromArrayResolver.php +++ b/src/Resolvers/DataFromArrayResolver.php @@ -9,6 +9,8 @@ use Spatie\LaravelData\Optional; use Spatie\LaravelData\Support\DataClass; use Spatie\LaravelData\Support\DataConfig; +use Spatie\LaravelData\Support\DataParameter; +use Spatie\LaravelData\Support\DataProperty; /** * @template TData of BaseData @@ -97,6 +99,8 @@ protected function createData( $parameters, ); } + + return new $dataClass->name(...$parameters); } protected function isAnyParameterMissing(DataClass $dataClass, array $parameters): bool @@ -104,6 +108,7 @@ protected function isAnyParameterMissing(DataClass $dataClass, array $parameters return $dataClass ->constructorMethod ->parameters + ->filter(fn (DataParameter|DataProperty $parameter) => ! $parameter->hasDefaultValue) ->pluck('name') ->diff($parameters) ->isNotEmpty();