r84395 MediaWiki - Code Review archive

I can't say I like this. This also effects recordUpload2() and seems to assume a backend function is only called from a certain UI entry point.

I concur. purgeThumbnails() could be made to accept a list of extensions to exclude, and let the entry point determine to set that.

I don't think that’s a good idea. This would mean adding another hook of some sort to build this list since each extension adds in support for new file extensions to be supported its impossible for every call to purgeThumbnails to know about every extension that adds new file types.

I suggest that we have another function called removeThumbnailFolder or a parameter to "force remove everything" if you want to have the ability for purge calls to clear all the thumbnails associated with an asset.

We want to keep some concept of default "light purge" that user invoked action=purge calls so that we are not constantly removing assets that are expensive to recreate. We can't have all the calls to purgeThumbnails know about all the file types that extensions add without some variable similar to what we already have in place .

Fair enough. Still the use of a global variable bugs me. Perhaps we can have the File object request from the MediaHandler which thumbnail types to remove on light purge. That looks more correct to me.

+1

Marking as fixme

We should clear this up and backport to 1.18 before it's released and we're stuck with it

Perhaps we can revert this in 1.18, so that we have time to discuss this and include it in some form in 1.19. Michael, what is the deployment plan for TimedMediaHandler?

Any update? I would argue for keeping it around as a good simple solution while the more complicated solution is developed.

It may be simple, but it's not good. We should take the time to do it right like Bryan suggested above. His idea isn't complicated.

right.... I don't think its that "bad", its essentially just representing a property of file extension via a global instead of through its media handler. Other file extension properties are represented in globals.

At any rate as mentioned below, if we can get this into 1.18 / part of next deployment update, and we swap in the new code at the same time. I am all for it!

The plan is to work on deploying TMH starting in September. I will have more specifics early next week.

Bryan yes it would be a good solution, to add some method to MediaHandler object handler that had a pagePurge call so that the given media handler could decide if its costly to recreate the derivative or not and purge based on that. And that makes sense because the MediaHandler object is responsible for creating the thumbnails/derivatives in the first place. Also would need a removeDerivatives call that force removed the derivatives that backed functions could call when they are sure they want to remove the thumbnails/ derivatives regardless of how costly it is to create them. And or we tightly couple the remove "costly derivative" rights that TMH adds.

At any rate this is a larger change to core. Which I think is fine to do if we commit that to that change at the same time that we commit the change to TMH. It would not be ideal to have trunk or a release in a state that removes all derivatives on pagePurge and extensions have no way to control that purge.

The configuration option is a good way to address this need without a complex patch. I don't think its that big a deal if we support these 3 or 4 lines of code to give TMH a much much larger compatibility window while the effort to improve MediaHandler is addressed. I don't see many other extensions making use of this configuration option which would make it a good candidate for rapid deprecation once we have a better solution in place.

There are other aspects of MediaHandling that could use clean up as well. Like the ridiculous long sets of arguments that get passed around instead of configuration objects like TMH does.

I have added MediaHandler::filterThumbnailPurgeList() in r98710. Somebody still needs a way to indicate in the UI the distinction between a full and partial purge.