Relation clone

You have some issues with drupal coding style. http://ventral.org/pareview/httpgitdrupalorgsandboxwqmeng1405168

and you will have to work in separate git branch rather than in master branch. more information http://drupal.org/node/1127732

PS: set category to task.

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. Get a review bonus and we will come back to your application sooner.

FILE: ...ew/sites/all/modules/pareview_temp/test_candidate/relation_clone.module
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 65 | ERROR | Concatenating translatable strings is not allowed, use
    |       | placeholders instead and only one string literal
--------------------------------------------------------------------------------

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

Manual review
- Not use concate string for input of t function, change to use placeholder, and make it more readability:

          '#title' => t('Clone ' . l(t('Relation') . ' ' . $relation->rid, 'relation/' . $relation->rid) . ' type:' . $relation->relation_type . ' ' . implode(($r_type->directional) ? " -> " : " -- ", $relation_column)),

Automatic review reported no problem.
Manual review:
The API seems to be used correctly, except for one point: as said before, you should use placeholders in your t() functions. t('My name is %name', $replacements) is better than t('My name is') . ' ' . $name.

And 3 minor details:

• Is line 21 really needed? If so, you might tell why in a little comment.
  

  if (isset($form['type']['#value']) && $form['type']['#value'] . '_node_form' == $form_id) 
  

• $node->relation_clone is not such a clear name. Something like $node->relation_clone_source_nid would be more self-describing.
• calling relation_insert() "inside" a call to drupal_set_message is correct but unclear. I would separate this into 2 lines. We save the relation, then we output a message to the user.

Auto review is still good.

A manual review reveals that:

* Beware of name space conflicts, there are no problems right now but there is a relation module that could implement a function named relation_clone_xxx
I would consider renaming the module but that could be done later.
* There are several lines that go over 80 characters (minor).
* line 64 in relation_clone.module:

        '#title' => t('Clone ') . l(t('Relation @rid', array('@rid' => $relation->rid)), 'relation/' . $relation->rid) . ' ' . t('type:%relation_type', array('%relation_type' => $relation->relation_type)) . ' ' . implode(($r_type->directional) ? " -> " : " -- ", $relation_column),

- Goes over 80 characters.
- Concatenates t() calls which could lead to a problem when translating.
I'd suggest separating the links to make it clearer. and usgin something like

$connector = ($r_type->directional ? " -> " : " -- ");
$link_args = array('@rid' => $relation->rid);
$title_args = array(
  '!link' => l(t('Relation @rid', $link_args), 'relation/' . $relation->rid),
  '%relation_type' => $relation->relation_type,
  '%columns' => implode($connector, $relation_column),
);
t('Clone !link type:%relation_type %columns', $title_args);

Seems way more readable.
Everything else looks good to me.

Hey wqmeng.

I'd like to review your module but it looks like your repo is now on the Master branch alone.

.....which I think is fine for your README file but not for your other files which aren't showing since they're probably on your other, working branch.

You probably wanna fix that, no?

EDIT: And not to pick gnits, but the only text within the readme is "See major version branches." which, technically, isn't all that helpful. OK....I'll stop. ;-)

hey @AaronELBorg the branches seem good to me, there's an empty master with a dummy README.txt and there's a 7.x-1.x branch with the module, created 5 weeks ago.
see
http://drupalcode.org/sandbox/wqmeng/1405168.git/tree/refs/heads/master
and
http://drupalcode.org/sandbox/wqmeng/1405168.git/tree/refs/heads/7.x-1.x

So technically the text in README.txt is right.

Also, setting to needs work from #11

lucascaro,
You got me again.

Granted, I still had to switch from the master branch to the 7.x branch manually but, yes, the files are there (after the switch).

I don't know what you mean here though:

Also, setting to needs work from #11

Oh, sorry @AaronELBorg, that's just that I forgot to set the status to "needs work" after my review in comment #11 :)

I like this module. Can't believe that it's not baked into Node Clone.

wgmeng, I think the new name sounds like a good idea.

I agree with @AaronELBorg. Also, keep in mind that according to http://drupal.org/node/299070 the _ in a short project name is the part that could cause problems, but hey! there are lots of projects that use _ so it's just an FYI just in case you didn't already know :) (sorry if it's only adding noise).
Let us know when you have pushed your changes so we can review! (don't forget to set it as needs review).

Cheers!

Hello,

Please remove your master branch and add yor 7.x-1.x branch as default on version control tab. It's a good practise, even though documentation page do not mention it (because of CSV migration I believe).

On line 98 you have ; char after closing bracket. It is not neccessary.

On line 13 and 62 you use quotations mark for strings. It makes php parse those string searching for any variables to render. It is PHP good practise to use ' in such situations. It's also Drupal practise to use the same type in your functions and you are using aphostropes for other strings already.

You should place full stop at the end of each comment.

But otherwise your module looks good. These should not be RTBC blockers. I've installed your module and it works fine for me.

Consider participating in Review bonus to get your application reviewed sooner.

Your project page is not very detailed, please have a look at the tips for a great project page, you may also use HTML-tags for better structure. Also your readme isn't very long, try to keep readme and page in sync.

There is still a master branch, make sure to set the correct default branch: http://drupal.org/node/1659588 . Then remove the master branch

git checkout 7.x-1.x
git branch -D master
git push origin :master

/**
 * Implements hook_form_alter().
 */
function relation_clone_form_alter(&$form, $form_state, $form_id) {
  if (isset($form['#node']->relation_clone_source_nid)) {

This seems like you rely on the base node_form, so rather to check whether you are on the right form by just checking whether '#node' exists, use the form id "node_form" and hook_form_FORMID_alter(). Otherwise there's a chance that you mess up all other forms that also have this '#node' key.

    foreach ($node->enable_clone_relation as $relation => $enable) {
      if ($enable) {
        $rids[] = $relation;
      }
    };

I'm pretty sure array_keys(array_filter($node->enable_clone_relation)) would do the same

Please try to never use such short variable names like $r, it's hard to guess what they're good for. (at least $rel would be fine for me)

if ($context['method'] == 'prepopulate') {
    if (empty($_POST['op'])) {
      drupal_set_message(t('Relations will be copied when you submit.'));
    }
  }

It makes no sense to state that "relations will be copied" - but they won't if you untick the checkbox. Just remove that message and avoid access the $_POST globals directly.

Please try to fix those and I'll have a final look

Closing due to lack of activity. Feel free to reopen if you are still working on this application.