Closed (fixed)
Project:
Drush Fields
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Reporter:
Created:
8 Jul 2014 at 10:44 UTC
Updated:
28 Jul 2014 at 00:10 UTC
Jump to comment: Most recent
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
Comment #1
ragnarkurm commentedSo 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?
Thanks for pointing out, was not aware of this before.
Good point.
Ignorance / Inexperience. Have not written many drush modules before.
Class-ify would indeed solve many things at once: autoloading, create_function(), too many files.
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:
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.
Comment #2
ragnarkurm commentedChecked the code.
Drush Fields uses DRUSH_BOOTSTRAP_DRUPAL_LOGIN, since it is supporting -u option as @mpdonadio requested in his review.
Comment #3
clemens.tolboom@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.
Comment #4
ragnarkurm commentedRegarding drupal bootstrap check - that was a lot of commits ago.
I'll convert field support to Objects, then I'll join.
Comment #5
ragnarkurm commentedAll 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.