Wiki Edit Url enhancement

This is an interesting idea... this would require some extra path work, but its doable. Postponing the issue for now.

Okay, so I've made the required patch against the 5.x-1.x. It was pretty easy and could be extended for other actions besides "edit", which is all it detects.

I don't like the way that this is hard-coded into wikitools_handle_request. The $action variable should be used to module_invoke('wikitools', $action .'_action'); so that way other actions can be added more easily.

I don't fully understand how that would work, cwgordon. I just imaged actions being resolved to child pages of the node, like Edit, Talk (for comments), etc. So you propose instead of redirecting a user you just invoke a specific function? But then what would that function do? In the case of allowing a user to edit a node, why wouldn't you just go to the node's edit page?

You would just go to the node's edit page in the case of the node. But to make this as coder-friendly as possible, let's make adding actions easy. There was nothing inherently wrong with your code— let's just see if we can do it in a way so as to make our lives easier in the future.

In the case of the edit action, a simple drupal_goto() function would work fine.

I think I shouldn't have used the variable name action, as I don't think that's what this should be thought of. It would be more of a subpage specifier, as it just directs us to a subpage of that wiki page.

I can't really imagine a usage for reading for specific actions, at least right now, especially when action naming conflicts could happen. I can, however, immediately see the benefit of providing a "hook" in a sense to other modules through their defined subpages for this specific node type. If we abstract out the hook to the menu system, then we have a perfectly extensible system for this: just add subpages to node/%node through wikitools_menu() and you're done.

I rerolled this now, based on Susurrus patch. I changed it so you can choose the possible subpages based on a setting. So you can now add "talk" to the setting if you have the talk module for example. It is still simple as there are no special actions besides a simple redirect to the activated subpages.

There is a patch against the DRUPAL-5 and DRUPAL-6--1 branches.

So why is this done this way? I don't understand why you'd need to whitelist subpages that are available under a node? If you don't want user to have access to the page or for it not to exist, modify that user's role access permissions or disable the module providing that subpage.

I don't see why any more logic other then just passing through that extra argument is needed as the menu system was designed specifically for handling these kinds of situations. Just always pass that argument through and let the menu system take care of it.

The problem is that page titles are allowed to have a "/" in them (This is also the case in Mediawiki). So you could not distinguisch between the (legal) page "title/subtitle" and the subpage "title/edit" where you want to redirect to the edit page.

So either we disallow "/" in node titles, or we need to know which subpages exist.

I would think that disallowing "/" in node titles would be the way to go. Either that or node titles would just need to be URL encoded so that "/" becomes "%2F", which is how it should be when those reserved characters are in a node title.

While it's good to emulate Mediawiki as close as possible, as I think that's pretty much the standard for wikis, I think this deviation would not significantly reduce functionality/usability and would drastically simplify things code-wise. As it is now, those specific words can't be used after a forward slash in a node title anyways as it is, so user who create nodes (who may be anonymous and not fully understand the node title restrictions) may become confused. I think disallowing this is better than unknown functionality.

pearwiki_filter and freelinking do encode slashes in links, so that would be ok. Then it's only manually entering the link, but that is probably not much of a problem if that doesn't work.

Looks great, much better, I have no objections to it being committed now after someone has had a chance to test it.

It doesn't really work with encoded slashes. In my setting they are already decoded by apache (or Drupal). So the path

wiki/abc%2Fdef is treated as wiki/abc/def
even wiki/abc%252Fdef is treated as wiki/abc/def
Only if I use wiki/abc%25252Fdef it is interpreted as wiki/abc%2Fdef

So you cannot use a menu item 'wiki/%' as this only matches one part of the title even if the slash is encoded.

So would banning "/" in node titles be a big loss since encoding isn't an option?

I just read the wikipedia policy about it. The problem is just when you have a title which really has a slash in it (examples from Wikipedia: OS/2, AM/PM). I'm not sure if I would be willing to ban slashes on my site just for this feature (probably I would ;)

But since the number of possible subpages is quite limited (view, edit, delete, revisions, talk are all I know of), I think it would be overkill to ban the slash because of a couple of subpages.

About confused users: Maybe improving the error message could help there? Something like "Your title is not allowed to end with any of the following: /edit, /delete, ..."

Why can't this still work like normal? Even if you have a node titled "Page/edit", why couldn't you access the edit page by just going to "Page/edit/edit". I guess the strategy would be to, if there're any "/" in the path, strip off the last query part (like edit, talk, etc.) and see if that's in the list included in the module and then find the node using the part that's left.

Examples:
/wiki/page/edit
1) Has "/" so strip off ending
2) Ending ("edit") matches our list(edit, delete, view, revisions) so..
3) Actually find the node based on the title "page"

/wiki/os/2
1) Has "/"
2) "2" doesn't match our list
3) Append "/2" back onto node title and search for node

The problem is if you have two pages:

"Page"
"Page/edit"

Now the url "wiki/page/edit" is ambiguous, either the page "Page/edit" or the edit page of "Page".

I admit that this will probably never happen, but we have to consider it nonetheless.

I think we're thinking about this all wrong, we're trying to take the approach of throwing more crap into the "q" variable, what about a new variable? What about it looking like:
"wiki/page" -> Takes you to node titled "page"
"wiki/page?action=edit" -> Takes you to node titled "page" and it's subpage named "edit"

While it's technically "uglier", it solves all of our problems and users are used to seeing lots of ugly GET variables in the URL anyways, so no loss there. While this impedes the typability of just adding /edit on the end of the URL, I highly doubt normal users do that, and they'd get the hint pretty quickly once they say the action variable.

Of course we could add "?action=edit", but this issue was made for the "/edit" suffix ;)

If we can't decide, how about both?
- You specify a list of subpages (edit, revisions, ...)
- You can enable/disable the "?action=xyz" query (where the name "action" can be customized)
- You can enable/disable the "/xyz" path suffix and thus disallowing titles to end in "/xyz"

And wikitools already allows you to forbid the use of "/" in titles.

With this setup, you can choose for yourself how (if) you want this feature.

Although all these possibilities bring back the need for proper documentation.

well, why don't we change the available options to the user based on if the user has selected to disable the "/"? That way if the user has that option checked, they don't need to worry about anything. Otherwise the user will have those other more complicated options.

Next try (There is some general code cleanup changes in the patch which you can ignore):

Have a look at the settings and tell me what you think. You can now choose to enable one or the other (or both) variants. Also, the 'title' check is only done if you have the "/" allowed for titles. I don't think the options are very complicated, although it doesn't mention the fact yet that you will ban titles ending in "/edit" if this option is activated.

The patch is for the 6.x branch. I will do a backport once this feature is in the 6.x version.

Okay, I've tested the patch and made some improvements that are attached. Patch is against 6--1 branch:

• Override freelinking module is only an option if the module's installed
• Overriding the freelinking callbacks is now done in hook_menu_alter()
• Options to allow subpage visits are now either a query string or an extra url parameter depending on if the user disabled '/' or not
• The query argument is no longer configurable. It's confusing to have more options, so I just changed the name to 'wt_action' instead of leaving it 'action', which should prevent any conflicts with other modules.
• Added explanation about "/" character and how it changes the query passing.

I don't like that you have to disable "/" in titles for the URL suffix option. Personally I would allow "/" in titles AND use URL suffixes (thus disallowing titles to end in "/edit").

The other problem I see is that the settings page changes based on the "/" in disallowed characters. I admit this is maybe far fetched, but consider the following scenario:

1. Disallow "/" in titles and save.
2. Enable URL suffixes and save.
3. Remove "/" from titles and save.
Now, "/" is again allowed in titles, but URL suffixes are still activated.

If these two options are exclusive, you need to check after form submission that the settings are still consistent.

I moved the 'freelinking/%' path to hook_menu because it's not altering the menu. The freelinking module only registers the 'freelinking' path, so by adding the '/%' it's actually a new path.

Have you tested that procedure you outlined above? These options are guaranteed to be exclusive in wikitools_admin_settings_validate(). What would happen is that the URL suffixes would turn into the query string option automatically, as the "/" character was added to the disallowed characters setting. I think this is clearly enough documented, that it should make sense.

I would think that the freelinking filter menu item should be hijacked using the hook_menu_alter() and reregistering the "freelinking" path instead of adding the new "freelinking/%" path, as I think that makes more sense.

I overlooked the "validate" part. My bad.

By adding the new "freelinking/%" path, the original freelinking page which shows all the links is still available. Otherwise if you go to "freelinking" it goes to the default page.

So, what needs to be changed for this patch? It seems like that menu registration should technically go in hook_menu(), but is that it? You mentioned that you didn't like not being able to have slashes in titles and not being able to do "wiki/page/edit". What should happen with that?

I combined now the patches. There are also some other changes:

- description of "unique titles" changed
- default options set to "create node", "search node", "unique titles" and "underscore as space"
- select fields as checkboxes for better usability

You can still enable the url suffix with allowed slash in titles. Thus you also have to be able to specify a list of allowed subpages and the validation check for titles is also necessary.

I'm going to commit the patch if you don't have any big objection. Then we can discuss this further based on the checked in code as it has now a couple of changes not related to that issue.

While I don't like the "allowed subpages" option, as I think it's unnecessary, I think this patch is a definite improvement for this module. I think it's ready for a commit. I like the new checkboxes and radios

It's commited.

If no major issues arise I will do an official release next week.

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