[Security] Ban \DateTime from Security component #47772

WebMamba · 2022-10-03T10:51:09Z

Q	A
Branch?	6.2 for features
Bug fix?	no
New feature?	yes
Deprecations?	yes
Tickets	#47580
License	MIT

This PR is next to: #47730

The purpose here is to remove the use of \DateTime in the security component. I don't think we can remove it completely without leading any BC break. So I propose to use DateTimeInterface with a deprecation notice.
Feel free to discuss!

stof · 2022-10-03T11:00:09Z

src/Symfony/Component/Security/Core/Authentication/RememberMe/TokenProviderInterface.php

@@ -39,7 +39,7 @@ public function deleteTokenBySeries(string $series);
     *
     * @throws TokenNotFoundException if the token is not found
     */
-    public function updateToken(string $series, #[\SensitiveParameter] string $tokenValue, \DateTime $lastUsed);
+    public function updateToken(string $series, #[\SensitiveParameter] string $tokenValue, \DateTimeInterface $lastUsed);


This is a BC break for implementors of the interface

Can we remove the param from 6.2 and replace it with an @paramannotation? I think you said we cannot but I forgot the reasoning once again 😬

It doesn't have a default value, which means removing the param is not allowed as that would mean the implementors are flagged as incompatible with the interface?

stof · 2022-10-03T11:00:32Z

src/Symfony/Component/Security/Core/Authentication/RememberMe/PersistentTokenInterface.php

@@ -37,7 +37,7 @@ public function getTokenValue(): string;
    /**
     * Returns the time the token was last used.
     */
-    public function getLastUsed(): \DateTime;
+    public function getLastUsed(): \DateTimeInterface;


This is a BC breaks for consumers of the interface

What I try to do here is to use the covariance feature of PHP. I don't get why is it a BC breaks ? If someone consumes this interface with DateTime he still can because DateTime is a child of DateTimeInterface.

This is a BC break for the caller, as they could rely on the method returning DateTime before. E.g. this code works before this change, but breaks after:

$lastUsed = $token->getLastUsed(); $lastUsed->modify('+30 minutes'); if ($lastUsed < new \DateTimeImmutable('now')) { throw new \RuntimeException('This token was last used 30 minutes ago'); }

(or simpler use-cases e.g. where this is passed in a function requiring DateTime).

The consumer of a return type is not choosing whether it uses DateTime or a DateTimeInterface. It has to handle everything.

Ok thanks both of you. I understand your point. But I don't see any way to it without leading to a BC break. Maybe we should target the next major version?

Also next major versions cannot have BC breaks that didn't have a deprecation (and way to get out of deprecation) in the last minor before it.

We would have to create a new method with the new return type and deprecate the old method. But I'm personally not sure if that is worth is: What about delaying to remove mutable DateTime in user-facing code?

@nicolas-grekas I ping you here. I think you should be part of this discussion

I've no perfect solution to this. We would document a way to detect all such uses of DateTime and rewrite them automatically:

Before:

$lastUsed = $token->getLastUsed(); $lastUsed->modify('+30 minutes');

After:

$lastUsed = $token->getLastUsed(); $lastUsed = $lastUsed->modify('+30 minutes');

That'd make the code the same whether DateTime or DateTimeImmutable is used.
Can Rector do that? /cc @TomasVotruba
Should the PHP engine trigger a deprecation when the return value of one of DateTime's setters is not used? I'm not sure how this would be possible technically, but let's just dream a bit :) /cc @derickr

@nicolas-grekas If the type is known, Rector can handle it :)

And regarding the widened type, we might need something like ReturnTypeWillChange to indicate that this interface will change its return type in the future. But there is no userland solution for that.

nicolas-grekas · 2023-08-01T10:09:20Z

I'm closing because I think we went as far as possible on the topic already. Thanks for the interest in pushing this forward 🙏

band \DateTime from security component

8681462

WebMamba requested review from wouterj and chalasr as code owners October 3, 2022 10:51

carsonbot added Status: Needs Review Deprecation Feature labels Oct 3, 2022

carsonbot added this to the 6.2 milestone Oct 3, 2022

stof reviewed Oct 3, 2022

View reviewed changes

nicolas-grekas added the Security label Oct 3, 2022

carsonbot changed the title ~~Ban \DateTime from Security component~~ [Security] Ban \DateTime from Security component Oct 3, 2022

WebMamba mentioned this pull request Oct 6, 2022

Ban DateTime from HttpFoundation component #47802

Closed

fabpot modified the milestones: 6.2, 6.3 Nov 1, 2022

nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023

nicolas-grekas closed this Aug 1, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security] Ban \DateTime from Security component #47772

[Security] Ban \DateTime from Security component #47772

[Security] Ban \DateTime from Security component #47772

[Security] Ban \DateTime from Security component #47772

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment