Change initStatements to charset option when create db connection.

[forfin]

• This option pass to Zend_Db_Adapter_Pdo_Abstract (parent of Mage_Core_Model_Resource_Type_Db_Pdo_Mysql) when init new PDO
• SET NAMES utf8 might be there as a workaround for php < 5.3.6 but should not be a case anymore.
• Ref
  • https://stackoverflow.com/questions/4361459/php-pdo-charset-set-names
  • https://stackoverflow.com/questions/1650591/whether-to-use-set-names

*When Openmage start. It seems to merge every etc/*.xml config including this file. So before this commit, end-user who want to remove initStatements must explicitly set empty to <initStatements /> in later file. Otherwise it still be there.

Contribution checklist (*)

• [x] Pull request has a meaningful description of its purpose
• [x] All commits are accompanied by meaningful commit messages
• [x] All automated tests passed successfully (all builds are green)
• [x] Add yourself to contributors list

[kiatng]

Have you put this in production? What are the effects/benefits with or without this PR?

[forfin]

Yes, I have used it in production without breaking anythings.

I found this because I was thinking that SET NAMES utf8 statement cause connection pinning in my AWS RDS Proxy env. but seems it not the case.

So, this commit did not breaking anythings and did not solve my original problem either. But 1 lesser query on every connections might be a beneficial for overall.

[kiatng]

Ref https://dev.mysql.com/doc/refman/8.4/en/charset-connection.html

SET NAMES 'charset_name' [COLLATE 'collation_name']

SET NAMES indicates what character set the client uses to send SQL statements to the server. Thus, SET NAMES 'cp1251' tells the server, “future incoming messages from this client are in character set cp1251.” It also specifies the character set that the server should use for sending results back to the client. (For example, it indicates what character set to use for column values if you use a SELECT statement that produces a result set.)

I also read the referenced stackoverflow. I am not sure how it related to our use case. UTF8 has been default in OpenMage since the beginning and we don't have any issues mentioned in the stackoverflow such as threat and wrong encoding in client.

I propose to close this PR unless there is a more compelling reason.

[forfin]

Sorry that I give bad reference.

It start when Openmage create new PDO in Zend_Db_Adapter_Pdo_Abstract

• When init new PDO($dsn, ...); $dsn string are produce by config xml files.
• Before this commit $dsn was

$dsn = "$mysql:model=mysql4;initStatements=SET NAMES utf8;type=pdo_mysql;host=mysql;dbname=openmage;active=1";

• After this commit $dsn will be

$dsn = "$mysql:model=mysql4;charset=utf8;type=pdo_mysql;host=mysql;dbname=openmage;active=1";

• PDO dsn valid options are host, port, dbname, unix_socket, charset for MySQL
• PDO dsn ignore non-existing option so initStatements is ignore.
• charset=xxx option is equivalent to SET NAMES xxx .
  • https://www.php.net/manual/en/ref.pdo-mysql.connection.php
  • SET NAMES utf8 might be there as a workaround for php < 5.3.6 but should not be a case anymore.

So when no initStatements was given, 1 extra query is not call. https://github.com/OpenMage/magento-lts/blob/611bbe7ba8b1087452d18e423a93d0eb0ae52f02/app/code/core/Mage/Core/Model/Resource/Type/Db/Pdo/Mysql.php#L33-L35

[Axepih]

Seems OK to me with my tests. Do you see any problems with this change?

[mattdavenport]

I'm leaning towards merging this for a few reasons:

• Cleaner implementation via DSN construction than a distinct command each connection
• While minimal latency savings, it does save an additional command to the database
• I believe we're under the assumption OpenMage is MySQL/MariaDB only at this point (correct me if I'm wrong), but <charset> is more DB-agnostic than a specific command in <initStatements>

I haven't tested this yet, but will approve once confirmed unless there are other objections.

[sonarqubecloud[bot]]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[kiatng]

I have 2 issues with this PR:

1. app/etc/local.xml often has <initStatements>SET NAMES utf8</initStatements>, which will override config.xml and add the initStatements anyway, making this PR ineffective.

2. app/etc/local.xml.template contains

                    <initStatements>{{db_init_statemants}}</initStatements>

How do we deal with these 2 issues?

[sonarqubecloud[bot]]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[forfin]

For me.

• app/etc/local.xml is userspace config, So Openmage can't enforce this unless user change it themself.

• for app/etc/local.xml.template

  • It seems it convert to app/etc/local.xml on installation https://github.com/OpenMage/magento-lts/blob/18140508a9c58a0e844c465329f9b28807bd16d8/app/code/core/Mage/Install/Model/Installer/Config.php#L90-L95
  • If db_init_statemants is empty, It should produce app/etc/local.xml with empty initStatements

       ...
       <initStatements></initStatements>
       ...
    

And when Openmage evaluate configuration, It seems it look for every etc/*.xml sort by filename alphabetically.

• app/etc/config.xml (Openmage enforce config)
• app/etc/local.xml (User customization. Apply later, win app/etc/config.xml)

Here example setting on user side (local.xml)

Before this commit

<!-- local.xml, To enable charset -->
<config>
    <global>
        <resources>
            <default_setup>
               <connection>
                    <host>host</host>
                    <username>user</username>
                    <password>pass</password>
                    <dbname>openmage</dbname>
                    <!-- extra 2 line needs before this commit, To override config.xml  -->
                    <initStatements/>
                    <charset>utf8</charset>
               </connection>
            </default_setup>
            ...
        </resources>
        ...
    </global>
</config>

<!-- local.xml, To use initStatements -->
<config>
    <global>
        <resources>
            <default_setup>
               <connection>
                    <host>host</host>
                    <username>user</username>
                    <password>pass</password>
                    <dbname>openmage</dbname>
                    <!-- The default config in config.xml is already declare initStatements -->
               </connection>
            </default_setup>
            ...
        </resources>
        ...
    </global>
</config>

After this commit

<!-- local.xml, To enable charset -->
<config>
    <global>
        <resources>
            <default_setup>
               <connection>
                    <host>host</host>
                    <username>user</username>
                    <password>pass</password>
                    <dbname>openmage</dbname>
                    <!-- No extra config need to override config.xml -->
               </connection>
            </default_setup>
            ...
        </resources>
        ...
    </global>
</config>

<!-- local.xml, To use old initStatements -->
<config>
    <global>
        <resources>
            <default_setup>
               <connection>
                    <host>host</host>
                    <username>user</username>
                    <password>pass</password>
                    <dbname>openmage</dbname>
                    <!-- If user prefer initStatement, Need to do this -->
                   <initStatements>SET NAMES utf8 </initStatements>
                   <charset/>
               </connection>
            </default_setup>
            ...
        </resources>
        ...
    </global>
</config>

• edit: Add example config on user side.