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: Convert DateTimeImmutable "Now" into DateTime for account deactivation #2859

Open
wants to merge 1 commit into
base: 4.3
from

Conversation

@dlubitz
Copy link
Contributor

dlubitz commented Jan 10, 2020

Convert "Now" based on DateTimeImmutable to DateTime, as Account::setExpirationDate expects it.

Fixes #2131

@dlubitz dlubitz changed the base branch from master to 4.3 Jan 10, 2020
@bwaidelich

This comment has been minimized.

Copy link
Member

bwaidelich commented Jan 10, 2020

Thanks for this.
I'm just wondering: Wouldn't it be better to change the signature of Account::setExpirationDate() so that it supports DateTimeInterfaces?

@dlubitz

This comment has been minimized.

Copy link
Contributor Author

dlubitz commented Jan 10, 2020

Sure, but I didn't want to change the "contract". IDK if this might be a breaking change than.

@dlubitz

This comment has been minimized.

Copy link
Contributor Author

dlubitz commented Jan 10, 2020

Actually it should be fine, as \DateTime implements \DateTimeInterface

@dlubitz

This comment has been minimized.

Copy link
Contributor Author

dlubitz commented Jan 10, 2020

Hmm, if I think about it a bit longer, it will break because we have to replace also the getter and the return types. If someone relies on \DateTime it could break that.

So what about using this PR for current branches, and a new one with changed contract for master? @bwaidelich

Copy link
Member

bwaidelich left a comment

looks good (by testing)

So what about using this PR for current branches, and a new one with changed contract for master?

Yes, makes sense. Thanks for investigating!

@dlubitz

This comment has been minimized.

Copy link
Contributor Author

dlubitz commented Jan 10, 2020

Nehhhh,
DateTime::createFromImmutable is PHP 7.3 :/
https://www.php.net/manual/de/datetime.createfromimmutable.php

@dlubitz dlubitz force-pushed the dlubitz:2131-account-deactivation branch from 189fc8b to be95459 Jan 10, 2020
…ctivation
@dlubitz dlubitz force-pushed the dlubitz:2131-account-deactivation branch from be95459 to 74eabc3 Jan 10, 2020
@dlubitz

This comment has been minimized.

Copy link
Contributor Author

dlubitz commented Jan 10, 2020

Fixed with a less smart solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.