I created a new node using CCK that was called "book recommendations". Trying to store a title pattern for this succeeds, but the variable name gets truncated to 48 characters "pathauto_node_content-book_recommendations_patte" so then trying to retrieve this pathauto fails to see the pattern and thinks that there is no pattern for this node type. I tried saving it a second time and got a "duplicate entry" error on

INSERT INTO variable (name, value) VALUES ('pathauto_node_content-book_recommendations_pattern', 's:13:\"books/[title]\";

I'm not sure what the answer is - so I don't provide a patch. I have a couple of proposed solutions:

1. create a new pathauto table that has a much longer column for the "name" field
2. expand the width of the "name" field in the variable table
3. for pathauto to truncate the name of the node type before storing/retrieving the data from the variable table

I think that solution number 1 or 3 is the best, but leave this for the input of others.

CommentFileSizeAuthor
#15 pathauto_pattern_variables_5x-2x-dev_0.patch4.58 KBashtonium
#14 pathauto.module_patternvariables_3.patch2.42 KBashtonium
#8 pathauto.module_patternvariables_1.patch2.35 KBashtonium
#5 pathauto.module_patternvariables_0.patch3.45 KBashtonium
#2 pathauto_node.inc_4.patch1.65 KBashtonium

Comments

ashtonium’s picture

ashtonium commented
Assigned: Unassigned » ashtonium

Confirming this bug (and glad I searched before submitting a new issue).

The name collumn in the variable table has a max character length of 48, so $node->type will need to be truncated to 26 characters when a node patern value is created or retrieved.

This should be relatively simple to create a patch for, but in the mean time: if you keep your CCK type names to 18 characters (because of the appended "content-" on CCK node types) that should avoid the problem.

ashtonium’s picture

ashtonium commented
Status: Active » Needs review
StatusFileSize
new pathauto_node.inc_4.patch1.65 KB

whoops, forgot to spellcheck that last post, oh well

This patch modifies pathauto_node.inc to pre-truncate the node types to 26 characters so that the resulting variable name doesn't go over 48 characters after "pathauto_node_" and "_pattern" are added before and after the node type.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

The idea of truncating the name before attempting to match seems to introduce a new bug. What if I have two different cck nodes with the names

content-thisisalongnametouse1
content-thisisalongnametouse2

That would be a problem for this patch, right?

After thinking more about the three proposals I laid out in opening the issue, there is a 4th option of changing pathauto to limit internal node names to 48 characters.

So, to recap the possible solutions:

1. create a new pathauto table that has a much longer column for the "name" field
2. expand the width of the "name" field in the variable table
3. for pathauto to truncate the name of the node type before storing/retrieving the data from the variable table
4. change CCK (and other node related modules) to limit internal node names to 48 characters

While I previously thought that 1 or 3 were the best, I am now starting to think that proposal 2 is the best. Before we write up code, what do others think is the best idea?

Solution number 1 has the drawback of requiring a new database table which makes pathauto installation/upgrade more difficult. Number 2 has the drawback of affecting Drupal core. Number 3 has the drawback of possible name collisions. Number 4 has the problem of possibly impacting dozens of modules (e.g. flexinode?).

I am now thinking that number 2 might be the best...

ashtonium’s picture

ashtonium commented

I don't know about Flexinode, but CCK already uniquely truncates node types to 32 characters... which is of course still too long after "pathauto_node_" and "_pattern" get added on for pathauto's node pattern variable name.

yes, it is true that the patch does create a new issue of long node types potentially being truncated to non-unique strings. I've been trying to create a function that makes sure that the truncated, 26 character node types are unique (similar to how pathauto already makes sure it is creating a unique URL alias). But we may have to create a new variable of pre-truncated node types to compare against in order to make it work properly.

It is by no means my call, but I'd prefer to keep the patch internal to pathauto, which leaves:
1. - Create a new table for storing pathauto variables (name field would have to have a limit of at least 54 characters)
2. - Pre-truncate node types to a custom set of unique, 26 character strings
3. - Reduce the number of characters pathauto uses to label variable names (pathauto currently adds 22 characters on to the node type string for the pattern variable names, if this could be reduced to 16 characters, the node types (for CCK at least) wouldn't even need to be truncated.

ashtonium’s picture

ashtonium commented
Title: pathauto fails to retrieve pattern if node types have a long title » variable name compatibility with long CCK names
StatusFileSize
new pathauto.module_patternvariables_0.patch3.45 KB

changed the name of the issue, as it seems this is specifically a fix for CCK compatibility (flexinode names its node types flexinode-1, flexinode-2, etc.)

I have looked into CCK and, as previously stated, it uniquely truncates node types to 32 characters in length. So rather than mess about re-truncating, I decided that the easiest fix would be to shorten the number of characters pathauto uses to label its variables. By shortening the trailing "_pattern" string to "_p", and not touching the "pathauto_node_", we manage to come in just under the line at 48 characters.

This patch modifies pathauto.module to append "_p" instead of "_pattern" onto all pattern variables and also adds an update function to fix any pattern variables currently in your database. (build number is also upped to 6)

In order to update your pathauto database variables, you only need to visit /admin/settings/pathauto on your patched Drupal site.

If you have applied the pathauto_node.inc_4.patch patchfile in this issue, you will want to remove it in order to avoid the separate bug that it caused.

greggles’s picture

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

Ashtonium, thanks for your work on this. I think it's pretty darn close to being ready.

A couple comments:

1. it didn't apply cleanly. my reject file was as follows:

***************
*** 603,606 ****
      drupal_set_message('Upgraded pathauto from '.$installed_version['build'] .
        ' to '. $current_version['build']);
    }
- } // end function _pathauto_update --- 616,619 ----
      drupal_set_message('Upgraded pathauto from '.$installed_version['build'] .
        ' to '. $current_version['build']);
    }
