Media Issuu

welcome,

As installation and usage instructions are quite important for us to review, please take a moment to make your project page follow the tips for a great project page. Also make sure your README.txt follows the guidelines for in-project documentation.

Automated tools don't throw any errors, well done!

Please make sure your always using /**/ comments in global spec (outside of functions).

Please don't call drupal_get_path() so often, rather save it in a variable temporary.
Also you can replace this
include_once DRUPAL_ROOT . '/' . drupal_get_path('module', 'media_issuu') . '/includes/media_issuu.variables.inc';with
include_once dirname(__FILE__) . '/includes/media_issuu.variables.inc';

Remove your .gitignore from git (git rm .gitignore) and add it to itself.

We do really need more hands in the application queue and highly recommend to get a review bonus so we can come back to your application sooner.

regards

1. Some errors from parview:

FILE: ...dules/pareview_temp/test_candidate/includes/MediaIssuuStreamWrapper.inc
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
13 | ERROR | Line indented incorrectly; expected 2 spaces, found 3
--------------------------------------------------------------------------------

2. You should put the element of array in new line if the line is longer than 80 chars:

'description' => t('Issuu Styles will display embedded Issuu documents and thumbnails to your choosing, such as by resizing, setting colors. You can !manage.', array('!manage' => l(t('manage your Issuu styles here'), 'admin/config/media/media-issuu-styles'))),

3. Please follow the coding standards: http://drupal.org/node/318

The Drupal Code Sniffer finds a bunch of issues. I suggest getting a clean report from that before requesting another review.

media_issuu_install() does nothing and be be left out.

In media_issuu_uninstall(), what is the purpose of the call to drupal_load()? It's already loaded if you are uninstalling it.

In media_issuu.module some comment lines begin and end with /* and */. Use a regular comment block for these.

Licensing: No problems noted.

Have you looked at the Issuu integration module? There may be some overlap. Your module could potentially depend on that one to reduce duplication.

Looks good. I haven't tested that it works, just that the code appears sound. I am presuming that you're making sure that the module works as it should.

Minor security issue in MediaIssuuStreamWrapper.inc:

  public function getOriginalThumbnailPath() {
    $issuu_properties = self::getIssuuProperties($this->parameters);
    return 'http://image.issuu.com/' . $issuu_properties['documentId'] . '/jpg/page_1_thumb_medium.jpg';
  }

getIssuuProperties() does not check that the value it returns is a string safe for including in a URL. It will be unless issuu.com is returning stuff that it shouldn't, but the getIssuuProperties() should check the value anyway, just to be sure. A simple regex check should be fine for this. Similar to the principle of not trusting user input, we shouldn't trust the values returned by outside services.

It's not a Drupal coding standard, but I prefer to use === instead of ==. If casting is needed, I would rather do the casting myself so that I know exactly what comparison is being made.

You can probably determine a regex for what a issuu documentId is supposed to contain.

Module duplication is a huge problem on drupal.org, and I really think this effort should go into the existing issuu module. Please get in contact with the maintainer to discuss, to get access to the repository and to develop from there. If you cannot reach the maintainer please follow the abandoned modules process: http://drupal.org/node/251466

If that does not work out for whatever reason and you absolutely must develop your own fork of issuu please come back here and set this application back to "needs review".

awesome! thank you!

Added reviews