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

BUGFIX: Consider accounts with expirationDate in the future as active #875

Merged
merged 3 commits into from
Feb 27, 2017

Conversation

maltemuth
Copy link
Contributor

@maltemuth maltemuth commented Feb 22, 2017

What I did

I fixed Account::isActive() behavior when an expirationDate is set.

How I did it

The "Now" injection is now non-lazy, thus the comparison of DateTime objects works.

How to verify it

Run the new functional test TYPO3.Flow/Tests/Functional/Security/AccountTest.php

Checklist

  • Code follows the PSR-2 coding style
  • Tests have been created, run and adjusted as needed
  • The PR is created against the lowest maintained branch

Fixes #873

@mention-bot
Copy link

@maltemuth, thanks for your PR! By analyzing the history of the files in this pull request, we identified @kdambekalns, @daniellienert and @robertlemke to be potential reviewers.

@aertmann aertmann changed the title BUGFIX: consider Accounts with expirationDate in the future as active BUGFIX: Consider accounts with expirationDate in the future as active Feb 22, 2017
@aertmann
Copy link
Collaborator

tests don't pass

Copy link
Collaborator

@aertmann aertmann left a comment

Choose a reason for hiding this comment

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

LGTM but not sure if the non lazy $now has any side-effects

@maltemuth
Copy link
Contributor Author

@aertmann it does; Now is the timestamp of object instantiation and not equal to \DateTime("now") - this is why tests failed if the suite takes longer than one minute to run.

However, lazy-loading had the side effect of breakage.

Copy link
Member

@albe albe left a comment

Choose a reason for hiding this comment

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

Looks good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants