inclusive-design / AChecker

Automated interactive Web content accessibility checker.
https://achecker.ca
GNU General Public License v2.0
69 stars 61 forks source link

ACHECKER-4 : Upgrading MySQL Syntax to support version 4.1.13+ #84

Closed VictorAlagwu closed 6 years ago

VictorAlagwu commented 6 years ago

This is the complete pull request for this task https://issues.fluidproject.org/browse/ACHECKER-4

VictorAlagwu commented 6 years ago

Added a new update

VictorAlagwu commented 6 years ago

Working on all changes. Thanks

CLAassistant commented 6 years ago

CLA assistant check
All committers have signed the CLA.

VictorAlagwu commented 6 years ago

All requested changes have been made, except that of the installation folder, when we have different installation steps, calling different database step3 and step4

cindyli commented 6 years ago

If you search addslashes (with a space in front) across the code base, they show up in some files such as

In summary:

cindyli commented 6 years ago

Please test this PR with magic_quotes_gpc turned on and off. Thanks.

VictorAlagwu commented 6 years ago

Would do that, Thanks

VictorAlagwu commented 6 years ago

Testing magic_quotes_gpc from my own end is difficult because as of PHP 5.4, support for magic_quotes_gpc was removed, and the PHP version I am using for testing are 5.4 and 7.0, though, it was said that get_magic_quotes_gpc will always return false as from PHP 5.4.

cindyli commented 6 years ago

Searching the string mysql_ shows some mysql_xxx calls still appear in some files, most of these lines are commented. Please remove commented lines.

The only place that it's in use is in include/classes/Language/LanguageEditor.class.php. Please fix it.

Thanks.

VictorAlagwu commented 6 years ago

Will work on all changes, thanks for the review

cindyli commented 6 years ago

This line can be removed as vitals.inc.php no longer defines global $db.

cindyli commented 6 years ago

Running the installation process with this branch hits "cannot connect to mysql server" error. Can you test both installation and upgrade processes? Thanks.

VictorAlagwu commented 6 years ago

screenshot from 2018-06-19 21-14-02

cindyli commented 6 years ago

@VictorAlagwu, using the master code to do the installation failed at creating AC_themes table because of the use of

`last_updated` datetime NOT NULL DEFAULT CURRENT_TIMESTAMP,

According to this stackoverflow discussion, the support of this usage with DATETIME starts from mysql 5.6.5, which is quite recent. My recent MAMP download uses mysql 5.5.42. I'm thinking we probably should remove DEFAULT CURRENT_TIMESTAMP to support earlier version of mysql. What do you think?

cindyli commented 6 years ago

@VictorAlagwu, when you tested the installation process, did you first drop the achecker database? The installation should create the database from scratch, instead of connecting to an existing one.

VictorAlagwu commented 6 years ago

I added the achecker database, it was not added automatically, getting Unable to connect to database server. error now.

cindyli commented 6 years ago

Thanks for the fix. Now the installation creates the database properly. Thanks.

A couple of other issues:

  1. Can you merge the master into this branch to keep it up to date? This also ensures you always test with the latest code.

  2. The installation requirement verification page shows this mysql version:

    screen shot 2018-06-20 at 10 48 55 am

Please make sure the verification and display of the mysql version is correct.

  1. The index page after the installation looks like: screen shot 2018-06-20 at 10 59 38 am

    It seems the theme is not applies.

VictorAlagwu commented 6 years ago

The reason for the theme not showing some features, is because of some deprecated PHP codes, which were fix in ACHECKER-5

cindyli commented 6 years ago

You are right, thought I was using PHP 5.6 to test but it was actually PHP 7. Switching to 5.6 fixes the index page display. Thanks.

VictorAlagwu commented 6 years ago

2.The installation requirement verification page shows this mysql version: Please make sure the verification and display of the mysql version is correct.

Answer The MySQL version, which is the server side information can only be gotten after the user has entered the DB details using the mysqli_get_client_info($db), meanwhile, client information can be retrieved using mysqli_get_client_info(), without entering any connection details. and DB details are entered in step 2, therefore we can not get the MySQL version used by the client, except the client information.

cindyli commented 6 years ago

Thanks for the explanation, @VictorAlagwu.

Right now, the verification and comparison of mysql version is performed at the installation step 2 and the upgrade step 2, which is too late and incorrect.

It makes more sense for it to be performed at the index page before proceeding with the installation, same as other verification that is performed on the index.

Another issue is, the detected mysql version is Found Version mysqlnd 5.0.11-dev - 20120503 - $Id: 3c688b6bbc30d36af3ac34fdd4b7b5b787fe5555 $. It would be wrong if this is the string that is used to be compared with the required version number "4.1.13", where the comparison will always return good. It might worthwhile to use a regular expression to extract the actual version "5.0.11" from the string and compare.

Let me know what you think.