silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
721 stars 821 forks source link

Pass additional flags into MySQLi::real_connect #9394

Open elliot-sawyer opened 4 years ago

elliot-sawyer commented 4 years ago

https://github.com/silverstripe/silverstripe-framework/blob/c6f5e7e2fa02ed1163c5a9622898499ef3b0fc80/src/ORM/Connect/MySQLiConnector.php#L114

The real_connect method accepts two additional parameters ($socket and $flags, which both default to null) which is able change the behaviour of MySQLi. At the moment it's not possible to pass any of these flags into the function without overloading the entire MySQLiConnector::connect method.

Ideally this can be replaced with a public method that can be overloaded in a subclass and replaced by Injector.

PRs

emteknetnz commented 4 years ago

Which bit are you proposing go into a public method? lines 108 to 114?

elliot-sawyer commented 4 years ago

Yep! I was writing a proof-of-concept but it looks something like this:

public function connect($parameters, $selectDB = false) {
    ...
    $this->connectToDB($parameters, $selectedDB);
    ...
}

public function connectToDB($parameters, $selectedDb) {
    $this->dbConn->real_connect(
            $parameters['server'],
            $parameters['username'],
            $parameters['password'],
            $selectedDB,
            !empty($parameters['port']) ? $parameters['port'] : ini_get("mysqli.default_port"),
            $parameters['socket'],
            $parameters['flags']
    );
}

A subclass of MySQLiConnector could overload the connectToDB with its own implementation. For example, the subclass could set some extra parameters before calling connectToDB

emteknetnz commented 4 years ago

How is $parameters populated before the call to connect()? (Sorry, I'm not personally familiar)

Would it work if, instead of splitting this off as a separate public function, you kept it as is and simply added something like isset($parameters['socket']) ? $parameters['socket'] : null ?

elliot-sawyer commented 4 years ago

MySQLDatabase.php builds a parameters array and passes it into DB::connect($parameters). DB::connect() also adds some data into the array before passing it into real_connect.

The issue is that there's presently no way to add $socket or $flags without overloading the entire connect method. If you need to change the behaviour of the mysqli connection by passing in one more more flags, you can't.

emteknetnz commented 4 years ago

It seems like the proper place to fix this may be all the way down in CoreKernel.php where the database config is read from environment variables - https://github.com/silverstripe/silverstripe-framework/blob/4/src/Core/CoreKernel.php#L340

You may also want to look into using the global $databaseConfig, though I think this may be deprecated in silverstripe 5 - https://github.com/silverstripe/silverstripe-framework/blob/4/src/Core/CoreKernel.php#L223

elliot-sawyer commented 4 years ago

Even if you defined 'socket' or 'flags' in CoreKernel, they are ignored once they get to the connector. This is the current code:

        $this->dbConn->real_connect(
            $parameters['server'],
            $parameters['username'],
            $parameters['password'],
            $selectedDB,
            !empty($parameters['port']) ? $parameters['port'] : ini_get("mysqli.default_port")
        );

There are two more parameters after 'port' for real_connect available with default values. I'm proposing this needs to be fixed.

emteknetnz commented 4 years ago

Yes, seems like you'd need to do two things: a) make a change to CoreKernel to get the environment variables for socket + flags in to $parameters b) Update real_connect in MySQLiConnector to use the new $parameters

elliot-sawyer commented 4 years ago

CoreKernel is a good approach for the most common cases, but 'flags' gets a bit tricky because they're binary values intended to be XOR'ed together: MYSQLI_CLIENT_COMPRESS | MYSQLI_CLIENT_FOUND_ROWS | MYSQLI_CLIENT_IGNORE_SPACE | MYSQLI_CLIENT_INTERACTIVE | MYSQLI_CLIENT_SSL | MYSQLI_CLIENT_SSL_DONT_VERIFY_SERVER_CERT

That will resolve to an integer value in PHP, but I don't know how you'd define that in an environment variable or a YML file if you need to pass in more than one flag. Moving real_connect into it's own method would allow a subclass to handle situations like that.

Worth mentioning CoreKernel covers all database connectors, not just MySQLi, so you'd have to consider the impact on other connectors like PDO, as well as the postgres and sqlite modules.

jkersu commented 3 years ago

I have also encountered another scenario for this issue when I was trying to connect to a cloud database on GCP.

I needed to specify the socket parameter for the DB connection as it was being proxied to the database but there was no way to pass the value into real_connect.

I temporarily worked around it by extending @elliot-sawyer's module to also include the socket parameter and it seems to be holding up: https://github.com/elliot-sawyer/silverstripe-mysql-ssl

GuySartorelli commented 1 month ago

New ENV vars have been added in https://github.com/silverstripe/silverstripe-framework/pull/11383 to support this. Still needs to be added to docs