Content Type Thumbnail

Hello, please try to fix issues raised by automated review tools:
http://ventral.org/pareview/httpgitdrupalorgsandboxmikespence1934058git

Currently you only have a master branch, please branch to 7.x-1.x and delete the master branch.

Thanks.

You are working on "Master" branch. You should be working on "7.x-1.x" branch.
Firstly correct the code reviewed by http://ventral.org auto review script. You may find review result of your project from below URL:
http://ventral.org/pareview/httpgitdrupalorgsandboxmikespence1934058git

Nice idea for a module!

Here is my feedback:

• Use hook_menu_alter to change the page callback of node/add instead of redirecting to your own node/add-cct path, or alter the output of node/add instead of building your own implementation (core updates).
• Include more informations in README.txt (see http://drupal.org/node/447604)
• Make the default preview image configurable.
• Never use "!important" in your css, as this prevents themes from overwriting these properties.
• As the image is rendered with an dedicated image_style, remove the "160x160" hint in the description, as this image_style can be changed to other dimensions.
• Make css more flexible to size changes of the image_style, as the css expects 160px images and uses a 180px to add some padding.
• As your module is for content-types only, you should declare a dependency on the node module.

Don't get me wrong, using an image_style was a very good idea, as this image_style was configurable by the user and could include more effects than just resizing. Just the 160x160 was not correct, as the style could be changed. Why did you remove it?

The code in content_type_thumbnail_form_node_type_form_alter():

$form['thumbnail']['#collapsible'] = 'True';

Should be changed to:

$form['thumbnail']['#collapsible'] = TRUE;

Additionally, content_type_thumbnail_add_node() now includes inline styles, which prevent styling via themes.

Nice module idea. Overall the code looks pretty good to me.

Here's my feedback:

So I kind of take issue with overriding the node/add page callback. There's a few issues here:

• Overriding the full page prevents other modules from making their own adjustments ( unless the function is at least similar to the original )
• The core version of the page operates on menu_links not node_types. It's true that the menu_links generally correlate exactly with node_types on this page. However this page can normally be edited by editing the menu ( weights can be adjusted, menu_link descriptions don't necessarily have to match node_type descriptions, menu_links could be added/removed/hidden, etc)
• Furthermore, simply iterating through nodetypes would list all nodetypes in the system regardless of menu access logic. ( Granted the access logic would still protect the individual node/add/* pages, but even so, you've potentially exposed a full list of nodetypes to an unprivileged user )

I would approach this problem by overriding the theme_node_add_list() theme function instead of the full menu router item. The normal node_add_page() function calls this to theme the output from system_admin_menu_block().

But let me just step backwards for a moment. I would start by wrapping the content_type_thumbnail image logic itself in a theme function. That gets it isolated out of the rest of the display logic. This lets you easily reuse it, and makes it possible for other themes or modules to override it if necessary. That would look something like this (untested) :

/**
 * Implementation of hook_theme().
 */
function content_type_thumbnail_theme($existing, $type, $theme, $path) {
  $items['content_type_thumbnail'] = array(
    'variables' => array('type' => NULL),
  );
  return $items;
}

/**
 * Default theme function for content_type_thumbnail
 */
function theme_content_type_thumbnail($variables) {
  $type = $variables['type'];

  if (variable_get('content_type_thumbnail_image_' . $type)) {
    $image = file_load(variable_get('content_type_thumbnail_image_' . $type));
    $vars = array( 'path' => $image->uri,  'style_name' => 'content_type_thumbnail');
    return theme('image_style', $vars);
  }
  else {
    $vars = array( 'path' => drupal_get_path('module', 'content_type_thumbnail') . '/images/no-image.png');
    return theme('image', $vars);
  }
}

Now we can generate the image anytime we want like so:

$image = theme('content_type_thumbnail',array('type' = 'some_type'));

Ok, so back to overriding theme_node_add_list(). Basically theme_node_add_list() gets a list of menu items to theme. Since these are not nodetypes, you have to derive the nodetype from the path, but that's easy enough. I think the override could look something like this:

/**
 * Implementation of hook_theme_registry_alter().
 */
function content_type_thumbnail_theme_registry_alter(&$theme_registry) {
  // Note: may require high enough module weight to win against other modules and themes
  $theme_registry['content_type_thumbnail']['function'] = 'content_type_thumbnail_node_add_list';
}    

/**
 * Override of theme_node_add_list().
 */
function content_type_thumbnail_node_add_list($variables) {
  $content = $variables['content'];
  $output = '';

  if ($content) {
    $output = '<dl class="node-type-list">';
    foreach ($content as $item) {
      
      // This is a copy of theme_node_add_list with alterations
      // Alterations are all between the marked area here
      ////////////////////////////////////
      // make sure these are emptied on each itteration
      unset($image, $image_link);
      
      // Extract the type from the path and theme the content_type_thumbnail
      // we can only extract types from node/add paths however,
      // it is possible for other menu items to be filed here
      if (strpos($item['href'], 'node/add/') === 0) {
        $type = substr($item['href'], 9 );
        $type = str_replace('-', '_', $type);
        if(!empty($type)) {
          // theme image + image link
          $image = theme('content_type_thumbnail', array('type' => $type));
          $image_link = l($image, 'node/add/' . $type, array('html' => TRUE, 'attributes' => array('class' => 'node-type-image')));
        }
      }
      
      $output .= '<dt>' . $image_link . l($item['title'], $item['href'], $item['localized_options']) . '</dt>';
      ///////////////////////////////////
      
      $output .= '<dd>' . filter_xss_admin($item['description']) . '</dd>';
    }
    $output .= '</dl>';
  }
  else {
    $output = '<p>' . t('You have not created any content types yet. Go to the <a href="@create-content">content type creation page</a> to add a new content type.', array('@create-content' => url('admin/structure/types/add'))) . '</p>';
  }
  return $output;
}

Note that this markup is a bit different from yours. This is mainly because I didn't feel taking the time to match it. I think this illustrates the main concept. However, I do also think it's best for modules to keep their markup as similar to core as possible so that other modules and themes know what to expect. Themes can then make whatever radical overrides they want - this is one reason to make all markup come from a theme function, not directly from a page callback.

Anyway, It looks like a pretty cool module :) I hope this is helpful. I think most of this is just nit-picking on implementation details, however exposing a full list of node_types without access checks is a potential security issue i think.

