Skip to content

Conversation

@chrisryan
Copy link
Contributor

What does this PR do?

Added support for caching results in Memcache

Checklist

  • [ ] Pull request contains a clear definition of changes
  • [ ] Tests (either unit, integration, or acceptance) written and passing
  • [ ] Relevant documentation produced and/or updated

@chrisryan chrisryan requested a review from a team as a code owner April 21, 2025 15:18
Comment on lines 132 to 134
if (PHP_VERSION_ID < 80000) {
require_once 'MemcacheMockable.php';
return $this->getMockBuilder(MemcacheMockable::class)
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'm not thrilled with this but it was the only solution I could find to get the tests to pass across all versions of PHP we are currently testing against.

Copy link
Contributor

@chadicus chadicus left a comment

Choose a reason for hiding this comment

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

Just a suggestion about cleaning up the php version check.
The TravisCI files should be unnecessary. Since we're using github actions for the build. We can remove TravisCI, coveralls and scrutinizer in a separate PR though.

.travis.yml Outdated
env:
- PREFER_LOWEST="--prefer-lowest --prefer-stable"
- PREFER_LOWEST=""
services:
Copy link
Contributor

Choose a reason for hiding this comment

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

This file should be removed

@@ -0,0 +1 @@
extension=memcache.so
Copy link
Contributor

Choose a reason for hiding this comment

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

This file shouldn't be necessary since we're using github actions and not TravisCI for builds

@@ -0,0 +1 @@
extension=memcached.so
Copy link
Contributor

Choose a reason for hiding this comment

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

This file shouldn't be necessary since we're using github actions and not TravisCI for builds

Comment on lines 132 to 138
if (PHP_VERSION_ID < 80000) {
require_once 'MemcacheMockable.php';
return $this->getMockBuilder(MemcacheMockable::class)
->setMethods(['get', 'set'])->getMock();
} else {
return $this->getMockBuilder(\Memcache::class)->setMethods(['get', 'set'])->getMock();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (PHP_VERSION_ID < 80000) {
require_once 'MemcacheMockable.php';
return $this->getMockBuilder(MemcacheMockable::class)
->setMethods(['get', 'set'])->getMock();
} else {
return $this->getMockBuilder(\Memcache::class)->setMethods(['get', 'set'])->getMock();
}
$isOlderPHPVersion = PHP_VERSION_ID < 80000;
$memcacheClass =$isOlderPHPVersion ? MemcacheMockable::class : Memcache::class;
return $this->getMockBuilder(\Memcache::class)->setMethods(['get', 'set'])->getMock();

Just a suggestion to avoid the inline require_once call

@chrisryan chrisryan merged commit 95e5fd3 into master Apr 21, 2025
10 checks passed
@chadicus chadicus deleted the master-memcache branch April 21, 2025 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants