[D7] Content Dependency

There are some errors reported by automated review tools, did you already check them? See http://ventral.org/pareview/httpgitdrupalorgsandboxerez1111868890git

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.

Check this: http://ventral.org/pareview/httpgitdrupalorgsandboxerez1111868890git
And solve the reported errors/warnings.

It appears you are working in the "master" branch in git. You should really be working in a version specific branch. The most direct documentation on this is Moving from a master branch to a version branch. For additional resources please see the documentation about release naming conventions and creating a branch in git.
See http://drupal.org/project/1976520/git-instructions
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

First of all solve the suggested changes in #2 and #3.

I can still see the issues reported in #2 and #3, see the attached screenshot.

You still have some warnings: http://ventral.org/pareview/httpgitdrupalorgsandboxerez1111868890git

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

./README.txt: the byte order mark at the beginning of UTF-8 files is discouraged, you should remove it.
Coder Sniffer has found some issues with your code (please check the Drupal coding standards).

FILE: /var/www/drupal-7-pareview/pareview_temp/content_dependency.api.php
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
14 | WARNING | Hook implementations should not duplicate @param documentation
26 | WARNING | Hook implementations should not duplicate @param documentation
--------------------------------------------------------------------------------

FILE: ...var/www/drupal-7-pareview/pareview_temp/includes/content_dependency.inc
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
161 | WARNING | Only string literals should be passed to t() where possible
164 | WARNING | Only string literals should be passed to t() where possible

I installed the module and went to a piece of content that already existed, went to the content dependencies tab and the page was totally blank. I could still see the tabs/top menu but the content of the tab is all white. I created a new content type and new content to see if it would work but I get the same thing.

I re-installed the module, and I appreciate the message instead, but it is not entirely clear to me how I actually set up a content dependency.

"That's it! Now, you can see what module does, just enter to your domain

path"/node/NUMBER/edit" and you see "Content Dependency" tab

(just verify you have some entity reference and/or node reference

that refer to this entity (node)."

In the README.txt is a tad confusing as I thought it implied the Content Dependency tab was where you configured the dependencies. I think you should make it a step to add a entity reference field (just to be clear).

--

In content_dependency.inc you have a lot of double, and triple nested foreach loops. I am wondering if there is away to simplify these, possibly through recursion or through changing data structures? Not a deal breaker, but when I see a bunch of nested loops I try and think about how I could optimize it.

--
content_dependency.class line 294 is this supposed to be $key2? On that note, perhaps you could sue a little more clear key/value names. What does value2 represent?

Hi erez111,

define('CONTENT_DEPENDENCY_FIELD_ARRAY_NAME_KEY', 'name');
// Label of field_array name key.
define('CONTENT_DEPENDENCY_FIELD_ARRAY_TYPE_KEY', 'type');
// Label of field_array type key.
define('CONTENT_DEPENDENCY_NEW_CONTENT_ANCHOR_TARGET_TYPE', '_blank');
// Anchor target type of new content (_blank,_self,_window, etc.).
define('CONTENT_DEPENDENCY_NEW_CONTENT_ADD_NO_ENTITY_OUTPUT', TRUE);
/* Boolean. Define whether to show add new content link
    when no dependency content exists. */
define('CONTENT_DEPENDENCY_DISPLAY_NO_ENTITY_BLOCK', TRUE);
/* Boolean. Determine whether to display or not content dependency
    block when there are no entities. */

1. Use comments above line of code
2. Use inline comments

/**
 * Implements hook_init().
 */
function content_dependency_init() {
  drupal_add_js(array('arg' => arg()), 'setting');
  module_load_include('class', 'content_dependency', 'includes/content_dependency');
  module_load_include('inc', 'content_dependency', 'includes/content_dependency');
}

Why you include files in hook_init()? Use place below constants.

$value = "'" . $value . "'";
Use $value = (string) $value;

  if (CONTENT_DEPENDENCY_DISPLAY_NO_ENTITY_BLOCK === FALSE &&
    empty($entity_type_array_categories)) {

    return FALSE;
  }

What is it?:) Why you compare constant with FALSE?
Both static, result always the same.

content_dependency_execute_hook_filter_entities_edit_build()
and content_dependency_execute_hook_filter_entities_edit_alter()