Update how to use it

Just a heads up, looks like your git link above should be

git clone --recursive --branch 7.x-1.x http://git.drupal.org/sandbox/mikespence/1934058.git content_type_thumbnail

so that it doesn't ask people for your mikespence@git.drupal.org password when trying to clone.

edit: forgot a word

Hi Mike,

nice simple module. Here is my feedback:

• image file uploads fail to load properly without the content_type_thumbnail image style. Consider including a db_insert() call in content_type_thumbnail.install to create a 'default' version of this image style. Otherwise, mention in the README that the user needs to create an instance of this image style.

Other than that, module works like a charm.

Good luck.

manual review:

1. content_type_thumbnail_install(): do not juggle with module weights as that is very unreliable. Use hook_module_implements_alter() if you have to run before/after a particular module.
2. content_type_thumbnail_install(): no need to set variables upon installation as you can always use default values with variable_get() anyway.
3. "'access arguments' => array('administer modules'),": why "administer modules" and not administer content types?
4. Maybe admin/structure/types page should also use the images?

But otherwise looks RTBC to me.

Assigning to mitchell as he might have time to take a final look at this.

Cool module. Thanks for contributing it, mikespence!

I updated your account to let you promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here 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 lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and get involved!

Thanks, also, for your patience with the review process. Thanks to the dedicated reviewer(s) as well.