Entity Reference Cascade Delete

Hi,
Firstly there is lots of issue regarding line spaces, indentation & comments.

Look at this !
http://ventral.org/pareview/httpgitdrupalorgsandboxatozstudio1892808git

Here you can check source code whether it meets drupal coding standards or not, and advise you what to change in your code. You can repeat review after your commits, and can fix those errors.

Also your code repository is in master branch, move your code to new branch 7.x-1.x & delete master branch.

Hi atozstudio,

Thanks very much for submitting this very interesting module.
Thank you for fixing all the coding standards validation issues brought up by osscube.

I didn't have much time to go into the module in depth, but what I can already tell is that ercd.install seems to be well coded and you avoided making the common mistake of forgetting to remove variables upon module uninstall. So this looks good already.

So I pushed a little bit further my investigations and looked at ercd.js, which looks like it could be greatly improved and I would have the following comment:
Could you please consider using the Drupal Javascript behavior handling, for example, as it was suggested by DYdave on #1846194-7: Custom Add Block?

So your code would look more like:

(function ($) {

/**
 * TODO: This is an example:
 * TODO: Change this: Provide the toggle features for the fieldsets on settings
 * page [Watchout not to be longer than 80 characters]
 */
Drupal.behaviors.ercd = {

  attach: function(context, settings) {
    $('.ercd-trigger').change(function() {
      if ($(this).attr('checked')) {

        [...The rest of your JS code and logic should be here...]

      }
    }
  }
}

})(jQuery);

One more validation related advice would be for you to run the coding standards validation (PAReview) again, after you have committed your changes, to make sure you didn't introduce any new coding standards errors when you made the changes. After that, feel free to change the status back to needs review, that will save you one not so useful review from another user who would have simply told you about the coding errors mistakes (more back and forth = more time). This is something very common that happens to all of us (and just for one whitespace at the end of the line... the status is changed back to needs work again).

I didn't push my investigations any further than the JS file yet, but I think we should be giving this project more reviews after these issues have been fixed.

Feel free to let me know if you would have questions, comments or concerns on any of the reported items, I would surely be glad to provide more information.

Hope that helps improving module's code.
Cheers!

Documentation:
This is how I'm using the terms parent/child: Parent is the node being deleted, and child is the node being referenced by parent's entity reference field.

I'm confused by the module's description, and wording compared to how it works. I was expecting this module to delete the child node(s) that would be orphaned when the parent is deleted. Whereas it deletes the parents of the node being deleted.

...manage dependent entities when their parent entities are deleted.

Instead of parent, should it be child here?
When I think of dependent, I think that the child depends on the parent, so when the parent is deleted, it deletes the child. I'll help with this wording when it's clearer.

The summary/description could be more explicit that it adds extra functionality to the delete confirmation page.

Upon parent entity deletion, it gives the option to either delete all dependent entities.

This blurb doesn't do this module justice as to when it works. Something like "Before confirming the deletion of a node, the administrator is able to choose other ('parent'/'child') nodes that can be deleted."

Testing
I tried this out on a fresh install and used Devel Generate to generate some nodes for a new node type named 'random' that had an Entity Reference field that referenced the new node type('random'). Devel Generate filled in the entity reference to nodes of the same content type so afterwards I had a pretty complicated list of parent/child relationships several levels deep.

Not sure if this an error or not now...
I think I'm getting an error. The module seems to be working in reverse and allowing me to delete the parents, but not the children. See pics. The first image shows a view with nodes on the left, and children on right. The first image has 'Cogo Macto Usitas' highlighted which is what I'm deleting, but instead of getting to delete child node 'Abico'(second image), I'm getting parent nodes of 'Cogo.'

Javascript review
The Drupal.behaviors.ercd.attach is effectively the Drupal equivalent of $(document).ready(), so you should move the code at line 73 into the 'attach' function, or if you want to keep it separate, change the $(document).ready into a named function like ercd_init and call it from the 'attach' function.

In ercd_set_state, you could save some clock cycles by using variables, instead of repeating code.

Instead of repeating this: $(this).parent().next('.ercd-child'). (lines 139,140,142,147)

Add a variable near the top of the function.
var $next = $this.parent().next('.ercd-child');
and shorten the lines to use the variable:$next.fadeIn(); (line 140)

Note that because '$' is a valid character for JavaScript variables, $next is a variable and has nothing to do with jQuery's $ other than that it is storing jQuery's return.

Line 119 you send $(container) into ercd_set_state(), but every time you use 'container' in ercd_set_state you pass it through $() again(lines 135,153,155). This is the equivalent of doing $($(container)). Also line 77 is the same unnecessary wrapping. You should remove the $() wrapping the variables on 77 & 119.

Line 144, slipped through the pareview for comments. Add a space after //, capitalize the t, and put period at the end. //this is borrowed from misc/collapse.js

Automated review

All fine, no errors.

Manual review

1. ercd_admin_settings() instructions have no support for translations (t()).
2. functions in ercd.admin.inc not properly documented, e.g. ercd_build_settings_reference_tree() parameters.

Recommendations

1. In ercd_uninstall() you could as well directly delete all variables instead of iterating through the result.
2. Having large chunks of markup in translated strings is not a good idea.

manual review:

1. "$field_name}['und']": do not use 'und', use the constant LANGUAGE_NONE instead.
2. ercd_get_referencing_child_ids(): why do you use check_plain() here? This seems like an API function and sanitization should be done as close as possible/useful to the actual output. If the ids are never printed they don't have to be sanitized, and the data should be unaltered when it is passed around internally.
3. "$_SESSION['ercd_selections'] = $selections;": why do you need that? I think hook_entity_delete() does not make sense in your use case, just use a regular form submit handler where you delete stuff. Then you don't have to mess around with session variables.
4. " '@referenceType' => check_plain($reference_type),": the "@" placeholder will already sanitize the variable for you and double escaping is bad.
5. "t($entity_info['label'] . ': ' . $entity_info['bundles'][$reference_entity['bundle']]['label']),": do not concatenate strings in t(), use placeholders instead.
6. Javascript: "ercd_set_message($container, 'Entities Selected');": use Drupal.t() to translate the messages, see http://drupal.org/node/323109
7. VERSION.txt: why is that needed?

But that are not major application blockers, so I guess this is RTBC. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Formatting adjustment.

manual review:

• ercd_install()): do not assume that the administrative role has role ID 3! I think there is a variable that you can retrieve to get the administrative user role ID. And you should use the anonymous role constant instead of 1.

Back to RTBC.

no other objections for more than a week, so ...

Thanks for your contribution, atozstudio!

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, 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 to the dedicated reviewer(s) as well.