-
-
Notifications
You must be signed in to change notification settings - Fork 54
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 support for psr/cache 2 and 3 versions #216
Conversation
FWIW, let's drop PHP 7.4 as part of this? It has a shelf life of ~2 months left. |
128ae76
to
6ef5a41
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Thanks for working on this.
Just a few things but overall, good work 👍🏼
src/Psr/CacheItemPool/CacheItem.php
Outdated
@@ -106,7 +106,7 @@ public function setIsHit(bool $isHit): self | |||
/** | |||
* {@inheritdoc} | |||
*/ | |||
public function set($value): CacheItemInterface | |||
public function set($value): static |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also add argument types so we are aligned with the interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added (needed to drop "psr/cache": "^1.0 for it to work)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@boesing if you want the parameter types to match the interface, we will need a new major version here.
If we can keep psr/cache:^1
and BC, then we can release a minor.
@@ -84,7 +83,7 @@ public function __destruct() | |||
/** | |||
* {@inheritdoc} | |||
*/ | |||
public function getItem($key) | |||
public function getItem($key): CacheItemInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also add argument types so we are aligned with the interface?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added (needed to drop "psr/cache": "^1.0 for it to work)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding the return type here is still a BC break :|
We're still in a "new major target" scenario, I fear...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most likely previously it was just return type in documentation but support for psr/cache v2 and v3 it needs to be added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup: not blaming your changes, just saying that the lack of final
has exposed us to BC issues here, since the dependency change leads to a transitive BC break.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I've expected the decorators to be final since v3.
Seems that I forgot these...
I would be open for having a new major but only if we also add support for psr/simple-cache
in its newer versions.
In the next major we can add final
to the decorators as well.
So we do have to drop v1 support of the psr packages as well in the next major? bummer...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would be open for having a new major but only if we also add support for
psr/simple-cache
in its newer versions.
👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So what is the plan with this one? Does it mean that v4 will happen in September?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not give an ETA but I will take care that this lands in v4.
Since that requires all supported adapters to support v4 as well plus the fact that there are some other things I've planned for v4, I dont think this will happen in september.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your response let me know how I can help with v4.
If you will create v4 branch I will create new pr against it and will fix this code for it.
262c477
to
a07ec72
Compare
Signed-off-by: Renovate Bot <bot@renovateapp.com> Signed-off-by: Rimvydas Zilinskas <rimvydas@loftdigital.com>
Signed-off-by: Renovate Bot <bot@renovateapp.com> Signed-off-by: Rimvydas Zilinskas <rimvydas@loftdigital.com>
Signed-off-by: Rimvydas Zilinskas <rimvydas@loftdigital.com>
Signed-off-by: Rimvydas Zilinskas <rimvydas@loftdigital.com>
Signed-off-by: Rimvydas Zilinskas <rimvydas@loftdigital.com>
Signed-off-by: Rimvydas Zilinskas <rimvydas@loftdigital.com>
Signed-off-by: Rimvydas Zilinskas <rimvydas@loftdigital.com>
Signed-off-by: Renovate Bot <bot@renovateapp.com> Signed-off-by: Rimvydas Zilinskas <rimvydas@loftdigital.com>
Signed-off-by: Renovate Bot <bot@renovateapp.com> Signed-off-by: Rimvydas Zilinskas <rimvydas@loftdigital.com>
Signed-off-by: Renovate Bot <bot@renovateapp.com> Signed-off-by: Rimvydas Zilinskas <rimvydas@loftdigital.com>
Signed-off-by: Rimvydas Zilinskas <rimvydas@loftdigital.com>
…idArgument exceptions Signed-off-by: Rimvydas Zilinskas <rimvydas@loftdigital.com>
Signed-off-by: Rimvydas Zilinskas <rimvydas@loftdigital.com>
Signed-off-by: Rimvydas Zilinskas <rimvydas@loftdigital.com>
Signed-off-by: Renovate Bot <bot@renovateapp.com> Signed-off-by: Rimvydas Zilinskas <rimvydas@loftdigital.com>
Signed-off-by: Renovate Bot <bot@renovateapp.com> Signed-off-by: Rimvydas Zilinskas <rimvydas@loftdigital.com>
Signed-off-by: Renovate Bot <bot@renovateapp.com> Signed-off-by: Rimvydas Zilinskas <rimvydas@loftdigital.com>
Signed-off-by: Rimvydas Zilinskas <rimvydas@loftdigital.com>
bd9872e
to
c1b9654
Compare
… feature/psr-cache3 � Conflicts: � composer.lock
Signed-off-by: Rimvydas Zilinskas <rimvydas@loftdigital.com>
Signed-off-by: Rimvydas Zilinskas <rimvydas@loftdigital.com>
Hi @rimvislt could you please fix the branch conflicts? Let me know if I can help on fixing this branch! |
I think above was decided that we will not use this pr and will be new version of it, so not sure if it is a point to fix it. |
Yah, this PR provides a lot of changes which are not only related to We will support I'd love to get some prepared PRs so I can simply merge them once we are creating the next major version(s). This will most likely happen after we created The changes in the PRs (please create dedicated PRs for both PSR packages) should contain:
Thanks! |
No description provided.