Skip to content

Update the 4.0.0 minimum checks postinstall#26104

Merged
wilsonge merged 10 commits intojoomla:stagingfrom
zero-24:400minimumupdate
Sep 12, 2019
Merged

Update the 4.0.0 minimum checks postinstall#26104
wilsonge merged 10 commits intojoomla:stagingfrom
zero-24:400minimumupdate

Conversation

@zero-24
Copy link
Contributor

@zero-24 zero-24 commented Aug 31, 2019

Summary of Changes

Implement the new minimum requirements:

  • php 7.2
  • mysql 5.6
  • mariadb 10.1 (no final decision on this yet) cc @HLeithner
  • postgress 11

Testing Instructions

first test

  • Install 3.x on a server that does not meet the requirements (for example a server running php lower than 7.2)
  • apply this patch
  • make sure the postinstall comes up

seccond test

  • install 3.x on a system that meet the requirements for 4.0 (php 7.2.x, mariadb 10.1+ or mysql 5.6+)
  • apply this patch
  • make sure the message does not show up.

Expected result

On systems where the new rules are not meat we show this postinstall message

Actual result

The checks are run against an older version of the minimum requirements.

Documentation Changes Required

none.

@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators PR-staging labels Aug 31, 2019
@brianteeman
Copy link
Contributor

@zero-24 i have been working on some layout changes for this page - specifically related to j4 but i could backport them if you want

@zero-24
Copy link
Contributor Author

zero-24 commented Aug 31, 2019

What kind of layout changes, can you share a POC with us?

@brianteeman
Copy link
Contributor

The branch is here https://github.com/brianteeman/joomla-cms/tree/sxm31-preupdate
image
but of course you cant see the a11y changes in the screenshot

@zero-24
Copy link
Contributor Author

zero-24 commented Aug 31, 2019

hmm i'm not talking about the pre-update checker i'm talking here about the postinstall message in 3 that we include to tell the existing sites they run on a system that 4.0 can't be installed on. ;)

But for the pre-update checker I'm sure a backport would be great too but I think we should first finish the work for 4.0 and than we see what should and can be backported to 3.10 anyway everything is out of scope for this PR as this is against 3.9 where no pre-update checker is installed ;)

@brianteeman
Copy link
Contributor

Sorry for my confusion. Too much sun today I think

@zero-24
Copy link
Contributor Author

zero-24 commented Aug 31, 2019

no problem enjoy the sun and SXM :)

@sebenns
Copy link
Contributor

sebenns commented Sep 2, 2019

I have tested this item 🔴 unsuccessfully on 6a341c2

The Database-check on line 49 returns false which causes it to skip the php-check further down. So which means, if I have a correct database version, but I am running php 5.6 I am not getting a notification that my php version is not supported.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/26104.

@zero-24
Copy link
Contributor Author

zero-24 commented Sep 3, 2019

Thanks please check agian @datsepp

@infograf768
Copy link
Member

Drone still needs some care. :)

@sebenns
Copy link
Contributor

sebenns commented Sep 4, 2019

I have tested this item ✅ successfully on 40c45d6

Checked every step again and got no problems.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/26104.

@zero-24
Copy link
Contributor Author

zero-24 commented Sep 4, 2019

Thanks @datsepp 👍

@richard67
Copy link
Member

richard67 commented Sep 4, 2019

I have tested this item 🔴 unsuccessfully on 40c45d6

The postinstall message is also shown if on a MySQL database which fulfills the requirement (in my case MySQL 5.7 with MySQLi driver, PHP 7.2 and so on, all requirements for 4.0 fulfilled).

Reason: The regex to check for MariaDB is good for extracting the version number when having a MariaDB, but it is not good for checking if the DB is a MariaDB or not, because the "(mariadb-)?" part of the regex may appear zero ore one time. It is better just to check if the dbversion string contains "Mariadb", like I did it here: https://github.com/joomla-framework/database/pull/166/files#diff-9ba2bd07bed1c63e753d9278ae5e1d31R160


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/26104.

@richard67
Copy link
Member

richard67 commented Sep 4, 2019

@zero-24 Additional info about my test result: You can see in this issue #25245 that the version string coming from db can be "5.5.5-10.1.29-MariaDB".

The regex @mbabker has provided to you will properly extract the version numbers regardless if that mariadb part comes at beginning or end, but it will also match if the version string is the one of a MySQL database, so it can't be used for checking if MariaDB or not, it can really only be used to extract the version when already knowing it is a mariadb.

Therefore it will not help if you change it to using "+" instead of "?" for the "(mariadb-)". As I said, easiest solution is to add a call to instr() for checking if the version string really contains "mariadb" (case insensitive) somewhere.

@richard67
Copy link
Member

@zero-24 P.P.S.: Here and in the 4 lines below you can see how it is done in the DB driver, and there it works: https://github.com/joomla-framework/database/pull/166/files#diff-9ba2bd07bed1c63e753d9278ae5e1d31R440.

@mbabker
Copy link
Contributor

