Add bin/magento commands to list store/website data #7982

convenient · 2016-12-28T19:17:55Z

Proposal

I used commands in n98-magerun to output a list of store data and website data, nothing that can't be achieved by connecting to the database and doing a few SELECT statements but the magerun wrapper was very helpful in Magento 1.

I'd like to have these commands back in use for Magento 2

magerun command	proposed magento2 command
magerun sys:store:list	bin/magento store:list
magerun sys:website:list	bin/magento store:website:list

Naming things

There are only two hard things in Computer Science: cache invalidation and naming things.
-- Phil Karlton
http://martinfowler.com/bliki/TwoHardThings.html

I think the code for the website:list command should live in the same directory as the store:list command because both entities are part of the Mage/Store module. Also any commands in the Mage/Store module should be prefixed with store. However, this gives us the odd naming hierarchical structure in the command names listed, listing a website has to include the word store or break convention.

If anyone has any suggestions I'm all ears, I've gone for option 1 by default and option 3 breaks convention.

option	list stores	list websites
Option 1	bin/magento store:list	bin/magento store:website:list
Option 2	bin/magento store:list:store	bin/magento store:list:website
Option 3	bin/magento store:list	bin/magento website:list

A comment on my code

I'm not sure if this is the correct standard for creating new CLI commands. I copied the approach from the sample data module. If this method is outdated shout out.

Example usage and output

$ bin/magento store:website:list
+----+------------------+--------------+-------+------------+------------+
| ID | Default Group Id | Name         | Code  | Sort Order | Is Default |
+----+------------------+--------------+-------+------------+------------+
| 0  | 0                | Admin        | admin | 0          | 0          |
| 1  | 1                | Main Website | base  | 0          | 1          |
| 2  | 2                | Other Site   | other | 0          | 0          |
+----+------------------+--------------+-------+------------+------------+

$ bin/magento store:list
+----+------------+----------+--------------------+---------+------------+-----------+
| ID | Website ID | Group ID | Name               | Code    | Sort Order | Is Active |
+----+------------+----------+--------------------+---------+------------+-----------+
| 0  | 0          | 0        | Admin              | admin   | 0          | 1         |
| 1  | 1          | 1        | Default Store View | default | 0          | 1         |
| 2  | 2          | 2        | Other Store View   | other   | 0          | 1         |
+----+------------+----------+--------------------+---------+------------+-----------+

hostep · 2016-12-29T09:03:26Z

Just FYI, these command also exist in n98/magerun2, but contain less detailed output then what you are suggesting:

$ vendor/bin/n98-magerun2 sys:website:list


  Magento Websites


+----+------+
| id | code |
+----+------+
| 1  | base |
+----+------+

$ vendor/bin/n98-magerun2 sys:store:list
+----+------+
| id | code |
+----+------+
| 1  | nl   |
| 3  | fr   |
+----+------+

So if this PR is accepted, I think it's best if these commands then are removed from n98/magerun2, since otherwise we will have 2 commands to do the same thing.
Let's include @ktomk in here as well.

convenient · 2016-12-29T09:08:54Z

@hostep From a purely outsider point of view, I'm wondering whether any of the commands in n98-magerun2 should have any bearing on what is baked into the magento core.

Eg, there are many third party apps that handle layered navigation but magento still includes their own one.

Furthermore, if the core bin/magento commands do what I need then I'm wondering if there would be much value in having magerun2 installed on production servers for example.

I do like magerun a lot and have contributed to the v1 version, but am just not sure where the divide in logic goes now that a lot of the core deployment steps are baked into bin/magento

Also I know the tests are failing for this PR, will review.

hostep · 2016-12-29T09:23:58Z

Well, n98-magerun2 works by inheriting all the commands from bin/magento and then adding a few more on top of them. So my concern is not with the commands baked into bin/magento, I just wanted to make sure the maintainers of n98-magerun2 were aware of your PR.

The upside of the n98-magerun2 project, is that new (and useful) features are implemented much faster then in Magento core. It wouldn't surprise me that this PR is going to take a couple of months to be included into core Magento, while if you would create a PR to enhance the feature in n98-magerun2, it will probably be released in a new version in less then a month time. And you could then use it on all Magento 2 versions and not only on the very last one.

I do agree with you that some features from n98-magerun2 might as well be ported over to core Magento for users who don't want to use n98-magerun2, but then those users will have to wait a long time until it happens I'm afraid.

In the end, it all boils down to what the maintainers from Magento and n98-magerun2 agree upon on where certain features should be implemented I think.

Just my 2 cents :)

convenient · 2016-12-29T09:39:02Z

