Skip to content

Conversation

@FosseWay
Copy link

@FosseWay FosseWay commented May 18, 2019

Currently, if a required file or directory does not exist or is inaccessible, init.php will warn:

"The $c ('$p') at $path is not found, isn't accessible or writable.
                You should check your config and permission settings.
                Or maybe you want to <a href=\"install.php\">run the
                installer</a>?

Based on several forum postings, the existing message isn't always sufficient to get people the information they need.

Purpose

This PR aims to provide a bit more information by giving the specific nature of the permission failure: if the file isn't found, say it's not found; if not readable, say it's not readable and so on.

This PR also adds a link to the dokuwiki.org file permissions page, which might help someone find the information they need to resolve their permission issue during DokuWiki setup.

Rather than display the full path of the missing file (e.g. /var/www/html/wiki/some/file), the message should only display the path relative to the DokuWiki root (e.g. <DOKUWIKI>/some/file).

Example

image

Copy link
Collaborator

@phy25 phy25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea looks good to me.


// Self-help URL for DokuWiki Setup Error page
define('DOKU_SETUP_SELFHELP_URL', "https://www.dokuwiki.org/install:permissions");
define('DOKU_SETUP_SELFHELP_LINK',"<hr />You may wish to:<ul><li>Run the <a href=\"install.php\">DokuWiki installer</a></li><li>Review <a href=\"$self_help_url\">https://www.dokuwiki.org/install:permissions</a></li></ul>");
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess only one constant DOKU_SETUP_SELFHELP_LINK should be enough. Also I never see where $self_help_url is (which means it will never work).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @phy25. I used two constants so that the URL need only be explicitly written out once. If you strongly prefer that there be a single constant with the URL written out twice, I can do that of course. The $self_help_url is my mistake and must be corrected.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am still not sure where DOKU_SETUP_SELFHELP_URL is used (more than one)... My thought is eventually DOKU_SETUP_SELFHELP_LINK is used.

Copy link
Author

@FosseWay FosseWay Jun 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It isn't - that's my mistake - but the intention is that the code should contain both a link to the URL (the href attribute) and the link should also display the URL as the text of the link, something like this pseudocode:

<a href="DOKU_SETUP_SELFHELP_URL">DOKU_SETUP_SELFHELP_URL<a>

The intention is to avoid writing out the actual URL twice, since if it ever changes we would then have to change it in two places. Thank you for taking the time to review my PR. I apologize for the confusion.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah that makes sense. It's fine to make two constants then.

By the way thank you for following up - everyone is busy (and that's why I left this PR email in my inbox without doing the actual review for almost a month).

Copy link
Collaborator

@phy25 phy25 Jun 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK my initial thought when writing my last comment is that DOKU_SETUP_SELFHELP_URL can be changed by third-party developers or the environment without touching DOKU_SETUP_SELFHELP_LINK; but it seems that it will not work (You have to replace strings for DOKU_SETUP_SELFHELP_URL when reading DOKU_SETUP_SELFHELP_LINK which makes extra work). Also I don't think it is valuable just to avoid duplicate (= 2) occurrences of the URL, just in the link string. So, I prefer keeping only one DOKU_SETUP_SELFHELP_LINK constant here.

* @return bool|string
*/
function init_path($path){
function init_path($path,$required=null){
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please add comment before the function about the behavior of $required? I think this should be a boolean, not based on NULL or !NULL.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both good ideas. Thank you @phy25.

@mprins
Copy link
Contributor

mprins commented Jun 19, 2019

Rather than display the full path of the missing file (e.g. /var/www/html/wiki/some/file), the message should only display the path relative to the DokuWiki root (e.g. /some/file).

Why? I disagree - because it would make troubleshooting cases where the data or conf directories have been moved outside the webroot harder - but please elaborate.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants