Cm_RedisSession Module Rewrites Session Handler

[rvelhote]

Preconditions (*)

1. Have the session handler configured as <session_save>db</session_save>
2. Update to OpenMage 20.1.0 using composer

Steps to reproduce (*)

1. Have the session handler configured as <session_save>db</session_save>
2. Update to OpenMage 20.1.0 using composer
3. Clear the core_session table (not required but makes it easier to check the result)
4. Browse your store, add things to cart

Expected result (*)

1. Sessions should be stored in the database and not in Redis. OpenMage should respect the session_save configuration

Actual result (*)

1. The core_session does not have any new sessions (because they are now in Redis)
2. The session handler gets rewritten by the Cm_RedisSession module and the database handler is ignored. The module Cm_RedisSession is installed and enabled by default.

[fballiano]

did you open the issue in @colinmollenhour repo at https://github.com/colinmollenhour/Cm_RedisSession?

[rvelhote]

I can open it there as well, no problem. I didn't because I thought it's primarily a discussion to have within the OpenMage context. It can also be helpful to people that face the same issue when upgrading.

[fballiano]

now that the Cm_RedisSession code is not in our repo, probably we wouldn't even have the possibility to fix it

[colinmollenhour]

Ahh, I think this relates to colinmollenhour/Cm_RedisSession#190..

[fballiano]

@colinmollenhour I can confirm that I can reproduce the problem.

my local.xml has no redis configurations at all:
<session_save><![CDATA[db]]></session_save>

but somehow the sessions get saved on redis (which I've running on my machine) anyway

[colinmollenhour]

@fballiano Do you have a <redis_session> element in your config?

[fballiano]

@colinmollenhour noup, this is my whole local.xml

<config>
    <global>
        <install>
            <date><![CDATA[Fri, 07 Jul 2023 19:18:55 +0000]]></date>
        </install>
        <crypt>
            <key><![CDATA[540b96d9f2e4b7e7245f063e1b9349d5]]></key>
        </crypt>
        <disable_local_modules>false</disable_local_modules>
        <resources>
            <db>
                <table_prefix><![CDATA[]]></table_prefix>
            </db>
            <default_setup>
                <connection>
                    <host><![CDATA[localhost]]></host>
                    <username><![CDATA[X]]></username>
                    <password><![CDATA[X]]></password>
                    <dbname><![CDATA[X]]></dbname>
                    <initStatements><![CDATA[SET NAMES utf8]]></initStatements>
                    <model><![CDATA[mysql4]]></model>
                    <type><![CDATA[pdo_mysql]]></type>
                    <pdoType><![CDATA[]]></pdoType>
                    <active>1</active>
                </connection>
            </default_setup>
        </resources>
        <session_save><![CDATA[db]]></session_save>
    </global>
    <admin>
        <routers>
            <adminhtml>
                <args>
                    <frontName><![CDATA[admin]]></frontName>
                </args>
            </adminhtml>
        </routers>
    </admin>
</config>

[colinmollenhour]

@rvelhote I think as a quick workaround you could override the rewrite in your local.xml:

  <global>
    <models>
      <core_resource>
        <rewrite>
          <session>Mage_Core_Model_Resource_Session</session>
        </rewrite>
      </core_resource>
    </models>
  </global>

[colinmollenhour]

I could remove the rewrite again and release it as version 3.2.0 with a note in the README to add the rewrite to local.xml where needed, but non-OpenMage users or non-updated users that don't have the version fixed in composer.json and people using the repo directly would be affected unexpectedly since the rewrite would be missing.

That said, does the db adapter really add any value? For small installations the files adapter is jsut as good or better and for large (multi-node or high-traffic) installations the Redis adapter is much better than the MySQL one. Is there a use-case I'm missing for the 'db' adapter?

[rvelhote]

@colinmollenhour @fballiano Thank you for the quick help. I was already discussing this topic in Discord and I ended up using a composer patch to disable the module rather than rewriting the rewrite (fits our workflow better).

Index: vendor/colinmollenhour/magento-redis-session/app/etc/modules/Cm_RedisSession.xml
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/vendor/colinmollenhour/magento-redis-session/app/etc/modules/Cm_RedisSession.xml b/vendor/colinmollenhour/magento-redis-session/app/etc/modules/Cm_RedisSession.xml
--- a/vendor/colinmollenhour/magento-redis-session/app/etc/modules/Cm_RedisSession.xml
+++ b/vendor/colinmollenhour/magento-redis-session/app/etc/modules/Cm_RedisSession.xml    (date 1692697724130)
@@ -33,7 +33,7 @@
 <config>
   <modules>
     <Cm_RedisSession>
-      <active>true</active>
+      <active>false</active>
       <codePool>community</codePool>
     </Cm_RedisSession>
   </modules>