danielbrendel / steamwidgets-web

SteamWidgets Web Backend
https://www.steamwidgets.net
MIT License
4 stars 2 forks source link

Redis Caching implementation #24

Open SophiaAtkinson opened 3 months ago

SophiaAtkinson commented 3 months ago

The long-awaited Steam API caching (#16)

This is a fairly simple integration, that is expandable to other Key store systems.

I first tested this with Memcached and verified that it's backward compatible, with a few code changes.

If you have any questions feel free to ask me!

-SRA

danielbrendel commented 3 months ago

Hi @SophiaAtkinson

thank you for your contribution! Before I review this further, can you merge the current main branch into your fork? Right now there are merge conflicts and unrelated commit changes included because the main branch and your fork have differences that need to be resolved.

Thank you in advance!

SophiaAtkinson commented 3 months ago

@danielbrendel That's my bad! I swore I did that, Let me fix that

SophiaAtkinson commented 3 months ago

@danielbrendel That should clear the conflicts!

danielbrendel commented 3 months ago

Thank you!

Just two things:

  1. We should make a separate module for the redis specific management. The get and set methods look rather similar, so I think we should put them in a module like RedisModule. You can create a new module with php asatru make:module RedisModule. This way we don't need to have the same two methods multiple times (to reduce redundancy).
  2. How did you include the Redis library? Do you use phpredis? Because I don't see any modifications in the composer.json, so it seems that the Redis class would be unknown in my dev environment. In order for everyone to have the same dependencies, we need composer to install them.

Other than that, we're on a good track! :) I'll set up a Redis environment on my machine as soon as I find a minute.

SophiaAtkinson commented 3 months ago

To address your first part yes that would be much smarter, to be honest didn’t even think about that lol. For the second, both my production and development environment used phpredis, I assume because I had installed Redis with composer in testing I completely forgot to add it to the JSON, or it was lost in preparation for the PR I can fix these issues later today, or tomorrow. -SRA

danielbrendel commented 3 months ago

Hi, I saw you added a dependency to the composer.json, but it seems that you choose the wrong package. Did you mean phpredis instead of predis? The latter one does not expose the used methods to communicate with the redis environment, but the former one does.

SophiaAtkinson commented 3 months ago

Yes that's my bad, I'm not too familiar with Composer, so I did mean to use phpredis

danielbrendel commented 3 months ago

I quickly added a database related caching. There is now a SteamCache module which checks for the selected cache driver and handles it accordingly.

Example:

/**
     * @param $appid
     * @param $lang
     * @return mixed
     */
    public static function cachedSteamApp($appid, $lang)
    {
        $cache = config('cache');

        if ($cache->driver === 'db') {
            return json_decode(CacheModel::remember('steam_app_' . $appid . '_' . $lang, $cache->duration, function() use ($appid, $lang) {
                return json_encode(SteamApp::querySteamData($appid, $lang));
            }));
        } else if ($cache->driver === 'redis') {
            throw new \Exception('Not implemented yet.'); //Here you can insert the redis related code. A call to a Redis Module would be the best approach here.
        } else {
            return SteamApp::querySteamData($appid, $lang);
        }
    }

Then you can add all redis stuff to a RedisModule and everything is pretty clean. ^_^ You can then choose the desired cache driver via CACHE_DRIVER from the env file.

SophiaAtkinson commented 3 months ago

G'day! Just saw this, I'm not going to be able to do work on this project until next month.