At first glance this doesn't look B/C as you're changing the types of data being passed as parameters into some methods and changing the return types in other methods (i.e. JCacheController::get() won't unserialize data now). You're also not updating any of the method documentation; with this change you're injecting objects into places that are expecting strings which would fail massively if we had scalar typehinting. This would also presumably need new test cases to validate the behaviors of the individual components of the API, not just that you're introducing new logic in the controllers and JCache itself mitigates any potential changes.

At first glance this doesn't look B/C

If someone has own custom cache controller then B/C should be ok, but serialize run twice.

i.e. JCacheController::get() won't unserialize data now).

This is unserialize earlier in JCache class.

You're also not updating any of the method documentation

Yes this is missing.

This would also presumably need new test cases to validate the behaviors of the individual components of the API

I only move serialize to other (low level) place.
Serialize should not be related with JCacheController but more with JCacheStorage.
For B/C I moved it to JCache and check result of supportRawData().

All current storages does not have implemented supportRawData() and return false then there should not be any difference.

All current storages does not have implemented supportRawData() and return false then there should not be any difference.

Right now, no. But right now you're opening the door to state that one day in the future the adapters MAY support objects after this pull request is merged. Therefore I think we need to ensure our adapters will handle cases where objects may slip in (remember none of the API is marked internal and there is a public factory to direct retrieve the adapters and completely bypass JCache or the controllers, so it's 100% fair game for someone to put their own caching API in front of our adapters).

Either way I still think the controllers and JCache itself should get an updated test case where it validates the potential new behavior. And maybe even a stub storage adapter in the test suite that does allow direct object injection to specifically validate that portion of the API works (right now it's all false so existing behavior should be retained but if someone creates a custom adapter that allows objects we need to stand confident in our API that it'll be supported).

Yes some additional test and documentations will be required.
For me it takes longer than write that code:)

@csthomas is this still live or can it be closed - its been quite some time since your last comment

@mbabker What do you think about this PR?
I have added unit tests for available joomla storages and for 3rd party storage which does not support raw data.

Now $rawData for B/C by default is false, but all joomla cache controllers use $rawData=true.

This and #16307 are going to collide. We can add features to our Framework package if need be, but based on the two caching PSR interfaces, we can't rely on certain functionality we already have (i.e. locking) without losing the interoperability using a greater PHP standard would give.

I took a look only a few minutes but I saw that your PR has a serialize/unserialize functions in adapters (explicitly in File). That was my idea but I tried to do that in staging and because of B/C I did it in more complex way.

https://github.com/joomla/joomla-cms/pull/16307/files#diff-63a9b83d6fde7ce6d33ee990cf2c649cR126

The filesystem adapter is serializing since it has to handle writing the data on its own. For the rest of the adapters though, we're just pushing the data (in whatever source it's given) to the storage API without any type of processing (so we're letting APCu as an example figure out if it needs to serialize or do whatever internally).

I close it