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

[10.x] Add the ability to re-resolve cache drivers #46203

Merged
merged 6 commits into from Feb 21, 2023

Conversation

stancl
Copy link
Contributor

@stancl stancl commented Feb 21, 2023

(Resubmission of #46202, opening this PR from my own repo this time)

We're working on improving our cache tenancy support in Tenancy for Laravel and one of the things we'd need would be the ability to re-resolve the drivers for cache stores.

Currently (in v3), we only support Redis since we use cache tags to scope cache for tenants.

In v4, we'd like to have driver-agnostic cache tenancy using prefixes. We already have all of the code written (and tested with a laravel/framework fork that includes the changes from this PR) so we'd only need this added in Laravel.

We experimented with several different implementations and this is the one that requires the least changes to Laravel. It essentially only makes one method public and adds a setter to Illuminate\Cache\Repository.

The changes here could in theory be breaking if:

  1. someone overrode the resolve() method in Illuminate\Cache\Manager and kept it protected
  2. someone had a custom setStore() in Illuminate\Cache\Repository. We tried to avoid this being a problem by making the method as predictable as possible, so it's a setter that does exactly what its name implies.

I think both of these changes are very low-risk and easily fixable if anyone overrode these methods. We also avoided touching any contracts and only changed the default classes that implement them.

(Please squash when merging)

@henzeb
Copy link
Contributor

henzeb commented Feb 21, 2023

is Adding to an interface not a breaking change?

@stancl
Copy link
Contributor Author

stancl commented Feb 21, 2023

We aren't changing any interfaces.

@henzeb
Copy link
Contributor

henzeb commented Feb 21, 2023

We aren't changing any interfaces.

yeah, I realized it later on. I read upside down. I'd suggest when resubmitting, don't use old branches with commits that you have undone in a later commit (or squash them), to avoid conversations like this. :)

@stancl
Copy link
Contributor Author

stancl commented Feb 21, 2023

Just look at the Files changed, I mentioned that the PR should be squash merged.

@jmarcher
Copy link
Contributor

This is in fact a breaking change, changing the signature of a method is a breaking change

@stancl
Copy link
Contributor Author

stancl commented Feb 21, 2023

I discussed that in the PR description in detail. I'm pretty sure PRs that change method visibility to public are often merged into Laravel when they're reasonably low risk. It isn't about whether something is a breaking change by the strictest definition of the term, it's about the real world impact.

Also, Laravel 10 is freshly released and sometimes there are small breaking changes early on in a version release when bugs that weren't caught before the release are being fixed. And as I wrote above, in the (very unlikely) scenario that someone extended this method and kept it protected, they would get a clear error message and would just change it to public. It isn't a change that would change actual behavior in an unpredictable way.

@taylorotwell taylorotwell merged commit cee53ed into laravel:10.x Feb 21, 2023
@stancl
Copy link
Contributor Author

stancl commented Feb 21, 2023

Thanks!

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

Successfully merging this pull request may close these issues.

None yet

5 participants