Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merging default values doesn't work on non associative array. #24

Closed
ncou opened this issue Jul 16, 2020 · 4 comments
Closed

Merging default values doesn't work on non associative array. #24

ncou opened this issue Jul 16, 2020 · 4 comments

Comments

@ncou
Copy link

ncou commented Jul 16, 2020

Hi,

I encounter a bug with the merge function when the schema use a list of string.
Here is an example :

`
$schema = Expect::structure([
'paths' => Expect::arrayOf('string')->default(['path_1']),
]);

$data = ['paths' => ['path_1']];
`
after the processSchema() i got the following result :

['path_1', 'path_1']
but i expect the values to be merged, and only have ['path_1'] as a result. This wrong behavior is when the array has numeric keys. If a use an associative array (ex : ['default' => 'path_1']) the merge work fine and i got only on element for the result.

Keep up the good work, this library is amazing :)

@mabar
Copy link
Contributor

mabar commented Jul 16, 2020

What would be result if $data = ['paths' => ['path_1', 'path_1', 'path_1']];?
Multiple identical values in dataset are not necessarily an error.

@ncou
Copy link
Author

ncou commented Jul 16, 2020

yes you got a point, but if there is already 'path_1' in the user value, why add the default value (which is the same). I don't think a unique function should be applyed, i only think it's not really logical to add again the default value while it's already present in the data array.

@ncou
Copy link
Author

ncou commented Jul 16, 2020

I think my issue is the same than the ticket #13
If it's a duplicate, i will close my ticket.

@dg
Copy link
Member

dg commented Dec 15, 2020

Yes, it is duplicate issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants