Add a core command to list container services (needs UI) [#1950930]

Comment	File	Size	Author
#23	devel-list-container-services-ui-1950930-23.patch	3.67 KB	willzyx

#20	1950930-20.patch	1.81 KB	damiankloip

#18	1950930-18.patch	1.81 KB	damiankloip

#18	interdiff-1950930-18.txt	515 bytes	damiankloip
#15	1950930-15.patch	1.81 KB	damiankloip

#13	1950930-12.patch	1.79 KB	damiankloip

#12	1950930-12.patch	0 bytes	damiankloip

#9	1950930-8.patch	1.79 KB	damiankloip

#8	1950930-8.patch	1.85 KB	damiankloip

#1	drush.cs-command.patch	1.83 KB	damiankloip

File	Size
drush.cs-command.patch	1.83 KB

Comment #2

damiankloip CreditAttribution: damiankloip commented 23 March 2013 at 13:33

Issue tags:

+Needs tests

If this idea flies, I can write some tests.

Btw, using the output engine stuff is awesome!

Log in or register to post comments

Comment #3

moshe weitzman CreditAttribution: moshe weitzman commented 23 March 2013 at 20:36

Project:	Drush	» Devel
Version:	8.x-6.x-dev	» 8.x-1.x-dev
Component:	Core Commands	» drush

Is there a reason why they should ever be unsorted?

I put the event listing command in devel, and i think this is similar. So, moving to devel project. I know that some folks won't like this decision but drush can't carry all the stuff that folks want to do with drupal. Devel exists for a reason.

Log in or register to post comments

Comment #4

damiankloip CreditAttribution: damiankloip commented 23 March 2013 at 21:54

I guess there is no reason why you would want that unsorted, I can remove that option.

Also, I think moving this to devel is a great idea. I'm all for it. I didn't actually think about that. Agree that everything can't live in the drush codebase.

Log in or register to post comments

Comment #5

damiankloip CreditAttribution: damiankloip commented 23 March 2013 at 21:54

Assigned:

Unassigned

» damiankloip

Log in or register to post comments

Comment #6

salvis

he/his

CreditAttribution: salvis commented 24 March 2013 at 07:46

I'd love to see this in the Devel GUI, too.

Log in or register to post comments

Comment #7

dawehner

German

CreditAttribution: dawehner commented 24 March 2013 at 09:32

+++ b/commands/core/core.drush.incundefined
@@ -1237,3 +1260,25 @@ function drush_core_execute() {
+  $container = Drupal::getContainer();
...
+  if (empty($services)) {

Maybe we should also check for an empty container, for some reasons? No idea, though I can't imagine how there could be no services, if there is a container and a full bootstrap.

Just wondering whether there are some thoughts about dependency injection in drush :)

Log in or register to post comments

Comment #8

damiankloip CreditAttribution: damiankloip commented 25 March 2013 at 08:25

File	Size
1950930-8.patch	1.85 KB

Ok, I have rerolled for devel instead of core drush, removed the sort option, and added a check for the container.

Log in or register to post comments

Comment #9

damiankloip CreditAttribution: damiankloip commented 25 March 2013 at 08:26

File	Size
1950930-8.patch	1.79 KB

Sorry, wrong patch.

Log in or register to post comments

Comment #10

damiankloip CreditAttribution: damiankloip commented 25 March 2013 at 08:41

Do we want to use if($container = Drupal::getContainer()) {...} syntax instead? I don't mind, I guess it's all down to code style preferences.

Log in or register to post comments

Comment #11

dawehner

German

CreditAttribution: dawehner commented 25 March 2013 at 22:04

I don't mind either, it's just that these checks feels unneeded. Shouldn't we assume that if a full boostrap of drupal is done, it should work everything as expected?

+++ b/devel.drush.incundefined
@@ -244,3 +266,28 @@ function _drush_devel_print_function($file, $start_line, $end_line) {
+  if (isset($prefix)) {
+    $services = preg_grep("/$prefix/", $services);
+  }

Should we return something special, if there is no services with a certain prefix, like trigger an error?

Log in or register to post comments

Comment #12

damiankloip CreditAttribution: damiankloip commented 28 March 2013 at 17:38

File	Size
1950930-12.patch	0 bytes

How about we just move the check for services below the prefix matching?

I feel we should leave the container check in, if someone has really broken their D8 installation this may help them at some point? If not, I'm happy to remove this too.

Log in or register to post comments

Comment #13

damiankloip CreditAttribution: damiankloip commented 28 March 2013 at 17:39

File	Size
1950930-12.patch	1.79 KB

Oops, soory. a real patch is usually helpful.

Log in or register to post comments

Comment #14

dawehner

German

CreditAttribution: dawehner commented 29 March 2013 at 13:09

Status:

Needs review

» Needs work

+++ b/devel.drush.incundefined
@@ -244,3 +266,28 @@ function _drush_devel_print_function($file, $start_line, $end_line) {
+  return sort($services);

PHP is so cool, that it doesn't need to return the sorted array if you sort :)

Log in or register to post comments

Comment #15

damiankloip CreditAttribution: damiankloip commented 29 March 2013 at 13:15

Status:

Needs work

» Needs review

File	Size
1950930-15.patch	1.81 KB

Thanks dawehner! that will not return us any services :)

Log in or register to post comments

Comment #16

dawehner

German

CreditAttribution: dawehner commented 29 March 2013 at 13:34

Status:

Needs review

» Reviewed & tested by the community

Awesome!

Log in or register to post comments

Comment #17

dawehner

German

CreditAttribution: dawehner commented 31 May 2013 at 08:24

#15: 1950930-15.patch queued for re-testing.

Log in or register to post comments

Comment #18

damiankloip CreditAttribution: damiankloip commented 31 May 2013 at 13:01

File	Size
interdiff-1950930-18.txt	515 bytes
1950930-18.patch	1.81 KB

@amateescu spotted an extra space in the format description. Minor :)

Log in or register to post comments

Comment #19

moshe weitzman CreditAttribution: moshe weitzman commented 31 May 2013 at 13:15

Status:

Reviewed & tested by the community

» Needs work

Some command like this (e.g. core-config) will provide a choice menu when no argument is passed. The user then picks the service they care about and detail is shown. IMO thats better UX.

Is it really an error if no services are found. Instead use drush_log('No services found', 'ok').

Log in or register to post comments

Comment #20

damiankloip CreditAttribution: damiankloip commented 31 May 2013 at 16:49

Status:

Needs work

» Needs review

File	Size
1950930-20.patch	1.81 KB

Made that change for drush_log. We talked about this on IRC, and think the actual implementation is good.

Log in or register to post comments

Comment #21

pcambra

he/him

Spanish

Asturies

CreditAttribution: pcambra commented 20 June 2013 at 15:34

Status:

Needs review

» Active

Committed the drush part in #20 to the D8 branch, it'd be great to have an UI for this as well.

Log in or register to post comments

Comment #22

moshe weitzman CreditAttribution: moshe weitzman commented 25 March 2014 at 19:06

Title:	Add a core command to list container services	» Add a core command to list container services (needs UI)
Issue summary:	View changes

Log in or register to post comments

Comment #23

willzyx CreditAttribution: willzyx commented 19 March 2015 at 23:21

Status:

Active

» Needs review

File	Size
devel-list-container-services-ui-1950930-23.patch	3.67 KB

First try for the UI part

Log in or register to post comments

Comment #24

jibran

he/him

Urdu

Sydney, Australia

CreditAttribution: jibran commented 23 March 2015 at 18:02

Log in or register to post comments

Comment #25

willzyx CreditAttribution: willzyx commented 23 March 2015 at 20:07

@jibran maybe I'm wrong but I do not think the issues are related.. Also the drush part of this issue has already been committed (see 9f4c384); that is left to do is to create the UI