Point to the right problem on ArgumentCountError exception

[nsvetozarevic]

Fixes #882

[Tofandel]

You could also just check the trace of the exception and run some reflection in the catch to decide wether to throw the original error or not https://onlinephp.io?s=zZBBS8NAEIXPDeQ_jBCaDVTqOTVKWnvUg3grpaybSVNJdsNkYi3S_-5mY0t68STispfdee_xzbu9r4va96ZTWGpGgoNpCZTJEAoknADqN3O48j1VyqaBFD59b5S3WvHOaNhslNENU6tYBHICwWvkBEffs_dkmgN-MOrsR3tvHNWSUHMcX4xmg0jfYzo4rcY9pG54BCVZFSBS2raV9S9Mq3lJZAgC7JMDJqkQEvtxfbdFfumeIlrdrLv0XQ6iV6zCE164hiRJIByghDAew0nnlvsWiQ7mGfMSnfURuTCZSOPYiSaXIVHkCB7QDmmnt4tOI_rPJ1lZqh4Z7HmXtMnaqhZ2D1cDYNlg3xUXZPZ2n2E9w3Lm_7qc-d-X43tnx28zzb4A&v=8.3.8%2C8.2.20%2C8.1.29

Because right now the isAnyParameterMissing code doesn't account for optional parameters or variadic parameters

[Tofandel]

Suggested change

	if ($this->isAnyParameterMissing($dataClass, array_keys($parameters))) {
	try {
	return new $dataClass->name(...$parameters);
	} catch (ArgumentCountError $error) {
	$trace = current($error->getTrace());
	if ($trace && $trace['function'] === '__construct' && $trace['class'] === (new \ReflectionMethod($dataClass->name, '__construct'))->getDeclaringClass()->getName()) {
	throw CannotCreateData::constructorMissingParameters(
	$dataClass,
	$parameters,
	$error
	);
	} else {
	throw $error;
	}
	}

[nsvetozarevic]

@Tofandel That was my first idea, to check the trace. But this approach looked cleaner. But yeah, I didn't take optional or variadic parameters into account. I'll take a look. Thanks for the review 👍

[nsvetozarevic]

@Tofandel Optional parameters should work actually. Hence all optinal parameters have a default value and they are present in $parameters array. I added an optional parameter to the test case.

As for variadic parameters, I really couldn't figure out how would it create problems? Can you please show me an example?

On the other hand if you're really against this approach, I could check the trace and rethrow the exception.

[Tofandel]

have a default value and they are present in $parameters array

That is only the case for non promoted parameters, promoted parameters do not appear in the array, according to the code, this would cause issues and throw exception where it was not previousy throwing (can you check your new test case? It should definitely not pass)

Same for variadic parameters which are ALWAYS optional

https://onlinephp.io?s=nVPBTtwwFDxvpPzDQ4qIIy0L7XG3BSHooRdAVdXLCkXGeem69caR7SysEP_eZ282MbBcGuU045k3b-J8uWhXbZoIxa2Fn2gdPKfJpDVywx2CbBxkjtBFBHJj-BaytTYY4O5BSQF11wgndQNlKXRjnemEY0RPBg_4CmdTj7xz_0zUp0DNZrNsw43klRQeKEKcSeZW0p6c9y5Doj3uo3h8r_TcS5rQmyanp_CtcWhgqzsDQlcIKzQ4BWz-6O1RmmSCpKzBR_iBtcKwxJWvg_k65vNQTVGcnP9Gd7XfTBtWLLx2BO644WukSdZHEeH8iIXjJGiNbtE4if7U8t57tLEwgm4IHaCaVuRiBeyDkdzCaLRrLXJZ3vtMAx-yeSKkmlBtZdWtW5Z_t6BbXwBXkMPssITwPLAspqW97ZV04gJyiog5zCGvubKYF34S0CNrYOEGlX9xW-KTtM6yg3OmELVVhJ2gf6LOlge1u30H9QeHFqMjtepk02EPvYxhj-D1mndGr7XDitY8Pn7DXWPNO-U2XHV4ueFS8QdFg_4n-yui9_3lfVkxZgw3fPh8kfN095eWlaxrFl2EPU7dx61TvcXiHw%2C%2C&v=8.3.8%2C8.2.20%2C8.1.29

You would need to filter out optional and variadic params before plucking the name and diffing, but the DataParameter does not contain the isVariadic (or isOptional) information

[Tofandel]

Suggested change

	->pluck('name')
	->filter(fn (\Spatie\LaravelData\Support\DataParameter $p) => !$parameter->hasDefaultValue)
	->pluck('name')

[Tofandel]

Wouldn't this filter out required parameters as well, which do not have a default value?

It would keep only parameters without default values aka required ones, though there is one obscure area, is if you have a default value before a required param (which normally emits a warning)

Which is why filtering on is optional would provide the perfect solution, as it would work with variadic parameters, but it would need to be added to the DataParameter class and factory..
Or a new Reflection needs to be made on the spot

[nsvetozarevic]

@Tofandel Wow, needed some time to wrap my head around this :) Sorry for the late reply.

