[D7] Drupal DDP

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

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.

@bfodeke,

Please start by fixing up the issues mentioned here:

http://pareview.sh/pareview/httpgitdrupalorgsandboxbfodeke2354859git

Looks good bfodeke.

For whom it may concern, I have been reviewing and working with bfodeke on this project. I think he is good to have access to promote this project to a full project. Most of the comments I would have here, were taken care of during my mentorship of bfodeke, prior to his application here. He has done quite a bit of work and made changes identified.

Thanks!
shrop

Fixing title and category.

Hi Bayo,
I'm reviewing the code (upto bcc45879daf0a15c3d), and unfortunately it has a security issue that needs to be addresses before we proceed with this application.

The admin/config/development/drupal-ddp path is not protected by any access control mechanism. I can see you have defined the access drupal ddp permission in a hook_permission, but it is not used in the hook_menu. This results anyone can access the administration page and make any change they want.

I'm adding the PAReview: Security tag for statistical and educational purposes. Once this is fixed, and any other bugs you might come across are fixed, please set the status back to "Needs review".

Automated Review

http://pareview.sh/pareview/httpgitdrupalorgsandboxbfodeke2354859git
These definitely need to be fixed up.

Manual Review

 

Individual user account

 

Yes: Follows the guidelines for individual user accounts.

 

No duplication

 

yes: does not Cause module duplication and/or fragmentation.

 

Master Branch

 

Yes: Follows the guidelines for master branch.

 

Licensing

 

Yes: Follows the licensing requirements.

 

3rd party assets/code

 

Yes: Follows the guidelines for 3rd party assets/code.

 

README.txt/README.md

 

No: Does not follow the guidelines for in-project documentation and/or the README Template.
Doesn't follow the template exactly. Not a blocker.

 

Code long/complex enough for review

 

Yes: Follows the guidelines for project length and complexity.

 

Secure code

 

Yes: Meets the security requirements

 

Coding style & Drupal API usage

 

[List of identified issues in no particular order. Use (*) and (+) to indicate an issue importance. Replace the text below by the issues themselves:
   

     
1. (*) Do not concatenate on translated strings!!(blocker because I am not sure if this is security issue or not)

     

2. Not entirely sure why there are two .js files? These could maybe become one?? Not sure

   

   

The starred items (*) are fairly big issues and warrant going back to Needs Work. Items marked with a plus sign (+) are important and should be addressed before a stable project release. The rest of the comments in the code walkthrough are recommendations.

 

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

Hi,

Did some manual review and resulted in the following:

1/ Remove console.log in Javascript files.

2/ drupal_ddp_sync_queue.inc

function _drupal_ddp_unsynced_content_form(array $form, array &$form_state, $key = 'nid', $type = 'node') {
  if (!isset($form_state['storage']['confirm'])) {

Add an extra isset to $form_state['storage'], it is possible that this is not set.

And some remarks recommended by some community members:
1/ Avoid writing complex code. Try to focus on having a cyclomatic complexity as low as possible.

2/ Try not to rewrite code that already can be made possible by making use of contrib modules.
For example in _drupal_ddp_unsynced_content_form, a table with sorting and a pager is created.

$result = db_select($table, 'd')
      ->fields('d')
      ->condition('sync_successful', 0, '=')
      ->extend('TableSort')
      ->orderByHeader($header)
      ->extend('PagerDefault')
      ->limit(50)
      ->execute()
      ->fetchAll();

If you would make use of the entity and views module, your code can be accomplished a lot easier and the support for your module would decrease enormously. You would then need to create the sync_successfull entity type and query it through views.

Please don't assign the review task to yourself. Please see this page about assigning ownership to issues.

Hi bfodeke! Glad to see you made https://www.drupal.org/project/drupal_ddp a full project. If you'd like to opt in to security coverage, please address nico.knaepen first two issues.

Apologies for the long, long gaps between reviews. Thanks for this impressive module!

I am closing the issue for the lack of response. Please re-open it when the reported issues are fixed.