Closed (fixed)
Project:
Project
Version:
6.x-1.x-dev
Component:
Projects
Priority:
Normal
Category:
Feature request
Assigned:
Issue tags:
Reporter:
Created:
21 Mar 2007 at 04:58 UTC
Updated:
2 Jan 2014 at 23:34 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
dwwlooks pretty good to me. but, i'm not using my normal workstation, so i can't actually test this well right now. will take another look when i have access to my test sites again.
Comment #2
dwwstill haven't tested this, but i noticed a minor gripe with the code style:
Trueshould beTRUE. otherwise, this really should be RTBC.Comment #3
aclight commentedNote also that this patch creates a new variable, but does not include code in project.install to delete the variable in hook_uninstall(). That function might not have existed when this patch was originally written, but it's there now.
Comment #4
agentrickardThe 'project' path is a normal menu item and can be disabled via the menu system.
Turned off path aliasing makes sense to me, patch attached.
Comment #5
aclight commentedI could be wrong, and haven't checked the code, but it seems like we'd need some additional code in project_project_breadcrumb (or whatever that function is called), since I seem to recall that this function relies on the path of projects being /project/
Comment #6
agentrickardDoes not appear to be an issue. The attached screenshots show the output of project_project_set_breadcrumb() with the patch applied and not. The menu system recognizes the project node as part of the Project* menu hierarchy in both cases.
Comment #7
aclight commentedThis looks good to me, but I have a few comments:
1. Do we want to wrap:
in something like:
Because if the path module isn't enabled, the setting will make no sense because aliases won't be generated anyway.
2. I fixed a code style error in the patch and reworded the title and description of the setting to be (IMO) more clear.
3. The description text for the Project short name (URI) element on the project node form is this:
"This will be used to generate a /project// URL for your project. This string is also used to generate the name of releases associated with this project."
However, if this new setting is disabled, an alias as such won't actually be created. This would also be the case in the module as it is now if the path module is disabled. Do we want to somehow change this description to be more clear? I'm not sure it's necessary, but for example if the user wanted to use pathauto to create aliases at /project/foo//, then I think the description in that field could confuse project owners.
Comment #8
aclight commentedAlso, what about the code in project_project_view(). $project->uri is used directly there to create links, and that's going to cause a problem if the shortname isn't aliased as expected, I believe. Setting to CNW for that reason.
Comment #9
agentrickardAttached revision addresses the issues above, as follows:
1) Wrapped in a check, and the variable is set to FALSE if path does not exist.
2) Incorporated.
3) Fixed by adding variant text based on the setting. Also required a modification to project_release_alter_project_form().
As for the links in project_project_view(), these are not affected. $node->uri is actually the stored value of the short name from the form. These do not seem to be path aliases. Instead they are links that are interpreted by project_project_retrieve(), which accepts either the short name or the node id of the project.
Comment #10
dwwI'd love to get this in before 6.x-1.0 (and I'd probably be willing to commit a backport to D5 if anyone still cares). Anyone up for a re-roll?
Comment #11
fuzzy_texan commentedTagging for #439958: Document issues left before 6.x official release
Comment #12
dwwRerolled for D6 in HEAD.
Comment #13
dwwCommitted to HEAD. I don't care enough for a backport to D5, but if anyone's interested, feel free to reopen this with a patch.