Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
Console table cleanup#19406
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
Console table cleanup#19406
Uh oh!
There was an error while loading.Please reload this page.
Conversation
| if (!self::$styles[$name]) { | ||
| thrownewInvalidArgumentException(sprintf('Style "%s" is not defined.',$name)); | ||
| if (isset(self::$styles[$name])) { | ||
| returnself::$styles[$name]; |
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.
What is the advantage of this change?
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.
$array = ['default' =>newTableStyle(),];if (!$array['name']) {}// undefined offset 'name'
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'd keep the original order: throw, then return, which means changing this line toif (empty(self::$styles[$name])) { (usingempty() to keep throwing when defined but value is== 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.
@nicolas-grekas, Done
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.
@ReenExe did you forget to push?
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.
@nicolas-grekas, now good? Or need change?
Sorry, hard understand what need change after last comment
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.
yes, needs change to me
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.
@nicolas-grekas, need addreviewed label?
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.
but I don't see any change on the discussed line/code block, did you forget to push?
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.
Yes, is my mistake
i extract method toresolveStyle
if ($nameinstanceof TableStyle) {return$name; }if (isset(self::$styles[$name])) {returnself::$styles[$name]; }thrownewInvalidArgumentException(sprintf('Style "%s" is not defined.',$name));
And ingetStyleDefinition change
if (!self::$styles[$name]) {thrownewInvalidArgumentException(sprintf('Style "%s" is not defined.',$name)); }returnself::$styles[$name];
to
if (isset(self::$styles[$name])) {returnself::$styles[$name]; }thrownewInvalidArgumentException(sprintf('Style "%s" is not defined.',$name));
But after review need changes to:
if (empty(self::$styles[$name])) {thrownewInvalidArgumentException(sprintf('Style "%s" is not defined.',$name)); }returnself::$styles[$name];
So a think must be same code - and make same changes in both places
| } | ||
| /** | ||
| * @expectedException \InvalidArgumentException |
nicolas-grekasJul 28, 2016 • 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.
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.
\Symfony\Component\Console\Exception\InvalidArgumentException
nicolas-grekas commentedJul 28, 2016
👍 |
| } | ||
| if (!self::$styles[$name]) { | ||
| if (empty(self::$styles[$name])) { |
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.
Why not usingisset() 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.
By my suggestion: using isset would be written as:if (isset(self::$styles[$name]) && !self::$styles[$name]) {
which happens to be exactly whatempty() does.
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.
When can the style be falsy?
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 don't know :)
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.
Reading the code, it looks like it is defined with an object or not defined at all. So, I think we should keepisset() as before in the newresolveStyle() method and change this one to useisset() as well. Whenever possible, I tend to avoid usingempty().
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.
@fabpot, before little change - what prefer?
1
if ($nameinstanceof TableStyle) {return$name; }if (!isset(self::$styles[$name])) {thrownewInvalidArgumentException(sprintf('Style "%s" is not defined.',$name)); }returnself::$styles[$name];
or
2:
if ($nameinstanceof TableStyle) {return$name; }if (isset(self::$styles[$name])) {returnself::$styles[$name]; }thrownewInvalidArgumentException(sprintf('Style "%s" is not defined.',$name));
As for me is2
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.
Ok for 2
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.
Done
fabpot commentedJul 28, 2016 • 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.
LGTM now. Can you submit another PR for Symfony 2.7 (keep this one open as it's going to be merged as well for more recent versions of Symfony)? |
ReenExe commentedJul 28, 2016 • 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.
@fabpot, try with cherry-pick but have conflicts. Can i make new PR in 2.7 (arm move changes) and close this PR? |
ReenExe commentedJul 28, 2016 • 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.
Also in 2.7
|
nicolas-grekas commentedJul 28, 2016
you should close this one yes, then open a new one against 2.7, then if you'd like another one against the first branch where setColumnStyle exists (and so on). |
fabpot commentedJul 28, 2016
No, please, don't close this one.@ReenExe You are right, that's why I need a specific PR for 2.7. This one will still be merged for the additional changes. Thanks. |
ReenExe commentedJul 28, 2016 • 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.
@fabpot, i will try, but conflict stay between branches - it's normal? |
fabpot commentedJul 28, 2016
Just submit a PR on 2.7 and I will take care of merge conflict myself. |
ReenExe commentedJul 28, 2016
Make new PR - thats only covers bugfix#19470 |
ReenExe commentedJul 28, 2016
@fabpot, i see this after try submit (make my branch from |
This PR was squashed before being merged into the 2.7 branch (closes#19470).Discussion----------undefined offset fix (#19406)| Q | A| ------------- | ---| Branch? | 2.7| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -Commits-------9813888 undefined offset fix (#19406)
* 2.7: undefined offset fix (#19406) [EventDispatcher] Removed unused variable
fabpot commentedJul 28, 2016
Thank you@ReenExe. |
This PR was submitted for the master branch but it was merged into the 2.8 branch instead (closes#19406).Discussion----------Console table cleanup| Q | A| ------------- | ---| Branch? | master| Bug fix? | yes `notice`| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | -| License | MIT| Doc PR | -Commits-------217274b Console table cleanup
* 2.8: Minor fixes [Console] Overcomplete argument exception message tweak. fixed bad auto merge Console table cleanup undefined offset fix (#19406) [EventDispatcher] Removed unused variable
* 3.0: Minor fixes [Console] Overcomplete argument exception message tweak. fixed bad auto merge Console table cleanup undefined offset fix (#19406) [EventDispatcher] Removed unused variable
* 3.1: Minor fixes [Cache] Fix abstract AdapterTestCase cache property [Console] Overcomplete argument exception message tweak. fixed bad auto merge Console table cleanup undefined offset fix (#19406) [EventDispatcher] Removed unused variable
* 2.8: Relax 1 test failing with latest PHP versions bumped Symfony version to 2.8.10 Remove usage of __CLASS__ outside of a class [HttpKernel] Fix variable conflicting name [Process] Fix double-fread() when reading unix pipes [Process] Fix AbstractPipes::write() for a situation seen on HHVM (at least) [Validator] Fix dockblock typehint in XmlFileLoader bumped Symfony version to 2.8.10 updated VERSION for 2.8.9 updated CHANGELOG for 2.8.9 bumped Symfony version to 2.7.17 updated VERSION for 2.7.16 update CONTRIBUTORS for 2.7.16 updated CHANGELOG for 2.7.16 Minor fixes [Console] Overcomplete argument exception message tweak. fixed bad auto merge Console table cleanup undefined offset fix (#19406) [EventDispatcher] Removed unused variableConflicts:CHANGELOG-2.7.mdCHANGELOG-3.0.mdsrc/Symfony/Bridge/Swiftmailer/DataCollector/MessageDataCollector.phpsrc/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.phpsrc/Symfony/Bundle/FrameworkBundle/Tests/DependencyInjection/FrameworkExtensionTest.phpsrc/Symfony/Component/Console/Tests/Helper/LegacyDialogHelperTest.phpsrc/Symfony/Component/Console/Tests/Helper/TableTest.phpsrc/Symfony/Component/DependencyInjection/Tests/Fixtures/containers/legacy-container9.phpsrc/Symfony/Component/EventDispatcher/Tests/AbstractEventDispatcherTest.phpsrc/Symfony/Component/HttpFoundation/Tests/Session/Storage/Handler/LegacyPdoSessionHandlerTest.phpsrc/Symfony/Component/HttpKernel/Kernel.php
Uh oh!
There was an error while loading.Please reload this page.
notice