[D7] Drush Fields

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

In the files in drush/fields, I believe that you should be wrapping the argument help text in calls to dt().

Otherwise, looks really good to me.

For reference, total disregard for using t() and dt() is a blocking issue (falls under the Major API issue category). A few missing strings when good faith has been made to catch them should be pointed out, but does not warrant going back to Needs Work.

Given the followup comments, I rescind my earlier objection, and, rather than pushing this to the bottom of the queue for my mistake, I'll go ahead and mark it as RTBC.

Updated git link to public one.

Duplication

This sounds like a feature that should live in the existing Drush Entity project. Module duplication and fragmentation is a huge problem on drupal.org and we prefer collaboration over competition. Please open an issue in the Drush Entity issue queue to discuss what you need. You should also get in contact with the maintainer(s) to offer your help to move the project forward. If you cannot reach the maintainer(s) please follow the abandoned project process.

If that fails for whatever reason please get back to us and set this back to "needs review".

This is currently Postponed, but here is my review.

Automated Review

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch, see also step 6 and 7 in http://drupal.org/node/1127732

Review of the 7.x-1.x branch:

• Coder Sniffer has found some issues with your code (please check the Drupal coding standards). See attachment.
• DrupalPractice has found some issues with your code, but could be false positives, and may be duplicate results from Coder Sniffer. See attachment.

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

Fix the long lines. Unused variables seem to be accounted for via the comments before the extract().

Manual Review

Individual user account

Yes: Follows the guidelines for individual user accounts.

No multiple applications

Yes

No duplication

Maybe? Please comment on whether this should be part of https://www.drupal.org/project/drush_entity

Master Branch

No: Follows the guidelines for master branch.

Licensing

Yes: Follows the licensing requirements

3rd party code

Yes: Follows the guidelines for 3rd party code.

README.txt

Yes: Follows the guidelines for in-project documentation and the README.txt Template.

Code long/complex enough for review

Yes: Follows the guidelines for project length and complexity.

Secure code

Yes/No. If "no", list security issues identified.

Coding style & Drupal API usage

Place code review here using this format:

(+) Get rid of the master branch from the repo.

README has private git link. Use the public one.

How would a module that implements a entity_metadata_wrapper() field go about making it compatible with this module?

The chown() / chmod() / chgrp() will require elevated permissions on the server, and have these been tested on Windows? May need to
document this behavior.

(+) In general, db_select() against the node table should have the node_access() tag. This is an interesting case. In a module, this
would be a security problem. With a drush command, you bypass just about all security. However, if you run
drush with the -u option (to get it to run as a user), you may end up in a weird situation where a hook_node_update() runs
on a node that the user doesn't really have access to. I would add the tag, and document some of the weirdness that may result
because of hooks / rules running as the user that drush is run as (which defaults to the anonymous user). Some of the same
general concerns pop up in drush_fields_target(). Maybe an explicit entity_access() instead? Maybe warn/prompt users in drush_fields_target()
when the drush user don't have update access to the entity?

I concur about the dt() usage in hook_drush_command entries. I am not seeing any in the code drush files.

Can't drush_fields_target() just use entity_load() instead of building up the load funciton name?

This is just a note to myself and other reviewers that drush_exit() got removed a while ago. However, I am not 100% certain
that calling exit() is appropriate. Double check this.

I think drush_fields_file_add() will need 'description' set on the file object for it to work properly with entity_metadata_wrapper() (I have had
problems with this in the past).

Comments about files also apply to images.

Not a huge fan of extract(), but I think you documented it well enough.

Will the file add work with remote files? Look into system_retrieve_file(), this may work with local files, too.

Seems to me that some refactoring could be used to share / simplify things, eg float / int / text share a lot in common, other than validaton.

You may want to look into whether using field_attach_update() is an option rather than a full blown save.

There is a way to package drush-only "modules". I am pretty sure you get rid of the .module file and the core= from the .info file.
Look at drush_extras to see how it works.

Do the file/image functions work when File Entity is enabled? I can't recall if entity_metadata_wrapper() behaves differenly when
they are.

I'm seeing escaped text when I do a 'drush fields-text-add'.

I think that these operations should be logged via watchdog(), as normal content updates are.

(+) I also think that these should propmt the user before updating, unless the -y option is present.

A couple of these are fairly big (the + ones), but I am not seeing any blocking issues (other than the duplication issue mentioned above).
If this gets committed to drush_entity, and other admins concur, I don't see a problem giving vetted access based on this code.
Whether or not this gets folded into drush_entity, I see this as a very useful addition to the module space. I have a lot of
one-off drush scr scripts for altering data, and this would simplify my life.

Grrr.

The field_attach_update() was just a suggestion to investigate. You are correct that a proper node_save() will invoke hooks so things like Rules run, which is best. I frequently run into situations where I need to do touchup on data (esp after a import) where I don't necessarily want the Rules to run.

One quick remark:
You have a lot of create_function calls, about which the php.net documentation warns ...

This function internally performs an eval() and as such has the same security issues as eval().

I would opt for a duplicate project which is motivated below.
---
Drush core has a command set field

$ drush @drupal.d7 help | grep field
All commands in field: (field)
 field-clone           Clone a field and all its instances.
 field-create          Create fields and instances. Returns urls for field editing.
 field-delete          Delete a field and its instances.
 field-info            View information about fields, field_types, and widgets.
 field-update          Return URL for field editing web page.

Drush Entity project has a command set entity. It can read entity type and CRUD on entities. It's update can filter on (list of) field(s).

$ drush @drupal.d7 help | grep entity
All commands in entity: (entity)
 entity-create (ec)    Create an entity from a json object
 entity-delete (ed)    Delete entities.
 entity-list (el)      Get a list of entity type information in a summary table.
 entity-read (er)      Print entity contents
 entity-type-read      List details of entity types
 entity-update (eu)    Update an entity as json in the default editor

As an example let's update node/53 only it's title (starts default editor)

drush @drupal.d7 entity-update node 53 --fields=title
drush @drupal.d7 entity-read node 53 --fields=title

Or even through STDIN

echo '{"title":"XXX"}' | drush --pipe @drupal.d7 entity-update node 53 --fields=title --json-input=-
drush @drupal.d7 entity-read node 53 --fields=title

@ragnarkurm Sorry for the long delay :-/

I'll review the code through the sandbox issue queue.

I will update the project page https://www.drupal.org/project/drush_entity with these example.

Next process issue #2290005: Merge Drush Fields into Drush Entities?

@clemens.tolboom, I'm confused. Do you want to merge this into Drush Entities or do you think this should stay a separate project?

@mpdonadio sorry for the confusing text.

I think the project is a duplicate (Closed (duplicate)) of drush_entity only different in drush commands and arguments.

I've invited @ragnarkurm to join drush_entity per #2290005-5: Merge Drush Fields into Drush Entities?

Hope that helps.

Yes, we can do that. We just need to determine what the review will be against. Is the patch against Drupal Entity mostly standalone that we could evaluate that? Or, is the sandbox version essentially what got committed?

Since you don't have any other sandboxes to promote currently I think that we can wait until your changes to drush_entity land.

You should start to provide smaller patches against drush_entity so that it is easier for the maintainers to accept them. Once there is enough of your stuff in drush_entity let's review that and proceed here. I think it would be a waste of time to review the sandbox code now which will soon be obsolete. Thank you for your yonsideration and your contribution!

So I guess I must work harder on drush_entity besides oter projects :-p

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.

The user who is applying should not set the status to Reviewed & tested by the community.

I will update your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thank you, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks go the dedicated reviewer(s) as well.