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
[Error Handling] Checks the currently running PHP-version #2353
Conversation
Let people know if they are running an unsupported version of PHP
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.
👍 - given the many bug reports with old PHP versions this seems really useful.
doku.php
Outdated
die('<h2 style="margin:1.2em;text-align:center;color:red;line-height:1.6;" >Error: You are currently running PHP/'.phpversion().'.<br> | ||
<span style="color:#606060;" >DokuWiki requires PHP/'.$required_php.' or higher.</span></h3>'); | ||
|
||
} else { |
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.
This else is not required as die()
terminates the script.
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.
Hi michitux, is it ok, when I write in german? I kwow this is not usual here but it is quite arduous to me to write english. And I have some questions...
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.
else
has been deleted.
doku.php
Outdated
if (version_compare(phpversion(), $required_php, '<')) { | ||
|
||
die('<h2 style="margin:1.2em;text-align:center;color:red;line-height:1.6;" >Error: You are currently running PHP/'.phpversion().'.<br> | ||
<span style="color:#606060;" >DokuWiki requires PHP/'.$required_php.' or higher.</span></h3>'); |
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 wonder if this could use nice_die()
(defined in inc/init.php
). For this, probably nice_die()
would need to be moved out of inc/init.php
to ensure that no code that is incompatible with old PHP versions is executed.
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.
So far as I can see (but I am really a raw recruit in PHP programming and in DokuWiki core anymore) there is no significant benefit that make it warrantable to move nice_die() to doku.php
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 understand why people want this kind of check, but I wonder...
- wouldn't this need to be checked in the installer, too?
- we also have the minimal required PHP version in the composer file, would it make sense to load it from there?
- is checking this on every page load again and again, worth it? (it's not a huge overhead now, but if we would get the version from the composer.json it would be)
- we may want to use newer PHP features in doku.php some time, which would break this feature again
doku.php
Outdated
@@ -11,6 +11,16 @@ | |||
// update message version - always use a string to avoid localized floats! | |||
$updateVersion = "50"; | |||
|
|||
// let people know if they are running an unsupported version of PHP | |||
$required_php = "5.6"; |
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.
This should probably be a constant and be reused in inc/info.php where we also check for the PHP version.
doku.php
Outdated
|
||
die('<h2 style="margin:1.2em;text-align:center;color:red;line-height:1.6;" >Web Server Setup Error: The web server currently runs PHP/'.phpversion().'.<br> | ||
<span style="color:#606060;" >DokuWiki requires PHP/'.$required_php.' or higher.</span></h3>'); | ||
} |
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 agree with @michitux, this duplicates the nice_die()
method
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.
Sorry for the answer in german! I know that's not usually here! And I know the reason why...
Ja, das mit dem duplicate ist wohl richtig, aber ist das in irgendeiner Hinsicht ein Problem oder ist damit ein ernsthafter Nachteil verbunden? Sollte (wie michitux erwähnt hatte) ein Risiko bestehen, dass bei der Verwendung von nice_die()
ältere PHP-Versionen nicht damit arbeiten können, dann würde ja gerade in diesem Fall das Abfangen des Fehlers scheitern. An dieser Stelle sage ich: safety first! Ich halte es also grundsätzlich für am besten, an dieser Stelle so wenig wie irgendmöglich zu tun, damit das auch bis in alle Ewigkeit funktioniert. Und wer weiß, was in Zukunft noch alles in nice_die()
hineinprogrammiert wird.
Außerdem gefällt mir nicht, dass in nice_die()
ein "DokuWiki Setup Error" ausgegeben wird. So eine Meldung ist für Änfänger irreführend, da sie den Eindruck vermittelt, die Fehlerursache wäre innerhalb von DokuWiki zu finden.
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.
is checking this on every page load again and again, worth it?
Was aus meiner Sicht dafür spricht ist folgende Geschichte: Vor einigen Jahren, als blutiger Anfänger mit DokuWiki und PHP (ich wusste kaum was das ist) funktionierte meine erste DW-Installation 3 Tage lang ganz prima. Bis dann plötzlich ein fataler Fehler auftrat und gar nichts mehr ging. Ursache: Weil ich das "Versäumnis" begangen hatte, die PHP-Version explizit festzulegen, wurde sie providerseitig nach 3 Tagen auf eine ältere Version zurückgeschaltet.
Ich meine, man sollte sehr bedenken, dass das hier ein Problemfeld ist, was alle Nicht-Nerds in schwerste Bedrängnisse bringt. Gerade dann, wenn man den Otto-Normal-User so vehement auffordert mal so eben auf das allerneuste Release zu upgraden.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
This comment was marked as off-topic.
This comment was marked as off-topic.
Sorry, something went wrong.
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.
we may want to use newer PHP features in doku.php some time, which would break this feature again
Das verstehe ich nicht. Warum sollten irgendwelche zukünftigen Änderungen den PHP-Versionscheck kaputt machen, da unser Fehlercheck doch ganz am Anfang stattfindet?
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.
Modern PHP in the same file will break the check, because the file is parsed before executed. When the file contains code that is not understood by the used PHP, the parser will fail before any code (including our check) is executed.
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.
Modern PHP in the same file will break the check, because the file is parsed before executed. When the file contains code that is not understood by the used PHP, the parser will fail before any code (including our check) is executed.
Ok, verstanden. Kann ich eigentlich auch selbst meine eigenen Kommentare als "off topic" markieren?
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 parser will fail before any code (including our check) is executed.
Ok, dann dürfen wir in doku.php
nur noch den Versionscheck machen und eine PHP-Routine aufrufen, in der alles das getan wird, was jetzt in doku.php
getan wird.
If we keep this in doku.php, I would argue for the most simple and minimal implementation. Something like this: define('DOKU_MIN_PHP', '5.6');
if (version_compare(PHP_VERSION, DOKU_MIN_PHP, '<')) {
die('Your PHP version '.PHP_VERSION.' does not meet the required minimum of '. DOKU_MIN_PHP);
} |
I would prefer a more meaningful error message:
|
why has this been closed? |
@splitbrain I need more time to make a (slightly) better solution. (for example to implement it (as you said) in INSTALL.PHP too) And I need time to trying Github - particularly Github Desktop and something near it. I want to know how to work with all these mysterious things before I should make my next steps at this place here ... For example I'm looking for an easy to handle way to sync between Github Desktop and my webspace so I can comfortable and securely test my "code-snipped". {off-topic}... und jetzt brauche ich erst einmal eine Kopfschmerztablette ... {/off-topic}
What is your opinion about the idea to move the whole code out of I'm an enthusiastic fan of easy to handle systems ... therefore for me the overhead is less important ... I love all solutions there are |
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.
To be needed: Ch [...] L.PHP too
Sorry, I want to delete this review but cannot.
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 should [...] oser.json
Sorry, I want to delete this review but cannot.
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.
And think about [...] ne-comments.)
Sorry, I want to delete this review but cannot.
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.
Needed: [ ... } ...ting
Sorry, I want to delete this review but cannot.
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.
Decision [... ... ...] furthermore.
Sorry, I want to delete this review but cannot.
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.
Sorry, I want to delete this review but cannot.
Sorry I'm learning by doing...
My (Michaelsy's) Joblist and open issues: Open tasks
Open Issues
Vacant job Decision needed from one of the maintainers: Maybe to move the whole code out of |
Question: What is the function of If only during run time it makes sense (IMHO) to insert a comment which informs the editor that |
Comments in json-files I found out this: -------------------8<----------------
jsonlint does not validate this. So comments are a parser specific extension and not standard. -------------------8<---------------- "Comments are not an official standard". 👎 --> seems to be an outdated statement |
I haven't looked into composer myself yet but it loads all dependencies which your project has (in our case DokuWiki). Basically it loads all external classes which are needed to make DokuWiki work. Maybe do a quick web search yourself or have a look at:
PHP is an interpreted language, there is no compile/building time for PHP. |
But there is a building time for the project (so far I understand). My idea is to edit two PHP-files during this building. (to define the same constant twice)
Thank you for your effort! |
…DOKU.PHP too" Warning: Potentially this comment is not valid code.
@splitbrain said:
Cause in the install process overhead is not an issue we can perform inside |
@splitbrain Hi Andi
We only compare one constant with another (*1). This compare costs around the same as the following check which we also do at the beginning of several PHP's:
(*1) IMHO we should not get the version from the |
@@ -5,7 +5,7 @@ | |||
"type": "project", | |||
"license": "GPL v2", | |||
"require": { | |||
"php": ">=5.6", | |||
"php": ">=5.6", // IMPORTANT: If required PHP changed you have to update INSTALL.PHP and DOKU.PHP too. |
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.
Comments are not allowed in composer.json
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.
Comments are not allowed in composer.json
Thanks for your statement!
Do you have already read my collection of "reserch-informations" in this PR? And please take a look on the buildings protocols accessible via the given links.
Update: Oh sorry maybe my review was not stored by my and you cannot read it.
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.
Test
@@ -5,7 +5,7 @@ | |||
"type": "project", | |||
"license": "GPL v2", | |||
"require": { | |||
"php": ">=5.6", | |||
"php": ">=5.6", // IMPORTANT: If required PHP changed you have to update INSTALL.PHP and DOKU.PHP too. |
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.
Comments in composer.json ?
Problem: Potentially / maybe the inserted comment
is not valid code.
Research
-
"Comments in JSON files are not an official standard. " --- Read this --- but: posting date: Oct 26 2011 (?) --- seems to be a bit outdated
-
Does DokuWiki's JSON-parser supports comments? --> probably : YES
-
Which JSON-parser does DokuWiki's project use? --> seems to be no more a relevant question
Travis Builds: --- 1 "allowed failure": // Travis1 - 10 builds ok - 1 build fails // Travis2 This is the one build with the one ("allowed") failure //
AppVeyor Builds: AppVeyor 1 AppVeyor 2 (no failures, no warnings)
Conclusion
The comment // ...
in composer.json
seems to be no problem.
// ...
in composer.json
seems to be no problem.
Please have a look at https://www.dokuwiki.org/devel:composer for explanation how composer is used in the development of DokuWiki. |
According to your information the test-buildings are working with other parsers as our builder/composer?! |
The unit tests don't use composer. Composer is used for updating the included libraries. But to have a complete download file of dokuwiki, the files are checked into the git repository as well. (e.g. an composer update that is checked in: ddb94cf) |
Let people know if they are running an unsupported version of PHP