+ } // end function _pathauto_update

2. The update system that pathauto uses is not standard - which confused me when I went to admin/modules and tried to run the update.php. However, making pathauto conform to the normal upgrade process should be its own bug.

3. The update process left the variable "pathauto_node_pattern" unchanged. The result of that was that pathauto used the default [title] value for my "Default path pattern (applies to all node types with blank patterns below):"

Number 3 seems like the only one worth investigating/fixing. Is there any reason you prefer to use the PHP method of changing the string to something like this query:

update `variable` set name = left( name, length( name) -6 ) where name like 'pathauto_%_pattern'

I only tested the query against mysql4.1.x but I have reason to believe that it should work in general.

gateone’s picture

gateone commented

Yeay!!! Worked nice with me. Patch did complain thought that it failed on the last hung at line 615. Comparing files though did definitively not show any errors, all the relevant patch stuff was inside the module file. Upon calling admin/settings/pathauto, the version number was also updated and now long CCK type names work absolutely like a charm. THANK YOU SO MUCH!

ashtonium’s picture

ashtonium commented
Status: Needs work » Needs review
StatusFileSize
new pathauto.module_patternvariables_1.patch2.35 KB

thanks for posting your results panatlantica it brought this issue back up on my radar (and apologies to greggles for taking so long to re-roll this patch)

This patch is against the latest 4.7.x-1.x-dev, and should apply cleanly. It's also using greggles' suggested SQL query instead of the longer PHP method from before.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

ashtonium - thanks for the update. What do you think about doing this using the hook_update_n system for it? I'm not terribly excited about continuing more updates that use the pathauto schema update system. There's a start of a pathauto.install file in the drupal-5 branch.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented


attempting to close the em tag...

ashtonium’s picture

ashtonium commented

Yeah, I think that's an excellent idea.

How much work would be needed to convert pathauto over to the standard hook_install & hook_update system?

I know the current updates don't need to be converted over, but how much of pathauto_admin_settings would need to go into a pathauto_install function?

I almost created a separate issue for converting pathauto to the standard install/upgrade process, but if nothing really needs to be done other than how this update is handled, then a new issue isn't needed.

ashtonium’s picture

ashtonium commented

my attempt at stopping itallics....

ashtonium’s picture

ashtonium commented

i spammed the last message with </em> tags in between the words... looks like the 2nd one worked.

ashtonium’s picture

ashtonium commented
Version: 4.7.x-1.x-dev » 5.x-1.2
StatusFileSize
new pathauto.module_patternvariables_3.patch2.42 KB

For those who may need it, I've updated the patch to apply cleanly to the latest pathauto release: 5.x-1.2

ashtonium’s picture

ashtonium commented
Version: 5.x-1.2 » 5.x-2.x-dev
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new pathauto_pattern_variables_5x-2x-dev_0.patch4.58 KB

Porting to 5.x-2.x-dev

I noticed that the 2.x branch got a full Drupal install profile (which is awesome!) so I decided to port this patch to that branch as well.

I've tested the patch and the update process (from both 5.x-1.2 and 5.x-2.x-dev), and everything seems to be working, and seeing as how greggles' request from comment #9 is now met, I'm marking it as RTBC.

BTW: I'm following the update_status module's lead by naming the update version number after the actual release number, but it can be easily changed to pathauto_update_1 without impacting anything else if desired.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented

ashtonium - you rock!

Thanks so much.

I'll definitely do a quick review and then apply this soon.

greggles’s picture

greggles
he/him
Primary language English
Location Denver, Colorado, USA
commented
Status: Reviewed & tested by the community » Fixed

So - somewhere between the creation of this patch and actually committing it the system_update_6002 got committed which made me think "why not just do system_update_6002 in pathauto?

See http://cvs.drupal.org/viewvc.py/drupal/drupal/modules/system/system.inst...

So, this should now be fixed since 128 characters should be plenty of space. You'll need to run update.php after installing this.

@Ashtonium - thanks so much for your work on this. I do appreciate it. In the end this (easier) solution makes more sense to me since we can still know that "_pattern" means pattern and not have to puzzle about what "_p" means.

Since it's in the dev release, I'm still open to other criticisms/ideas but I think this makes a lot of sense.

And of course I should have read your comment more closely about numbering my updates. Ugh.

ashtonium’s picture

ashtonium commented

yeah, much simpler. I'm glad my patch isn't needed anymore, I liked the self documenting "_pattern" as well. Good work.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)