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

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

Conversation

Drewdan
Copy link

@Drewdan Drewdan commented Jul 27, 2021

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 Drewdan force-pushed the feature/fix-incorrect-pagination-links-across-domains branch from 829be8a to 4cc4f11 Compare July 27, 2021 01:34
…eys. This prevents pagination links from being incorrectly cached on a website which has multiple domains.
@Drewdan Drewdan force-pushed the feature/fix-incorrect-pagination-links-across-domains branch from 4cc4f11 to 8923899 Compare July 27, 2021 01:41
@Drewdan
Copy link
Author

Drewdan commented Jul 27, 2021

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
Copy link

Nutickets commented Jan 19, 2022

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
Copy link
Author

Drewdan commented Jan 19, 2022

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 :)

@Drewdan Drewdan closed this Jan 19, 2022
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.

Pagination links are incorrectly cached when the website has multiple domains
2 participants