DevelGenerate as a D8 plugin type

Thanks! Looks like a good modernization. Lets make sure we keep the Drush integration working as we patch this. It is a good exercise to assure that the new code is reusable. Drush has an "engine" system where code can vary by Drupal version. I think that would be appropriate here since the D8 code will be so different. If the author wants to ignore engines for now, I can layer that in later.

First of all, thanks Enxebre for starting this.

This patch is getting bigger and bigger and I'm afraid we'd end up with something not manageable.

The least we can do here is to provide an interdiff, otherwise it will be impossible to review.

Options we have here:

• Create a sandbox and work in there until we have something to put back.
• Create a [META] issue and split the work in several chunks, i.e. one issue for the base+1 component and one issue for each one of the rest.

Here's a review of what I've seen so far.

1. +++ b/devel_generate/lib/Drupal/devel_generate/DevelGenerateBase.php
   @@ -0,0 +1,78 @@
   +  /**
   +   * Whether default settings have been merged into the current $settings.
   +   *
   +   * @var bool
   +   */
   +  protected $defaultSettingsMerged = FALSE;
   

   Why do we need this? seems like overhead to me

2. +++ b/devel_generate/lib/Drupal/devel_generate/DevelGenerateBaseInterface.php
   @@ -0,0 +1,73 @@
   + * This interface details base wrapping methods that most DevelGenerate implementations
   + * will want to directly inherit from Drupal\devel_generate\DevelGenerateBase.
   

   Comments go over 80 chars, we need to stick to Drupal standards

3. +++ b/devel_generate/lib/Drupal/devel_generate/DevelGenerateFactory.php
   @@ -0,0 +1,22 @@
   +/**
   + * Factory class for the ElementGenerate plugin type.
   + * Not necessary at the moment.
   + */
   +class DevelGenerateFactory extends DefaultFactory {
   +
   

   Why do we add it then?

4. +++ b/devel_generate/lib/Drupal/devel_generate/DevelGeneratePluginManager.php
   @@ -0,0 +1,27 @@
   +    //$this->setCacheBackend($cache_backend, $language_manager, 'devel_generate_plugins');
   +    //$this->alterInfo($module_handler, 'devel_generate_info');
   +    $this->factory = new DevelGenerateFactory($this);
   

   Watch out this commented code, we probably need alter hooks?
   Also the factory is added.

5. +++ b/devel_generate/lib/Drupal/devel_generate/Form/GenerateForm.php
   @@ -0,0 +1,54 @@
   +    $element_to_generate = arg(4);
   

   We should be able to get the element to generate better

6. +++ b/devel_generate/lib/Drupal/devel_generate/Plugin/DevelGenerate/UserDevelGenerate.php
   @@ -0,0 +1,180 @@
   + *     "num" = 53,
   

   53?

Thanks, this one seem way more complete, let me give it a try and give proper feedback.

I think this is a good version to be patched and to continue working on later to suit the code to fit with the changes of the Drupal 8 API. Anyway if you don't agree with that we can create a sandbox and apply the incoming feedback toward the sandbox.

Let's try to get the tests green, probably need to be adapted to fit the new plugin system, also a more complete test coverage is desirable.

Regarding this:

- The module provides 5 instance of this plugin type (User, Content, Menu, Terms and Vocabulary).

It would be ideal to have a generic entity system, but let's leave this for a second iteration maybe.

Just let me know if you would like me to send a new patch, otherwise I adding this changes later with the incoming feedback.

Reviewing patches that introduce a 100k interdiff and move files is quite hard, that's why I suggested a sandbox instead.

First let's make the tests green, then we can go on with further reviews.

Nice, I'll be creating issues in https://drupal.org/project/issues/2167477 instead of making more noise here, once we've got something stable, we can put a patch back in here.

Anyone interested in contributing to this patch, please refer to the sandbox https://drupal.org/sandbox/enxebre/2167477

Could you give me access to commit in there?

Thanks @Enxebre

This would be a really needed change and would allow us to get further and abstract the entity generation to generate any kind of entity.

Assigned to moshe for review as we're completely changing devel generate :)

I need to review this further. My first impression is to be a bit skeptical about the dynamic declaration of drush commands. How hard would it be to do this with one command? We could do further dispatching in the command callback.

Does anyone know of other modules that do dynamic declaration of commands?

I think @moshe is referring to this part here.

+++ b/devel_generate/devel_generate.drush.inc
@@ -8,243 +8,41 @@
+  $items = array();
+  $definitions = \Drupal::service('plugin.manager.develgenerate')->getDefinitions();
+
+  foreach ($definitions as $id => $plugin_info) {
+    $command_suffix = $plugin_info['drushSettings']['suffix'];
+    $alias = $plugin_info['drushSettings']['alias'];
+    $items["generate-$command_suffix"] = array(
+      'callback' => 'drush_devel_generate',
+      'description' => "Create $id.",
+      'callback arguments' => array(
+        'plugin_id' => $id,
+      ),
+      'aliases' => array("gen$alias"),
+    );
+    $items["generate-$command_suffix"]['options'] = $plugin_info['drushSettings']['options'];
+    $items["generate-$command_suffix"]['arguments'] = $plugin_info['drushSettings']['args'];
   }

Maybe a better solution could be to have just one command:

drush devel-generate [entity-type]

Keeping this in sync with d8 head and devel head these days is quite hard, could we revert the change refering to drush, keep the "static" old generate commands and have a followup for the one above?

Looks good. Could we rename and/or document handleDrushParams(). I'm not clear on what it does, and why it is Drush specific. In the implementations we do a fair amount of input validation which would usually belong in a drush validate hook.

We are preserving all existing tests and assertions?

The Drupal code looks good enough to me.

@Enxebre: It looks like devel itself is failing tests at the moment. If you click view on your patch you'll see the testing is postponed, if you click the branch link from there you'll see failures in Drupal\devel\Tests\DevelMailTest and Drupal\devel_generate\Tests\DevelGenerateTest. A function that it was depending on has probably been removed from core.

The testbot went away but we're green again, if the drush part looks better, I think we're good with the plugin approach and open followups for the next steps.

Again, huge kudos to @Enxebre for working on this :)

OK, I'm happy with this. I've been away for a week, so pcambra can commit this if now is a good time. Thanks @Enxebre

Suppose now devel should be fixed to work with current HEAD #2208597: Devel HEAD broken: Update calls to config(), drupal_map_assoc, Drupal\Core\Mail\PhpMail

The patch has got in! huge thanks to @Enxebre for the work on this.

We've got a remaining issue with the menus, I'll investigate it and open a follow up.