[SoftDeleteable] Remove dollar sign from cache key

[adambalint-srg]

This PR removes dollar sign ($) from the default cache key. It's not a globally reserved characters in filenames, but there are cases when it can cause problems, as I read. This is why e.g. Laminas's filesystem cache disables this character. I think using __ instead of _$ isn't a problem, and won't cause any problems.

[VincentLanglet]

Hi, do you have any example when it's an issue ?

[adambalint-srg]

In my application (Laminas+Doctrine stack) I'm using this repo's extensions, and for Doctrine I'm using Laminas's filesystem cache. So the cacheId extension metadata will be used in file system cache. Now it throws an error. I've created a quick example where you can reproduce:

{
    "name": "test/test",
    "require": {
		"php": "^8.3",
		"laminas/laminas-cache-storage-adapter-filesystem": "^3.1.0",
		"gedmo/doctrine-extensions": "^3.20.1"
	}
}

<?php

declare(strict_types=1);

include 'vendor/autoload.php';

use Gedmo\Mapping\ExtensionMetadataFactory;
use Laminas\Cache\Storage\Adapter\Filesystem;

$cacheId = ExtensionMetadataFactory::getCacheId('Foo', 'Bar');
//cacheId is 'Foo_$BAR_CLASSMETADATA';

$fileSystemCache = new Filesystem();
$fileSystemCache->addItem($cacheId, ['some' => 'data']);
//Uncaught Laminas\Cache\Exception\InvalidArgumentException: The key 'Foo_$BAR_CLASSMETADATA' doesn't match against pattern '/^[a-z0-9_\+\-\.]*$/Di'

[mbabker]

Since this library requires a PSR-6 cache, and from what I can tell the Laminas cache doesn't directly implement that PSR (it does provide a decorator for this), is there an issue when you use their PSR-6 decorator with this library? (On a side note, I'd say the ExtensionMetadataFactory::getCacheId() really should've been marked internal and not used externally or in the exact way that reproducer snippet is written, but it's a little late for that now)

From a practical perspective, there isn't an issue with this PR (and presumably improves PSR-6 compatibility by ensuring only characters the spec explicitly says an implementation must support are used). But, it seems that the MongoDB ODM also uses a dollar sign in its cache keys which would imply that ODM and Laminas Cache are incompatible for the same reason.

[adambalint-srg]

@mbabker I'm using PSR-6 decorator in my application. But this decorator does nothing with the cache keys.

/** @var StorageAdapterFactoryInterface $storageFactory */
		$storageFactory = $container->get(StorageAdapterFactoryInterface::class);

		/** @var StorageInterface&FlushableInterface $fileCache */
		$fileCache = $storageFactory->create('filesystem', $options);

		return new CacheItemPoolDecorator($fileCache);

The direct use of the getCacheId is only for this example, I don't use it directly.

[adambalint-srg]

@phansys Changelog has been updated

[VincentLanglet]

I think a new rebase is needed since 3.21 is released

[adambalint-srg]

@VincentLanglet Sorry for mixing changelog parts. Is it OK now?

[VincentLanglet]

@VincentLanglet Sorry for mixing changelog parts. Is it OK now?

I think it is. It's on @phansys side now

[codecov[bot]]

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 78.75%. Comparing base (c9cafa6) to head (b3e7b0d). :warning: Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2978   +/-   ##
=======================================
  Coverage   78.75%   78.75%           
=======================================
  Files         169      169           
  Lines        8695     8695           
=======================================
  Hits         6848     6848           
  Misses       1847     1847           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:

• :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[adambalint-srg]

@phansys @VincentLanglet Is there anything what is still problematic with this PR? Or for what we are waiting? I'm still on a fork in my projects, and the production is coming :)

[VincentLanglet]

It's on @phansys side, I dont have any right on this project

[phansys]

@phansys @VincentLanglet Is there anything what is still problematic with this PR? Or for what we are waiting? I'm still on a fork in my projects, and the production is coming :)

From my side, the only request that is waiting for resolution is this: https://github.com/doctrine-extensions/DoctrineExtensions/pull/2978#discussion_r2373700631.

[adambalint-srg]

@phansys I did a changelog update after this unresolved comment (9f129431ebf97764d18389dceb876a0e4279390f), but didn't get any update or feedback about this.

[phansys]

@phansys I did a changelog update after this unresolved comment (9f12943), but didn't get any update or feedback about this.

IMO, the update is correct. Thank you.

Currently, the commits in this PR require a rebase to allow the merge.

Could you please perform a rebase @adambalint-srg?

[adambalint-srg]

@phansys Currently I can't see any commits to rebase. Yesterday I did a conflict resolve, and it says now my branch is ahead of main, and not behind. Or I don't see something?

[phansys]

@phansys Currently I can't see any commits to rebase. Yesterday I did a conflict resolve, and it says now my branch is ahead of main, and not behind. Or I don't see something?

This PR currently contains more commits than those related to your changes:

This situation introduces conflicts that prevent the PR to be merged using the Rebase strategy:

[adambalint-srg]

@phansys I don't know how it happened, but now it seems OK for me

[phansys]

Thank you @adambalint-srg!