Actually, it would be more accurate to say move the drush commands related to typed data out of D8 Rules Essentials (where they have been developed and tested) and into the Typed Data API module, which is a more logical place.

These commands are:

$ drush --filter=typed-data
Drush Commandline Tool 10.3.1

Run `drush help [command]` to view command-specific help.  Run `drush topic` to read even more documentation.

 Available commands:
                                                                               
 typed-data:                                                                                          
   typed-data:contexts (cl, context-list)       Show a list of available contexts.                    
   typed-data:datafilters (fl, datafilter-list) Show a list of available TypedDataFilter plugins.     
   typed-data:datatypes (tl, datatype-list)     Show a list of available Typed Data datatypes.        
   typed-data:entities (el, entity-list)        Show a list of available entities.                    
   typed-data:formwidgets (wl, formwidget-list) Show a list of available TypedDataFormWidget plugins.

Comments

TR created an issue. See original summary.

tr’s picture

Status: Active » Needs review
StatusFileSize
new4.5 KB
jonathan1055’s picture

Status: Needs review » Needs work

Nice work. Just a few points/questions:

  1. You have the $categories = TRUE and $short = FALSE parameters in formatOutput() but no way to alter or set thm. Is it worth having command options to allow these to be changed?
  2. In my test environment for listDataFilters and listFormWidgets, the category value is blank for all, so the two outputs are the same (apart from indentation). How do we get category text values? Or is this just allowing for a future enhancement when there may be values? Or did you mean $plugin['provider'] not $plugin['category']?
  3. You have left in a commented source code line.
        foreach ($definitions as $plugin) {
          // $datatypes[] = $plugin['class'];
          $datatypes[] = $plugin['id'];
    

Thanks for this, it will be useful.

tr’s picture

OK, so the history of this code is that every one of these commands started out as a quick-and-dirty script to fetch information I found useful, then I turned them into individual drush commands for the tr_rulez module. Then I refactored the code because all those commands were doing essentially the same thing - getting the plugin definition from the plugin manager and printing out some of the array elements from the plugin definition. The only real difference was which plugin manager was used and which array elements were available - that code went into formatOutput() and the list...() functions became thin wrappers around formatOutput().

These commands have been in tr_rulez for more than 2 years now. And I have documented them all in the D8RE documentation. I have always meant to improve the output control "eventually", like I did with the Rules drush rules:list command, but since it seems that it's only me who has been using them (no feedback ever in the issue queue or documentation for any of these features) I have put off working on that - I have what I currently need; further work on the output formatting is a low priority for me when there are more important things that need to be done. But of course I'm totally for it if someone else wanted to contribute.

Now, as part of porting these drush commands to Drupal 9+, I decided that a better place for the commands related to typed data is in the Typed Data module. To split these commands out of tr_rulez I had to duplicate formatOutput(), so it looks like some of the options aren't needed (and they're not, at least not for the typed_data module). Specifically, Data Filter and Form Widget plugin managers don't implement CategorizingPluginManagerInterface, so those plugins do not have categories thus the category parameter to formatOutput() really is not needed in Typed Data. But that parameter IS needed for the copy of formatOutput() that still exists in tr_rulez. Categories are used for Rules events, conditions and actions - these are the "Content", "System", "User" etc. divisions you see in the UI when selecting an event, condition or action - the category is a parameter set in the annotation.

I left the commented code in because sometimes I want to know the class that implements the plugin (useful for when there's a deriver involved, for instance) but I never took the time to make this into a proper option - it was easier just to uncomment the line when I needed it.

So yes, I think there's a lot of room for improvement. My goals for this issue are primarily:

  1. Get this code out of tr_rulez and into typed_data where it is more likely to be used and more likely to be useful.
  2. Provide this code as Drush 9+ code, since Drush 8 is not supported in D9 and the old .drush.inc commands are not really useful anymore.
jonathan1055’s picture

Status: Needs work » Reviewed & tested by the community

Thanks for explaining the background. Yes you are right that we have more pressing tasks, so RTBC for patch #2. At some time I could look at catering for the display options with drush arguments and adding a switch to show the full class, if that would be helpful.

  • TR committed 89088e5 on 8.x-1.x
    Issue #3164489 by TR: Add Drush commands for typed data
    
tr’s picture

Status: Reviewed & tested by the community » Fixed

Committed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

jonathan1055’s picture

The commit above introduced four coding standards warnings.

FILE: typed_data/src/Commands/TypedDataCommands.php
------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 4 WARNINGS AFFECTING 4 LINES
------------------------------------------------------------------------------------
 19 | WARNING | \Drupal calls should be avoided in classes, use dependency injection
    |         | instead (DrupalPractice.Objects.GlobalDrupal.GlobalDrupal)
 34 | WARNING | \Drupal calls should be avoided in classes, use dependency injection
    |         | instead (DrupalPractice.Objects.GlobalDrupal.GlobalDrupal)
 49 | WARNING | \Drupal calls should be avoided in classes, use dependency injection
    |         | instead (DrupalPractice.Objects.GlobalDrupal.GlobalDrupal)
 86 | WARNING | \Drupal calls should be avoided in classes, use dependency injection
    |         | instead (DrupalPractice.Objects.GlobalDrupal.GlobalDrupal)
------------------------------------------------------------------------------------

Is this something we want to fix? How much effort is it? We have plenty of other things to do.

tr’s picture

I deliberately did not inject services into the Drupal command class for several reasons. First, it's a Drush command class - we will not be writing tests for these Drush commands because they are just thin wrappers around the core and module services that the commands are using (which are already tested elsewhere), and testing would be the primary motivation for injecting the services here. Second, we would have to inject at least 5 services, each for a one-time use, and that would add ~80 lines of boilerplate code which would obscure and overwhelm the ~125 lines of actual functional code in there. Plus every new command would require a new service to be injected.

The coding standard is "should be avoided", not "MUST be avoided", so instead of treating this as a commandment we should use our discretion to determine when this coding standard provides a benefit and when it doesn't.

jonathan1055’s picture

StatusFileSize
new1.81 KB

Yes I thought the overhead would be detrimental, and your decisions were deliberate, but good to check.

Here's a patch which uses
// @phpcs:ignore DrupalPractice.Objects.GlobalDrupal.GlobalDrupal to mark these four warnings as known and accepted. It's worth keeping the codebase clean, so that when a new coding standard message is shown it is more apparent.

I cannot re-open this issue, so won't be able to set this patch for testing on drupal.org. On local testing phpcs is clear, we get no messages.

tr’s picture

Thanks. Committed.