Drush clone

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

Drupal Code Sniffer has found some code style issues (please check the Drupal coding standards). 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. Go and review some other project applications, so we can get back to yours sooner.

FILE: ...al-7-pareview/sites/all/modules/pareview_temp/test_candidate/README.txt
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
2 | WARNING | Line exceeds 80 characters; contains 86 characters
--------------------------------------------------------------------------------

FILE: ...ew/sites/all/modules/pareview_temp/test_candidate/drush_clone.drush.inc
--------------------------------------------------------------------------------
FOUND 20 ERROR(S) AND 1 WARNING(S) AFFECTING 15 LINE(S)
--------------------------------------------------------------------------------
4 | ERROR | The second line in the file doc comment must be " * @file"
6 | ERROR | Missing function doc comment
95 | WARNING | Line exceeds 80 characters; contains 90 characters
96 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
102 | ERROR | else must start on a new line
103 | ERROR | Inline comments must end in full-stops, exclamation marks, or
| | question marks
112 | ERROR | There must be an empty line before the parameter block
112 | ERROR | Last parameter comment requires a blank newline after it
112 | ERROR | Parameter comment must be on the next line at position 1
113 | ERROR | Missing comment for @return statement
116 | ERROR | No space before comment text; expected "// File extensions we
| | need to handle." but found "//File extensions we need to
| | handle."
123 | ERROR | There must be an empty line before the parameter block
123 | ERROR | Last parameter comment requires a blank newline after it
123 | ERROR | Parameter comment must be on the next line at position 1
124 | ERROR | Missing comment for @return statement
127 | ERROR | No space before comment text; expected "// File extensions we
| | need to handle." but found "//File extensions we need to
| | handle."
134 | ERROR | There must be an empty line before the parameter block
134 | ERROR | Parameter comment must be on the next line at position 1
135 | ERROR | Last parameter comment requires a blank newline after it
135 | ERROR | Parameter comment must be on the next line at position 2
136 | ERROR | Return comment must be on the next line
--------------------------------------------------------------------------------

Source: http://ventral.org/pareview - PAReview.sh online service

Is there any difference between your implementation of feature-clone and the existing one? It would be nice to have all of these in one module, but does this make the two mutually exclusive? (i.e. will drush break when I install your module and the features-clone module?)

Thanks for clarifying. When running with drush 7.x-4.5 the extra commands you have added appear in my drush help, but the individual commands do not execute (can not be found).

% drush help
...
Other commands: (coder_review,drush_clone,omega_tools)
 coder-review          Run code reviews         
 feature-clone         Clone a feature.         
 model-clone           Clone an entity model.   
 module-clone          Clone a module.          
 omega-subtheme        Create a Omega subtheme. 
% drush feature-clone
The drush command 'feature-clone' could not be found.                                                                                  [error]
% drush module-clone
The drush command 'module-clone' could not be found.                                                                                   [error]
% drush drush model-clone
The drush command 'drush model-clone' could not be found.                                                                              [error]

Are any additional steps required to get this working?

Thanks for the tips regarding where this has to be to work. I'm used to Drush commands that are part of modules and so normally place them inside the sites/all/modules directory. You should make it clear in your INSTALL.txt that the only valid options for locations are the two you have mentioned above, and that the commands won't work if you place them:

b) Along with one of your enabled modules. If your command is related to an
existing module, this is the preferred approach.

it won't work as this module isn't actually attached to a full Drupal module.

Manual code review

• You should use dt() to surround possible localizable strings in your .drush.inc command declaration as you do later in the file. While there doesn't seem to be consistency across all drush commands that this is done, it would be best to follow the standard Drupal practice of making these strings localizable.
• When executing the commands with no arguments and a high level of PHP debugging, (e.g. php error_reporting set to E_ALL) you receive the following warnings:
  

  %drush module-clone
  Missing argument 1 for drush_drush_clone_module_clone_validate() drush_clone.drush.inc:67                                                                      [warning]
  Missing argument 1 for drush_drush_clone_module_clone() drush_clone.drush.inc:74                                                                               [warning]
  drush: The module or feature "" could not be found.
  Drush command terminated abnormally due to an unrecoverable error.                                                                                             [error]
  

  You should modify your code to accept no arguments so as to prevent the PHP warnings regarding missing arguments.

• Avoid using \' where possible - it would be most consistent with other Drush commands to change the escaped single quotes in your strings to double quotes

After 'installing':

rudie@rudie-lpt-ubuntu:~$ drush | grep clone
 field-clone           Clone a field and all its instances.

