MoPublication

Hi chrisdawson,

first of all, remove "?>" PHP delimiter at the end of these files

./css/mopub_demo.php
./includes/mopub_demo.php
./xml/config_file.php
./xml/category_listing.php

use coder module to check errors ad warnings before pushing it to Git. (this module will help to meet drupal coding standards.)

thanks

Manual Review :

1) Please don't add LICENSE.txt, it will be added by drupal community after completing review process and full project release.

2) In hook_help(), please do not write coming Soon. If there is any help information, write it now else it is not necessary to use hook_help() function.

3) Use t() function in line no: 21 in install file.

Hi chrisdawson,

You also have to clean up your GIT - Repository - there is still a master branch in your project.
Have a look at http://drupal.org/node/1127732 for further information.

I also run coder_review on your project and found the following issues:

mopublication.module

Line 16: Missing parenthesis after function name
 * Implements hook_help
Line 30: Missing parenthesis after function name
 * Implements hook_menu

audio_listing.php
Line -1: @file block missing

category_listing.php
Line -1: @file block missing

config_file.php

Line -1: @file block missing (Drupal Docs)
Line 67: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
      echo isset($j) ? '|' : ''; $j = true;
Line 73: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
      echo isset($k) ? '|' : ''; $k = true;
Line 79: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
      echo isset($l) ? '|' : ''; $l = true;