@hostep All valid points regarding M2 turnaround time etc. However, I'd at least want to try and make the magento 2 code a better place to be rather than building scaffolding on top of it.

Thanks for highlighting this to me, I wasn't aware of how magerun2 worked and that does seem like an area for conflict.

convenient · 2016-12-29T16:45:46Z

Fix Failing test-

Here's my attempt at stopping the suite from failing, but I'm not really sure what's going on.
11ee118

The only other core module to include commands like this that I can see is the sample data module, the dependencies for that module don't look remarkably different from the Magento\Store\Api\StoreRepositoryInterface dependency I require but clearly I'm missing something.

~~If anyone can review the integration test failures and point out whats going on i'd be thankful, I cannot replicate the failure locally.~~

~~The changes in 3db275d7700dabaa495bd05b850e5d6c38b53d40 correctly handle an uninstalled magento instance, solving the test breaking problem. It was a legit breakage, who knew!~~

Read the comments below, my folly was to use the Sample Data module as an example of how to add CLI commands.

antonkril · 2017-01-07T21:33:19Z

app/code/Magento/Store/Console/Command/StoreListCommand.php

+ * Class StoreListCommand
+ * @package Magento\Store\Console\Command
+ */
+class StoreListCommand extends Command


Have you tried to run these commands when magento is not installed? Please check this case. Requesting StoreManager in constructor might lead to DB calls.

@antonkril Damn fine point you make there. The additions I made threw an exception when I ran bin/magento without any parameters at all on an uninstalled instance.

I couldn't see any precedence for conditional loading of console classes, so I rolled my own as a proof of concept. Is there a preferred way to handle this kind of thing or is my solution acceptable?

Please review, thank you.

@antonkril Any thoughts on the above? :)

buskamuza · 2017-01-17T15:34:51Z

Hi @convenient ,

there are several ways of registering Magento CLI commands. The one used in Sample Data is to allow the commands appear before the application is installed. This is an exceptional case. Usually, commands from the modules register the commands via di.xml configuration for Magento\Framework\Console\CommandListInterface. You can see an example in Sample Module - https://github.com/magento/magento2-samples/tree/master/sample-module-command or in one of the core commands (e.g., https://github.com/magento/magento2/blob/develop/app/code/Magento/Cron/etc/di.xml#L29-L37). I'd recommend this approach, as it provides the same support as you implemented, just on the framework level.

buskamuza · 2017-01-17T15:42:48Z

app/code/Magento/Store/Console/Command/StoreListCommand.php

+
+/**
+ * Class StoreListCommand
+ * @package Magento\Store\Console\Command


Please add more details in the description of the classes. Also, @package is not necessary.

phpdoc updated in commit a9ca0045d50d5a94e0f83fe7c9ef293e7957438e

The format for this updated doc is in a format consistent with app/code/Magento/Cron/Console/Command/CronCommand.php

convenient · 2017-01-17T21:15:19Z

Thank you for explaining all that @buskamuza.

I guess my big mistake was using the SampleData module as an example of how to add core commands. From what you're saying I picked one of the modules that is purpose built to act differently. For my own learning and out of curiosity are there any other modules like this? I don't think so as I can only find the SampleData module having a cli_commands.php file.

I've fixed this, tested locally, and have pushed up the changed. Travis is showing all green too. Just waiting on codacy.

Many thanks

buskamuza

The implementation looks good. But there are couple necessary changes:

See new comment in the code
Please implement tests

buskamuza · 2017-02-10T17:31:04Z

app/code/Magento/Store/Console/Command/StoreListCommand.php

+     */
+    protected function execute(InputInterface $input, OutputInterface $output)
+    {
+        $table = $this->getHelperSet()->get('table');


Please wrap the logic in try-catch and specify return codes for success and failure explicitly. See an example in https://github.com/magento/magento2/blob/develop/app/code/Magento/Deploy/Console/Command/SetModeCommand.php#L108
The same for another test

Will do 👍

@buskamuza

- magento#7982 (comment) - As @buskamuza suggested

- Return non-zero code on error - With unit tests

convenient · 2017-02-11T11:35:41Z

@buskamuza @vrann

I've rebased so just to be clear what i've done

Added new scripts for listing websites and stores
Added a tests for each command class
Each script will return non-zero on error, supporting the verbosity flag.
Tests passing.

Is that everything you need?

vrann · 2017-02-15T19:12:54Z

app/code/Magento/Store/Console/Command/WebsiteListCommand.php

+
+            return \Magento\Framework\Console\Cli::RETURN_SUCCESS;
+        } catch (\Exception $e) {
+            $output->writeln('<error>' . $e->getMessage() . '</error>');


Similar catch logic is already implemented in the CLI entry point script in bin/magento. I would suggest not to duplicate similar code here. The one difference which I see is that this code will output exception message regardless of verbosity. Can this be done using exact exception types for the cases where you want to output exceptions?

One problem which I see is that the return code is just returned from the execute method. CLI framework will consider that this is valid result of the command and will end process successfully.

Oh, just checked comments from previous reviewers. Let me sync up with them.

vrann · 2017-02-15T19:13:12Z

app/code/Magento/Store/Console/Command/StoreListCommand.php

+
+            return \Magento\Framework\Console\Cli::RETURN_SUCCESS;
+        } catch (\Exception $e) {
+            $output->writeln('<error>' . $e->getMessage() . '</error>');


same comment about catching logic here

vrann · 2017-02-15T19:14:26Z

@convenient Than you, I've added 2 comments. Can you please take a look.

vrann · 2017-02-15T19:22:49Z

@convenient nevermind previous comment, all looks good. Apparently this is new convention in CLI framework to require wrapping in try catch for each command.

convenient · 2017-02-15T20:51:02Z

@vrann Thanks for the updates, you gave me a scare there with so many emails ;)

