altrugon [altrugon]

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.

subscribe

We actually have 7 modules already in this very small feature area: http://groups.drupal.org/node/19573. Seems like this is very much duplicated of http://drupal.org/project/suy, which has an open issue to disable the overlay/ui requirement. I wonder if you could help making that module better already?

I am changing the status as per previous comment.

I guess that Dave is referring to the fact the requirements page talks of duplication of work, not duplication of code. That means that to create your module you had to work out similar issues that the author of the already existing project had to resolve too (although the created code can differ).

Generally speaking, we much prefer cooperation, instead of work duplication.

Hi Kiam -

I think Alberto's done a great job in explaining the parts of his module that are different, and also all of the extra admin options he's built that go far and above comparatively, and the fact that it works out of the box (not having to tinker with tpl files). The SUY module that Dave pointed to is by far the closest as far as having very similar purposes, *but* like Alberto describes, he's engineered the module differently, and I'd hazard to say more completely.

As much as I agree that blatant duplication is not useful, in a case of a module being built differently (I won't say better, because I am not able to review the code myself), I don't know how we could expect the maintainers of the other module to accept someone coming in and wanting to completely overhaul how their module functions, and if I'm going to be honest, being that the SUY module never released a non-dev version, I would say that is the one that should not have received a CVS account in the first place.

In the spirit of http://drupal.org/node/703116 it seems reasonable to me that this module and CVS application be approved, also taking into account that the SUY module is not nearly as polished as noIE6, only has a dev version, and hasn't had a commit for nearly a year.

Alberto is committed to maintaining documentation for the module, as well as porting it to D7, and it would seem to me that it will serve the community well to approve the request.

I was talking with arianek on IRC and said I would take a look at this.

Dave points out that there are 7 modules that currently do similar things. @altrugon, you should have reviewed each of these. Here is a quick synopsis.

• http://drupal.org/project/iedestroyer - only 5.x-1.x-dev version.
• http://drupal.org/project/ie6_warning - only 5.x-1.0 version
• http://drupal.org/project/sevenup - 6 version
• http://drupal.org/project/suy - only 6.x-1.x-dev version and no activity in last year
• http://drupal.org/project/ie - 6 version but says it is no longer maintained in favor of SUY module
• http://drupal.org/project/ie6update - 6 and 7 version

So, that leaves pretty much 2 modules that are current that do similar things: SevenUp and IE6 Update.

I think the significant point that you have failed to leave out is that these modules both use third-party libraries to do their thing. This one is all based on custom code (I have not actually looked at the code yet).

IMO, this is significant enough to set it apart and actually make it better than the other two solutions. I think this should go ahead with the review.

zzolo,

that is SUPER helpful, thank you - it's alberto's first time making a major contribution and both our first experiences with the CVS process (and i am no help trying to review the inner workings of the other modules), so we seriously appreciate all your mentoring.

so just to clarify - do you think alberto's best off sticking with attempting to keep this a stand alone module submission? or ought we try and plan for something more complicated like taking over one of the depreciated/abandoned projects and somehow merging them (even if they are not similar code-wise)?

thx again!

By the name, I take that http://drupal.org/project/sevenup is for IE version 7 or higher, not IE version 6 or higher; that would eliminate a competitor.

I understand the worries about duplicated modules that could confuse Drupal users; I also understand that blocking the developing of a module because the current maintainer is close to suggestions is not something that takes good things.
If it is possible, I think that taking over an abandoned module would reduce the confusion on users as at least they would not see a new module with the same purpose of existing modules (which doesn't implicate duplication of code); in other words, the module would be taken over to just reuse a namespace already used by a previous module, and this doesn't implicate that the new module must keep the old code, or make little changes to it.

I have been thinking about this more. I did sort of forget about taking over one of the older modules.

I don't think it is a good idea to take over any of the existing modules, though. @altrugon has not created a new version of any of these modules and the project name of these is significantly different. This would create a big disconnect between the current version and a new version on top of it.

My suggestion would be for @altrugon to go through the "deprecated" or "abandoned" module process for the following projects respectively:

http://drupal.org/project/iedestroyer - only 5.x-1.x-dev version.
http://drupal.org/project/ie6_warning - only 5.x-1.0 version
http://drupal.org/project/suy - only 6.x-1.x-dev version and no activity in last year
http://drupal.org/project/ie - 6 version but says it is no longer maintained in favor of SUY module

Please see this documentation for the process:
Abandoned modules: http://drupal.org/node/251466
Deprecated modules: Not sure exactly, but I think a webmaster queue to add that taxonomy term. This only applies to the ie project

This will clear the space for these module a little, and can be done in parallel with the CVS process. I don't see this as a necessary step but it is a good thing to do.

I think you might want to change the module so that the the aim should become the single point to notifying uses about their browsers. You could rename it to something like "browser_notification" or something similar. Then after getting your CVS account, you could start to work with the maintainers of these other modules to get things consolidated.

I think the core problem here is that no one currently or in this proposal is taking up the torch to consolidate a lot of disparate efforts, and I think this is our goal with not duplicating modules. Hence my suggestions above.

I do apologize for the complexity and time of this issue; I hope I am making sense. I am sure its not really what you expected when applying. But, i see this process as an introduction to community and code contribution and so it is very beneficial to all of us to take the time to do things right, and this is a complex issue. And keep in mind that our goal is to help you get a CVS account (though it may sometimes appear differently).

FYI, I added the taxonomy term Obsolete to http://drupal.org/project/ie. It should be more clear that the project should be considered obsolete, and the users should look for a different project, if they need the features implemented in that project.

I also created a support request for each of the other projects:

• #790314: Is IE Destroyer still being maintained?
• #790322: IE6 warning seems abandoned
• #790326: Is SUY still being maintained?

Wow, nice work kiam.

that is fantastic, thanks so much kiam! i was going to take that on later this week, but you saved me a ton of time figuring out how to take care of all of that.

So far, I have only gotten the reply from a maintainer, who said to be happy to cooperate as he thinks four modules with the same purpose are too much (free translation from what reported from the user).

I have marked as abandoned the other projects reported in comment #17.
As reported by zzolo too, we can go forward with the review of this application, which proposes a module that has pros against the modules I marked as abandoned.

I am assigning this report to me because I will review it tomorrow morning (which means about 18 hours from now).

1.     <div id="noie6-message">
         <div class="noie6-message-inner">
           <h2><?php print t('Internet Explorer 6 is a very old browser and it\'s not supported in this site.'); ?></h2>
           <div id="noie6-reasons">
             <p><?php print t('The following list contains some of the reasons why this browser it is not supported:'); ?></p>
             <ul>
               <li><?php print t('It doesn’t support the'); ?> <?php print l('W3C standards', 'http://www.w3.org/standards/agents/browsers'); ?></li>
               <li><?php print t('It is not compatible with'); ?> <?php print l('CSS3', 'http://www.css3.info/selectors-test/'); ?></li>
               <li><?php print t('It has a very low score in'); ?> <?php print l('Acid 3 test', 'http://acid3.acidtests.org/'); ?></li>
               <li><?php print t('It is insecure and slow'); ?></li>
               <li>
                 <?php print t('There are several movements against this browser:'); ?>
                 <?php print l('IE6: Do NOT want!', 'http://ie6.forteller.net/index.php?title=Main_Page'); ?>,
                 <?php print l('IE Death March', 'http://iedeathmarch.org/'); ?>,
                 <?php print l('Dear IE6', 'http://dearie6.com/'); ?>,
                 <?php print l('Stop IE6', 'http://stopie6.com/'); ?>;
               </li>
               <li>
                 <?php print t('Google has officially ') . l('stop', 'http://googleenterprise.blogspot.com/2010/01/modern-browsers-for-modern-applications.html') . t(' giving it support.'); ?>
               </li>
               <li>
                 <?php print t('And even a ') . l('funeral', 'http://ie6funeral.com/') . t(' has been celebrated.'); ?>
               </li>
             </ul>
           </div>
           <h4><?php print t('We strongly recommend that you download one of the following browsers:'); ?></h4>      
         </div>
       </div>
   

   1. Avoid to escape the string delimiter in strings passed to t(); if the string delimiter is present inside the string, then use the other delimiter (as in "It's just an example").
   2. l() should not be used together t(); see the documentation for t(), which reports such code as wrong code.
   3. Rather than using strings that are separately printed, it would be better to use a single string; this would give to who translates strings more context to understand how to translate strings.

2.     <?php if (isset($browsers) && sizeof($browsers) > 0) { ?>
         <div id="noie6-browsers">
           <ul>
             <?php $last = sizeof($browsers) - 1; ?>
             <?php foreach ($browsers as $key => $browser) { ?>
   

   Use the alternative syntax for the control statements.

3.   $t = get_t();
   

   t() is available from the installation hook.

4. Rather than using a custom database table, the code could use a single Drupal variable (Drupal variables can contain arrays too).
5. Database schema descriptions should not be passed to t(); see what done from Drupal core modules.

6. if (Drupal.jsEnabled) {
     Drupal.behaviors.noie6 = function (context) {
   

   When you use Drupal behaviors, you don't need to check first Drupal.jsEnabled; that is done from Drupal for you.

7.   switch ($path) {
       case "admin/help#noie6":
         $output = '';
         $output .= '<h3>' . t('About') . '</h3>';
         $output .= '<p>'.  t('It\'s time to stop Internet Explorer 6, let your visitors know why they can\'t use IE6 in your website and why they should choose another browser. For more information, see the online handbook entry for <a href="@noie6">No IE6</a>.', array('@noie6' => 'http://drupal.org/handbook/modules/noie6')) . '</p>';
         $output .= '<h3>' . t('Uses') . '</h3>';
         $output .= '<dl>';
         $output .= '<dt>' . t('Prompting users to upgrade IE6') . '</dt>';
         $output .= '<dd>' . t('bla bla bla') . '</dd>';
         $output .= '</dl>';
     }
   

   I have already reported about escaping the string delimiter (I usually report something to change in one place, but that doesn't mean there aren't other places where similar code needs to be changed). I get the string bla bla bla means the help hook has not been completed.

8.     '#title' => t('Apply overlay layer.'),
   

   Form field titles don't have a full stop at the end.

9.   if (!empty($form_state['values']['noie6_firefox']) && !valid_url($form_state['values']['noie6_firefox'], TRUE)) {
       form_set_error('noie6_firefox', t('You must enter a valid URL.'));
     }
   

   I am not sure the first argument of form_set_error() is correct, when the form field is declared as $form['rows'][$row->id].
   The code seems a little confused, here. First the code creates the form fields dynamically by reading each row of a database, then the code takes some form fields will be present. What would happen if a row is not present in the database because of some writing errors?

10. /**
     * Submit function
     */
    function noie6_admin_settings_submit($form, &$form_state) {
      $op = isset($form_state['values']['op']) ? $form_state['values']['op'] : '';
    
      if ($op == t('Reset to defaults')) {
        noie6_restore();
        drupal_set_message(t('The configuration options have been reset to their default values.'));
      }
      else {
        // Save overlay
        variable_set('noie6_overlay', $form_state['values']["noie6_overlay"]);
        
        // Save browser list
        foreach($form_state['values'] as $key => $data){
          //update url elements
          if (substr($key, 0, 3)=='url'){
            //cunningly we have the DB id of the row in the element name
            $id = str_replace('url-', '', $key);
            db_query("UPDATE {noie6} SET url='%s' WHERE id=%d", $data, $id);
          }
          //update exclude elements
          if (substr($key, 0, 7)=='exclude'){
            //cunningly we have the DB id of the row in the element name
            $id = str_replace('exclude-', '', $key);
            db_query("UPDATE {noie6} SET exclude='%s' WHERE id=%d", $data, $id);
          }
          //update weight elements
          if (substr($key, 0, 6)=='weight'){
            //cunningly we have the DB id of the row in the element name
            $id = str_replace('weight-', '', $key);
            db_query("UPDATE {noie6} SET weight=%d WHERE id=%d", $data, $id);
          }
        }
        
        drupal_set_message(t('The configuration options have been saved.'));
      }
    

    All that code would not be necessary if the code would use Drupal variables for each setting contained in a form field. It actually doesn't make sense to use a database table if users are not allowed to add their own settings.

11. /**
     * Restore the original values.
     */
    function noie6_restore() {
      $t = get_t();
      db_query("UPDATE {noie6} SET url = '%s', exclude = %d, weight = %d WHERE name = '%s'", $t('http://www.mozilla.com/firefox/'), 0, 1, $t('Firefox'));
      db_query("UPDATE {noie6} SET url = '%s', exclude = %d, weight = %d WHERE name = '%s'", $t('http://www.apple.com/safari/download/'), 0, 2, $t('Safari'));
      db_query("UPDATE {noie6} SET url = '%s', exclude = %d, weight = %d WHERE name = '%s'", $t('http://www.google.com/chrome/'), 0, 3, $t('Chrome'));
      db_query("UPDATE {noie6} SET url = '%s', exclude = %d, weight = %d WHERE name = '%s'", $t('http://www.opera.com/download/'), 0, 4, $t('Opera'));
      db_query("UPDATE {noie6} SET url = '%s', exclude = %d, weight = %d WHERE name = '%s'", $t('http://www.microsoft.com/windows/internet-explorer/default.aspx'), 0, 5, $t('Update IE'));
    
      variable_del('noie6_overlay');
    }
    

    t() is available when module code is executed.

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

13.   //Make sure the header count matches the column count
      $header=array("Browser", "URL", "Exclude", "Order");
    

    Strings used in the user interface should be translated.

1. The code should use Drupal Unicode functions, when available; this means to use drupal_strtolower(), instead of strtolower(), in example.

2. I'm not really sure how to get rid of the custom database table, I thought I would need it for the drag and drop table order in the settings page. I only have this tutorial to know how to create the drag and drop table.

   It's not necessary to have a database table. Once the array passed as third argument to theme('table') is filled, the function doesn't know how the array has been filled. As far, you are not using any features of the theme function that require a database table.
   Actually, it doesn't make sense to use a table for a settings page that will always have static entries. It would much more sense to use a table for that purpose if the module would allow the user to add settings for more browsers. As it is, the use of a database table makes the code more complicated than it should be, as it requires the module to use a custom submission function that differently would not be necessary.

3.   $form['#theme'] = theme('noie6');
   

   That line seems wrong, as there is not theme_noie6() function.

4. My previous point #11 is still valid.
5. License files cannot be committed in Drupal.org repository. The license file is already added from the packaging script, and a CVS script blocks the temptative to commit such files.

1.   if (user_access('administer noie6', $user)) {
       $items = array();
       
       $items['admin/settings/noie6'] = array(
         'title' => 'No IE6',
         'description' => 'Modify this settings for a better fit with your design. Remember you can also style the module by CSS.',
         'page callback' => 'drupal_get_form',
         'page arguments' => array('noie6_admin_settings'),
         'access arguments' => array('access administration pages'),
         'type' => MENU_NORMAL_ITEM,
        );
       
       return $items;
     }
   

   There is no reason to first check if the user has the permission, and then add the menu item; just use the right permission with 'access arguments'.

2. function noie6_preprocess_page(&$vars, $hook) {
       global $user;
   }
   

   The function receives only an argument.

Thank you for your contribution!
I am going to update your account so you can opt into security advisory coverage now.
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.

Hey @altrugon, you have been approved! I think @kiamlaluno did not make it obvious, but if the issue is fixe,d then you get approved.

Please read the following resources to make sure you know how to use CVS and the specifics to the Drupal CVS infrastructure, as well as how to be a good module maintainer on Drupal.org. The Drupal community is very large and dynamic; we welcome you as a module maintainer and hope that you embrace and challenge the Drupal community and continue to contribute.

• CVS reference: http://drupal.org/handbook/cvs/quickstart
• Maintaining modules: http://drupal.org/node/7765

Oh, also, many thanks for your patience and eagerness through this process. It's not always possible to have a checklist of specifics to say this is approved or not, as you can tell from this application in particular. :) But I hope its been valuable in learning how the Drupal community functions on a basic level and hope that it doesn't turn you off from contributing further and more than just code.

hey i just wanted to add a thanks to both of you kiamlaluno and zzolo, this has been a great learning experience for me too helping alberto navigate the application process, and it'll be great to see how it works getting a new project set up. i think i'll be better equipped to talk others through this in the future now too!