Cm_RedisSession Module Rewrites Session Handler about magento-lts HOT 18 CLOSED

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

from magento-lts.

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.

from magento-lts.

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

from magento-lts.

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

from magento-lts.

@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

from magento-lts.

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

from magento-lts.

@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>

from magento-lts.

@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>

from magento-lts.

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?

from magento-lts.

@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>

from magento-lts.

I added #3465 so that v19 users at least have the option of setting 'redis' explicitly rather than relying on the rewrite.

from magento-lts.

@colinmollenhour I can reproduce the problem on v20 too so I don't know if #3465 would help

from magento-lts.

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?

@colinmollenhour I am using the DB adapter in a multi-node environment. At this moment, I would rather keep it in the database but am open to try Redis for sessions again in the future. In the past (2017/18?) I ran into some issues - I believe - with locks or connections.

from magento-lts.

I see. The db adapter uses zero locking so if you are ok with that it is not a bad choice. However, you can tune the redis adapter so that it is more forgiving like setting max_concurrency=1, break_after_*=1, etc. or just disable locking entirely with disable_locking=1. Aside from the locking it still has a few benefits like compression and fast bot session expiration.

from magento-lts.

@colinmollenhour I can reproduce the problem on v20 too so I don't know if #3465 would help

It doesn't really "fix" this issue, the problem is that it seems impossible to fix all environments simultaneously - either we remove the rewrites in Cm_RedisSession or we don't - merging #3465 would just help reduce the impact if we were to remove the rewrites so even if we left it alone for now it would be good to go ahead and merge #3465 so people can start using 'redis' instead of 'db' in their configs.

from magento-lts.

but @rvelhot is on v20 so I don't know, should we do that in v19, with all the discussions we had?

from magento-lts.

but @rvelhot is on v20 so I don't know, should we do that in v19, with all the discussions we had?

There is zero downside - had we done it a long time ago we could probably remove the rewrite now and just update the README so I thought we should go ahead and get it in there in case there is another v19 release at some point.

from magento-lts.

Since it's fixed upstream and it covers the original bug report, I'll close this.

from magento-lts.