convenient force-pushed the cli-info-store branch from 11ee118 to 6fe5e4f Compare January 5, 2017 16:56

antonkril suggested changes Jan 7, 2017

View reviewed changes

buskamuza reviewed Jan 17, 2017

View reviewed changes

buskamuza assigned vrann Feb 10, 2017

buskamuza suggested changes Feb 10, 2017

View reviewed changes

convenient added a commit to convenient/mtwo that referenced this pull request Feb 10, 2017

Use more traditional command registration

a6aa2d5

- magento#7982 (comment) - As @buskamuza suggested

convenient force-pushed the cli-info-store branch from a9ca004 to 1b8f48c Compare February 10, 2017 19:16

okorshenko added this to the February 2017 milestone Feb 11, 2017

convenient force-pushed the cli-info-store branch from 1b8f48c to 591ca38 Compare February 11, 2017 09:52

Add store magento commands to list data

996de8a

- Return non-zero code on error - With unit tests

convenient force-pushed the cli-info-store branch from 591ca38 to 996de8a Compare February 11, 2017 09:54

Website list test fix line length

f0c2150

vrann suggested changes Feb 15, 2017

View reviewed changes

vrann approved these changes Feb 15, 2017

View reviewed changes

vrann added Progress: accept improvement labels Feb 15, 2017

mmansoor-magento merged commit f0c2150 into magento:develop Feb 18, 2017

convenient deleted the cli-info-store branch February 18, 2017 19:00

vrann added the Award: complex label Mar 11, 2017

convenient mentioned this pull request Aug 15, 2017

Debug layout file processing by print final merge layout xml ? #10533

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add bin/magento commands to list store/website data #7982

Add bin/magento commands to list store/website data #7982

convenient commented Dec 28, 2016 •

edited by okorshenko

Loading

hostep commented Dec 29, 2016

convenient commented Dec 29, 2016

hostep commented Dec 29, 2016 •

edited

Loading

convenient commented Dec 29, 2016

convenient commented Dec 29, 2016 •

edited

Loading

antonkril Jan 7, 2017

convenient Jan 9, 2017

convenient Jan 16, 2017

buskamuza commented Jan 17, 2017

buskamuza Jan 17, 2017

convenient Jan 17, 2017

convenient commented Jan 17, 2017

buskamuza left a comment

buskamuza Feb 10, 2017

convenient Feb 10, 2017

convenient commented Feb 11, 2017

vrann Feb 15, 2017

vrann Feb 15, 2017

vrann Feb 15, 2017

vrann commented Feb 15, 2017

vrann commented Feb 15, 2017

convenient commented Feb 15, 2017

Add bin/magento commands to list store/website data #7982

Add bin/magento commands to list store/website data #7982

Conversation

convenient commented Dec 28, 2016 • edited by okorshenko Loading

Proposal

Naming things

A comment on my code

Example usage and output

hostep commented Dec 29, 2016

convenient commented Dec 29, 2016

hostep commented Dec 29, 2016 • edited Loading

convenient commented Dec 29, 2016

convenient commented Dec 29, 2016 • edited Loading

Fix Failing test-

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

buskamuza commented Jan 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

convenient commented Jan 17, 2017

buskamuza left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

convenient commented Feb 11, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vrann commented Feb 15, 2017

vrann commented Feb 15, 2017

convenient commented Feb 15, 2017

convenient commented Dec 28, 2016 •

edited by okorshenko

Loading

hostep commented Dec 29, 2016 •

edited

Loading

convenient commented Dec 29, 2016 •

edited

Loading