James Marks [jamesmarks]

1. Typo in filename READEME.txt
2. CSS class .alpha-index is outside of the module's namespace.
3. minor: you're encouraged to follow Drupal's coding standards for CSS, http://drupal.org/node/302199

4. function siteindex_install() {
   ...
     drupal_set_message(st("Site Index settings are available under !link", 
       array( '!link' => l('Administer > Site configuration > Site Index settings ', 'admin/settings/siteindex' ))
     ));
   }
   

   Drupal modules should not output such messages after performing (any) installation tasks. There are dedicated modules, which may be doing this. All other modules should follow practices of Drupal core modules.

5. You can remove t() everywhere from hook_schema().

6. /**
    * Path to the site index page used for building a variety of links.
    */
   define('SITEINDEX_PAGE_PATH', variable_get('siteindex_page_path', 'site_index'));
   
   /**
    * Title of the Site Index menu link in the Navigation menu.
    */
   define('SITEINDEX_MENU_TITLE', variable_get('siteindex_menu_title', 'Site index'));
   

   These two constants need to be removed. variable_get() is enough of an abstraction.

7. siteindex_entry_list() generates a PHP notice for $table if there are no entries. There is more code in the module that will likely generate notices and warnings. In general, you should develop with E^ALL for PHP's error_reporting.
8. minor: siteindex.css is loaded on all/any pages.
9. Most module functions are declared as "private" (leading underscore). This is very untypical for a Drupal module, especially, since siteindex API functions are private, too.
10. minor: MENU_CALLBACKs do not necessarily need a 'title' or 'description', as they are not displayed anyway. Might be a good idea to "group" the autocomplete callbacks into siteindex/autocomplete/[thing].
11. The HTML/CSS ID siteindex-node-form likely clashes with a possible node type name "sideindex".
12. In the siteindex_add_edit_entry() form, the submit button label string needs to be passed through t() where the variable string is set, not where it is assigned to the button. Otherwise, the string cannot be translated.

    The cancel button should use a dedicated #submit handler, so as to not clobber the regular form submission.

    minor: "confirm" in [_]siteindex_confirm_first_level_value() and other functions should be renamed to "validate". Ideally, they should get $form and $form_state as arguments, just like any other form validation handler.

    It looks like _siteindex_test_path() should be a more common form validation handler, too. At least the error handling tied to it looks a bit weak currently.

    You may also find the #element_validate Form API property interesting. It allows you to perform form element value validation on a per element basis, which might heavily simplify your form handling here. _siteindex_form_alter_validate() looks like you really could use #element_validate.

    _siteindex_add_edit_entry_submit() performs identification and routing logic, which was already done in _siteindex_add_edit_entry() or elsewhere. That's a bit fragile and might impose security issues, since the form processing should not be able to entirely manipulate the target thing that is submitted. You can store originally identified properties in $form['#siteindex'] or in #type 'value' form elements; both ways are accessible in form validation and submit handlers.

    The submit button label text should not have an effect on data storage and CRUD logic. You should check whether an entry needs to be created or updated, based upon whether the form resp. submitted form values contain an id or not. -- Ideally, you might want to consider to move the actual data storage into API functions, i.e. siteindex_entry_save(&$entry).

13. In several places, you are fiddling with drupal_get_destination() for form submissions. You should not have to, as Drupal core automatically cares for alternating destinations for you. If not, then you should document what is being done there and why.
14. The siteindex_notices admin setting, and potentially others, should use a checkbox as UI element.

15.   $form['#submit'][] = menu_rebuild();
    

    No way. :) First of all, menu_rebuild() is invoked immediately here, not as a submit handler. Thus, everytime you visit the admin settings, the entire menu is rebuild. You should be using a dedicated submit handler for the admin settings here, which performs that action.

16. _siteindex_change_index_entry_status() and likely other functions might better use the more common [subject]_[verb] naming - i.e., here: siteindex_entry_change_status($ieid, $status). In this very special case, passing strings that are translated into 1/0 looks a bit weird.

17.   $sql = "UPDATE {siteindex}
              SET    status = %d
              WHERE  ieid = %d";
      $sql_args = array($status, $ieid);
      db_query($sql, $sql_args);
    

    minor: In general, in Drupal, we write SQL statements on one line and without needless variables (unless they are needed, of course):

      db_query("UPDATE {siteindex} SET status = %d WHERE ieid = %d", array($status, $ieid));
    