promoted parameters do not appear in the array

I can now see the confusion. They do appear. They are passed to createData function as part of the properties array. (DataFromArrayResolver class) .

can you check your new test case?

I did run my test case, and it passed. I think that the optional parameters are covered. In the example you provided, promoted parameter was private, that's why it didn't show up in $parameters, but it was also not present in the $dataClass->constructorMethod->parameters...

Variadic parameter on the other hand is a problem. As you stated, they are always optional, and if not supplied, they are not present in the parameters. I can't find a way to filter them out. Optional parameters have the default value, so it would be easy to filter them out, but variadic do not.

->filter(fn (\Spatie\LaravelData\Support\DataParameter $p) => !$parameter->hasDefaultValue)

Wouldn't this filter out required parameters as well, which do not have a default value?

Having all this in mind I think I'll have to switch back to checking the stack trace. Great catch btw 👍

[nsvetozarevic]

So I switched back to checking the trace, but it's not that straightforward either.

Trace of the Eeception caused by not passing right parameters to the data class
array:6 [
  "file" => "/Users/nikolasvetozarevic/Projects/laravel-data-parameters-check/src/Resolvers/DataFromArrayResolver.php"
  "line" => 95
  "function" => "__construct"
  "class" => "Spatie\LaravelData\Tests\Fakes\DataWithArgumentCountErrorException"
  "type" => "->"
  "args" => array:2 [
    0 => "string"
    1 => null
  ]
] 

Trace of the Exception caused by something else in the constructor
array:6 [
  "file" => "/Users/nikolasvetozarevic/Projects/laravel-data-parameters-check/src/Resolvers/DataFromArrayResolver.php"
  "line" => 95
  "function" => "__construct"
  "class" => "Spatie\LaravelData\Tests\Fakes\DataWithArgumentCountErrorException"
  "type" => "->"
  "args" => array:5 [
    0 => "string"
    1 => null
    2 => "default"
    3 => "optional"
    4 => "test"
  ]
] 

As you can see, they are basically identical. So the only Idea I have is to check the exceptions message. But that feels kind of hacky. I'll push those changes, please tell me what do you think.

[Tofandel]

I think it does not need to be 100% foolproof to the exact same exception thrown in the body of the same function, because in real life it will happen somewhere else or on another function, because in the end it's just exception wrapping meant for debugging.

I do wonder if maybe the laravel utils just don't provide a way out of the box to check the parameters of a class (but it might have the side effects of adding dependency injection to the constructor, which is not necessarily a bad thing)

[nsvetozarevic]

@Tofandel I reverted the code back to check the parameters and added the filtering you suggested. Is this what you meant?

Tests are passing for me locally

[nsvetozarevic]

@Tofandel just checking if the PR is still alive :)

[Tofandel]

Looks good to me, except this deletion

[nsvetozarevic]

Previously ArgumentCountError was thrown, but now hence we do the checks based on the data class properties, I had to remove that because there is no previous error.