-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Fixed [RB-20541]: Label2 Empty Row Range #18224
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
base: develop
Are you sure you want to change the base?
Conversation
app/View/Label.php
Outdated
| if($emptyRowsCount) { | ||
| // Create empty rows | ||
| $emptyRows = collect(range(1, $emptyRowsCount))->map(function () { | ||
| $emptyRows = collect(range(0, $emptyRowsCount))->map(function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could prevent the error, but I think the problem lies with $emptyRowCount being a float. The range can accept floats but we need a non negative integer for the row count. Also starting at 0 will be N+1 rows. so if $emptyRowCount is 3, it would skip 4 rows.
I think casting $emptyRowsCount to an integer and round() the value might be the better fit here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh great catch! That's a good point.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
easier yet, we could put a step=1 on the input.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We would want to guard against manual database changes though so I think a code change in this chunk is still needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
interesting. I didn't read the code as it being an n+1.
it read as here is the start and the end of the range. so, 0 to 1 is 1.
additionally in my db its listed as a long, and I could NOT force it to be a decimal manually in the db. would always reset itself back to 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you're inferring the wrong N+1, which in our context usually refers to DB queries
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yea the php docs have a good example of this
|
@akemidx still need to revert the range start to 1. |
This is from rollbar#20541
User was out of range due to the min being 0, but the range starting at 1.
This allows for a negative range.
This fix should prevent that from 500ing again due to being out of range.