Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[ProgressBar] Empty iterable throws Exception on "maximum number of steps is not set"#47259
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
carsonbot commentedAug 12, 2022
Hey! I see that this is your first PR. That is great! Welcome! Symfony has acontribution guide which I suggest you to read. In short:
Review the GitHub status checks of your pull request and try to solve the reported issues. If some tests are failing, try to see if they are failing because of this change. When two Symfony core team members approve this change, it will be merged and you will become an official Symfony contributor! I am going to sit back now and wait for the reviews. Cheers! Carsonbot |
freimair commentedAug 12, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
well, the issues seems to be there since the creation of the progress bar#10356 back in 2014. Has been merged into the 2.5-dev branch554b28d. Please let me know if I should PR against 2.8? I would opt for including it in v6, because stirring up code that old seems not reasonable to me. plus, nobody encountered the issue since then or people made sure not to iterate over emtpy arrays. |
Uh oh!
There was an error while loading.Please reload this page.
`max` value has been prepared to contain a `null` value, however,the constructor with its default value of `max = 0` prevents`$max` to ever be `null`.Hence, declare `$max` as `int` instead of `?int`.
Up until now, it has been very hard to determin whether a progressbarhas a known maximum value or not, because the property `max` is aninteger. Up until now, `max == 0` has been treated like we do _not_know the `max` value. However, `max == 0` _is_ a valid `max` valuewhen iterating over an empty but countable array for example.We cannot simply keep `max == null` if unknown, because that would breakrendering mechanics and fixing those would require interface changes.Hence, enter `isMaxKnown` - a property which keeps track of whether the`max` value is known or not.
| privateint$startingStep =0; | ||
| private ?int$max =null; | ||
| privateint$max =0; | ||
| privatebool$isMaxKnown =false; |
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.
Do we need this new variable? I suppose0 is not a valid max, so having max at 0 probably means that it is not known?
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.
the whole point of this PR and the issue I found with Progessbars is that max = 0 IS a valid max value but it is treated like it is not.
we stumbled over that behavior because we need to do a lot of import/sync operations, nasty but necessary. Once in a while, one of our imports/sync iterables is empty and thus max value = 0.foreach([] as ...) does not throw errors in vanilla PHP. Using the progressbar withforeach($bar->iterate([]) as ...) does.
jwilson-ind commentedOct 11, 2023
I see this PR is in milestone6.4 - does that mean that this fix will be in the 6.4 release? |
jwilson-ind commentedOct 11, 2023
I'd like to note that I believe this fix isn't going to help if you just use $progressBar =newProgressBar($output,0 );$progressBar->start(); This only helps when you trigger the bar with |
GromNaN commentedNov 15, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I submitted an alternative patch, using |
fabpot commentedNov 16, 2023
Closing in favor of#52605 |
…ray (GromNaN)This PR was merged into the 7.1 branch.Discussion----------[Console] Support `ProgressBar::iterate()` on empty array| Q | A| ------------- | ---| Branch? | 6.4| Bug fix? | yes| New feature? | no| Deprecations? | no| Issues |Fix#47244| License | MITAlternative to#47259Use `$max = null` to indicate that the value is unknown. This allows `0` to be displayed by directly setting the progress bar to 100%.Zero is only supported for `iterate()`. When passed to the constructor or `setMaxSteps`, it means "unknown max".Commits-------574b8a3 Fix ProgressBar::iterate on empty iterator
Bug Description
When using the progress bar with an empty input and time estimates, an error has been thrown which stopped the execution:
I dug around in the code and found a semantically incorrect check for
getMaxSteps()that evaluated a maxSteps of 0 (because of an empty input) to be interpreted like maxSteps has not been set at all und thus, triggeringFix Proposal
I propose to change the check to be semantically correct as in "0 is a valid value of maxSteps, null is not".
More details and a bit of discussion in#47244.