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

Add Auto Expiry Feature #111

Merged

Conversation

mpchadwick
Copy link
Contributor

Implementation of feature discussed in #110.

*/
protected function _getAutoExpiringLifetime($lifetime, $id)
{
if ($lifetime) {
Copy link
Owner

Choose a reason for hiding this comment

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

No reason to check if it matches the pattern if the feature is disabled. I recommend short-circuiting by changing this to:

 if ($lifetime || ! $this->_autoExpireLifetime) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. Done.

return !empty($matches);
}

public function setAutoExpireLifetime($lifetime)
Copy link
Owner

Choose a reason for hiding this comment

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

Other options are not exposed this way... Is there a reason they need to be set aside from the constructor?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason I added the setters was to be able to change the properties for testAutoExpiry. Maybe there's a better way, but I'm not sure. I tried Googling to see if setUp could tell what test method as being called, but couldn't find any information about that.

Copy link
Owner

Choose a reason for hiding this comment

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

I think the proper way would be to have a new test case class for these tests that require a different setUp method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@colinmollenhour Updated with a separate test case. I'm following the pattern introduced with forceStandalone where a protected property is used and can be changed in the new test case class. Happy to squash these down as a final step if you'd like.

@colinmollenhour colinmollenhour merged commit 155f4dc into colinmollenhour:master Oct 3, 2016
@colinmollenhour
Copy link
Owner

Looks good! Thanks, Max!

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.

None yet

2 participants