cbovard [cbovard]

Hello, and thanks for applying for a CVS account. I am adding the review tags, and some volunteers will review your code, pointing out what needs to be changed.

As per http://drupal.org/cvs-application/requirements, the motivation message should be expanded to contain more details about the features of the proposed module, and it should include also a comparison with the existing solutions.
As reported in the requirements, the proposed module should not duplicate the work done in existing projects.

Just a little note on what reported on http://drupal.org/node/539608:

Module 'Foo' is just too complex so I have written a much simpler version. This is just straight duplication of functionality and applications like this will almost certainly be declined. It is worth remembering that many modules start out simple also and, over time, grow into more complex systems. If you have the know how to write a "simple version" from scratch rather than work out how a complex module works then maybe your effort would be better spent making the existing complex module easier to use (improve it's UI, write documentation, etc).

Thanks, had some problem with the module today

* warning: SimpleXMLElement::__construct() [simplexmlelement.--construct]: Entity: line 49: parser error : Entity 'rsaquo' not defined in .................../simple_tweet.module on line 204.
* warning: SimpleXMLElement::__construct() [simplexmlelement.--construct]:

in .................../simple_tweet.module on line 204.
* warning: SimpleXMLElement::__construct() [simplexmlelement.--construct]: ^ in .................../simple_tweet.module on line 204.

Will try the update now

btw. Why this module doesn't have own project page?

I can also see similar error on your site maximumjazz...

Warning: simplexml_load_string() [function.simplexml-load-string]: Entity: line 49: parser error : Entity 'rsaquo' not defined in /home/wicec/public_html/maximumjazzmarketing.com/wp-content/plugins/twitter-friends/twitter_friends.php  on line 322

Warning: simplexml_load_string() [function.simplexml-load-string]: <li class="first"><a href="http://twitter.com">Home &rsaquo;</a></li> in /home/wicec/public_html/maximumjazzmarketing.com/wp-content/plugins/twitter-friends/twitter_friends.php on line 322

Warning: simplexml_load_string() [function.simplexml-load-string]: ^ in /home/wicec/public_html/maximumjazzmarketing.com/wp-content/plugins/twitter-friends/twitter_friends.php on line 322

Warning: simplexml_load_string() [function.simplexml-load-string]: Entity: line 63: parser error : Entity 'copy' not defined in /home/wicec/public_html/maximumjazzmarketing.com/wp-content/plugins/twitter-friends/twitter_friends.php on line 322

I have notice this issue happen for logged users. It was working fine for a few days, I did not change nothing within module or my Twitter account. Cleared cache and installed new version dated 18 may. Instead of Tweeter block I can see errors:

warning: SimpleXMLElement::__construct() [simplexmlelement.--construct]: Entity: line 49: parser error : Entity 'rsaquo' not defined in website/modules/simple_tweet/simple_tweet.module on line 204.
warning: SimpleXMLElement::__construct() [simplexmlelement.--construct]: <li class="first"><a href="http://twitter.com">Home &rsaquo;</a></li> in website/modules/simple_tweet/simple_tweet.module on line 204.
warning: SimpleXMLElement::__construct() [simplexmlelement.--construct]: ^ in website/modules/simple_tweet/simple_tweet.module on line 204.
warning: SimpleXMLElement::__construct() [simplexmlelement.--construct]: Entity: line 63: parser error : Entity 'copy' not defined in website/modules/simple_tweet/simple_tweet.module on line 204.
warning: SimpleXMLElement::__construct() [simplexmlelement.--construct]: <li class="first">&copy; 2010 Twitter</li> in website/modules/simple_tweet/simple_tweet.module on line 204.
warning: SimpleXMLElement::__construct() [simplexmlelement.--construct]: ^ in website/modules/simple_tweet/simple_tweet.module on line 204.

Hmm just checked now and it is working fine so far... maybe there was a fault with Twitter?

I'm having an issue with this, the install is so simple that i'm hoping this is a common thing. I put the simple tweet block in play, and it just displays Thu, 01/01/1970 - 00:00 , i verified RSS feed and everything. Ideas?

Keep in mind this is not a project issue queue, nor a debugging session; the purpose of a CVS application is to verify if the code has security issues, uses the correct Drupal functions, follows the coding standards, and duplicates the work done in existing projects.

• 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. The version line needs to be removed from the .info file.

2. function simple_tweet_uninstall() {
     // this will delete any of the varables created in the variable table
     db_query("DELETE FROM {variable} WHERE name LIKE 'simple_tweet_%'");
     cache_clear_all('variables', 'cache');
   }
   
   

   Deleting Drupal variables using a query that matches any Drupal variable with a name that starts with the module name would remove also the Drupal variables of other modules.

3. Menu descriptions and titles, schema descriptions are not passed to t().

4. function simple_tweet_perm() {
     return array('administer simple tweet', 'see tweets');
   }
   
   

   The second permission should be renamed see simple tweets, or the first permission should be renamed administer tweets.

5. See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how the code should be formatted.

6.   $ch = curl_init($rss_url);
     curl_setopt($ch, CURLOPT_RETURNTRANSFER, TRUE);
     curl_setopt($ch, CURLOPT_HEADER, 0);
     //exec curl
     $gettwit = curl_exec($ch);
   
   

   Why isn't the code using drupal_http_request()?

7.     watchdog('simple_tweet', 'RSS Feed Error is: ' . curl_error($ch));
   
   

   Use a t()-placeholder.

8.   $text = preg_replace("#(^|[\n ])([\w]+?://[\w]+[^ \"\n\r\t< ]*)#", "\\1<a href=\"\\2\" target=\"_blank\">\\2</a>", $text);
     $text = preg_replace("#(^|[\n ])((www|ftp)\.[^ \"\t\n\r< ]*)#", "\\1<a href=\"http://\\2\" target=\"_blank\">\\2</a>", $text);
     $text = preg_replace("/@(\w+)/", "<a href=\"http://www.twitter.com/\\1\" target=\"_blank\">@\\1</a>", $text);
     $text = preg_replace("/#(\w+)/", "<a href=\"http://search.twitter.com/search?q=\\1\" target=\"_blank\">#\\1</a>", $text);
   
   

   Multiple calls to preg_replace() can be replaced by a single call.

9.   else {
       $form['#prefix'] = '<p>' . t('For more control over date presentation insert the !date module.', array('!date' => l('Date API', 'http://www.drupal.org/project/date', array('absolute' => TRUE))));
     }
   
   

   l() should not be used together with t(). See the documentation for t(), where this code is reported to be wrong:

   $output .= t('<p>Go to the @contact-page.</p>', array('@contact-page' => l(t('contact page'), 'contact')));
   

   The correct code to use is the following:

   $output .= '<p>'. t('Go to the <a href="@contact-page">contact page</a>.', array('@contact-page' => url('contact'))) .'</p>';
   

10.     '#field_prefix' => 'http://' . $_SERVER['SERVER_NAME'] . '/',
    
    

    There is a function that can be used to obtain the absolute URL of the front page.

11. function simple_tweet_admin_settings_submit($form, &$form_state) {
      if (!$form_state['values']['simple_tweet_rss_url']) { 
        form_set_error('simple_tweet_rss_url', t('Please Enter the Twitter Users RSS URL!'));
      }
      if (!$form_state['values']['simple_tweet_username']) { 
        form_set_error('simple_tweet_username', t('Please Enter your Twitter Username'));
      } 
    }
    

    A form submission handler doesn't validate the values passed in the form; that is the task of a form validation handler.

Please read the following links as this is very important information about CVS applications.

• Drupal.org has moved from CVS to Git! This is a very significant change for the Drupal community and for these applications.
• Please read Migrating from CVS Applications to (Git) Full Project Applications and Applying for permission to opt into security advisory coverage on how this affects and benefits you and the application process. In short, every user has now the permissions necessary to create new projects, but they need to apply for opt into security advisory coverage. Without applying, the projects will have a warning on projects that says:
  

  This project is not covered by Drupal’s security advisory policy.