[Error Handling] Checks the currently running PHP-version #2353

Michaelsy · 2018-04-26T11:02:20Z

Let people know if they are running an unsupported version of PHP

michitux

👍 - given the many bug reports with old PHP versions this seems really useful.

michitux · 2018-04-26T14:43:42Z

doku.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>');
+
+} else {


This else is not required as die() terminates the script.

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...

else has been deleted.

michitux · 2018-04-26T14:45:33Z

doku.php

+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>');


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.

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

splitbrain

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

splitbrain · 2018-04-27T06:21:37Z

doku.php

@@ -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";


This should probably be a constant and be reused in inc/info.php where we also check for the PHP version.

splitbrain · 2018-04-27T06:22:28Z

doku.php

+
+  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>');
+} 


I agree with @michitux, this duplicates the nice_die() method

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.

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.

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?

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.

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?

[...] 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.

splitbrain · 2018-04-27T07:42:11Z

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);
}

Michaelsy · 2018-04-27T08:11:46Z

I would prefer a more meaningful error message:

define('DOKU_MIN_PHP', '5.6');
if (version_compare(PHP_VERSION, DOKU_MIN_PHP, '<')) {
die('WEB SERVER SETUP ERROR: Your PHP version '.PHP_VERSION.' that is running on your web server does not meet the required minimum of '. DOKU_MIN_PHP);
}`

splitbrain · 2018-04-28T10:37:57Z

why has this been closed?

Michaelsy · 2018-04-28T11:01:14Z

@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}

~~One more reason: I don't understand why "Some checks were not successful".... and what this mean to me ...~~ Update: Seems to be outdated

What is your opinion about the idea to move the whole code out of doku.php so doku.php cannot be touched by newer PHP? I guess that's not yours ... ?

I'm an enthusiastic fan of easy to handle systems ... therefore for me the overhead is less important ... I love all solutions there are ~~really safe~~ as far as possible safe.

Michaelsy

~~To be needed: Ch [...] L.PHP too~~
Sorry, I want to delete this review but cannot.

Michaelsy

~~I should [...] oser.json~~
Sorry, I want to delete this review but cannot.

Michaelsy

~~And think about [...] ne-comments.)~~
Sorry, I want to delete this review but cannot.

Michaelsy

~~Needed: [ ... } ...ting~~
Sorry, I want to delete this review but cannot.

Michaelsy

~~Decision [... ... ...] furthermore.~~
Sorry, I want to delete this review but cannot.

Michaelsy

Sorry, I want to delete this review but cannot.
Sorry I'm learning by doing...

Michaelsy · 2018-04-29T08:29:35Z

My (Michaelsy's) Joblist and open issues:
(The creating and editing of the joblist itself is a work in progress too)

Open tasks

~~To take a look at composer.json (and probably insert a redundancy comment)~~ to take a look done... Next step: Clearing the question whether and how to insert comments in composer.json (of course only if this makes sense) --> done
~~Checks the currently running PHP-version in INSTALL.PHP too~~ done
Code testing
Resolving a conflict which causes that the merge do not work (test-merging between my own branches) (Ich kapiere nicht, warum dieser Konflikt in diesem PR hier nicht auftritt, aber bei meinem Test-PR schon.) --> obsolet

Open Issues

Thinking about to handle the redundancy of maybe three times defining DOKU_MIN_PHP. (Presumably it should be handled by appropriate inline-comments. ~~IMHO probably the most pracmatic solution in the face of given circumstances.~~ ) ~~Flash of inspiration: Aaaah: To solve the redundancy issue is the job of the composer-file ??!~~ ???? Is the composer file active in compile time or in run time ??? Is there a possibility to solve the redundancy during ~~compile~~ building/"compile" time? (Like C's preprocessor) So there is no redundancy (during "coding time") and no unnecessary overhead during run time. --> obsolet
I already readed the relevant help pages (multiple times) but still need assistant: Can I labeling this PR? If yes: how? If no: why not? I founded no way to labeling this PR ~~but cannot understand this~~ meanwhile I understand a little bit better. ->no more a question for me

Vacant job

Decision needed from one of the maintainers: Maybe to move the whole code out of doku.php? So doku.php cannot be touched by newer PHP-versions-code. For the goal of these PR it's important Doku.php don't lose the compatibility with every old PHP-version in the future.

Michaelsy · 2018-04-29T09:05:58Z

Question: What is the function of composer.json? And by which way (resp. when) the informations inside composer.json comes to effect? During "comile/building-time" or run time?

If only during run time it makes sense (IMHO) to insert a comment which informs the editor that doku.php and install.php needs to be edited too.

Michaelsy · 2018-04-29T09:37:22Z

Comments in json-files I found out this:

-------------------8<----------------
Comments are not an official standard. Although some parsers support C-style comments. One that I use is JsonCpp. In the examples there is this one:

// Configuration options
{
    // Default encoding for text
    "encoding" : "UTF-8",

    // Plug-ins loaded at start-up
    "plug-ins" : [
        "python",
    "c++",
        "ruby"
        ],

    // Tab indent size
    "indent" : { "length" : 3, "use_space": true }
}

jsonlint does not validate this. So comments are a parser specific extension and not standard.

-------------------8<----------------
Source: stackoverflow.com

"Comments are not an official standard". 👎 --> seems to be an outdated statement
"Although some parsers support C-style comments." 👍 --> meanwhile a lot of parsers does it (most of all?)

lpaulsen93 · 2018-04-29T09:55:59Z

Question: What is the function of composer.json? And by which way (resp. when) the informations inside composer.json comes to effect?

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:

During "comile/building-time" or run time?

PHP is an interpreted language, there is no compile/building time for PHP.

Michaelsy · 2018-04-29T10:04:19Z

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)

~~I assume there is no way to transfer some information during this building time from composer.json to some PHP-Script (like the preprocessor in C) ???~~
Sorry, first I should read your links...

Thank you for your effort!

…DOKU.PHP too" Warning: Potentially this comment is not valid code.

Michaelsy · 2018-04-30T04:47:09Z

@splitbrain said:

we may want to use newer PHP features in doku.php some time, which would break this feature again

Cause in the install process overhead is not an issue we can perform inside install.php only the PHP-check and nothing else (and call a script where we done what we done in install.php at the moment). So our PHP-check cannot be touched from newer PHP features in the future.

Michaelsy · 2018-04-30T08:54:27Z

@splitbrain Hi Andi

is checking this on every page load again and again, worth it?

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:

if (!defined('DOKU_INC')) die();  /* must be run from within DokuWiki */

(*1) IMHO we should not get the version from the composer.json. I suggest to handle the redundancy by comments. (However it's an open question whether this is possibel in composer.json.)

Test PR 2

Klap-in · 2018-05-01T22:16:01Z

composer.json

@@ -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.


Comments are not allowed in composer.json

@Klap-in

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.

Michaelsy

Test

Michaelsy · 2018-04-30T03:23:03Z

composer.json

@@ -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.


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

JSON5 supports comments

JSON5 Parsers

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.~~

Klap-in · 2018-05-01T22:44:42Z

Please have a look at https://www.dokuwiki.org/devel:composer for explanation how composer is used in the development of DokuWiki.

Michaelsy · 2018-05-01T23:07:43Z

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?!

Klap-in · 2018-05-02T07:44:18Z

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)

Michaelsy · 2018-05-02T14:20:05Z

Reason of closing

This PR is too ugly as @Klap-in mentioned in #2362. Furthermore it includes 12 commits (for only 3 file edits!) with conflicts between the commits.

I will create a fresh new one. Maybe it will takes some time.

Checks the currently running PHP-version

8586cd6

Let people know if they are running an unsupported version of PHP

michitux requested changes Apr 26, 2018

View reviewed changes

Michaelsy added 2 commits April 26, 2018 19:55

More significant error message & "else" deleted

809a771

One more step better error message

b8a20db

splitbrain reviewed Apr 27, 2018

View reviewed changes

Michaelsy added 2 commits April 27, 2018 11:29

The most simple and minimal implementation

8650878

One more little step of more informative value

23c43cb

Michaelsy closed this Apr 28, 2018

Michaelsy added 2 commits April 28, 2018 10:18

+comment

bd8237d

Checks PHP version in INSTALL.PHP too

bed21d1

splitbrain reopened this Apr 28, 2018

Michaelsy commented Apr 28, 2018

View reviewed changes

Michaelsy added 3 commits April 30, 2018 05:00

Refer to COMPOSER.JSON also

6d40126

Checks also the PHP version in INSTALL.PHP

382d3a2

Comment: "If required PHP changed you have to update INSTALL.PHP and …

eebca71

…DOKU.PHP too" Warning: Potentially this comment is not valid code.

Michaelsy mentioned this pull request Apr 30, 2018

[Feature Request] [Error Preventing] Checking PHP version in the pre-upgrade phase #2362

Open

Michaelsy added 2 commits May 1, 2018 11:24

Merge pull request #3 from splitbrain/master

9a86d77

Test PR 2

Merge branch 'php_version_check' into patch-1

a374db1

Klap-in reviewed May 1, 2018

View reviewed changes

Michaelsy commented May 1, 2018

View reviewed changes

Michaelsy changed the title ~~Checks the currently running PHP-version~~ [Error Handling] Checks the currently running PHP-version May 2, 2018

Michaelsy closed this May 2, 2018

[Error Handling] Checks the currently running PHP-version #2353

[Error Handling] Checks the currently running PHP-version #2353

Conversation

Michaelsy commented Apr 26, 2018

michitux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michaelsy Apr 26, 2018 • edited

Choose a reason for hiding this comment

Michaelsy Apr 26, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

splitbrain left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michaelsy Apr 27, 2018 • edited

Choose a reason for hiding this comment

Michaelsy Apr 27, 2018 • edited

Choose a reason for hiding this comment

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

Choose a reason for hiding this comment

splitbrain Apr 27, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Michaelsy Apr 27, 2018 • edited

Choose a reason for hiding this comment

splitbrain commented Apr 27, 2018

Michaelsy commented Apr 27, 2018 • edited

splitbrain commented Apr 28, 2018

Michaelsy commented Apr 28, 2018 • edited

Michaelsy left a comment • edited

Choose a reason for hiding this comment

Michaelsy left a comment • edited

Choose a reason for hiding this comment

Michaelsy left a comment • edited

Choose a reason for hiding this comment

Michaelsy left a comment • edited

Choose a reason for hiding this comment

Michaelsy left a comment • edited

Choose a reason for hiding this comment

Michaelsy left a comment • edited

Choose a reason for hiding this comment

Michaelsy commented Apr 29, 2018 • edited

Michaelsy commented Apr 29, 2018 • edited

Michaelsy commented Apr 29, 2018 • edited

lpaulsen93 commented Apr 29, 2018

Michaelsy commented Apr 29, 2018 • edited

Michaelsy commented Apr 30, 2018 • edited

Michaelsy commented Apr 30, 2018

Choose a reason for hiding this comment

Michaelsy May 1, 2018 • edited

Choose a reason for hiding this comment

Michaelsy left a comment

Choose a reason for hiding this comment

Michaelsy Apr 30, 2018 • edited

Choose a reason for hiding this comment

Comments in composer.json ?

Research

Conclusion

The comment // ... in composer.json seems to be no problem.

Klap-in commented May 1, 2018

Michaelsy commented May 1, 2018

Klap-in commented May 2, 2018

Michaelsy commented May 2, 2018

Reason of closing

Michaelsy Apr 26, 2018 •

edited

Michaelsy Apr 26, 2018 •

edited

Michaelsy Apr 27, 2018 •

edited

Michaelsy Apr 27, 2018 •

edited

splitbrain Apr 27, 2018 •

edited

Michaelsy Apr 27, 2018 •

edited

Michaelsy commented Apr 27, 2018 •

edited

Michaelsy commented Apr 28, 2018 •

edited

Michaelsy left a comment •

edited

Michaelsy left a comment •

edited

Michaelsy left a comment •

edited

Michaelsy left a comment •

edited

Michaelsy left a comment •

edited

Michaelsy left a comment •

edited

Michaelsy commented Apr 29, 2018 •

edited

Michaelsy commented Apr 29, 2018 •

edited

Michaelsy commented Apr 29, 2018 •

edited

Michaelsy commented Apr 29, 2018 •

edited

Michaelsy commented Apr 30, 2018 •

edited

Michaelsy May 1, 2018 •

edited

Michaelsy Apr 30, 2018 •

edited

The comment `// ...` in `composer.json` seems to be no problem.