Skip to content

Replace cache_name_function with NameFilter class#762

Merged
mblaney merged 6 commits intosimplepie:masterfrom
Art4:replace-cache-name-function-with-class
Dec 13, 2022
Merged

Replace cache_name_function with NameFilter class#762
mblaney merged 6 commits intosimplepie:masterfrom
Art4:replace-cache-name-function-with-class

Conversation

@Art4
Copy link
Contributor

@Art4 Art4 commented Nov 3, 2022

It is possible to change the name of a cached value by providing a callable string via SimplePie\SimplePie::set_cache_name_function(). The default function is md5(). This gives us two problems:

  1. The provided callable is not guarantied to return a string. This could lead to errors.
  2. A callable could also be an array. Something like this is not possible:
$simplepie->set_cache_name_function([$class, 'do_something']);

This PR adds a new interface SimplePie\Cache\NameFilter that will be used internally. Implementations can be set via the new method SimplePie\SimplePie::set_cache_namefilter(). For BC reasons there is also a new class SimplePie\Cache\CallableNameFilter, that handles all kinds of callables (not just strings).

$simplepie->set_cache_namefilter(new \SimplePie\Cache\CallableNameFilter([$class, 'do_something']);

This PR also deprecates the SimplePie\SimplePie::set_cache_name_function() method.

Copy link
Member

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

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

I am not sure the added boilerplate is worth it for an obscure function like this.

Hopefully, PHP will eventually add support for generics so we will be able to annotate the argument with callable(string): string natively. For now, the combination of static analysis and runtime crashes (which will occur earlier once we enable strict type checking).

Actually, I am not even sure if SimplePie needs to make this customizable.

@Art4
Copy link
Contributor Author

Art4 commented Nov 4, 2022

I think I get your point. Unfortunately, we cannot know, if and how this function is used, so at least we could add stricter rules.

On the other hand I can see a legit use case for this function. Especially in view of the fact that we give the user the possibility to use his own PSR-16 cache. PSR-16 provides some rules for the cache key (only specific characters, not more than 64 chars, and so on), which are fulfilled with md5(). But a provided PSR-16 cache is allowed to widening this rules.

Examples:

Use longer cache key

$simplepie->set_cache_namefilter(new class () implements \SimplePie\Cache\NameFilter {
    public function filter(string $name): string {
        return hash('sha3-512', $name);
    }
});

Add prefix to cache key

$simplepie->set_cache_namefilter(new class () implements \SimplePie\Cache\NameFilter {
    public function filter(string $name): string {
        return 'simplepie_' . hash('md5', $name);
    }
});

Use raw key and handle everything in the PSR-16 cache

$simplepie->set_cache_namefilter(new class () implements \SimplePie\Cache\NameFilter {
    public function filter(string $name): string {
        return $name;
    }
});

In my opinion this function offers the best flexibility for all use cases.

@Art4 Art4 marked this pull request as ready for review November 4, 2022 09:33
@Art4
Copy link
Contributor Author

Art4 commented Nov 28, 2022

Ping @mblaney

@mblaney
Copy link
Member

mblaney commented Dec 2, 2022

looks good but there's a conflict now from the other merges

@Art4
Copy link
Contributor Author

Art4 commented Dec 2, 2022

Thank you. I fixed the conflict.

@mblaney mblaney merged commit 93fe459 into simplepie:master Dec 13, 2022
@Art4 Art4 deleted the replace-cache-name-function-with-class branch December 13, 2022 10:17
Art4 added a commit to Art4/simplepie that referenced this pull request Dec 13, 2022
mblaney pushed a commit that referenced this pull request Jan 20, 2023
* bump version to 1.8.0

* Update CHANGELOG.md

* Fix version tags in deprecated messages

* fix version in old deprecation messages

* Fix typo

see comment from @jtojnar in #752

* Add comment for DataCache interface

see comment from @jtojnar in #752

* Update CHANGELOG.md for #760, #764 and #765

* Update CHANGELOG.md for #762, #767 and #763

* Update CHANGELOG.md for #768 and #770

* Update release date

* Update CHANGELOG.md for #769 and #771

* Update CHANGELOG.md for #766
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants