Skip to content
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

[4.0] Various adjustments in database driver checks #19989

Merged
merged 1 commit into from Apr 15, 2018
Merged

[4.0] Various adjustments in database driver checks #19989

merged 1 commit into from Apr 15, 2018

Conversation

mbabker
Copy link
Contributor

@mbabker mbabker commented Mar 25, 2018

Summary of Changes

  • Removes code from install app for non-supported database platform versions
  • Changes checks for pdomysql database type to mysql
  • Adds checks for pgsql database type (the PDO PostgreSQL driver) to ensure things map correctly to established postgresql structures (i.e. SQL schema paths)

Testing Instructions

Mostly code review, if someone's running an environment supporting PDO PostgreSQL I think the CMS would actually be installable now (if I'm following the code right it wanted SQL files at installation/sql/pgsql and that's not the case), same if you've got extension code floating around that might support PostgreSQL (patch tester would fall under this).

@infograf768
Copy link
Member

I have tested this item ✅ successfully on 3a51db5

Works fine for mysql(PDO) and mysqli


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

@laoneo
Copy link
Member

laoneo commented Apr 3, 2018

@alikon can you have a postgres look on this one? Thanks.

@alikon
Copy link
Contributor

alikon commented Apr 4, 2018

got this one
Call to undefined method Joomla\Database\Postgresql\PostgresqlDriver::convertUtf8mb4QueryToUtf8()
should'nt this one strictly related to mysql land ?
screenshot from 2018-04-04 19-52-51

@mbabker
Copy link
Contributor Author

mbabker commented Apr 4, 2018

At one point I'm pretty sure all that stuff was wrapped in checks. Apparently that got lost along the way too.

@alikon
Copy link
Contributor

alikon commented Apr 4, 2018

to move forward

in the libraries/vendor/joomla/database/Postgresql/PostgresqlDriver.sql we should consider to add

	/**
	 * True if the database engine supports UTF-8 Multibyte (utf8mb4) character encoding.
	 *
	 * @var    boolean
	 * @since  1.4.0
	 */
	protected $utf8mb4 = false;

and

	/**
	 * Determine whether the database engine support the UTF-8 Multibyte (utf8mb4) character encoding.
	 *
	 * @return  boolean  True if the database engine supports UTF-8 Multibyte.
	 *
	 * @since   __DEPLOY_VERSION__
	 */
	public function hasUTF8mb4Support()
	{
		return $this->utf8mb4;
	}

and in https://github.com/joomla/joomla-cms/blob/4.0-dev/installation/src/Model/DatabaseModel.php#L1190

			if($db->hasUTF8mb4Support())
					
				{
					$query = $db->convertUtf8mb4QueryToUtf8($query);
				}

it's becoming to be a long journey ....

@mbabker
Copy link
Contributor Author

mbabker commented Apr 4, 2018

No. The concept of UTF8MB4 is a MySQL specific thing, it doesn't belong in all drivers. At the Framework level this is addressed with UTF8MB4SupportInterface (which declares the same two helper methods already present in the API, if you need cross-version compatible code just run method_exists('hasUTF8mb4Support'); until you can check $db instanceof UTF8MB4SupportInterface).

@alikon
Copy link
Contributor

alikon commented Apr 4, 2018

sorry i'm unfamiliar with 4.x code, i'm just thinking out of loud

@mbabker
Copy link
Contributor Author

mbabker commented Apr 4, 2018

I've done a large share of the work in porting the CMS specific changes to the Framework repo (and internal restructuring for things like prepared statement support), I know some of this better than the back of my hand 😉

@alikon
Copy link
Contributor

alikon commented Apr 6, 2018

sounds better ?
https://github.com/mbabker/joomla-cms/blob/008cd4891e881ab08194ee3190bfde568ae644ca/installation/src/Model/DatabaseModel.php#L1122

				if ($db instanceof UTF8MB4SupportInterface)
				{
				
					if($db->hasUTF8mb4Support())					
					{
						$query = $db->convertUtf8mb4QueryToUtf8($query);
					}

					/**
					 * This is a query which was supposed to convert tables to utf8mb4 charset but the server doesn't
					 * support utf8mb4. Therefore we don't have to run it, it has no effect and it's a mere waste of time.
				 	*/
					if (!$db->hasUTF8mb4Support() && stristr($query, 'CONVERT TO CHARACTER SET utf8 '))
					{
						continue;
					}
				}

@mbabker
Copy link
Contributor Author

mbabker commented Apr 13, 2018

Rebased and the utf8mb4 stuff wrapped in appropriate checks now (again).

@alikon
Copy link
Contributor

alikon commented Apr 13, 2018

sorry but it's still not correct for postgresql install
https://github.com/mbabker/joomla-cms/blob/b5c32d2dc6cda3f678c59b9fb44622bb0c995837/installation/src/Model/DatabaseModel.php#L1120-L1132

look at my previous comment #19989 (comment)

at line 1129
https://github.com/mbabker/joomla-cms/blob/b5c32d2dc6cda3f678c59b9fb44622bb0c995837/installation/src/Model/DatabaseModel.php#L1129

$db->hasUTF8mb4Support() is invoked whatever $db is an instanceof UTF8MB4SupportInterface or not

@mbabker
Copy link
Contributor Author

mbabker commented Apr 13, 2018

My bad, didn't read far enough down the method. Fixed.

@alikon
Copy link
Contributor

alikon commented Apr 13, 2018

I have tested this item ✅ successfully on 65f1be8

on postgresql


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

@wilsonge wilsonge merged commit 5b21c76 into joomla:4.0-dev Apr 15, 2018
@wilsonge wilsonge added this to the Joomla 4.0 milestone Apr 15, 2018
@mbabker mbabker deleted the fix-db-checks branch April 15, 2018 12:19
@austre
Copy link

austre commented Nov 12, 2018

Hi,
first, sorry about reroll topic or something like 😄

I start a dev enviroment for J4 Alpha5, PDO did not work.
j4a5_img01

No MySQL logs.
Apache logs are just warnings:

PHP Warning: "continue" targeting switch is equivalent to "break". Did you mean to use "continue 2"? in /some_dir/plugins/system/debug/debug.php on line 617, referer: https://some_url/installation/index.php?view=remove&layout=default

When i select MySQLi installation works.
j4a5_img02

[Debian 9.5][PHP 7.3][MySQL 8][Joomla 4 Alpha 5]

@alikon
Copy link
Contributor

alikon commented Nov 12, 2018

uhmmm
what is your mysqlmode ?
SELECT @@GLOBAL.sql_mode global, @@SESSION.sql_mode session

@austre
Copy link

austre commented Nov 12, 2018

Global
ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION

Session
ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_ENGINE_SUBSTITUTION

@alikon
Copy link
Contributor

alikon commented Nov 12, 2018

well, then can you replace all of '0000-00-00 00:00:00' in the *.sql
with '1000-01-01 00:00:00' as a test ?

@austre
Copy link

austre commented Nov 12, 2018

I'll try later, looks like 355 sql files 😨
With some 🍻 we can do magic
😄

@austre
Copy link

austre commented Nov 13, 2018

I'll try later, looks like 355 sql files 😨
With some 🍻 we can do magic
😄

Not 355 files, when i wrote this i was looking J3.9 folder. 😄

@alikon i changed files like u suggested, but i got same problem.

@alikon
Copy link
Contributor

alikon commented Nov 13, 2018

i can suggest you to open a new issue....

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.

None yet

7 participants