Where's the rest?

Priority+ after 4 weeks.

Note to other reviewers: I do not have much experience with Model Entities, so I'm focusing on Features and pure code review here.

1. I'm not sure I understand the installation instructions completely. These are steps for installing on a server. But drush works just fine with Drupal modules that have Drush hooks inside a site hierarchy as long as they're enabled. Is there a reason this can't or shouldn't be used within a virtual host? If you just flesh out your INFO file this could be a standard Drupal module. (If you don't plan to release it on Drupal.org as a full module I don't understand why you would go through this process...?)

2. @file needs a more expressive comment. This is here for scripts and code tools to look for: "Provide implementation of drush command" doesn't tell me which Drush command.

3. You're obeying the 80-col limit which is good, but you don't need to be so aggressive about it. There's an exception for things like long strings especially where it improves readability to keep it on one line. This:

      'module-clone model my_entity' => dt('Clone "model" module in to "my_entity" module.'),

is preferable to this:

      'module-clone model my_entity' => dt('Clone "model" module in to
      "my_entity" module.'),

4. This is nit-picking, but there are some odd whitespace uses for newlines:

  $source_path = drupal_get_path('module', $source);
  $files = file_scan_directory($source_path, '/.*/');

  foreach ($files as $file) {
    $target_uri = str_replace($source, $target, $file->uri);

is more readable than:

  $source_path = drupal_get_path('module', $source);
  $files = file_scan_directory($source_path, '/.*/');
  foreach ($files as $file) {

    $target_uri = str_replace($source, $target, $file->uri);

5. You are not checking user input very much. I do not believe this is a security risk: Anybody with Drush access can do whatever they want with a site so the usual check_plain category of checks is probably a waste of time.

However, DUPE detection would be very handy. You don't do much in the way of checking to be sure the target doesn't already exist before overwriting it. Currently you call module_exists but that only returns true if the module is enabled. What if it's there but not enabled? What if I clone, modify some code in the new clone, and then accidentally re-clone? My work gets overwritten.

6. You do not copy recursively. If I try to clone a module with subdirectories (templates are very common) this fails.

7. You should probably also check to be sure $source != $target.

8. The behavior where I try to clone a system module is not consistent. Modules are cloned to and from the same path, so if I clone "user" I get modules/user_clone, where I would have expected it to go to sites/all/modules (outside core-space). Can this be an option? What about multi-site installations, too?

9. You should warn the user when copying .tpl.php files. Once the clone is enabled Drupal will scan for and find these files in the new folder automatically, which can be a problem when cloning some types of modules. I do not think you can fix this - it is up to the user to address it. But you should ask or tell them so they know.

10. I SORT OF understand why you are copying only certain extensions. But not really. Why do you not copy .txt files? What about modules with JS/CSS? Many module break without these files. Are there types of files where it actually is useful to not copy them? I think as a user I would want them all.

This looks very useful. Thank you for your contribution.

This has been really useful to me already! Do you think it would be possible to extend this into cloning themes. This would be really helpful for setting up "starterkits".

Another feature that would be nice for the model clone would be to be able to have different module and entity names. For example, a course_registration module that created a registration entity.

@jlyon that would be a nice feature... but because this is on hold for code review, not suggestions, that doesn't look like a blocker.

Other reviewers: the items I identified in my own review were addressed. As I said above, I'm not a Model Entities guru, but the enhancements were enough to address the points that I found.

Unfortunately, PAReview has been enhanced to look for new issues, and this project now fails those issues:
http://ventral.org/pareview/httpgitdrupalorgsandboxlucor1397300git

@patrickd and @MiSC have been correcting statuses from needs work -> needs review when this happens but only when a project hasn't had a proper, human-driven review. This project HAS had that review. So I'm marking it needs work so @lucor is alerted that they need to be addressed. After those issues are resolved this project is ready for RTBC IMHO.

I think you're getting this error because normally the @return comment should read:
@return [type] [comment]

as in:
@return string The formatted HTML tag

In your case you aren't returning anything so you shouldn't need this @return at all - I would try removing it and see if that passes. It is not necessary to specify @return unless you return something.

Nonetheless, all other points have been addressed from both manual and automated code review. This looks RTBC to me.

We are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

Thanks for your contribution, lucor!

I updated your account to let you 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 get involved!

Thanks crobinson, jlyon, andrewyager for your awesome reviews and great questions for lucor! It was very helpful to me. I encourage you to continue working together and posting issues for any potential improvements you've come across. Thanks again!