-
assigned issue to
- edited description
Wrong assumptions
Good health to you, TorMec!
Your new code is better :).
Still, it's wrong.
Firstly, there is no such thing as "standard directory to install DokuWiki". It can be installed in various places, in various OS.
Secondly, you shouldn't assume that people use something "standard".
But this string is based on such assumption:
$dir = DOKU_INC . ltrim($conf["savedir"], "./") . "/pages";
As DW docs say, $conf["savedir"] can be full path, and in that case your code fails again.
Instead of taking fantasies for truth, you should make sure the code works with any possible data. You should think and understand which data can actually be there.
In this case, docs say there is $conf['datadir'], which actually shows the place of those .txt files. It can be './data/path/', but also it can be './hello/snusmumrik/'.
So what you should write there is:
$dir = realpath($conf['datadir']);
See realpath() in PHP manual.
Also, when you do such tasks, it's useful to search how others do such things.
Then you could learn elegant programming and discover many caveats.
See this string:
$sidebar = implode("/", array(str_replace(":", "/", $ns), $conf["sidebar"]));
implode is useful to automatically join many pieces. In case of few pieces you should just join them with "." (string concatenation operator):
$sidebar = str_replace(":", DIRECTORY_SEPARATOR, $ns) . DIRECTORY_SEPARATOR . $conf["sidebar"];
Use DIRECTORY_SEPARATOR constant instead of "/", because in Windows for example it's "\", not slash. So if you compare strings with different separators, your code would fail.
https://secure.php.net/manual/en/dir.constants.php
By the way, think about writing simple code.
Instead of
if (file_exists($dir . "/" . $sidebar . ".txt")) {
$ns_acmenu = $ns;
break;
}
write
if (file_exists($dir . "/" . $sidebar . ".txt")) return $ns;
and the last string return $ns_acmenu;
should be replaced with
return "";
Variable $ns_acmenu
isn't needed at all.
I hope you could learn to program in a safe and simple manner, and would do many useful things for people.
Good bye!
Comments (5)
-
repo owner -
repo owner As DW docs say, $conf["savedir"] can be full path, and in that case your code fails again.
You are right.
See this string:
$sidebar = implode("/", array(str_replace(":", "/", $ns), $conf["sidebar"]));
implode is useful to automatically join many pieces. In case of few pieces you should just join them with "." (string concatenation operator):
Actually, the right string is:
$sidebar = implode("/", array_filter(array(str_replace(":", "/", $ns), $conf["sidebar"])));
because
$ns
could be an empty string""
, which means$sidebar
could be equal to the wrong value/$conf["sidebar"]
wheres the right one is$conf["sidebar"]
.
$sidebar = str_replace(":", DIRECTORY_SEPARATOR, $ns) . DIRECTORY_SEPARATOR . $conf["sidebar"];
Use DIRECTORY_SEPARATOR constant instead of "/", because in Windows for example it's "\", not slash. So if you compare strings with different separators, your code would fail.
Can you provide an example in which the code would fail? The same DokuWiki engine doesn't use the constant
DIRECTORY_SEPARATOR
(for instance see https://xref.dokuwiki.org/reference/dokuwiki/inc/io.php.source.html#l35).
By the way, think about writing simple code.
Instead of
if (file_exists($dir . "/" . $sidebar . ".txt")) { $ns_acmenu = $ns; break; }
write
if (file_exists($dir . "/" . $sidebar . ".txt")) return $ns;
and the last string return
$ns_acmenu
; should be replaced withreturn "";
Variable
$ns_acmenu
isn't needed at all.I appreciate your coding style suggestions but I prefer to remain coherent with my old style code unless it causes bugs or reduces significantly the performances.
-
repo owner - changed component to code
-
repo owner - changed status to resolved
→ <<cset af179f439c32>>
-
repo owner - removed version
Removing version: v1.0 (automated comment)
- Log in to comment