Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PR-11608 Modify redis backend #13563

Merged
merged 32 commits into from
Nov 4, 2018
Merged

PR-11608 Modify redis backend #13563

merged 32 commits into from
Nov 4, 2018

Conversation

niden
Copy link
Member

@niden niden commented Nov 1, 2018

Hello!

In raising this pull request, I confirm the following (please check boxes):

  • I have read and understood the Contributing Guidelines?
  • I have checked that another pull request for this purpose does not exist.
  • I wrote some tests for this PR.

Made the specialKey (_PHCR) optional for the Phalcon\Cache\Backend\Redis adapter. This is based on the work of @pantaovay

Thanks

@niden niden added the enhancement Enhancement to the framework label Nov 1, 2018
@niden niden added this to the 4.0.0 milestone Nov 1, 2018
@niden niden requested a review from sergeyklay November 1, 2018 00:25
@niden
Copy link
Member Author

niden commented Nov 1, 2018

cc @sergeyklay

@niden niden self-assigned this Nov 2, 2018
phalcon/cache/backend/redis.zep Outdated Show resolved Hide resolved
phalcon/cache/backend/redis.zep Outdated Show resolved Hide resolved
'host' => env('TEST_RS_HOST', '127.0.0.1'),
'port' => env('TEST_RS_PORT', 6379),
'index' => env('TEST_RS_DB', 0),
'statsKey' => '_PHCR',
Copy link
Contributor

Choose a reason for hiding this comment

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

Well, all these changes will now require the user to pass statsKey key more often, but previously not.

I think from the point of view of the Phalcon user this changelog entry "Made the specialKey (_PHCR) optional" is wrong because now he is forced to play with this key almost always (or at least more often)

Copy link
Member Author

Choose a reason for hiding this comment

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

I disagree. The reason all these statsKey elements appear in the tests is because we assumed that the key would always be there. So all the existing tests must have that in the initialization of the Redis component.

For an real life application, this initialization would happen mostly in the DI container and there will be no more changes.

phalcon/cache/backend/redis.zep Outdated Show resolved Hide resolved
@@ -413,7 +492,7 @@ public function queryKeysWithStatsKeyAndPrefix(UnitTester $I)
$keys = $cache->queryKeys();
sort($keys);

$I->assertEquals(['a', 'b'], $keys);
$I->assertEquals(['phalcon-a', 'phalcon-b'], $keys);
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the business logic was changed? Previously we'd expect here result w/o prefix. Should we notice about this in the blogpost to alert users?

Copy link
Member Author

Choose a reason for hiding this comment

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

No the business logic remains the same. The test was incorrect. The logic is:

$cache->get('a'); // 'a' (no statsKey, no prefix)
$cache->get('a'); // 'phalcon_a' (no statsKey, prefix "phalcon_")
$cache->get('a'); // '_PHCRphalcon_a' (statsKey "_PHCR", prefix "phalcon_")

@sergeyklay sergeyklay added the breaks bc Functionality that breaks Backwards Compatibility label Nov 3, 2018
niden added 9 commits November 3, 2018 13:54
…m/niden/cphalcon into feature/T11608-modify-redis-backend

* 'feature/T11608-modify-redis-backend' of git://github.com/niden/cphalcon:
  [#13547] - Updated changelog
  [#13547] - Corrected name of the method and tests
  [#13547] - Changed getTitle and added renderTitle
  change: change log write error
  -Fixed  `\Phalcon\Http\Response::setFileToSend` filename last much _
  Refactored `Phalcon\Db\Adapter\Pdo::query` to use PDO's prepare and execute. `Phalcon\Db\Adapter::fetchAll` to use PDO's fetchAll
  style: PSR2 style error
  Added: `Phalcon\Acl\Adapter\Memory::addRole` support multiple inherited
@sergeyklay
Copy link
Contributor

@niden Could you please rebase

* 4.0.x:
  [PR12189] - Removed unused variables; Corrected missing use statement
  [PR12189] - Corrected typo
  [PR12189] - Minor changes based on review
  [PR12189] - Modified header class and corrected tests
  [PR12189] - Corrected logic and test
  [PR12189] - Fixed tests
  [PR12189] - Added test for hasHeader
  [PR12189] - Updated changelog
  [PR12189] - Added hasHeader method
  Minor CI's config refactor and improvements
Copy link
Contributor

@sergeyklay sergeyklay left a comment

Choose a reason for hiding this comment

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

LGTM. Relevant documentation changes are necessary.

@niden niden merged commit f7d8357 into phalcon:4.0.x Nov 4, 2018
@niden niden deleted the feature/T11608-modify-redis-backend branch December 7, 2018 01:02
@niden niden added the documentation Documentation required label Apr 9, 2019
@niden niden added 4.0 and removed documentation Documentation required labels Dec 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaks bc Functionality that breaks Backwards Compatibility enhancement Enhancement to the framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants