stripped your speech [gh0st25]

Files available from third-party sites should not be included in Drupal.org CVS.
In Drupal.org CVS the only accepted files are the ones licensed under GPL license v2+; compatible licenses are not accepted.

1. 	$cck_files = file_scan_directory ( dirname(__FILE__) . '/cck' , '.*\.inc$' );
   

   That is not the way a module gets the directory where it is installed.

2. 	if (file_exists(drupal_get_path('module', 'cck') .'/modules/content_copy/content_copy.module')) {
   		include_once(drupal_get_path('module', 'cck') .'/modules/content_copy/content_copy.module');
   

   To load a module there is a Drupal function that can be used. Third-party modules should then be loaded only if they are enabled. If the proposed module depends on content_copy.module, then it should declare its dependency from that module.
   What would happen if content_copy.module has not been installed, and a module loads it with the code I reported here?

3. 		// The import will be populate into the $content variable.
   		$content = array();
   		
   		ob_start();
   		include $file;
   		ob_end_clean();
   

   I am not sure what you are trying to do. No existing module uses such code. Are you sure there isn't a better way to do what you are trying to do? If you are trying to populate $content, then you should use the value returned from a function.

4. 		$query = db_query('SELECT DISTINCT(ctl.field_mlsnumber_value), ctl.field_listing_type_value, ctl.nid, n.nid FROM {content_type_listing} ctl INNER JOIN {node} n ON n.nid = ctl.nid');
   

   To build such queries (the one with DISTINCT in) there is a Drupal function that should be called.

5. The way used to build a node object, and to save it in the database doesn't allow third-party modules to chime in. That is contrary of what done by Drupal core code, and it could create problems with other modules.

6. 			if ($rets->Error()) { 
   				$error = $rets->Error();
   				watchdog('rets', 'Error during rets_create_listing(). Error Type: @type. Error Code: @code. Error Message: @error', array('@type' => $error['type'], '@code' => $error['code'], '@error' => $error['text']), WATCHDOG_CRITICAL);
   			}
   

   Why is the code calling twice that object method when it could call it just once?

7. 			while ($property = $rets->FetchRow($search)) {
   				$node = node_load($nid);
   				
   				// correctly format Bathrooms
   				
   				$bathrooms = $property['BathsTotal'];
   				
   				if ($property['BathsPartial'] > 0) {
   					$bathrooms = $bathrooms . '.5';	
   				}
   				
   				// consolidate Street Number and Street
   				// make sure Street Number is a real value and not 0 or 000
   				
   				$street = '';
   				
   
   

   The code load the node without to even verify if it has been unpublished; there could be some problems with that.

8. See the Drupal coding standards to understand how a module code should be written. In particular see how the code should be formatted, and which string function a module is supposed to call.
9. If a function is not returning anything, then it is useless that return is the last instruction of a function.

10. 	  t($listing_type),
    	  t($street),
    	  t('$'.number_format($price, 0)),
    

    The first argument of t() is a literal string, not a dynamic value; differently, the script to extract the strings to translate will not extract the string.

11.   if ($count == 0) {
    	$rows[] = array('No listings to manage.', '', '', '', '', '', '');  
      }
    

    Strings used in the user interface needs to be translated; this includes also the strings used as options for the form fields.

12. 			$output .= '<ul>';
    			$output .= t('<li>RETS authentication successful!</li>');
    			$output .= t("<li>$rets_server</li>");
    			$output .= '</ul>';
    

    Whenever possible, HTML tags should be keep outside translated strings.

13. function rets_statistics() {
    	$output = '<h2>RETS Statistics</h2>';	
    	
    	$total_listings_query = db_fetch_array(db_query('SELECT count(DISTINCT(field_mlsnumber_value)) FROM {content_type_listing}'));
    	 
    	$total_listings = $total_listings_query['count(DISTINCT(field_mlsnumber_value))'];
    	
    	$published_listings_query = db_fetch_array(db_query('SELECT count(nid) FROM {node} WHERE type = "%s" AND status = %d', 'listing', 1));
    	$published_listings = $published_listings_query['count(nid)'];
    	
    	$unpublished_listings = $total_listings - $published_listings;
    	
    	$active_listings_query = db_fetch_array(db_query('SELECT count(n.nid), ctl.field_listing_status_value FROM {node} n INNER JOIN {content_type_listing} ctl on ctl.nid = n.nid WHERE type = "%s" AND ctl.field_listing_status_value = "%s"', 'listing', 'Active'));
    	$active_listings = $active_listings_query['count(n.nid)'];
    	
    	$rets_update_time_length = variable_get('rets_update_time_length', 0);
    	$local_time = date('M d Y h:i:s A', time());
    	$gmt_time = rets_convert_time_to_gmt();
    	
    	$output .= '<ul>';
    	
    	$output .= t("<li>Total listings: $total_listings</li>");
    	$output .= t("<li>Published: $published_listings</li>");
    	$output .= t("<li>Unpublished: $unpublished_listings</li>");
    	$output .= t("<li>Active listings: $active_listings</li>");
    	$output .= t("<li>Last update duration: $rets_update_time_length seconds</li>");
    	$output .= t("<li>Local time: $local_time</li>");
    	$output .= t("<li>GMT time: $gmt_time</li>");
    	
    	$output .= '</ul>';
    	
    	return $output;
    }
    

    The function is outputting too much HTML. It should use a theme function.

For #5, I am not sure what you mean. How would I let other modules chime in?

My comment was not correct; node_save() already calls the hook_nodeapi() implementations of third-party modules, and the hook_node() implementation of the module that defines the content type being saved.

So for #13, return theme('style', $output)?

theme('item_list'). I don't see any theme_style() defined from Drupal core code.

There have not been replies from the OP in the past 7 days. I am marking this report as won't fix.

Just wondering if this module will be available for download soon?

@rout: This report has been marked as won't fix; this means that stripped your speech has not obtained a CVS account, without of which he cannot commit anything in CVS.

subscribe