CVS edit link for nvanhove

I've created a new module called "blockdescription", which you can find on http://www.listmania.nl/drupal-module-blockdescription
There you can also see the screenshots for it.

What it does in short is add a textarea to the blocks detail page and displays them on the block overview page.
Ofcourse, this is not a lifechanger module, but it has helped our company with a project with lots of blocks.

I would like to have an CVS account so I can share it with the Drupal community.

Comments

nvanhove’s picture

Status: Postponed (maintainer needs more info) » Needs review
StatusFileSize
new2 KB
dawehner’s picture

Status: Needs review » Needs work
  1. Use <?php instead of <? for install file.
  2. *Minor* Add
    /**
    * Implemenation of hook_foo().
    */
    to each hook you implement.
  3.   global $blockdescription_allvalues;
      if(!$blockdescription_allvalues)
        $blockdescription_allvalues = blockdescription_get_all_values();
      
      if(isSet($blockdescription_allvalues[$module][$delta])) 
        return $blockdescription_allvalues[$module][$delta];
      else
        return false;
    
    
  4. Add static instead of global.
  5. use "{}" even for one-liner-ifs
  6. isSet should be isset
  7. false should be written as FALSE.
avpaderno’s picture

See the Drupal coding standards to understand how a module code should be written.

nvanhove’s picture

StatusFileSize
new2.03 KB

I've adjusted the points given by dereine and used the Coder module to fix some minor coding standard issues.

nvanhove’s picture

Status: Needs work » Needs review
ajk’s picture

Status: Needs review » Needs work
      db_query('DELETE FROM {blockdescription} WHERE module = "%s" AND delta = "%s"', $module, $delta);

Should be

      db_query("DELETE FROM {blockdescription} WHERE module = '%s' AND delta = '%s'", $module, $delta);

(and elsewhere) otherwise Postgres database users won't use your module

ajk’s picture

Issue tags: +Module review
    $txt = "This module creates a description field so you can define what the purpose of the block is.";

t() ?

        $form[$form_element_id]['info']['#value'] .= "<br /><i>". $value ."</i>";

Why are you embedding html where themers can't get at it?

    foreach ((array)$form as $form_element_id => $form_element) {
      if (drupal_substr($form_element_id, 0, 1) == "#") continue;

element_child()?

Lastly, I'm pretty sure blocks are keyed by module/theme/delta where as you only use module/delta. Did you intend for your descriptions to cover all themes?

nvanhove’s picture

Status: Needs work » Needs review
StatusFileSize
new2.23 KB

Good points.
I've adjusted all items and adjusted the module to be keyed by theme/module/delta.

nvanhove’s picture

StatusFileSize
new2.23 KB

Whoops, I uploaded the wrong file..

nvanhove’s picture

StatusFileSize
new2.17 KB

I've removed the module keying theme/module/delta from #7. It appears that blocks are not entirely keyed that way. For example, the title is also reused in all themes.

Please review my module :)

avpaderno’s picture

Status: Needs review » Fixed

Status: Fixed » Closed (fixed)
Issue tags: -Module review

Automatically closed -- issue fixed for 2 weeks with no activity.

avpaderno’s picture

Component: Miscellaneous » new project application
Assigned: Unassigned » avpaderno
Issue summary: View changes
Status: Closed (fixed) » Fixed

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.