[Console] Provide a DX where an array could be passed #25397

Simperfit · Dec 8, 2017

Q	A
Branch?	3.4
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#25394
License	MIT
Doc PR	none

I think that this is not fixing the root causes that does not appears in 4.1, so maybe there is something better to do. I did not find the root cause So I think it can bo good to fix the problem too.

nicolas-grekas · Dec 8, 2017

3.4?

yceruto · Dec 8, 2017

src/Symfony/Component/Console/Helper/Table.php

@@ -410,6 +411,9 @@ private function buildTableRows($rows)

            // Remove any new line breaks and replace it with a new line
            foreach ($rows[$rowKey] as $column => $cell) {
+                if (is_array($cell)) {
+                    continue;


This would silence the error and consequently we would have an unexpected behavior. IMO we should throw an exception instead (probably in $this->fillNextRows()) as the cell must be a string (always) at this moment.

done, i'm throwing now

sstok · Dec 9, 2017

Dude! could you please stop posting pictures! This is GitHub not Instagram 😂

Simperfit · Dec 9, 2017

This is now Instagit cc @iltar

sroze · Dec 10, 2017

I like the idea of giving issues something more than just lines of code as it's people doing things behind :)

nicolas-grekas · Dec 12, 2017

src/Symfony/Component/Console/Helper/Table.php

     */
    private function fillNextRows(array $rows, $line)
    {
        $unmergedRows = array();
        foreach ($rows[$line] as $column => $cell) {
+            if (is_array($cell)) {
+                throw new \UnexpectedValueException('The cell should always be a string, array given.');


be a string or a TableCell

if (!is_string($cell) && !$cell instanceof TableCell) ?

chalasr · Dec 15, 2017

src/Symfony/Component/Console/Tests/Helper/TableTest.php

+     * @expectedException \UnexpectedValueException
+     * @expectedExceptionMessage The cell should always be a string, array given.
+     */
+    public function testThrows()


could be best named :)

chalasr · Dec 15, 2017

src/Symfony/Component/Console/Helper/Table.php

     */
    private function fillNextRows(array $rows, $line)
    {
        $unmergedRows = array();
        foreach ($rows[$line] as $column => $cell) {
+            if (!is_string($cell) && !$cell instanceof TableCell) {
+                throw new \UnexpectedValueException('The cell should always be a string, array given.');


can we use the console InvalidArgumentException to ease catching it?

Simperfit · Dec 16, 2017

fabbot error is about #25397 (comment) so I'm not fixing it.

keradus · Dec 19, 2017

fabbot is right, @expectedException is always from root namespace. use FQCN there as fabbot ask you

Simperfit · Dec 20, 2017

PR rebased with current 3.4

Simperfit · Jan 5, 2018

Status: Needs Review

xabbuh · Jan 11, 2018

src/Symfony/Component/Console/Helper/Table.php

     */
    private function fillNextRows(array $rows, $line)
    {
        $unmergedRows = array();
        foreach ($rows[$line] as $column => $cell) {
+            if (null !== $cell && !is_string($cell) && !$cell instanceof TableCell && !is_scalar($cell)) {


What about dropping the is_string() check? It's already covered by the is_scalar() test.

ping @Simperfit

chalasr

Tests are broken

Simperfit · Jan 19, 2018

It feels strange that are broken this way, I don't see how I can fix them without adding the @expectedException because it really passes a Reference object to a factory.service

chalasr · Jan 19, 2018

Reference can be casted to string, this probably needs to deal with objects implementing __toString() in addition to strings.

Simperfit · Jan 19, 2018

So I need to cast it to string.

chalasr · Jan 19, 2018

No, you need to check for !is_scalar($cell) && !(is_object($cell) && method_exists($cell, '__toString')) (strings are strings, objects are not strings but can be casted too).

Simperfit · Jan 19, 2018

This is what I've done instead of doing a cast that could lead to errors.

chalasr · Jan 19, 2018

src/Symfony/Component/Console/Helper/Table.php

     */
    private function fillNextRows(array $rows, $line)
    {
        $unmergedRows = array();
        foreach ($rows[$line] as $column => $cell) {
+            if (null !== $cell && !$cell instanceof TableCell && !is_scalar($cell) && !(is_object($cell) && method_exists($cell, '__toString'))) {
+                throw new InvalidArgumentException(sprintf('The cell should always be a string, a TableCell or a scalar, %s given.', gettype($cell)));


I would say A cell must be a TableCell, a scalar or an object implementing __toString, %s given.

ping @Simperfit

chalasr · Jan 25, 2018

Thank you @Simperfit.

…perfit) This PR was merged into the 3.4 branch. Discussion ---------- [Console] Provide a DX where an array could be passed | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no  | BC breaks? | no | Deprecations? | no  | Tests pass? | yes | Fixed tickets | #25394 | License | MIT | Doc PR | none ![img_2941](https://user-images.githubusercontent.com/3451634/33766610-977f77ce-dc1e-11e7-93c1-38dd0038d7f5.jpeg) I think that this is not fixing the root causes that does not appears in 4.1, so maybe there is something better to do. I did not find the root cause So I think it can bo good to fix the problem too. Commits ------- de502f7 [Console] Provide a bugfix where an array could be passed

carsonbot added Status: Needs Review Console Bug labels Dec 8, 2017

chalasr added Form and removed Console labels Dec 8, 2017

yceruto reviewed Dec 8, 2017

View reviewed changes

chalasr added this to the 3.4 milestone Dec 8, 2017

Simperfit force-pushed the bugfix/fix-a-bug-where-an-array-could-be-passed branch from ded8402 to a07e8e1 Compare December 9, 2017 07:50

Simperfit changed the base branch from 4.0 to 3.4 December 9, 2017 07:51

Simperfit force-pushed the bugfix/fix-a-bug-where-an-array-could-be-passed branch from a07e8e1 to e6bdca2 Compare December 9, 2017 07:52

Simperfit changed the title ~~[Console] Provide a bugfix where an array could be passed~~ [Console] Provide a DX where an array could be passed Dec 9, 2017

nicolas-grekas reviewed Dec 12, 2017

View reviewed changes

Simperfit force-pushed the bugfix/fix-a-bug-where-an-array-could-be-passed branch from e6bdca2 to f4066a6 Compare December 12, 2017 14:00

xabbuh added Console and removed Form labels Dec 14, 2017

chalasr reviewed Dec 15, 2017

View reviewed changes

Simperfit force-pushed the bugfix/fix-a-bug-where-an-array-could-be-passed branch from f4066a6 to 85ee007 Compare December 16, 2017 04:38

Simperfit force-pushed the bugfix/fix-a-bug-where-an-array-could-be-passed branch from 85ee007 to b3519a9 Compare December 17, 2017 07:12

Simperfit force-pushed the bugfix/fix-a-bug-where-an-array-could-be-passed branch 3 times, most recently from d1f9a63 to 7996158 Compare December 20, 2017 15:49

carsonbot added Status: Needs Work and removed Status: Needs Review labels Dec 20, 2017

carsonbot added Status: Needs Review and removed Status: Needs Work labels Jan 5, 2018

xabbuh reviewed Jan 11, 2018

View reviewed changes

Simperfit force-pushed the bugfix/fix-a-bug-where-an-array-could-be-passed branch from 5551d0e to 2f2d254 Compare January 17, 2018 08:27

carsonbot added Status: Reviewed and removed Status: Needs Review labels Jan 17, 2018

xabbuh approved these changes Jan 17, 2018

View reviewed changes

carsonbot removed the Status: Reviewed label Jan 17, 2018

chalasr requested changes Jan 17, 2018

View reviewed changes

carsonbot added the Status: Needs Work label Jan 17, 2018

Simperfit force-pushed the bugfix/fix-a-bug-where-an-array-could-be-passed branch from 2f2d254 to 79663df Compare January 19, 2018 14:07

Simperfit force-pushed the bugfix/fix-a-bug-where-an-array-could-be-passed branch from 79663df to 23ee4ef Compare January 19, 2018 14:09

chalasr approved these changes Jan 19, 2018

View reviewed changes

carsonbot added Status: Reviewed and removed Status: Needs Work labels Jan 19, 2018

chalasr reviewed Jan 19, 2018

View reviewed changes

[Console] Provide a bugfix where an array could be passed

de502f7

Simperfit force-pushed the bugfix/fix-a-bug-where-an-array-could-be-passed branch from 23ee4ef to de502f7 Compare January 24, 2018 15:57

chalasr merged commit de502f7 into symfony:3.4 Jan 25, 2018

Simperfit deleted the bugfix/fix-a-bug-where-an-array-could-be-passed branch January 25, 2018 10:21

This was referenced Jan 29, 2018

Release v3.4.4 #25956

Merged

Release v4.0.4 #25957

Merged

Search code, repositories, users, issues, pull requests...

Uh oh!

[Console] Provide a DX where an array could be passed #25397

[Console] Provide a DX where an array could be passed #25397

Uh oh!

Conversation

Simperfit commented Dec 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolas-grekas commented Dec 8, 2017

Uh oh!

yceruto Dec 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sstok commented Dec 9, 2017

Uh oh!

Simperfit commented Dec 9, 2017

Uh oh!

sroze commented Dec 10, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Simperfit commented Dec 16, 2017

Uh oh!

keradus commented Dec 19, 2017

Uh oh!

Simperfit commented Dec 20, 2017

Uh oh!

Simperfit commented Jan 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chalasr left a comment

Choose a reason for hiding this comment

Uh oh!

Simperfit commented Jan 19, 2018

Uh oh!

chalasr commented Jan 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Simperfit commented Jan 19, 2018 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

chalasr commented Jan 19, 2018

Uh oh!

Simperfit commented Jan 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chalasr commented Jan 25, 2018

Uh oh!

Uh oh!

Simperfit commented Dec 8, 2017 •

edited

Loading

yceruto Dec 8, 2017 •

edited

Loading

chalasr commented Jan 19, 2018 •

edited

Loading

Simperfit commented Jan 19, 2018 via email •

edited

Loading