May be use module_invoke_all?

Issue priority is normal, see http://drupal.org/node/539608

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

• ./README.txt: the byte order mark at the beginning of UTF-8 files is discouraged, you should remove it.

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.

manual review:

1. hook_content_dependency_filter_entities_edit_build(): doc block is wrong, the first line should describe the hook. See http://drupal.org/node/1354#functions and hook definitions.
2. content_dependency.api.php: examples in the function bodies would be useful.
3. content_dependency_permission(): permission keys are usually all lower case.
4. content_dependency.info: if the dependencies are not needed you should remove those lines.
5. content_dependency.class: please use the *.php or *.inc file extension.
6. content_dependency_get_content_type_details(): do not concatenate variables directly into query strings, use placeholders with db_query() instead. Please read http://drupal.org/writing-secure-code again.
7. The content dependency tab is vulnerable to XSS exploits. If I have a node with the title <script>alert('XSS');</script> which has a reference to another node, then I will get a nasty javascript popup. You need to sanitize user provided text before printing, please read http://drupal.org/node/28984 again. And please don't remove the security tag, we keep that for statistics and to show examples of security problems.
8. content_dependency_get_entity_types_arrays_html(): The HTML layout should be rendered in a theme function, so that it can be overridden easily.
9. content_dependency_filter_entity_id_array_edit_permission(): entity_access() is an Entity API function, so you need a dependency to the entity module.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Seems it's my mistake, sorry:

Use inline comments

in 17#

erez111,

1) why you set status review bonus each time? For this you must check another 3 projects!
2) why you set major status? please see http://drupal.org/node/539608

please don't remove the security tag, we keep that for statistics and to show examples of security problems. Please add all your reviews to the issue summary so that we can track them.

manual review:

1. "Stable version14" is not a useful commit message, please read http://drupal.org//node/52287
2. buildEntityIdQuery(): why do you have to build the query yourself and cannot use entity field query?
3. "module_load_include('class', 'content_dependency', 'includes/content_dependency');": that file does not exist anymore? Also, why do need to include the fiel if you are already in the file?
4. getMainContentHtml(): do not use jQuery(document).ready(), use Drupal.behaviors instead. See http://drupal.org/node/756722 . And put the JS in a dedicated file, then it is easier to read and maintain.
5. "check_plain(mysql_real_escape_string($record->name));": what is this mysql_real_escape_string() function call used for? That output is never used in a query?
6. content_dependency_get_entity_types_arrays_html(): there is still markup in this function. You should prepare and array of variables here, all HTML should in a theme function and/or template.
7. content_dependency_get_entity_types_arrays_html(): do not create link markup yourself, use l() instead. And you could use theme('item_list', ...).
8. "t('You can') . ' <a href="/node/' . $entity_id . '/edit">' . t('click here') . '</a> ' . t('to edit current') . ' ' . t($entity_type)": Do not concatenate parts of a sentence like that, use placeholders with t() instead. And the link markup will fail if Drupal is installed in a subdirectory, always use url() or l() to create links.

Although you should definitively fix those issue I don't think they are critical application blockers, so I would say RTBC. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Assigning to chx as he might have time to take a final look at this.

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

Thanks for your contribution, erez111!

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.

Just leave it at fixed, it will close automatically after 2 weeks.

Sorry for not getting here earlier. I think a quick (==not really through) review is still useful. I usually dislike constants for the sake of having constants. Does CONTENT_DEPENDENCY_FIELD_ARRAY_NAME_KEY give you any value over 'name'? I doubt. Also, using CONTENT_DEPENDENCY_DISPLAY_NO_ENTITY_BLOCK as a ... what? variable? is simply broken, it's a constant, you shouldn't expect people to edit the module. Rather use variable_get('content_dependency_... and then it can be overridden from settings.php.

The class is unnecessarily verbose. For example, getEntityTypesArray should be just getEntityTypes. fullTargetEntityTypesField or content_dependency_execute_hook_filter_entities_edit_build is some Java-like naming. We do not need AbstractSingletonProxyFactoryBean kind of naming, do we :) ?

For getCurrentNodeContentType I highly recommend menu_get_object.