wp-oop/transient-cache

A PSR-16 wrapper for WP transients

transient-cache's Issues

Default TTL in CachePool

It would be great if the CachePool was aware of a default TTL, to avoid having to inject the pool instance and a TTL config value into all dependents.

At the moment, the CachePool defaults to 0 when no TTL is given in set(). This proposal would see that 0 literal changed with the instance's default TTL.

`has()` wrongly returns true for expired transients

The CachePool::has() method checks if the transient option exists using get_option(), while CachePool::get() retrieves the value using get_transient().

If a transient has expired but still exists in the options table, calling get_transient will result in the option record being deleted from the database. On the other hand, get_option will simple retrieve the value of the expired transient, without deleting it.

Therefore, if a transient has expired, the CachePool::has() method will find the transient and return true, while a subsequent CachePool::get() call will delete the transient and return the default value.

Import global WP function

e.g. in CachePool

use function set_transient;
use function delete_transient;
use function get_transient;
use function get_option;

What do you think about it?

Wrong Param Type for `get_col()`

in CachePool::selectColumn()

$this->wpdb->get_col($query, $columnName);

get_col expects an integer, not a string :(
https://developer.wordpress.org/reference/classes/wpdb/get_col/

phpstan.neon.dist

includes:
    - vendor/szepeviktor/phpstan-wordpress/extension.neon
parameters:
# TODO    level: max
    level: 5
    inferPrivatePropertyTypeFromConstructor: true
    paths:
        - src/

Exception Thrown If Setting Same Value

The Problem

When using CachePool to set one or multiple values, an exception is thrown when the value for the specified key is already what is being set. In other words, setting the same value for the same key twice in succession will result in an error.

This is because under the hood update_option() (the function that WP uses to set transients) will return false in such a scenario.

Suggested Solution

Introduce an additional check in CachePool: if setting fails, get the current value and if it's the same as the new one - don't throw.

Wrong Package License

The Problem

This package is licensed under MIT. However, it uses some WordPress functions, such as set_transient(). According to GPL, anything that derives from GPL must be GPL. And WordPress is GPL...

Suggested Solution

Change package license to GPL-2.0-or-later.

Add Static Code Analysis

This involves both code quality and style tools.

Also, see #7, #8, #11.

Exceptions Not PSR-Compliant

Hello @XedinUnknown!

What a release!

 ------ -------------------------------------------------------------------------------------------------------------
  Line   CachePool.php
 ------ -------------------------------------------------------------------------------------------------------------
  112    Method WpOop\TransientCache\CachePool::set() should return bool but return statement is missing.
  126    Method WpOop\TransientCache\CachePool::delete() should return bool but return statement is missing.
  136    Method WpOop\TransientCache\CachePool::clear() should return bool but return statement is missing.
  175    Method WpOop\TransientCache\CachePool::setMultiple() should return bool but return statement is missing.
  189    Method WpOop\TransientCache\CachePool::deleteMultiple() should return bool but return statement is missing.
  304    Parameter #2 $x of method wpdb::get_col() expects int, string given.
 ------ -------------------------------------------------------------------------------------------------------------

Could you return a bool?

Discovered by @phpstan

Better exception messages

The exception that could be thrown when clearing the cache provides no meaningful explanation as to why the clearing could have failed.

There are a few exceptions that this catch could catch, but most of them will most likely never happen during cache clearing (such as this one since the cache clearing process starts by retrieving keys from the database that do start with the prefix).

Clearing errors will typically arise from an exception thrown in delete(), which itself also catches and re-throws from deleteTransient().

Ideally, the actual error message is not lost when an exception is caught and re-thrown. This puts burden on consumers to iterate through the stack trace in hopes of finding the actual reason for a failure. That process alone is sketchy at best, as the consumer would need to somehow know how long they should recurse into the stack trace in order to obtain a meaningful error message.

---

Suggestions:

a. Do not catch and re-thrown exceptions wherever possible
b. Re-thrown exceptions re-use the inner exception's message
c. Re-thrown exceptions append the inner exception's message to their own