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

Load policies only when needed #28

Open
askvortsov1 opened this issue Mar 16, 2021 · 3 comments
Open

Load policies only when needed #28

askvortsov1 opened this issue Mar 16, 2021 · 3 comments

Comments

@askvortsov1
Copy link

https://github.com/FriendsOfFlarum/terms/blob/master/extend.php#L77-L85 means that the full terms are included with initial request payload, always. There isn't really a need for this, and it can cause significant issues

@clarkwinkelmann
Copy link
Member

It's supposed to be cached though

return $this->cache->rememberForever(self::CACHE_KEY, function () {
return $this->policy->newQuery()->orderBy('sort')->get();
});

Is it not working as intended then?

Maybe Laravel is doing some magic to re-fetch the resources from the database when unserializing? Maybe this should be cast to array before caching?

@askvortsov1
Copy link
Author

Regardless of how it's loaded / cached, large text shouldn't always be included in the payload because it increases transmission and TTFB size unnecessarily: https://discuss.flarum.org/d/26419-help-with-a-3-million-users-flarum/14

@clarkwinkelmann
Copy link
Member

Is there a place I can see that 1MB JSON payload? That's surprising. Did they copy their full conditions inside of the terms update message?

If you just include a short message in the update notes and don't have tens of different terms, there's no way to reach that big of a payload. I just checked and everything appears to be properly serialized and there's no extra data that shouldn't be in the boot payload.

Those observations are on the latest version. If there's a problem with the beta 13 version, we'll only consider patches as paid work.

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

No branches or pull requests

2 participants