Edge Compositions

Hello and Welcome,

Thanks for the module. The code is clean and easy to understand.

Automatic review

http://ventral.org/pareview/httpgitdrupalorgsandboxti2m1615254git-7x-1x

Only minor warnings regarding comments. No problem.

Manual review

- The use of $project_name in file paths looks unsecure. I think you should add a helper function to sanitize the project name to make it suitable for use in paths and html attributes.

- You create a directory to store sources in public:// and set it inaccessible using htaccess. It makes the module dependent of apache. Why not create the directory in private://

- In edge_compositions.module edge_compositions_admin_form: though fine as it is, you may want to use theme_table for the overview table.

- Regarding javascript. In edge_compositions.module edge_compositions_comp_render, you may want to use drupal_add_js to add the AdobeEdge variable to Drupal.settings and in edge_drupal.js add a Drupal.behaviors and then access this variable and do what you need to do with it (like copying the properties to window.AdobeEdge).

  drupal_add_js(array(
    'AdobeEdge' => array(
      'pathPrefix' => array(
        'libs' => $path,
        'comps' => array(
          $edge_comp_id => $path . "/" . drupal_strtolower($project_name),
        ),
      ),
    ),
  ), 'setting');

That's all for now. Could you provide a composition for testing?

Thanks

Added Drupal version

Hello,

- There is a displayed bug when I upload a zip file.

- It is better to add a description indicating the extensions files that we can upload (zip, tar, gz) in the form.

- Why does this function edge_compositions_admin_form_submit() is declared and not implemented?

Hey ti2m

I just finished a manual review of the functionality.

I thought it would be a good test to try out the ready-made examples that Adobe offers on their Edge website.
These where the Edge packages that I tested:

• Bubbleocity: http://edge.adobe.com/resources/bubbles/edge_sample_bubbles.zip
• Mobile B. Marketshare: http://edge.adobe.com/resources/mobilebrowser/edge_sample_mobilebrowser.zip
• Get Some Perspective: http://edge.adobe.com/resources/perspective/edge_sample_perspective.zip
• Adobe MAX: http://edge.adobe.com/resources/maxdemo/edge_sample_maxdemo.zip
• Getting Jumpy 2: http://edge.adobe.com/resources/gettingjumpy2/edge_sample_gettingjumpy2.zip
• Ninja: http://edge.adobe.com/resources/ninja/edge_sample_ninja.zip

All of these demos seem to work perfectly. The interface is clear, I can immediately test my uploaded Edge.
The rebuild and delete just function as they should.
The blocks become available immediately and also work as they should.

There are only 2 things that caught my attention:

1. The testing of the Edge should maybe happen in a JS model or a popup (though popups are evil, I know...) so the user does not leave her/his current screen.
2. I was a bit confused about the severity of the status messages I got after uploading an Edge zip file:
   

   edge.0.1.6.min.js library already existed and was overwritten!
   json2_min.js library already existed and was overwritten!
   jquery-1.7.1.min.js library already existed and was overwritten!
   jquery.easing.1.3.js library already existed and was overwritten!
   Edge_sample_gettingjumpy2.edge was ignored and was NOT copied to the server.
   Edge_sample_gettingjumpy2.html was ignored and was NOT copied to the server.
   

   Maybe also tell the user that this is not a problem, just a status report?

Good job Ti2m!

The module seems to work out-of-the-box, as advertised without any setup hassle.
It gets a big thumbs up from me...!

Cheers
T

Add reviews

Et voila, as stated above...looks fine to me!

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

• Drupal Code Sniffer has found some issues with your code (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. You have to get a review bonus to get a review from me.

manual review:

1. "admin/structure/edge/%/rebuild": this path is vulnerbale to CSRF. You should have some sort of token or confirmation form to not execute actions on GET requests. See http://drupal.org/node/178896
2. "require_once 'edge_compositions_builder.inc';": classes are autoloaded in Drupal 7 with a files[] entry in the module's info file. Remove those lines and add edge_compositions_builder.inc to the info file. See http://drupal.org/node/542202
3. edge_compositions_comp_render(): do not inject script tags. Use Drupal.settings with drupal_add_js() to pass configuration settings to Javascript. See http://drupal.org/node/756722
4. edge_compositions_comp_create(): use drupal_write_record() instead of db_insert() and you get schema validation for free.

The CSRF issue is a blocker, so I have to bump this back to "needs work". Otherwise this looks pretty ready. Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

Thanks for your contribution, ti2m! Welcome to the community of project contributors on drupal.org.

I've granted you the git vetted user role which will let you promote this to a full project and also create new projects as either sandbox or "full" projects depending on which you feel is best.

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.

As you continue to work on your module, keep in mind: Commit messages - providing history and credit and Release naming conventions.

Thanks to the dedicated reviewer(s) as well.

No, downloading files is not a problem as they are not hosted on drupal.org.

Timm's project is now a full-fledged awesome module. Check it out: http://drupal.org/project/edge_suite

Added review ref