SchumacherFM / Magento-CmsRestriction

Magento CMS Page Restriction on Customer ID and Group ID basis
10 stars 3 forks source link

Large group id fix #8

Open ajpirciovs opened 7 years ago

ajpirciovs commented 7 years ago

You're using a BIGINT here to store the acceptable group ids which works... until you hit the maximum integer size that a database can store. This causes either a number that's too large to store or wild inconsistencies when saving groups with group ids that are big. For instance we have about a dozen groups, some of which (for whatever reason) have ids > 100. Theoretically, this is a problem at quite a low number. This fix just makes your group field a comma separated list that gets imploded() and exploded() as needed which is much easier to manage. Not for nothing, but it does save on calculating these groups as being valid with a simple comparison. You could replace the BIGINT field with a text field but then you might run into issues where the integer values are simply too big for PHP to handle in a 64-bit architecture, which I'm sure you don't want :)

Otherwise an excellent extension. Thank you! Hope this helps

SchumacherFM commented 7 years ago

Thank you very much!

This looks like the perfect solution to avoid the limitation of the group ID due to the bit set feature.

One important point:

Do not change the install script. Rather provide an upgrade script which alters the columns and provides a proper migration of the bit set group IDs to the comma separated list. Otherwise lots of users will complain when they update this module in their Magento installation.

Would you please do this? :-)

ajpirciovs commented 7 years ago

Yeah I thought that might be a problem with existing installations after I added the pull request. I'll see if I can spare some time to whip that up

SchumacherFM commented 7 years ago

Thank you for your effort!