Hi,

I've found a minor bug in the links which are generated in the warning message when anonymous visitors cannot leave their contact details. The problem occurs when the content type name contains an underscore. These should be converted to hyphens when generating the admin link, as is done, for example, in content_types_overview() in cck/includes/content.admin.inc

Attached is a patch against 1.4

Thanks for creating the comment_notify module. This is exactly what I was looking for.

Jonathan

CommentFileSizeAuthor
#4 d7 comment notify link with underscores.jpg247.93 KBjonathan1055
_comment_notify.underscore_in_content_type.patch.txt1.14 KBjonathan1055
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented
Status: Active » Needs review

Looks good to me. I'll try to test/apply soon. Thanks!

jonathan1055’s picture

jonathan1055 CreditAttribution: jonathan1055 commented

Just in case you were wondering why I split the change over two lines when it could have been done in one, this is because I have extended the information shown and use $type_url_str in more places, to give links to the admin settings for each type. I will submit that next - did not want to include a bug-fix and an enhancement in the same patch (been told off for doing that before!)

Jonathan

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
CreditAttribution: greggles commented
Version: 6.x-1.4 » 7.x-1.x-dev
Status: Needs review » Patch (to be ported)

Fixed - thanks jonathan1055! http://drupal.org/cvs?commit=390498

It looks like this affects the Drupal 7 version as well. Could you "port" it?

I'd also be interested in your patch that fixes the other issue you mention.

jonathan1055’s picture

jonathan1055 CreditAttribution: jonathan1055 commented
FileSize
d7 comment notify link with underscores.jpg247.93 KB

I've just tested this with a D7 alpha6 site. Something clever is happening with the interpretation of the menu path because links to content types with underscores now work fine, as do links with hyphens. See the attached screen grab showing a link to a content type with underscores - clicking the link shown results in localhost:8888/drupal_7/#overlay=admin/structure/types/manage/type_with_underscores which shows the required page.

So this patch is not actually needed in D7 (at least it appears not in the testing I've done). Therefore the choices are:

  1. is it a good idea to make the change anyway, so that when making other enhancements or fixes in future which need to be done to D7 and D6 versions we make it easier to copy and paste code? Or
  2. is it better to keep D7 cleaner, and just have a good comment in the D6 version to remind us?

I read a useful post by Dries, linked from http://drupal.org/node/65922, which suggests that we should not drag baggage to a new release. So I favour option 2 but I would be happy to go with your preference.

Jonathan

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented
Version: 7.x-1.x-dev » 6.x-1.x-dev
Status: Patch (to be ported) » Fixed

It's because the menu link loader for node types in D7 auto-converts dashes back to underscores. Core itself uses links with just the node type un-altered, so that's what we should do too. I think the existing comment is good enough in D6, so marking back to fixed for D6.
http://api.drupal.org/api/function/node_type_load/7

jonathan1055’s picture

jonathan1055 CreditAttribution: jonathan1055 commented

Thanks for the background info Dave. At first glance, the header comment in D7 node_type_load() was confusing as it appeared to be the wrong way round:

$name The machine-readable name of a node type to load, where '_' is replaced with '-'.

Then I worked out what it means is that the $name parameter has already had underscores changed to hyphens, and these will be changed back to underscores by the strtr code in 

  return node_type_get_type(strtr($name, array('-' => '_')));

Status: Fixed » Closed (fixed)

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