[D7] 51Degrees.mobi Mobile Device Detector and Theme Switcher

change text

Issue Summary
-description-

attachments

your url

git clone http://git.drupal.org/sandbox/user/.git name_project

Drupal 7

Attachment Size
Web oficial xxx.xx KB

And

README.txt is missing, see the guidelines for in-project documentation.

You can check here: http://ventral.org/pareview/httpgitdrupalorgsandbox51degrees1629098git

Hi 51Degrees.mobi,

There's still a few outstanding PAReview issues:

http://ventral.org/pareview/httpgitdrupalorgsandbox51degrees1841688git

Your repository contains a master branch which should be deleted and a 7.x-1.0 branch which should be renamed to 7.x-1.x . 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.

Also, whilst possibly created by you the 51Degrees directory contains code that is not licensed under the GPL. All code hosted on Drupal.org must be GPL licensed: http://drupal.org/node/422996 . The Libraries API module is a recommended method for adding 3rd party dependencies without directly including the code on Drupal.org.

Hi Tom,

I feel I'd need to refer to an administrator such as Klausi on this issue. Whilst I know you've written the shared library in general 3rd party libraries shouldn't be hosted here - it means that project administrators such as yourselves would have to try and keep the drupal.org copy up to date at all times:

http://drupal.org/node/422996

I think the preferred solution would be for you to remove the shared library and ask users to copy it themselves from a location specified in your readme to sites/all/libraries. You can then use the Libraries API Module to load it. It also removes any need for your module hosted here on drupal.org to contain license information.

I'll ask for a second opinion here:

http://groups.drupal.org/node/157669

*accidental double post*

Manual review:

• fiftyone_degrees_platform_constants.php: When possible all your constant defines should be prefixed with your module name (e.g. RULES should be something such FIFTYONE_DEGREES_RULES). This helps to prevents definition conflicts between multiple modules.
   
• fiftyone_degrees_get_script(): This contains a large JS block. To ensure Drupal can cache your scripts on users machines you should use drupal_add_js() - you can also pass settings using Drupal.settings. See the Drupal Javascript API for more details.
   
• fiftyone_degrees_write_css(): Please seperate out your CSS - it can't currently be cached. You should output it using drupal_add_css(). This helps Drupal cache your CSS and can also be used sometimes to generate a combined CSS file for all modules.
   
• fiftyone_degrees_write_text(): This is used frequently, but does nothing apart from wrap t(). Additionally, it's sometimes called with non string literals. To ensure your module can be localised you must directly call t() with a string literal so that http://localize.drupal.org/ can pick up those strings. For example - t('This string should be localizable'). If you just want to ensure a string is escaped call check_plain().
   
• fiftyone_degrees_display_form(): Please use drupal_goto() instead of directly outputting header('location: ').
   
• fiftyone_degrees_write_theme_switcher(): You're writing a lot of HTML directly. The recommended manner for outputting HTML is to use theme(). You can then seperate out your HTML to a .tpl file.
   
• fiftyone_degrees.info: Please add a configure link - http://www.drupalcoder.com/blog/configure-links-on-modules-overview-page...
   

The list looks a lot, but you have a reasonably large module for review so it's likely for us to find a few problems. If you need a hand with the fixes let us know - we'll try and help when we can :-).

Can you please check\fix the above and set the status to 'needs review' once done.

Hi Tom,

• Use of theme(). Your module should implement the hook_theme() hook and provide a list of templates. You can then create .tpl files which calls to theme() will default to. This helps separate HTML output from the logic in your code. For example:
  

  $themes = array(
    'fiftyone_degrees_admin' => array(
      'template' => 'fiftyone_degrees_admin',
    )
  );
  
  return $themes;
  

  You can then create a fiftyone_degrees_admin.tpl.php file for your module:

  <div id="fiftyone_degrees_admin_panel">
  <form method="post">
  <a href="<?php print fiftyone_degrees_get_new_rule_url(); ?>"><?php print t('Add New Rule') ?></a>
  <?php print check_clean($my_variable); ?>
  etc...
  

  To use it, call theme():

  $variables = array(
    'my_variable' => 'foo',
  );
  theme('fiftyone_degrees_admin', $variables);
  

  Which should output your template. There's probably an argument for using the Forms API for outputting the form in your admin area - you might want to try that as an alternative. See here and here.
   

• fiftyone_degrees_get_script() - I'm happy to see you are now using Drupal.settings. However, you still have a large block of inline JavaScript - this isn't great for performance. You could move the JS in lines 30-125 to its own .js file and use drupal_add_js() to have the browser download this - the browser should then cache it and should it ever see a script reference to it again it'll get pulled from the users cache quickly.
   
• fiftyone_degrees_write_css() - Whilst you're using drupal_add_css() it's still inlined which isn't recommended for performance. You could move this css out into its own .css file which the browser can cache.
   
• Correct use of t() is required for localizability. At the moment the only call to t() is from fiftyone_degrees_write_text() - the localize.drupal.org scripts will see only that call and won't see any calls to your function. In other words, fiftyone_degrees_write_text('Add New Rule') won't be picked up whilst t('Add New Rule') would be if you used it.
   
  You also do pass non-string literals to fiftyone_degrees_write_text() - fiftyone_degrees_logic.php:515 contains "$options[$value] = fiftyone_degrees_write_text($value);" - you should use check_plain() for non-literals.

It's better :-). In general you're mixing a lot of generation of presentation level output with logic though - this makes it more difficult for people with design skills to contribute to your module either through updating templates or altering CSS. It's also not great to wrap calls to the Drupal API that don't add anything extra - other developers are used to seeing calls to the API rather than your wrappers and if they try and work with you they'll have to keep looking up what your functions really do.

As for bundling libraries unfortunately no clear answer ever came out, though I'd argue all but one reviewer said they felt code that is available elsewhere and that isn't Drupal specific shouldn't really be on d.o. Lots of modules do use the Libraries API - it also means if another module wants to use your shared library it's a simple case for them of calling library_load('fiftyone_degrees'). The bit of added pain for users installing your module by having to download and unzip an extra file can also be mitigated for drush users if you provide a .make file - they can just drush en fiftyone_degrees and have the required library downloaded automatically. Take a look again - if you get stuck I'd be happy to help you.

Could any other reviewers have a look at this? I'd also certainly appreciate some feedback on my reviews.

Closing due to lack of activity. Feel free to reopen if you are still working on this application.

If you reopen this please keep in mind that we are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)

Changed sandbox address