Skip to content

[Cache] Implement psr/cache 3 #41290

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

Merged
merged 1 commit into from
Jul 7, 2021

Conversation

derrabus
Copy link
Member

Q A
Branch? 6.0
Bug fix? no
New feature? yes
Deprecations? no
Tickets N/A
License MIT
Doc PR N/A

This PR adds the necessary return types to support version 3 of the psr/cache interfaces. I did not add parameter types in order to maintain v1 compatibility, but we could as well decide to drop v1 if we want.

@carsonbot

This comment has been minimized.

@carsonbot carsonbot added this to the 6.0 milestone May 19, 2021
@derrabus derrabus force-pushed the improvement/psr-cache-3 branch from 186e22f to 76ce5bd Compare May 19, 2021 18:07
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

I'm fine dropping support for v1 and adding types to parameters!

Before merging, and as much as I want this to be merged, let's take this PR as an opportunity to put our plan in practice: adding any return types in 6.0 must first issue a deprecation notice in 5.4.

This can be done by setting DebugClassLoader in deprecation mode by default:
https://github.com/symfony/symfony/blob/5.4/src/Symfony/Component/ErrorHandler/DebugClassLoader.php#L39

There is a direct link with https://wiki.php.net/rfc/internal_method_return_types also, which we should account for/rely on by eg reusing the attribute.

@@ -306,7 +290,7 @@ public function reset()
$this->clear();
}

private function generateItems(array $keys, $now, $f)
private function generateItems(array $keys, $now, $f): \Generator
Copy link
Member

Choose a reason for hiding this comment

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

all additions to private (and final if any) methods should be split on a lower branch

@derrabus
Copy link
Member Author

Before merging, and as much as I want this to be merged, let's take this PR as an opportunity to put our plan in practice: adding any return types in 6.0 must first issue a deprecation notice in 5.4.

Indeed. Shall we resurrect #33228?

There is a direct link with https://wiki.php.net/rfc/internal_method_return_types also, which we should account for/rely on by eg reusing the attribute.

Is there a list of methods anywhere that are affected by the RFC? We should make sure that if we override/polyfill any of those they should have the correct return type in 6.0

@nicolas-grekas
Copy link
Member

Indeed. Shall we resurrect #33228?

why not, if you think we need an issue for that :)
to me the plan is clear and is very similar to https://wiki.php.net/rfc/internal_method_return_types

Is there a list of methods anywhere that are affected by the RFC

not really, that maybe: https://github.com/php/php-src/pull/6548/files?file-filters%5B%5D=.h

@stof
Copy link
Member

stof commented May 20, 2021

to me the plan is clear and is very similar to https://wiki.php.net/rfc/internal_method_return_types

I think the DebugClassLoader should be improved to require an opt-in from the class to decide that its next major version will add a return type instead of just relying on the presence of @return. Adding a @return in a phpdoc does not automatically mean that the next major version will turn it into a native return type.
And supporting the ReturnTypeWillChange attribute as a way for the child class to opt-out from the warning would be great (as that would reuse the PHP feature doing the opt out)

The ideal solution would of course be that PHP exposes the tentative return type to userland instead of reserving it for internal classes.

but anyway, this discussion probably deserves its own issue

@stof
Copy link
Member

stof commented May 20, 2021

btw, the explicit opt-in for parent classes would allow us to activate that behavior by default in DebugClassLoader. As long as it requires projects to run the DebugClassLoader in a special mode, we don't have a graceful migration path for our users, as they won't get the warnings (because they won't even know that this special mode exists).

@derrabus
Copy link
Member Author

derrabus commented May 20, 2021

Adding the parameter types is lot more fun because the integration tests are not ready for that. 🙈 Will do that in a separate PR.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Jun 8, 2021

Should be updated with argument types, taking some changes from #41424

@derrabus derrabus force-pushed the improvement/psr-cache-3 branch from 76ce5bd to 14c4ff9 Compare June 8, 2021 21:40
@derrabus derrabus force-pushed the improvement/psr-cache-3 branch from 14c4ff9 to 4330526 Compare June 29, 2021 12:58
@nicolas-grekas nicolas-grekas mentioned this pull request Jul 6, 2021
57 tasks
Signed-off-by: Alexander M. Turek <me@derrabus.de>
@nicolas-grekas nicolas-grekas force-pushed the improvement/psr-cache-3 branch from 4330526 to 71d76c4 Compare July 6, 2021 17:14
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

(I'm merging to move this forward, we'll handle the deprecation path for all components at once anyway.)

@nicolas-grekas
Copy link
Member

Thank you @derrabus.

@derrabus
Copy link
Member Author

derrabus commented Jul 7, 2021

Thank you for completing this PR, @nicolas-grekas!

@derrabus derrabus deleted the improvement/psr-cache-3 branch July 7, 2021 09:59
@fabpot fabpot mentioned this pull request Nov 5, 2021
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