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

Redis Caching implementation #24

Open
wants to merge 13 commits into
base: main
Choose a base branch
from

Conversation

SophiaAtkinson
Copy link

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

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

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

@SophiaAtkinson SophiaAtkinson marked this pull request as ready for review April 3, 2024 23:32
@SophiaAtkinson
Copy link
Author

@danielbrendel That should clear the conflicts!

@danielbrendel
Copy link
Owner

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

SophiaAtkinson commented Apr 4, 2024 via email

@danielbrendel
Copy link
Owner

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

SophiaAtkinson commented Apr 5, 2024

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

@danielbrendel
Copy link
Owner

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

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

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.

None yet

2 participants