czingerline [czingerline]

Hello, and thanks for applying for a CVS account.

You still need to upload the archive containing the code in a comment here.

What is your reply to what reported by sun?

lastrun.php seems to duplicate includes/lock.inc and/or cron in Drupal core?

Most of the classes in this module more or less seem to be wrappers around simple database queries. AFAIK, there are not many OOP developers active in the CVS applications review process, and this usage of OOP additionally looks a bit questionable to me (which doesn't hold off this application in any way though), so this is very likely one major cause for the delay.

• The points reported in this review are not in order or importance / relevance.
• Most of the times I report the code that present an issue. In such cases, the same error can be present in other parts of the code; the fact I don't report the same issue more than once doesn't mean the same issue is not present in different places.
• Not all the reported points are application blockers; some of the points I report are simple suggestions to who applies for a CVS account. For a list of what is considered a blocker for the application approval, see CVS applications review, what to expect. Keep in mind the list is still under construction, and can be changed to adapt it to what has been found out during code review, or to make the list clearer to who applies for a CVS account.

1. License files cannot be committed in Drupal.org repository. Projects committed in Drupal.org repository have the same license used by Drupal.
2. The version line needs to be removed from the .info file.
3. t() is available when hook_install(), or hook_uninstall() are invoked. See the code executed from php_install() as example.

4.   include_once(dirname(__FILE__) . '/classes/intervals.php');
   
   

   There is a Drupal function to load files containing PHP code.
   Included files should have extension .inc, not .php.

5. Menu descriptions and titles, schema descriptions are not passed to t().
6. Functions that are implementations of hooks should have a comment that reports that.
7. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how the code should be formatted.

8. "For help, look at the <a href='!basic'>basic stylesheet</a>, which turns the
   
   

   The correct placeholder for URLs is prefixed by @.

9. The module doesn't implement hook_uninstall().

This is only a partial review.

1. // $Id$
   /*
    * Drupal Most Popular - Showcase the most popular content across your Drupal website and engage your audience.
    * Copyright © 2010 New Signature
    * 
    * This program is free software: you can redistribute it and/or modify it under the terms of the GNU General Public License as published by the Free Software Foundation, either version 3 of the License, or (at your option) any later version.
    * This program is distributed in the hope that it will be useful, but WITHOUT ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for more details.
    * You should have received a copy of the GNU General Public License along with this program.  If not, see <http://www.gnu.org/licenses/>.
    * You can contact New Signature by electronic mail at labs@newsignature.com –or- by U.S. Postal Service at 1100 H St. NW, Suite 940, Washington, DC 20005.
    */
   module_load_include('php', 'mostpopular', 'mostpopular.api');
   

   Any reference to the license should be removed from the code.
   The copyright statement would not be anymore exact from the moment you apply patches provided by users; to make an example, Drupal core code is not stated to be copyrighted from Dries Buytaert (it is copyrighted by all the contributors).

   It usually not a good idea to unconditionally include code contained in PHP files. If the files contain form handlers, theme functions, or menu callbacks, Drupal core code allows to third-party modules to define which files must be loaded in the implementations of hook_menu(), and hook_theme(); in these cases, Drupal loads the file only when necessary.

2.           global $is_page_authcache;
   
   

   Is the variable defined by this module?

3.                 drupal_set_message(t('You have an invalid cookie for the most popular service'), 'error');
   
   

   Messages strings should end with a period.

4.   public function MostPopularInterval($object) {
       $this->update($object);
     }
   
   

   PHP5 class constructor is __construct(), which is the one reported by the coding standards.

5.   public static $table = 'mostpopular_intervals';
   
   

   If the content of the variable is not changed, why isn't the code using a class constant?

6. The namespace respect is also valid for class names; AddThis is the name of a class that the module addthis.module would use.

7.           include_once(drupal_get_path('module', 'mostpopular_addthis') . '/mostpopular_addthis.classes.inc');
   
   

8.           $sql = "
               SELECT DISTINCT n.nid, n.title, c.totalcount
               FROM {node} n left join {node_counter} c on n.nid = c.nid
               WHERE n.status = 1 and n.moderate = 0 and n.created >= %d
                 AND c.totalcount >= 1 and title not like '%page not found%'
               ORDER BY c.totalcount desc, n.created desc";
   
   

   Reserved SQL keywords are written in uppercase.

9.         '#description' => l(t('Click here to authenticate and select a Google Analytics site to monitor.'),
             'admin/settings/google-analytics-api', array(
               'query' => drupal_get_destination()
             )),
   
   

   That code is similar to the code that in the documentation of t() is reported as wrong.
   the function l() should not be used in these cases.

10.   $form['block']['mostpopular_styling'] = array(
        '#type' => 'radios',
        '#title' => t('Stylesheet'),
        '#description' => '<p>' . t(
    "Choose how much styling to apply to the Most Popular block.
    You can add additional styling in your own theme.") . '</p>' .
          '<p>' . t(
    "For help, look at the <a href='@basic'>basic stylesheet</a>, which turns the
    service and interval links into tabs, and the <a href='@full'>full stylesheet</a>,
    which adds fonts, colors, formatting, and layouts.", array(
            '@basic' => url(drupal_get_path('module', 'mostpopular') . '/css/mostpopular-basic.css'),
            '@full' => url(drupal_get_path('module', 'mostpopular') . '/css/mostpopular-full.css'),
          )) . '</p>' .
          '<p>' . t(
    "When creating your own styles for the Most Popular block, we recommend you
    start with our full stylesheet and override it using drupal_set_css().") . '</p>',
    
        '#options' => array(
          MOSTPOPULAR_STYLE_NONE => t('No styling'),
          MOSTPOPULAR_STYLE_BASIC => t('Basic styling'),
          MOSTPOPULAR_STYLE_FULL => t('Full styling'),
        ),
        '#default_value' => variable_get('mostpopular_styling', MOSTPOPULAR_STYLE_FULL),
      );
    
    

    That code could be formatted better.
    There are other code lines that are not formatted as suggested by the coding standards (e.g., using a space before, or after the parenthesis in a function call).

11.     '#description' => t(
    "<p>These Drupal pages will be excluded from the most popular results for any
    services that return nodes.  The homepage will automatically be excluded, but
    you can use this field to hide landing pages or other non-content pages.</p>
    <p>Each URL should be an internal Drupal path with no leading slash, and can
    point either to node/%d or to an alias.   Put each path on a separate line.</p>"),
    
    

    Avoid to include HTML tags in strings to translate; that doesn't mean to split a sentence in phrases, anyway. In this case, as the <p> tag is at the beginning of the string, it's

Thank you for your contribution! I am going to update your account.
These are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find more contributors chatting on the IRC #drupal-contribute channel. So, come hang out and stay involved.
Thank you, 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.

I thank all the dedicated reviewers as well.