Line 106: Control statements should have one space between the control keyword and opening parenthesis
    if(variable_get('mopub_ad_option') == 'admob') {
Line 134: Control statements should have one space between the control keyword and opening parenthesis
    if(variable_get('mopub_ad_option') == 'custom_ads') {
Line 136: do not use mixed case (camelCase), use lower case and _
      $iphoneAdImageSplash = variable_get('mopub_ad_ipad_splash_banner');
Line 137: Control statements should have one space between the control keyword and opening parenthesis
      if(!empty($iphoneAdImageSplash)) {
Line 137: do not use mixed case (camelCase), use lower case and _
      if(!empty($iphoneAdImageSplash)) {
Line 138: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
        echo "<a href='".variable_get('mopub_ad_ipad_splash_url')."'><img src='".$iphoneAdImageSplash."' /></a>";
Line 138: do not use mixed case (camelCase), use lower case and _
        echo "<a href='".variable_get('mopub_ad_ipad_splash_url')."'><img src='".$iphoneAdImageSplash."' /></a>";
Line 141: Control statements should have one space between the control keyword and opening parenthesis
    } else if(variable_get('mopub_ad_option') == 'ad_server') {
Line 141: else statements should begin on a new line
    } else if(variable_get('mopub_ad_option') == 'ad_server') {
Line 141: Use "elseif" in place of "else if"
    } else if(variable_get('mopub_ad_option') == 'ad_server') {
Line 200: else statements should begin on a new line
  } elseif (variable_get('mopub_ad_option') == 'ad_server') {

node_detail.php

Line -1: @file block missing (Drupal Docs)
Line 51: Control statements should have one space between the control keyword and opening parenthesis
        <?php foreach($comments as $comment_id) :?>

node_listing.php

Line -1: @file block missing (Drupal Docs)
Line 22: in most cases, replace the string function with the drupal_ equivalent string functions

tag_listing.php
Line -1: @file block missing

video_listing.php
Line -1: @file block missing

mopub_demo.php

Line 25: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
            $class_inactive = true;
Line 36: Functions should be called with no spaces between the function name and opening parentheses
               style="display: <?php echo (variable_get('mopub_ad_option') == 'none') ? 'none' : 'block'; ?>">
Line 67: in most cases, replace the string function with the drupal_ equivalent string functions
                <h3><a><?php echo substr($node->title, 0, 55); ?></a></h3>
Line 70: in most cases, replace the string function with the drupal_ equivalent string functions
                  <?php echo substr($node->body['und'][0]['value'], 0, 75); ?>

mopub_help.php

Line 19: use <br /> instead of <br>
<p><b>A note on Taxonomy</b><br>
Line 22: The $text argument to l() should be enclosed within t() so that it is translatable.
<?php echo l('Taxonomy help', 'admin/help/taxonomy'); ?>.</p>
Line 24: use <br /> instead of <br>
<p><b>Audio and Video tabs</b><br>

mopub_service.php

Line 14: Control statements should have one space between the control keyword and opening parenthesis
  if( empty($_POST['comment_post_ID']) ||
Line 18: curly braces { should end a line, not start one
  {
Line 23: Potential problem: use the Form API to prevent against CSRF attacks. If you need to use $_POST variables, ensure they are fully sanitized if displayed by using check_plain(), filter_xss() or similar. (Drupal Docs)
  if ( ! node_load($_POST['comment_post_ID']) ) {
Line 33: Potential problem: use the Form API to prevent against CSRF attacks. If you need to use $_POST variables, ensure they are fully sanitized if displayed by using check_plain(), filter_xss() or similar. (Drupal Docs)
  $comment_body[LANGUAGE_NONE][0]['value'] = $_POST['comment_content'];
Line 49: Potential problem: use the Form API to prevent against CSRF attacks. If you need to use $_POST variables, ensure they are fully sanitized if displayed by using check_plain(), filter_xss() or similar. (Drupal Docs)
    $comment['homepage'] = $_POST['comment_author_url'];</pre>

<strong>mopub_settings_form.php</strong>
<pre>Line 152: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
        file_usage_add($file, 'mopublication', 'variable:'.$field, $value);
Line 158: curly braces { should end a line, not start one
  {

mopub_settings_form_1.php

Line 25: There should be no trailing spaces
    '#field_suffix' => '<span id="appNameStatus"></span>', </pre>

<strong>mopub_settings_form_2.php</strong>
<pre>Line 56: The $text argument to l() should be enclosed within t() so that it is translatable.
      l('Set up Taxonomies', 'admin/structure/taxonomy') . '.',
Line 66: The $text argument to l() should be enclosed within t() so that it is translatable.
      l('Set up Taxonomies', 'admin/structure/taxonomy') . '.',
Line 80: There should be no trailing spaces
    } 
Line 116: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
      'required' => true,
Line 122: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
      'required' => true,
Line 157: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
  $form['content']['mopub_tabs']['#tree'] = true;
Line 164: in most cases, replace the string function with the drupal_ equivalent string functions
    $tab_id = strtolower($tab['name']);
Line 167: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
      $tab['checked'] = true;
Line 168: else statements should begin on a new line
    } else {
Line 170: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
        $tab_vars[$tab_id]['tab_' . $tab_id . '_checked'] == true ) ? true : false;
Line 300: Use uppercase for PHP constants, e.g. NULL, TRUE, FALSE
  if ( empty($disqus_default) && variable_get('disqus_domain', false) ) {
Line 337: curly braces { should end a line, not start one
{

mopub_theme.php

Line 8: Missing parenthesis after function name
 * Implement hook_theme
Line 65: String concatenation should be formatted with a space separating the operators (dot .) and the surrounding terms
    $output .= '<div><input type="radio" id="' . $radio['#id'] . '-' . $key .'" ';

mopub_validate.php

Line 47: curly braces { should end a line, not start one
{

mopub_xml.php

Line 115: else statements should begin on a new line
  } else {
Line 154: else statements should begin on a new line
  } else {

mopublication.install

Line 8: Missing parenthesis after function name
 * Implement hook_install
Line 24: Potential problem: drupal_set_message() only accepts filtered text, be sure all !placeholders for $variables in t() are fully sanitized using check_plain(), filter_xss() or similar. (Drupal Docs)
  drupal_set_message(st('MoPublication has been activated - go ahead and !link.',
Line 30: Missing parenthesis after function name
 * Implement hook_disable
Line 38: Missing parenthesis after function name
 * Implement hook_uninstall

For later code-review you can use http://ventral.org/pareview or the coder-review module..

SteffenR

Hi,

My last PAReview: http://ventral.org/pareview/httpgitdrupalorgsandboxchrisdawson1845748git
only complains about : The "?>" PHP delimiter at the end of files is discouraged, see http://drupal.org/node/318#phptags
in 9 files.

Please correct this. It is quite simple to do :-)

git clone line in the description: prefer to usegit clone http://git.drupal.org/sandbox/chrisdawson/1845748.git mopublication

Coder only returns me 1 critical alert concerning a drupal_set_message() that should be sanitized in .install file.

Line 24: Potential problem: drupal_set_message() only accepts filtered text, be sure all !placeholders for $variables in t() are fully sanitized using check_plain(), filter_xss() or similar. (Drupal Docs)
  drupal_set_message(st('MoPublication has been activated - go ahead and !link.',

I agree this looks like a false positive despite the critical status.

Why is there an UPGRADE.txt file while there is no upgrade to do? You might decide to erase it.

There is a lot of code here, and I did not have reviewed every single lines but I feel weird that you load systematically 5 includes files at the beginning of .module file, while they should have been attached to hook_menu() entries. This limits the size in memory.

Concerning the way you generate feeds: I guess it does the job, but Drupal hook_theme() and template best practices are not respected. As a consequence, it won't be possible to override these templates without hacking your module :-(
This is a real problem from my point of view, event if PHP code do the job.
I invite you to consider this for next version.
You might consider integrating with Views: when generating list in Drupal, this is a well appreciated approach. Code already exists to generate XML formatted data. See: http://drupal.org/project/views_data_export

Hope this will help.
Regards,

Alexandre

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 :-)

Maybe the "The "?>" PHP delimiter at the end of files is discouraged," error is because of two line empty lines at the end of the files. Normally there should only be one.

mopublication.install:
- Line 44: Would be better to use variable_del()
- Line 45: Would be better to use cache_clear_all, but you don't need to do this, when using variable_del instead

mopublication.module:
- Don't add jQuery to your module. Create a dependency to jquery_update if you require a newer version of jQuery. Also check Libraries API how to use external jQuery plugins.
- Line 129: This setting is already available - Drupal.settings.basePath
- Line 149: Unused variable $path
- Line 154: You could use drupal_add_css to add css from the file and add it as style tag.
- Line 155: Here you have to use a theme function. Everything in mopub_demo.php is theming related and should be changeable by users. mopub_demo_content.php can be moved into that function, then call the theme, which uses the mopub_demo.php as external file.

mopublication.validate.php:
- Line 49: Use file_prepare_directory() instead of doing this yourself
- You can use file_save_upload with a destination too, so you don't need to copy afterwards. Basiclly get the destination with prepare_directory, save_upload with that destination, set status to permanent and do a save.

mopub_xml.php:
- You don't have to die at the end of each function.

xml/*
- Maybe the theme functions could be used for these. So people could overwrite it, when they want to change small parts.

helper_functions.php:
- Line 40: Missing return documentation
- Line 56: Missing type and comment of param (documentation)
- Line 57: Missing type and comment of param (documentation)
- Line 58, 75: There should be a free line between param and return (documentation)

app_store_options.php:
- mopublication_self_keyed_array == drupal_map_assoc()

Okay, I quickly got through again and it seems the requested changes was made.

Only one thing would be to check out jquery_update, instead of including jQuery by your self. So you easily could add a dependency to jQuery update.

Also it worth a look into libraries modules for the miniColors jQuery Plugin. Adding not-self written third-party code to modules isn't good.

We do not want to host third-party libraries on drupal.org for a multitude of reasons, starting with licensing, proceeding to different release cycles, and not necessarily ending with fatal errors due to conflicts of having the same library installed in multiple versions.

But I think the rest is fine and both described problems shouldn't be blockers, so setting to RTBC.

If I'm seeing that right you pushed "jquery-1.8.2.min.js" - please depend on JQuery update to do this instead.

Also as kfritsche said, your repository shouldn't contain external libraries when you could make use of the libraries module for this instead.

Please fix these issues first before we continue here

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

I'm a robot and this is an automated message from Project Applications Scraper.