mikebronner / laravel-model-caching

Eloquent model-caching made easy.
MIT License
2.23k stars 212 forks source link

Fix pagination links not linking to correct domain for websites with multiple domains #408

Closed Drewdan closed 2 years ago

Drewdan commented 2 years ago

This MR adds a key, so caches are per domain, rather than multi domain. This prevents issue like those stated in #407

Close #407

Drewdan commented 2 years ago

Wow! This has a lot more tests than I remember 😅 - all the tests are passing now. I have no access to the better code hub though, so I cannot see what is wrong there. Let me know if this PR needs any changes :)

Nutickets commented 2 years ago

I just encountered this scenario of pagination links containing the wrong domain when retrieved from cache and my first thoughts were exactly the same as your proposed solution of caching by domain.

However I think this isn't the right approach for 2 reasons:

  1. You are increasing the amount of data in the cache by caching the same data n times depending on how many alt domains you have.
  2. This doesn't consider scenarios where you pull the same paginated data from multiple different URL paths.

The second point is the bigger concern because imagine you have a page for assigning a role to multiple users and the URL is something like https://example.com/roles/1/assign (where 1 is a role ID). If you call User::paginate() on that page, then separately visit /roles/2/assign.. the moment you click a pagination URL, you are gonna be sent to the URL where the users were first cached, which would be /roles/1/assign.

It's highly likely that the user would even notice the URL change and now they are assigning role 1 instead of role 2, which could be catastrophic.

Proposed Solution Assuming pagination links should always be based off the current URL (cannot think of a situation where this wouldn't hold true), it makes sense to re-calibrate the URLs upon retrieval of a paginated results set from the cache. This turns out to be actually quite a trivial fix as you can simply hook into the part where Laravel unserializes the paginator after retrieving it from Redis.

// .. inside AppServiceProvider.php
$this->app->bind(LengthAwarePaginator::class, function ($app, $values) {
    return new YourCustomLengthAwarePaginator(...array_values($values));
});
// .. inside YourCustomLengthAwarePaginator.php
public function __wakeup()
{
    if ($path = request()->getPathInfo()) {
        $this->setPath($path);
    }
}

Of course the above involves overriding the native Laravel paginator and really could be done automatically from the model caching library, but this solution also worked for us in places where we were applying manual caching (rather than using model cache) to paginated results so thought it was worth sharing.

Drewdan commented 2 years ago

Hi @Nutickets

Thank you for your comment and thoughts.

In our software solution, we are also using a similar approach you have mentioned, in that we are setting the path every time we create pagination links rather than resolving a custom paginator.

Good spot on the caching of the URL. I guess as I am just adding the root URL to the key, it would not differentiate properly between the URLs.

I think, with this in mind, will close this PR and start a fresh with this new knowledge!

Thank you :)