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

Fix filter null values when array does not start with 0 #4937

Conversation

MrCrayon
Copy link
Collaborator

#3350 assumes arrays always starts with 0 and are ordered but this is not true for custom selects that might pass additional data for pivot tables like this:

[
    120 => ["order" => 1].
    240 => ["order" => 2],
]

That kind of data caused:

ErrorException
Undefined offset: 0

To be honest I cannot replicate at all the problem that #3350 PR was solving even totally removing the code, but I only changed it because it might happen in some occasion that I'm unable to reproduce.

```php
[
    120 => ["order" => 1].
    240 => ["order" => 2],
]
```

That kind of data caused:
```
ErrorException
Undefined offset: 0
```

To be honest I cannot replicate at all the problem that thedevdojo#3350 PR was solving even totally removing the code, but I only changed it because it might happen in some occasion that I'm unable to reproduce.
@MrCrayon MrCrayon force-pushed the fix-filter-null-values-in-contenttype-relationship branch from 23b5a59 to ef1d13b Compare May 14, 2020 05:35
@codecov
Copy link

codecov bot commented May 14, 2020

Codecov Report

Merging #4937 into 1.4 will increase coverage by 0.01%.
The diff coverage is 50.00%.

Impacted file tree graph

@@             Coverage Diff              @@
##                1.4    #4937      +/-   ##
============================================
+ Coverage     62.94%   62.96%   +0.01%     
+ Complexity     1374     1372       -2     
============================================
  Files           194      194              
  Lines          4008     4007       -1     
============================================
  Hits           2523     2523              
+ Misses         1485     1484       -1     
Impacted Files Coverage Δ Complexity Δ
src/Http/Controllers/ContentTypes/Relationship.php 83.33% <50.00%> (+11.90%) 2.00 <0.00> (-2.00) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2902a3c...ef1d13b. Read the comment docs.

Copy link
Collaborator

@emptynick emptynick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And it looks much cleaner. Thanks 👍

@emptynick emptynick merged commit 4b3eb20 into thedevdojo:1.4 May 17, 2020
rozaverta added a commit to rozaverta/voyager that referenced this pull request Jul 7, 2020
@MrCrayon MrCrayon deleted the fix-filter-null-values-in-contenttype-relationship branch August 10, 2020 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants