BlastChat [blastchat]

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

As per requirements, the motivation message should be expanded to contain more features of the proposed project. For themes, it should include also a screenshot of the theme, and (when possible) a link to a working demo site using the proposed theme; for modules, it should include also a comparison with the existing solutions.

Thanks a lot! This is the best chat solution for drupal so far!

You cannot edit the description as it appears on the top of this report; the link you tried takes to the page to edit your CVS account, to which you don't have access until your CVS account is approved.

On Drupal.org there are not hidden issue queues, and this issue queue is for the approval of CVS accounts, not a pre-approval page.

I was not aware that initial description I submitted would go to main page of the project.

The description you give here is not used for the project page.

The link to the project page of a module named blastchat is http://drupal.org/project/blastchat; the module is listed in every views where modules are listed.

You are applying for a CVS account; until this issue is not marked as fixed by a CVS administrator, your CVS application is not approved, and you cannot create new projects, or commit code 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. function bc_error($msg) {
   	return drupal_set_message(t($msg),'warning');
   }
   
   
   

   See http://drupal.org/coding-standards to understand how a module should be written. In particular, see how the code should be formatted; how Drupal variables, global variables, constants, and functions defined from the module should be named.
   The first argument of t() is a literal string, not a concatenation of strings. The script used to create the translation template is not able to handle any dynamic value, even in the case of code similar to t($variable); this means that if the argument of the function is not a literal string, it will not appear in the translation template.

2. License files cannot be committed in Drupal.org repository. Projects committed in Drupal.org repository have the same license used by Drupal.
3. Every module that is committed in Drupal.org CVS is licensed under the same license used by Drupal; modules that use a different license (as in this case) cannot be committed in Drupal.org.
4. Copyright statements should be removed from the code; when you apply to the module a patch provided by other users, the copyright statement would not be anymore correct.
5. The file COPYRIGHT.php doesn't contain PHP code; the same is true for LICENSE.php.

6. 	if (!is_writable($path)) {
   		bc_error("Directory " .$path. " <strong>is NOT writable</strong>, change permissions.");
   		
   	} else {
   		$fp = null;			
   		if (!file_exists($path.$ds."configuration.php")) {
   			$fp = fopen($path.$ds."configuration.php", "x");
   		} else {
   			$fp = fopen($path.$ds."configuration.php", "w");
   		}
   		if ($fp) {
   			fputs($fp, $config, strlen($config));
   			fclose ($fp);
   		} else {
   			bc_error("Error writing to " .$path.$ds."configuration.php file.");			
   		}
   
   

   Modules don't have permission to write files inside the directory where they are installed; the operation will fails.

7. 	db_query("DELETE FROM {cache}");
   
   

   To clear the cache is already done by Drupal; it's then a little excessive to clear all the caches.

8. function blastchat_help($section) {
   	if ($section == 'admin/help#blastchat') {
   		return 'Get more help at <a href="http://www.blastchat.com/support-forum">www.blastchat.com/support-forum</a>';
   	}
   	if ($section == 'admin/modules#description') {
   		return 'BlastChat - chat for your website';
   	}
   }
   
   

   Hook implementation comments should be like the following one:

   /**
    * Implements hook_menu().
    */
   

   Strings used in the user interface should be translated.

9. function blastchat_perm() {
   	return array('administer blastchat','user access');
   }
   
   

   Permissions follow the schema <verb> <object>, where the verb is written in lowercase.

10. A module that alters a database table used by another module should also implement hook_schema_alter().

11. 					if ($showicon == 1) {
    						$bc_list[$i] .= ' <a href="/index.php?q=blastchat"><img src="'.$imagepath.'/o'.($items["MAX(s.blastchat)"] >= $bc_time ? 'n' : 'ff' ).'line.png" title="Member '.($items["MAX(s.blastchat)"] >= $bc_time ? '' : 'not' ).' is chatting. Click to join chat"/></a> ';
    					}
    
    

    There is a Drupal function to use to create a link, and a theme function to use for images.

12. 		if ($guests_online>0 ||  $users_online>0) {
    			$bc_out .= '<div>We have '. ($users_online>0 ? ($users_online == 1 ? $users_online.' member' : $users_online.' members') : '');
    			if ($guests_online>0 && $users_online>0) {
    				$bc_out .= ' and ';
    			}
    			$bc_out .= ($guests_online>0 ? ($guests_online == 1 ? $guests_online.' guest' : $guests_online.' guests') : '').' online. </div>';
    		}
    
    

    Strings used in the user interface should be translated.

13. 			$form['blastchat_user_icon'] = array('#type' => 'checkbox',
    												'#title' => t('Profile icon '.$imagepath.'profiles.gif">'),
    												'#default_value' => variable_get('blastchat_user_icon',1)
    			);
    
    

    The first argument of t() is a literal string, not a concatenation of strings. The script used to create the translation template is not able to handle any dynamic value, even in the case of code similar to t($variable); this means that if the argument of the function is not a literal string, it will not appear in the translation template.

14. The module doesn't remove the Drupal variables it defines when it is uninstalled.

15. 	if(file_exists($path.'/configuration.php')){
    		require($path.'/configuration.php');
    		$website = bc_getWebsiteData();
    	} else {		
    
    

    There is a Drupal function to use to load a file that contains PHP code.
    I am not sure about the reason to create a PHP file that then included.

16. 			if (!defined("_BC_OLDBROWSER")) DEFINE("_BC_OLDBROWSER", "This website uses DHTML. We recommend that you upgrade your browser.");
    			if (!defined("_BC_OPENUNATTACHED")) DEFINE("_BC_OPENUNDETACHED", "Please click %s to load chat %s.");
    			if (!defined("_BC_OPENUNDETACHED_HERE")) DEFINE("_BC_OPENATTACHED_ATTACHED", "attached");
    			if (!defined("_BC_OPENUNDETACHED_HERE")) DEFINE("_BC_OPENATTACHED_HERE", "here");
    
    

    Is there a reason to define constants from inside a function, rather than defining them at the beginning of the module?

17. 				$output .= '
    	<script language="javascript" type="text/javascript">
    	<!--
    	var IE = document.all?true:false;
    	var minetitle = "'.$website->wintitle.'";
    	if (IE) {
    		minetitle = "'.str_replace("@", "|", $website->wintitle).'";
    	}
    	document.title = minetitle;
    	//-->
    	</script>';
    	}
     $output .= '
    <div id="errmsg"> 
    If you selected to open chat detached and it did not open in a new window, your browser is probably blocking pop-up windows.
    <br>' . sprintf(_BC_OPENUNDETACHED, '<a href="index.php?q=blastchat&d=0">'._BC_OPENATTACHED_HERE.'</a>', '<a href="index.php?q=blastchat&d=0">'._BC_OPENATTACHED_ATTACHED.'</a>').
    '
    </div>
    <script language="javascript" type="text/javascript">
    <!--
    window.open("'.$request.'",minetitle,"WIDTH='.$website->d_width.', HEIGHT='.$website->d_height.', location=no, menubar=no, status=no, toolbar=no, scrollbars=no, resizable=yes");
    //-->
    </script>';
    		} else {
    		
    			if($website->wintitle){
    		
    			$return  = "\n<script language=\"javascript\" type=\"text/javascript\">";
    			$return .= "\n<!--";
    			$return .= "\nvar IE = document.all?true:false;";
    			$return .= "\nvar minetitle = \" ".$website->wintitle."\";";
    			$return .= "\nif (IE) {";
    			$return .= "\nminetitle = \" ".str_replace("@", "", $website->wintitle)."\";";
    			$return .= "\n}";
    			$return .= "\ndocument.title = minetitle;";
    			$return .= "\n//-->";
    			$return .= "\n</script>";
    			
    			}
    			
    			$return .= "\n<iframe NAME=\"blastchatc\" ID=\"blastchatc\" SRC=\"$request\" HEIGHT=\"$website->height\" WIDTH=\"$website->width\" FRAMEBORDER=\"$website->frame_border\" marginwidth=\"$website->m_width\" marginheight=\"$website->m_height\" SCROLLING=\"$iframe_scroll\"></iframe>\n";
    		}
    		$return .= '<!-- !!! Do not remove, tamper with, obstruct visibility or obstruct readability of following code unless you have received written permission to do so by owner of BlastChat !!! -->';	
    		$return .='<div align="center" style="width:100%; font-size: 10px; text-align:center; margin: 0px 0px 0px 0px; padding: 0px 0px 0px 0px;">Powered by <a href="http://www.blastchat.com" target="_blank" title="BlastChat - free chat for your website">BlastChat</a></div>';
    
    

    The function is outputting HTML without to use theme functions.
    The JavaScript code should be places in a separated file.

Hi. Please read all the following and the links provided as this is very important information about your CVS Application:

Drupal.org has moved from CVS to Git! This is a very significant change for the Drupal community and for your application. Please read the following documentation on how this affects and benefits you and the application process:
Migrating from CVS Applications to (Git) Full Project Applications

• The status of this application will be put to "postponed" and by following the instructions in the above link, you will be able to reopen it.
• Or if your application has been "needs work" for more than 5 weeks, your application will be marked as "closed (won't fix)". You can still reopen it, by reading the instructions above.

As per previous comment, I am setting this issue as Won't fix.
Since users can now create full projects, applications have a different purpose and they are handled on a different issue queue. See Apply for permission to opt into security advisory coverage for more information.