Checking the projects code I have some remarks:

- README : nice writing :)

- Why are there so many files? It looks like there is missing some integration code. What is different for listing a float or string? Or a list of X?

- Please don't use create_function. It's eval | evil ... hard to code complete.

- As there are so many files why not create classes for them. That would resolve probably the create_function usage.

- Why don't you require 'bootstrap' => DRUSH_BOOTSTRAP_DRUPAL_FULL,

- A construct like looping within the module file seems like bad practice to me

# drush_fields.drush.inc

require_once 'fields/_commands.inc';
foreach (drush_fields_commands() as $c) {
  require_once "fields/$c.inc";
}

I'm curious what you will do now you know drush-entity is similar.

Thanks for sharing your code!

Comments

ragnarkurm’s picture

- Why are there so many files? It looks like there is missing some integration code. What is different for listing a float or string? Or a list of X?

So many files? Its just current state of code. Number of files can be reduced by half so that all operation per field are in one file. It is easy to manage code when there is one file per field (currently two) not one 100kb file with all fields. There is no point reading 100kb into memory when actually need 2kb.

It is easy to work with simple fields and few of them are identical. But consider working with more complex fields. What will you do with Images or Dates?

- Please don't use create_function. It's eval | evil ... hard to code complete.

Thanks for pointing out, was not aware of this before.

- As there are so many files why not create classes for them. That would resolve probably the create_function usage.

Good point.

- Why don't you require 'bootstrap' => DRUSH_BOOTSTRAP_DRUPAL_FULL,

Ignorance / Inexperience. Have not written many drush modules before.

- A construct like looping within the module file seems like bad practice to me

# drush_fields.drush.inc
require_once 'fields/_commands.inc';
foreach (drush_fields_commands() as $c) {
  require_once "fields/$c.inc";
}

Class-ify would indeed solve many things at once: autoloading, create_function(), too many files.

I'm curious what you will do now you know drush-entity is similar.

I'll withdraw project application. Probably keep sandbox project around for some time.

In addition. Drush Entity is not usable for non-developer nor for shell scripting for following reasons:

  • User has to be aware of Drupal data structures and Drupal internal data processing to some degree. It is developer-only solution. And too technical for rest (most).
  • Data structures are dependent of Drupal versions, so user scripts break when going from d7 to d8 if field structure is different.
  • Using Drupal internal data structures is not great way to interface or integrate with other systems.
  • It does not add much extra value for shell scripting as it uses JSON (or similar) structures. There is nothing wrong with JSON itself, it is great for data interchange. But to prepare it in shell each time carefully for Drush Entity is same as writing piece of situation-specific PHP code oneself. Using JSON could serve migration purposes but do not interface humans nor shell scripting. No easy way to automate one-off changes in field values.
  • Data listing is not suitable neither for humans nor for shell scripting. From humans it requires knowledge of Drupal internal data structures. For shell: how to combine common tools like grep, cut, sort, head, tail, uniq, etc with Drush Entity?
  • Drush Entity does not provide any value handling complex fields like Files, Images, Date etc.

Using Drush Entity, how would you add directoryful of images to field_gallery (type image)? This is actually very common case asked repeatedly around in forums.

ragnarkurm’s picture

- Why don't you require 'bootstrap' => DRUSH_BOOTSTRAP_DRUPAL_FULL,

Checked the code.
Drush Fields uses DRUSH_BOOTSTRAP_DRUPAL_LOGIN, since it is supporting -u option as @mpdonadio requested in his review.

clemens.tolboom’s picture

@ragnarkurm thanks for the feedback.

Regarding #2 I should have pasted the code snippet. There was a check somewhere about not fully bootstrapped in your code. Can't remember where.

I will copy over your points from #1 into drush_entity as some are good points. The choosen edit flow using json is just a format.

Regarding files we did only scratch that with drush_entity.

As I'm working a little on Drupal 8 core rest I ran into enough problems I want to move drush_entity forward. Hope you will join.

ragnarkurm’s picture

Regarding drupal bootstrap check - that was a lot of commits ago.

I'll convert field support to Objects, then I'll join.

ragnarkurm’s picture

Assigned: Unassigned » ragnarkurm
Status: Active » Fixed

All issues are ironed out. Major task was to make it Object-ive. Now it is ready to be zipped with Drush Entity. Expecting for further instructions from you regarding merge. Also added a chapter on fields-get on Drush Fields sandbox page, please take a look.

Status: Fixed » Closed (fixed)

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