Skip to content
This repository was archived by the owner on Apr 29, 2019. It is now read-only.

Update colinmollenhour/php-redis-session-abstract #39

Conversation

pmclain
Copy link
Contributor

@pmclain pmclain commented Feb 11, 2018

Description

Update colinmollenhour/php-redis-session-abstract to version supporting PHP 7.2.

Fixed Issues (if relevant)

  1. colinmollenhour/php-redis-session-abstract does not support PHP 7.2 #1

Manual testing scenarios

  1. Install Magento
  2. Configure caches to use Redis

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@pmclain pmclain force-pushed the update/cm-redis-session-abstract branch from fb7d357 to 68f9acc Compare February 12, 2018 15:16
@buskamuza buskamuza self-assigned this Feb 12, 2018
@buskamuza buskamuza added this to the February 2018 milestone Feb 12, 2018
@magento-engcom-team
Copy link
Collaborator

@pmclain thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

/**
* Try to break lock for at most this many seconds
*/
const DEFAULT_FAIL_AFTER = 15;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this one not configurable? Comparing to PARAM_BREAK_AFTER?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also necessary to update setup:config:set with new params (in \Magento\Setup\Model\ConfigOptionsList\Session).

Could you please also update http://devdocs.magento.com/guides/v2.2/config-guide/redis/redis-session.html ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I copied DEFAULT_FAIL_AFTER from 2.2-develop https://github.com/magento/magento2/blob/0839bd421bd5812e9aa7c943b89ecae9d5008832/lib/internal/Magento/Framework/Session/SaveHandler/Redis/Config.php#L121 Should I change it to be configurable here?

Thanks for pointing out the changes required to setup:config:set. I'll get that taken care of.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it... ok, let's leave it as is.

@pmclain pmclain force-pushed the update/cm-redis-session-abstract branch from 68f9acc to 1babe34 Compare February 14, 2018 13:50
* Update implementation of `Cm\RedisSession\Handler\ConfigInterface`
* Add Redis Sentinel config options to `setup:config:set` command
@magento-engcom-team magento-engcom-team merged commit 6b1ae42 into magento-engcom:2.3-develop Feb 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants