Adapt cacheFor to server changes

[CarlSchwan]

Instead of using the same name for the PicoAssetResponse cacheFor method as the OCP Response class, use a custom name. This is less likely to break in the future.

Signed-off-by: Carl Schwan carl@carlschwan.eu

[CarlSchwan]

An alternative would be to completely remove cacheFor and upstream the changes made to the method to the Response class.

We really also need in server to document better what is supposed to be overwritten and that isn't :( Currently, it's a bit of a mess and we almost never use the final keyword in the API :( I started documenting it a bit a while ago in https://github.com/nextcloud/server/pull/31447 but there is so much more that needs to be done

[PhrozenByte]

I don't think that a separate picoCacheFor method is the right approach here, because this is indeed the expected caching behaviour for a PicoAssetResponse. cacheFor is public API. When writing APIs we must always ensure that nothing breaks if someone else uses this API, even if we can't think of a reasonable use case right now. If one calls cacheFor for whatever reason, it breaks caching for PicoAssetResponse. Thus we can't do that.

I think we must rather think about the purpose of the cacheFor method. Please correct me if I'm wrong, but is this API really public (i.e. other code that consumes responses might call cacheFor)? Or isn't it rather an implementation detail of built-in responses? Doing a quick usage search in server I figured that cacheFor is always called right after the instance was created, i.e. making the cacheFor method rather an implementation detail. This is true for most methods of the Response class by the way. So we could indeed simply remove it from the public API - however, not by removing the cacheFor method altogether, but by introducing a IResponse interface instead (without this cacheFor method of course). Any other class consuming a response must expect IResponse instead of Response. This way we could indeed make Response::cacheFor() (and others) final. If one really needs to overwrite cacheFor, one can implement IResponse instead of extending Response. IMO this is the only "clean" solution, as it clearly states what the API is and what not.

Anyway, naturally there's another solution: Do nothing. The question is whether cms_pico is the only app with a response overwriting cacheFor. Adding the $immutable param is fully backwards compatible, so this is fixed for cms_pico, but will other apps break? And even if it breaks apps, it's an easy to fix issue. Thus I don't think that this is a major issue, so we might simply take the risk. Renaming 22.2.6 to 22.3.0 and 23.0.3 to 23.1.0 at least gives some "heads up", so I'd suggest doing this at least. And document it for devs of course.

[github-actions[bot]]

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in two days if no further activity occurs. Thank you for your contributions! :+1:

[PhrozenByte]

@CarlSchwan bump