Skip to content

Conversation

@innocenzi
Copy link
Member

@innocenzi innocenzi commented May 14, 2025

This pull request is a complete overhaul of the cache component. The main purpose behind it is to have a similar implementation to the database and storage components in regards to the configuration. The internal caches (config, view, icon) are now completely separate caches with their own implementations.

Summary of changes

  • Refactored internal caches out of tempest/cache
  • Abstracted Symfony Cache away with custom config objects
  • Added several convenience methods
  • Added cache locks
  • Added tempest about insights
  • Added testing utilities

Comparison between previous and new API

Previously, configuring the user cache required instantiating one of Symfony's adapter in the CacheConfig object:

return new CacheConfig(
    pool: new FilesystemAdapter(
        directory: __DIR__ . '/../../../../.cache',
    ),
);

Now, you may use one of the provided config objects:

// filesystem
return new FilesystemCacheConfig(
    directory: internal_storage_path('/caches/project'),
);

// in-memory
return new InMemoryCacheConfig();

// redis (not implemented yet, requires us to add a Redis package)
return new RedisCacheConfig(
    prefix: env('REDIS_CACHE_PREFIX'),
    host: env('REDIS_HOST', default: '127.0.0.1'),
    username: env('REDIS_USERNAME'),
    password: env('REDIS_PASSWORD'),
);

And of course, it's now possible to add multiple caches by adding a tag to one of the config objects:

// cache.config.php
return new FilesystemCacheConfig(
    directory: internal_storage_path('/caches/project'),
);

// another-cache.config.php
return new FilesystemCacheConfig(
    directory: internal_storage_path('/caches/another-cache'),
    tag: 'another-cache',
);

// or add one on-the-fly
$this->container->config(new FilesystemCacheConfig(
    directory: internal_storage_path('/caches/on-the-fly-cache'),
    tag: 'on-the-fly-cache',
));

The cache:clear and cache:status commands now separate user and internal caches. By default, cache:clear clears the default user cache. If more caches were created, you may select which ones to clear.

Environment variables

Last but not least, the way caches are enabled or disabled have been simplified. Instead of having to have a CACHE, PROJECT_CACHE, ICON_CACHE, VIEW_CACHE, CONFIG_CACHE, and DISCOVERY_CACHE in .env, caches are disabled by default in a local environment, and enabled in production (except for the icon cache, which is always enabled by default).

Of course, they can still be disabled with those environment variables, but they are optional. However, the CACHE environment variable has been renamed to INTERNAL_CACHES for clarity.

In short, here are the new environment variables (all optional):

# Setting to `false` force-disables all internal caches (icon, view and config)
INTERNAL_CACHES=true

# Enable or disable discovery cache. Can be `true`, `partial` or `false`.
# `false` by default in dev, `true` by default in production.
DISCOVERY_CACHE=

# Enable or disable the icon cache. True by default all the time.
ICON_CACHE=

# Enable or disable the view cache.
# `false` by default in dev, `true` by default in production.
VIEW_CACHE=

# Enable or disable the config cache.
# `false` by default in dev, `true` by default in production.
CONFIG_CACHE=

@brendt
Copy link
Member

brendt commented May 16, 2025

This all makes sense. I think it's better to merge before 1.0 though, because I believe it's technically a breaking change?

One remark

except for the icon cache, which is always enabled by default

Shouldn't view cache be enabled by default as well?

I still need to go through the code, but let's first hear your thoughts on merging it early?

@brendt brendt added this to the v1.0.0-beta.2 milestone May 16, 2025
@innocenzi
Copy link
Member Author

Shouldn't view cache be enabled by default as well?

I'm not sure, I think it was not enabled by default before. I haven't really taken a look at how it works exactly, but if you think it should be, we can change that

but let's first hear your thoughts on merging it early?

Totally fine by me (I would even prefer that)—but I don't mind if you prefer merging later

@brendt
Copy link
Member

brendt commented May 17, 2025

I'm not sure, I think it was not enabled by default before. I haven't really taken a look at how it works exactly, but if you think it should be, we can change that

I'm not sure either anymore, let's make a note to double check before merging

Totally fine by me (I would even prefer that)—but I don't mind if you prefer merging later

Let's merge before beta (but after the ORM refactor so that I can review the code as well)

@innocenzi
Copy link
Member Author

I'm not sure either anymore, let's make a note to double check before merging

I'm confident it wasn't enabled before! I just don't know if that was intended or not.

@brendt
Copy link
Member

brendt commented May 19, 2025

Then it's fine! I'll review today :)

Copy link
Member

@brendt brendt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Most of my remarks aren't real blockers, I think I rather just wanted to better understand your reasoning behind some features.

It's a great PR, looking forward to hearing your thoughts on those comments, and then merging before beta.2.

Apart from the cache interface changes, what kind of things will break for users because of this? For example: we merge this, what should be changed on tempest-docs production to make sure caching still works the way it's supposed to work?

@innocenzi
Copy link
Member Author

Apart from the cache interface changes, what kind of things will break for users because of this? For example: we merge this, what should be changed on tempest-docs production to make sure caching still works the way it's supposed to work?

Only the cache:clear command in the deploy script I believe. We need to add the --internal flag.

@brendt
Copy link
Member

brendt commented May 21, 2025

@innocenzi I'm fine merging this, just wanted to double-check with you if that's ok?

@innocenzi
Copy link
Member Author

@brendt Yes, let's go! I'm fixing the failures that are due to the merge of main, then I'll merge and update tempestphp/tempest-docs

@innocenzi innocenzi merged commit 36edbd8 into tempestphp:main May 21, 2025
65 checks passed
@innocenzi innocenzi deleted the refactor/cache branch May 21, 2025 12:35
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.

2 participants