mbabker commented Sep 4, 2019

I don't have time to find it at the moment but in the course of doctrine/dbal#2825 they eventually ended up at the regex used here instead of a more simplified solution of arbitrary stripping 5.5.5- then checking if what was left was a valid version string containing "MariaDB" (case insensitive) anywhere (doctrine/dbal@4f876ef is the diff for that specific change, couldn't find the part of the conversation leading to it). Considering Doctrine gets eyes on it by a wider range of PHP developers, and people I would say are vastly smarter than me, I'm going to assume there is a reason they went with a more in-depth solution than the simple one, which is why I advocate for using their solution.

And TBH looking fresh at the framework code again I'm not sure I 100% like the fact we made the driver strip the MariaDB extra version junk (I prefer giving back the info that the API gathered instead of trying to do arbitrary post-processing, and if consumers really need to they can post-process the data, otherwise you can argue all version strings are simplified down to nothing more than the "typical" <major>.<minor>.<patch>(-<stability>) string, which means stripping more cruft like the examples in #25281).

@richard67
Copy link
Member

@mbabker I had checked the dbal code when I did the PR for the framework. They use the regex inside a db specific function for extracting the version string. The detection which db was done somewhere else, also with a stripos like I did.

I think I have posted the links to the dbal code lines in our discussion, but am not sure anymore. But I think we dicsussed it.

That regex works well for extracting the version number, but not for detecting if mariadb or not. You can test it with an online regex tool and the diverse mariadb version strings which can be found in google.

Maybe we should use the dbal regesx to make a safe and proper version number, this should work with any of our supported databases, but we should not use it not to detect the db type. This I still recommend the stripos.

@richard67
Copy link
Member

@mbabker When testing the regex with a tool, test also if it matches to a MySQL version string and you will see it matches.

@mbabker
Copy link
Contributor

mbabker commented Sep 4, 2019

This is in part why I have always advocated for avoiding MySQL drop-in replacement (AKA fork) detection. It's flaky at best because it relies on information in the version string, that is never a reliable platform detection mechanism (and when you have platforms like Percona Server which don't put anything in the version string then it's 100% impossible to detect you're using them).

AFAIK the regex isn't intended to only match MariaDB version strings. It tries to extract a SemVer compliant string out of whatever you give it (and maybe the 5.5.5- and mariadb- prefixes are optional which would be why the regex extracts a version string out of a "pure" MySQL version).

@richard67
Copy link
Member

AFAIK the regex isn't intended to only match MariaDB version strings. It tries to extract a SemVer compliant string out of whatever you give it (and maybe the 5.5.5- and mariadb- prefixes are optional which would be why the regex extracts a version string out of a "pure" MySQL version).

Exact. But @zero-24 used it in his 2 PRs to detect if mariadb (does match) or not (does not match), and that's wrong because it matches in both cases.

With everything else I agree with you.

As quick fix I suggest we do it here and in the other PR like in the framework.

But sooner or later we should extract proper major.minor.patch strings, but then also for PostgreSQL.

@zero-24
Copy link
Contributor Author

zero-24 commented Sep 4, 2019

Merged thanks.

@richard67
Copy link
Member

Will test now 😄

@richard67
Copy link
Member

I have tested this item ✅ successfully on b77cb35

PostgreSQL (PDO) with too low version => Postinstall message shown.
MySQL with sufficient version => Postinstall message not shown.
Code review ok.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/26104.

@zero-24
Copy link
Contributor Author

zero-24 commented Sep 4, 2019

Thanks @richard67 👍

@richard67
Copy link
Member

@zero-24 Maybe you should extend your testing instructions by a test that the postinstall message is not shown when all requirements for 4.0 are fulfilled. That is missing, and that explains previous successful test. The unsuccessful test was exactly that case, that it shall not show up when all is ready for 4.0.

@zero-24
Copy link
Contributor Author

zero-24 commented Sep 5, 2019

updated the instructions. thanks @richard67

@roland-d
Copy link
Contributor

roland-d commented Sep 5, 2019

I have tested this item ✅ successfully on b77cb35

When the server requirements are not met the message is shown, otherwise it is not shown.

PHP version too low => Postinstall message shown.
PHP version above requirement => Postinstall message not shown.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/26104.

@zero-24
Copy link
Contributor Author

zero-24 commented Sep 5, 2019

Thanks @roland-d 👍

@ghost
Copy link

ghost commented Sep 5, 2019

Status "Ready To Commit".

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Sep 5, 2019
@zero-24 zero-24 added this to the Joomla! 3.9.12 milestone Sep 5, 2019
@wilsonge wilsonge merged commit 4c12aeb into joomla:staging Sep 12, 2019
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Sep 12, 2019
@wilsonge
Copy link
Contributor

Thanks guys! Much appreciated!

@zero-24 zero-24 deleted the 400minimumupdate branch September 13, 2019 03:12
@zero-24
Copy link
Contributor Author

zero-24 commented Sep 13, 2019

Thanks 👍

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

Labels

Language Change This is for Translators

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants