search

FriendsOfFlarum / terms

Ask your users to accept TOS and Privacy Policy
https://discuss.flarum.org/d/11714
MIT License
14 stars 9 forks source link

Load policies only when needed #28

Open askvortsov1 opened 3 years ago

askvortsov1 commented 3 years ago

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 commented 3 years ago

It's supposed to be cached though

https://github.com/FriendsOfFlarum/terms/blob/9f6611649bf09ac189485589d004e2f0f738a0be/src/Repositories/PolicyRepository.php#L38-L40

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 commented 3 years ago

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 commented 3 years ago

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.