spatie / laravel-query-builder

Easily build Eloquent queries from API requests
https://spatie.be/docs/laravel-query-builder
MIT License
4.02k stars 395 forks source link

[v5.1.0] BC Break -- `array_diff_assoc` vs `array_diff` -- Array to string conversion #826

Closed stevebauman closed 1 year ago

stevebauman commented 1 year ago

PHP Version: 8.0.25 Laravel Version: 9.41.0 Query Builder Version: 5.1.0

After updating from v5.0.3 to v5.1.0, all of my tests with this package broke, with the below exception:

Exception Screenshot Screenshot 2022-11-28 at 12 43 42 PM

This is because several of my filters contain multi-dimensional associative arrays, and not just key-value pairs.

For further context, we've built a condition system with this package. Dumping $value here:

https://github.com/spatie/laravel-query-builder/blob/09d22513dc02417781499b6b9e9b89bea5e85242/src/AllowedFilter.php#L155-L159

Results in:

array:1 [▼
  0 => array:2 [▼
    "attribute" => "name"
    "operator" => "equals"
    "value" => "John Doe"    
  ]
]

However, it appears array_diff does not support multi-dimensional arrays, breaking the value resolution function and throwing the above exception.

Let me know if you require further information. I'll work on a PR in the meantime, along with a test case.

stevebauman commented 1 year ago

Failing suite:

https://github.com/stevebauman/laravel-query-builder/actions/runs/3567463398

Failing test:

https://github.com/stevebauman/laravel-query-builder/commit/7c58630bef90520b1473ce1be6c6b188d880221b

davidjr82 commented 1 year ago

hi @stevebauman ,

Wasn't your code working because you had only one item in the array? array_diff_assoc would work because of that, as soon as you would have 2 keys in your array, then it would fail, wouldn't it?

Thinking on a solution, I don't see an obvious one. Could the solution be on the user side, flattening the array dimensions they want before evaluating the ignore?

I don't know if the solution goes in the direction of flattening the multidimensional array in the package, because then, how deep should the package flatten the array? If they flatten one dimension as in your case, why flatten just one dimension would work for everybody?

Looking forward to see your PR proposal. Sorry to see my PR broke your code! 😞

stevebauman commented 1 year ago

Hey @davidjr82,

as soon as you would have 2 keys in your array, then it would fail, wouldn't it?

Unfortunately this isn't the case, as shown in the second comment with the failing test.

No worries on the PR issue! Things happen, and there was no test case covering this, so you wouldn't have been able to easily catch it.

I've submitted a PR with passing tests if you're interested in reviewing: https://github.com/spatie/laravel-query-builder/pull/827

oulfr commented 1 year ago

I have the same issue and all my test are broken because of array_diff that not accept multi-dimensional arrays