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

[6.x] Fixes appendRow on console table #31469

Merged
merged 3 commits into from
Feb 14, 2020

Conversation

crynobone
Copy link
Member

There's some limitation with the new implementation.

  • You can't call command with table from another command (the request to command will use BufferedOutput).
  • You can't call command with table from a controller.

IMHO if developer want to use Table with appendRow() capability they should manually execute it inside Command instead of table() because the usage of ConsoleOutput on CLI usage while BufferedOutput when calling other commands or from a controller.

Signed-off-by: Mior Muhammad Zaki crynobone@gmail.com

Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
@GrahamCampbell GrahamCampbell changed the title [6.x] Fixes appendRow on console table. [6.x] Fixes appendRow on console table Feb 14, 2020
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
Signed-off-by: Mior Muhammad Zaki <crynobone@gmail.com>
@driesvints
Copy link
Member

Does this fixes all the issues from #31445 ? (see comments after closing)

@crynobone
Copy link
Member Author

crynobone commented Feb 14, 2020

Does this fixes all the issues from #31445 ?

It should because the current behaviour assume $this->output->getOutput() always have section() method but in reality only implementation of Symfony\Component\Console\Output\ConsoleOutput does while Laravel use Symfony\Component\Console\Output\BufferedOutput when using Illuminate\Foundation\Console\Kernel::call() instead of from artisan cli.

@driesvints I also include tests related to use of table() in command.

@crynobone
Copy link
Member Author

But at the same time, checking the instanceof $output will give different output based how you execute the command so I would say it is better to let developer construct Table command directly if they want appendRow() feature instead of having it available via InteractsWithIO::table()

$output = $this->output->getOutput();

$table = new Table($output instanceof ConsoleOutput ? $output->section() : $output);

@taylorotwell
Copy link
Member

@crynobone... sooo... do we want to merge this or not? You said it fixed the issue but then you seemed to imply the whole thing was a bad idea? I'm confused.

@crynobone
Copy link
Member Author

@taylorotwell

You can only use $table->appendRow() when you run it directly from say php artisan command:name. If you do Artisan::call('command:name') it wouldn't work as described in testItCantExecuteCommandWithTableAndAppendRowDueToBufferedOutput().

This PR fixed:

  • Running artisan command with table without mocking the output
  • Running existing artisan command with table from Controller or another artisan command.
  • Addes tests

@taylorotwell taylorotwell merged commit d3a9a96 into laravel:6.x Feb 14, 2020
@taylorotwell
Copy link
Member

Reverting this entire feature and all related pieces of it @adam-prickett @crynobone ... it's causing continual problems.

voku added a commit to voku/framework that referenced this pull request Feb 15, 2020
* upstream/master: (78 commits)
  [7.x] Implement anonymous components (laravel#31363)
  Apply fixes from StyleCI (laravel#31480)
  Apply fixes from StyleCI (laravel#31479)
  revert broken table feature
  move files
  add test for event payload of type object (laravel#31477)
  [7.x] Refactor route caching (laravel#31188)
  [6.x] Fixes appendRow on console table (laravel#31469)
  Apply fixes from StyleCI (laravel#31474)
  formatting
  Throw exception on empty collection (laravel#31471)
  Add tests for Query Builder when array value given (laravel#31464)
  Remove addHidden method (laravel#31463)
  allow afterResponse chain
  add getRawOriginal
  Remove unused use-statement
  Update comment
  [6.x] Change MySql nullable modifier to allow generated columns to be not null (laravel#31452)
  [6.x] Test for pushed events (laravel#31451)
  Fixed phpdoc
  ...
@crynobone
Copy link
Member Author

@taylorotwell I'm okay with that, but CommandWithTest and OuputStyle::getOutput() should be okay to stay in the core

@crynobone crynobone deleted the fix-append-row-on-console-table branch February 4, 2021 07:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants