Closed (fixed)
Project:
Ubercart
Version:
6.x-2.x-dev
Component:
Code
Priority:
Normal
Category:
Task
Assigned:
Reporter:
Created:
26 Dec 2008 at 19:35 UTC
Updated:
8 Jul 2009 at 17:40 UTC
Jump to comment: Most recent file
We have local tasks under node/% and user/%, but as we've seen referenced here: http://drupal.org/node/291554 that can lead to conflicts with other modules. Any local task under anything besides admin/store/% has been namespaced with uc- now. The exception to the rule is admin/user/user/expiration, which became admin/user/user/uc-roles-expiration now.
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | revert_menus.patch | 14.45 KB | Island Usurper |
| #6 | 351389.namespace_tasks.2.x.patch | 14.77 KB | cha0s |
| #4 | namespace_task.1.patch | 39.11 KB | cha0s |
| #3 | namespace_task.1.patch | 19.13 KB | cha0s |
| uc.task_namespace.patch | 14.62 KB | cha0s |
Comments
Comment #1
cha0s commented*bump*
Comment #2
rszrama commentedLooking over it it seems fine, but I'm curious if you've had this patch running and tested on a local install. i.e. actually clicking through to all the various affected pages to make sure we got it all covered.
Comment #3
cha0s commentedI'll do this in bite-size pieces.
Comment #4
cha0s commentedEr, got a little excited... this patch changes attribute and cart menu items.
Comment #5
rszrama commentedSo... I don't actually want to change top level paths like cart/*. There's no good reason for someone to have another shopping cart module on an Ubercart store, and this just doesn't look good/will screw with existing contribs in unknown ways. So... -1 to the last patch.
Just to go back to the original scope of this issue, it's for local tasks... meaning tabs on things defined by other modules, like nodes and users. We want to make sure our modules aren't conflicting on these things where other modules might also be adding tabs. Anything that's an Ubercart object or path doesn't need to be modified. People wanting to write modules interacting with Ubercart can work around its existing paths.
Comment #6
cha0s commentedTested... would appreciate another review. =)
Comment #7
Island Usurper commentedLooks good to me. Don't forget to clear your menu cache, at least.
Committed.
Comment #9
tr commentedThis patch was incomplete. In particular, in uc_order, the path user/#/orders was changed to user/#/uc-orders, but the paths starting with user/#/order (e.g. user/#/order/# and user/#/order/#/invoice) were NOT changed to user/#/uc-order.
I examined the menu_router table on my site and didn't notice any other paths that need a uc- qualification, but I don't have all the core Ubercart modules enabled so I can't say for sure that I caught everything.
Comment #10
rszrama commentedIn your opinion, do you think it would be better to go through and change all those or just revert uc-orders to orders? I don't know that I even remembered we had changed it to uc-orders, even though my comment above seems to indicate that's what I was expecting. : )
Comment #11
tr commentedIn a perfect world, I wouldn't want to see uc-orders appear in the first place. I don't think that URLs visible to the customer should contain uc- or other idiosyncrasies of the underlying implementation. The user knows what user/#/orders means, but user/#/uc-orders looks strange and might require you to explain to your customers (or your clients, if you're setting up a site for someone else) what the uc- is doing in there. They shouldn't have to or need to know.
However, I recognize WHY the change was made in the first place. Namely, the Menu API doesn't have any way to check on or prevent modules from making conflicting uses of a namespace. Thus for Ubercart to be relatively safe from menu path conflicts Ubercart should make an effort to use non-generic terms like "uc-orders" instead of just "orders" whenever Ubercart defines a sub-menu underneath some non-Ubercart path.
The use case that motivated the original change was node/#/edit/stock being claimed by both uc_stock and webforms. I don't know if this is really webforms, or this is just some name choice made by the admin when creating a webform. If the latter, I would classify it as a mistake by that admin, although the conflict really should be caught and indicated by the Menu API. If the former, then yes, we have a problem, because then webforms and Ubercart won't work together.
I think changing node/#/edit/stock to node/#/edit/uc-stock was reasonable, because the only people seeing these URLs are the people with permissions to edit these nodes. By definition, those people might need to have some idea of the underlying implementation, and the renaming is never seen by the customer. Although maybe product-stock might have been better than uc-stock, but I don't really care...
Likewise the admin/store tree is "owned" by Ubercart, so it wasn't changed. And although there is some small potential that another non-Ubercart module will come along and want to use the term "store", I agree there's no need to change this to "uc-store" to ensure its uniqueness.
I think user/#/orders should not have been changed to user/#/uc-orders, despite the small potential to conflict with other modules that use the user/ menu tree, mainly for the reason I stated in my first paragraph. BUT if the consensus IS to leave it as user/#/uc-orders, I think that user/#/order (without a trailing 's') should also be changed to user/#/uc-order. Not only for consistency but also because user/#/order has the exact same potential to conflict with other modules as user/#/orders. I don't see it as a real big deal either way, just apply the same reasoning to both, whatever you do!
Comment #12
Island Usurper commentedI'm of the opinion that adding uc- to the paths was a mistake. The original problem was caused by uc_stock adding its tab to all nodes, not just products. We just didn't realize it until recently. Now that this has been fixed, I think we should go back to the way it used to be.
Comment #13
Island Usurper commentedHeh. Forgot I committed this.