18. _siteindex_check_for_duplicate_entries() seems to be a form validation handler, which also invokes form_set_error(). Either it's an API function or it's a form validation handler, but please not both. I.e., same remarks as above.
19. _siteindex_delete_entry() may ideally be renamed to siteindex_entry_delete_form(), so as to clarify that the function is a form constructor. Ideally, the form submit handler should invoke a separate API function siteindex_entry_delete() to delete an entry.

    The delete form needs to use confirm_form().

20.     // If it's an index entry with no first- or second-level values
        // then it's an index entry being deleted form the node entry form
        elseif (!empty($entry['ieid'])) {
          $sql = "DELETE FROM {siteindex}
                  WHERE       ieid = %d";
          db_query($sql, $entry['ieid']);
        }
    

    This should and must not happen. It would mean that user input and stored data can get lost mysteriously. I.e., this entire elseif needs to vanish.

21. /**
     * Menu callback.
     *
     * Returns user to index entry list without submitting index entry form
     */
    function _siteindex_form_cancel($destination) {
      drupal_goto($destination);
    }
    

    This looks very very odd. Not sure what you were trying to do, but this is an illegal mix of sub-system APIs, which makes no sense. Is this supposed to be a form submit handler instead of a menu callback (as the PHPDoc states)? If it is, then the function gets $form and $form_state as arguments, so $destination can't work. If it's not, then it's indeed a menu callback that redirects to another path, which is needless, since you could specify 'page callback' => 'drupal_goto', 'page arguments' => array(1 [or 'static/destination'), in hook_menu() already. But in the end, a page callback that permanently and unconditionally redirects is entirely wrong in the first place. ;)

    Thus, something is wrong here and this entire function needs to go.

22. In _siteindex_page(), $title needs to be check_plain()ed for drupal_set_title(), as $title can contain arbitary user input.

    $index_letter and eventually all other stored user input that is output in the index needs to be check_plain()ed, too. This in particular applies to headings that are output further below.

23. Lastly, the module contains plenty of logic to allow to alter the internal system path the index is appearing on:
    

      $form['siteindex_page_path'] = array(
        '#type'          => 'textfield',
        '#title'         => t('Path to site index page'),
        '#description'   => t('Path to the site index page. Existing index entries will be updated if this value is changed.'),
        '#default_value' => variable_get('siteindex_page_path', 'site_index'),
      );
    

    This entire configuration setting is very uncommon for a Drupal module and actually needless, since 'site_index' is the internal system path. One wants to have it on a different path, users are able to create a URL alias for it. Modules like Subpath Alias additionally introduce support for dynamic arguments after URL aliases.

    Finally, the default path should be 'siteindex', not 'site_index', as the latter is not the module's namespace.

Hi. re #4. I'm responsible for the mixed messages you may have got there. I find it extremely useful for a module to say "Hi, I'm installed, here's how to use me" rather than silently hide itself deep in the menu tree making you search for it for the next 5 minutes. To me that's affordance. Sun appears to have a slight difference of opinion.
You can't please everyone :-)

Thanks! No time for a full review, sorry.

4. I've commented it out but I added the message following the suggestion to do so under 'Recommended practices' in 'Module documentation guidelines' (http://drupal.org/node/161085). If its use is incorrect, the guidelines should be probably be corrected.

Interesting. Not sure who put those up. In general, it's best to follow latest and greatest code in Drupal core. None of the core modules display a message after installing or uninstalling. And, I keep my major argument: To be really helpful, you should integrate with one of the existing "admin guide" modules instead, i.e., modules that allow other modules to expose structured information and ordered tasks to install and configure the module. Simple messages simply don't buy you anything useful, since they'll vanish, are only visible to one user, and stuff.

We should fix those guidelines.

6. Fixed. I had added the constants thinking it would be better to reduce the number of calls to the database to one at the top of the file.

Mad assumptions :) http://api.drupal.org/api/function/variable_get/6 does not query the database, it's a very fast function.

13. Fixed. Used $form['#redirect'] and dropped all 'destination' fiddling. (Ironically, it looks like $form['#redirect'] has been killed in D7 by you. :) )

IIRC, I actually meant to drop the entire special destination handling. If you want a form to redirect to somewhere else (than the default page/path after submission), then you can simply pass a 'destination' query parameter to it:

http://example.com/path/to/form?destination=custom/redirect/path/to/end/on

