Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add config option to allow the use of the default local repo, rather than $diagramsRepo #44

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

Longsight
Copy link

@Longsight Longsight commented Oct 6, 2021

In cases where file storage uses a non-local backend, assumptions made in $diagramsRepo do not always work. In our instance, Mediawiki file storage is on a Swift-compatible backend which proved to be incompatible with $diagramsRepo, but using $localRepo as set by MediaWikiServices::getInstance()->getRepoGroup()->getLocalRepo() worked as expected.

This change therefore adds a config value to allow the use of the default local repo rather than the one created in renderLocally(), for this and other use cases where the user may want to use the same repo settings as for other uploaded files. By default, it does not change the current behaviour.

@samwilson
Copy link
Owner

Thanks for this!

I wonder if actually that should be the default? I can't remember now what the point of having the subdirectory was (I think that's the only point of the custom LocalRepo). It's definitely neater, but it does seem that if people want to keep the generated files out of their main file store then they should use the Diagrams service. Maybe it's too late to change the default now though…

@Longsight
Copy link
Author

I don't think it necessarily needs to be default, given that most people probably do still leave the repo settings as is and host files locally (and Mediawiki doesn't encourage or even document anything else very well). I'd imagine the only reason for using a separate repo in the first place was to get the /diagrams subdirectory, and that will still work fine for most users, so there's no real need to break the default behaviour. There just needs to be the option, so that other repo backends work as expected if wanted.

@samwilson
Copy link
Owner

Yes, that makes sense, good points!

Do you know what specifically is making it not be compatible with your local repo configuration? It seems that the best thing would be for it to be made compatible, then it'd work for everyone. Are there missing or incorrect config values in this?

			'class' => 'LocalRepo',
			'name' => 'local',
			'backend' => $localRepo->getBackend(),
			'directory' => $localRepo->getZonePath( 'public' ) . '/diagrams',
			'url' => $localRepo->getZoneUrl( 'public' ) . '/diagrams',
			'hashLevels' => 0,
			'thumbUrl' => '',
			'transformVia404' => false,
			'deletedDir' => '',
			'deletedHashLevels' => 0,
			'zones' => [
				'public' => [
					'directory' => '/diagrams',
				],
			],

Some others are: 'descBaseUrl', 'scriptDirUrl', 'articleUrl', 'fetchDescription', 'thumbScriptUrl', 'pathDisclosureProtection', 'descriptionCacheExpiry', 'favicon', 'thumbProxyUrl', 'thumbProxySecret', 'disableLocalTransform',
'useJsonMetadata', 'useSplitMetadata', 'splitMetadataThreshold', 'updateCompatibleMetadata', 'reserializeMetadata'.

If we can't get that to work, then the config flag is fine. I do wonder about calling it something different though, such as $wgDiagramsLocalSubdir = true, because $wgDiagramsUseLocalRepo is a bit opaque for wiki admins who don't know about the internals of file storage. They'd be used to setting other upload vars, and the docs of this new Diagrams config var would be something like

$wgDiagramsLocalSubdir (bool): Use a diagrams/ subdirectory within $wgUploadDirectory (by default this would be images/diagrams/) to store the generated diagram files. Set to false to include these files in the same directories as uploaded wiki files. Will be ignored if $wgDiagramsServiceUrl is set.

@Longsight
Copy link
Author

Longsight commented Nov 5, 2021

In our case I think it's the zones setting that's tripping it up, although there may be others. Our repo config looks like this:

$wgLocalFileRepo  =  array(
  'class'             => 'LocalRepo',
  'name'              => 'local',
  'backend'           => 'localSwift-wiki',
  'scriptDirUrl'      => $wgScriptPath,
  'zones'             => [
    'public' => ['container' => 'public', 'url' => 'https://cdn.example.com/swift/v1/wiki-public'],
    'thumb' => ['container' => 'thumb', 'url' => 'https://cdn.example.com/swift/v1/wiki-thumb'],
    'temp' => ['container' => 'temp', 'url' => 'https://cdn.example.com/swift/v1/wiki-temp'],
    'archive' => ['container' => 'archive', 'url' => 'https://cdn.example.com/swift/v1/wiki-archive'],
    'deleted' => ['container' => 'deleted', 'url' => 'https://cdn.example.com/swift/v1/wiki-deleted'],
  ],
  'hashLevels'        => 2,
  'deletedHashLevels' => 0,
);

Note that most of the other values are missing anyway - either because they don't apply in our case or don't apply to Swift storage - but that this is still a valid repo. I suspect that with the plethora of available storage settings, a one-size-fits-all approach won't always work, and trying to correctly handle all possible use cases for the sake of a diagrams/ prefix is a needlessly complex task compared with simply adding a flag to use the repo already defined in config.

If backwards-compatibility weren't necessary I'd suggest just dropping the new repo altogether, but people might lose access to their existing diagrams!

@samwilson
Copy link
Owner

I'm sorry that I've neglected this for so long. :-(

I agree with you, it'd be best to not have to worry about having a separate diagrams/ subdirectory, but I think the main issue with this — and this extension in general — is that there's no way to purge old generated images. My thinking in having a separate directory was that it'd be possible at some point build something to get rid of out-of-date images. At the moment, every page edit or preview could create a new image, some of which will never actually be used.

What do you think about the $wgDiagramsLocalSubdir name?

@Longsight
Copy link
Author

I've neglected it too! The $wgDiagramsLocalSubdir approach seems fine to me. I'm currently in the middle of some other related Swift work which will probably lead to other PRs in the near future, so whatever approach you think works best for you here is good.

@Longsight Longsight closed this Mar 30, 2022
@samwilson
Copy link
Owner

Great!

Did you mean to close this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants