-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Handle tilde, initial dots and semicolons in ns links #140
base: master
Are you sure you want to change the base?
Conversation
Thanks for that PR, it looks interesting. |
|
||
// Convert all other separators to colons | ||
if ($conf['useslash']) $path = str_replace('/', ':', $path); | ||
$path = str_replace(';', ':', $path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand that line: in which case could it make sense to have a ;
in $path
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
;
as alternate namespace separator?
See: dokuwiki/dokuwiki#3857 (comment)
global $conf; | ||
|
||
// Convert all other separators to colons | ||
if ($conf['useslash']) $path = str_replace('/', ':', $path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I completely understand that line, or more precisely, whether we should test against that option. I mean: here $path
is provided by the user, so I guess the question is whether we want to allow a syntax like <nspages my/ns>
. If we do, then I think there is no ambiguity when this syntax is used (because, according to https://www.dokuwiki.org/pagename ; slashes are not allowed in page name, so if a user puts a /
it can only mean that it's intended to be a namespace separator).
To put it in a nutshell: if we want to allow such syntax, shouldn't we support it regardless of the value of that conf option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The short answer is that, that's just how dokuwiki implements it for page links.
For example if useslash
is not set then the page link my/ns
is resolves to my_ns
(because it goes through cleanID). But if it is set, it resolves to my:ns
.
But since nspages doesn't call cleanID and assumes the user input is valid, I suppose that it makes sense to always treat slashes as namespace separators.
Btw nspages, as it is, happens to already handles "/" in paths. That's because the slashes are never removed nor escaped before it is converted to a filesystem path.
$result = ''; | ||
$wantedNS = trim($path); | ||
if($wantedNS == '') { | ||
$wantedNS = $this->getCurrentNamespace(); | ||
} | ||
if( $this->isRelativePath($wantedNS) ) { | ||
$result = getNS($ID); | ||
// normalize initial dots ( ..:..abc -> ..:..:abc ) | ||
$wantedNS = preg_replace('/^((\.+:)*)(\.+)(?=[^:\.])/', '\1\3:', $wantedNS); | ||
} elseif ( $this->isPageRelativePath($wantedNS) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm afraid I don't understand in which case this would be used. In particular $wantedNS
is supposed to represent a namespace, but if I understand correctly https://www.dokuwiki.org/namespaces the syntax with a ~
(that I was not aware of btw, thanks for drawing my attention on that part of the doc) is supposed to represent a page, so it does not sound like something nspages should support.
Long story short: I would be grateful if you can give me an example where this would make sense (having an example would also make it easier for me to write non regression tests on that feature)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider the following hierarchy:
docs.txt
docs/process.txt
docs/process/work_process.txt
docs/process/hr_process.txt
if you use <nspages ~>
in docs:process
, it would list the pages in the docs:process:
namespace, which are the pages docs:process:work_process
and docs:process:hr_process
.
Compare this to <nspages>
which would list pages in the docs
namespace, which is docs:process
.
Honestly, I just wanted to help close #128.
As a side note, a typical dokuwiki user would instead have docs/process
moved to docs/process/start.txt
and just use <nspages>
in docs:process:start
.
Those paths are supported by dokuwiki (see doc at https://www.dokuwiki.org/namespaces ) so it makes sense to make it for nspages users to use that This is taken from PR #140 (because that part of the PR is already pretty clear to me so users can already have that feature without having to wait until I find the time to complete handling that PR)
Tilde operator was recently added, see https://www.dokuwiki.org/namespaces
This probably helps with #128.