Form API will respect this destination path and ignore any hard-coded path.

16. Changed passing strings then converting to integers to just passing integers. With regard to function naming, I'd looked through core modules in the past and hadn't been able to identify a standard function naming convention ([verb][subject] or [subject][verb]). Both appear to exist side-by-side. In some cases, one or the other appears to be used to logically group related functions together: [verb][subject]: user_validate_name, user_validate_mail, user_validate_picture; [subject][verb] node_type_save, node_type_delete. Are there published guidelines as to which is appropriate when? Is this a critical issue? I want my code to conform to standards, I just haven't been able to find clearly articulated ones when it comes to subject-verb order in function naming.

Good, modern, fancy: [subject]_[verb]
Old, ugly, bad: [verb]_[subject]

This concept is easily understandable and allows to quickly draft new APIs. Speaking of, any module/API should expose the base CRUD functions along with hooks, like many of core's APIs do:

siteindex_entry_load() => hook_siteindex_entry_load()
siteindex_entry_save() => hook_siteindex_entry_insert() / hook_siteindex_entry_update()
siteindex_entry_delete() => hook_siteindex_entry_delete()

Those user validation helper functions you mentioned (and others) date back to... Drupal 4.6 actually: http://api.drupal.org/api/function/user_validate_name/4.6 -- however, finding good names for such universal helper functions also is not an easy task. One could argue that "user" is the subject already (although it's the module name).

17. Fixed. Is there a general rule as to how long a one-line query can be before it is written on multiple lines? Arrays are written on multiple lines if they exceed more than 80 characters (according to the Coding Standards document). Would that standard apply to other long code strings?

Comments are required to wrap at 80 chars. Arrays are treated kinda special; although there is no clear rule that forces you to wrap arrays (write them as multi-line), many people always do and prefer it. Any other code or string does not need to wrap, but if there is a sane method of making it wrap, it may be used. Overly (very very) long strings, for example, may simply be concatenated:

$text = 'foo bar baz';
$text .= 'baz bar foo';

However, this really applies to text strings only. Code lines (most often conditions) may be split into multiple conditions, if possible.

19. Fixed. Is it a requirement to use confirm_form()? I'm using it now but I don't like it as much as what I had originally because it fails to offer the 'Cancel' option as a button, rendering it as a link instead. Since they're both options, it seems that they should both be rendered in similar fashion.

If you want to change this standard in Drupal, then you are welcome to change it in Drupal core. :) Otherwise, when implementing "mainstream" functionality, you and everyone else is and should be following common module development practices. This ensures compatibility with other modules, simplifies theming for front-end developers, and in this case, also ensures security.

But truth to be told: Changing the link to a form button has been actively discussed for D7, but deferred to D8 due to missing consensus and activity on the issue. :)

20. Ok, here's where I defend my code. :) The intent here is to allow deletion of site index entries from the multi-value fieldset in the node edit form similar to way specific values can be deleted from multi-value CCK fields by deleting the value and submitting the form. The removal of editable values indicates the intended deletion of the related record. In any other fields on a node edit form, CCK or not, submission of empty fields indicates intended deletion of those values. Why not allow the deletion site index values as well?

IIRC, then this code was/is executed during editing (or similar) - and not during/after final form submission. The code needs to make sure that all stored data is persistent, and data is only deleted, when the user actively and actually requested the deletion. Just removing values during a multi-step form operation would not be a serious deletion request.

Still no time for a full review, but:

I'd be an enthusiastic supporter of the two button approach (if I ever get to the point where I get to have any input on changing core). :)

You, me, everyone, can! Just pick yourself an issue + work on it! :) This issue, for example, is discussed in

#116939: Add a Cancel link on forms
#772758: Delete confirm form, make the cancel link to button

However, as mentioned before, that particular topic is too late for D7. But you can always play http://drupal.org/bug-bingo or http://drupal.org/patch-bingo :)

If you want to see D7 out of the door earlier, you can always help to fix remaining bugs. Writing patches for Drupal core makes you learn to write even more awesome code :)

Why did you decide to create a new module, rather than opening feature supports for the existing projects?

I think James already described in the original motivation message. Without having actually looked at the mentioned modules or otherwise knowing them, I actually think this module makes sense (to some extent). It looks like James is going to be a happy community citizen, and the only remaining thing missing here seems to be a final security review.

I approved the application as maintainer of Profile Plus (see http://drupal.org/node/866142).