Page MenuHomePhabricator

Make GraphViz 3.x Configurable Again
Closed, ResolvedPublic

Description

GraphViz versions 2.x used to be configurable via $wgGraphVizSettings. Ever since the version bump to 3, configuration was removed and things like the DOT execPath were hard-coded. This task is to add the configurability back. I propose we use the composer extension.json's field config for default setting values (read more here).

Details

Related Gerrit Patches:
mediawiki/extensions/GraphViz : masterMakes GraphViz configurable again

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 22 2018, 6:07 PM
Samwilson added a subscriber: Samwilson.

Change 472057 had a related patch set uploaded (by Juan Osorio (Microsoft); owner: Juan Osorio (Microsoft)):
[mediawiki/extensions/GraphViz@master] Makes GraphViz configurable again

https://gerrit.wikimedia.org/r/472057

@osorio-juan-microsoft I was going to merge your patch (it looks great) but then re-read the wiki...

The existing documentation says that something like the following should be possible:

wfLoadExtension( 'GraphViz' );
$wgGraphVizSettings->execPath = "/other/path/to/dot";

because that used to be possible when the GraphViz initialization process included:

$GLOBALS['wgGraphVizSettings'] = new GraphVizSettings();

but now results in:

Creating default object from empty value

And the above patch also doesn't support this because it treats the settings as an array, not an object.

It seems to me that using a single config var to hold an extension's whole configuration is not a common thing, and instead each variable should be named separately (and prefixed with the extension name). This wouldn't make sense, if we were maintaining backwards-compatibility, but if we can't maintain BC then perhaps we should think about moving to the more common way of doing it?

Sorry I didn't think of this earlier.

Of course, it's probably fine to just go with the array-style config var, as the patch is currently, and people can just update their LocalSettings.php when upgrading. What do you think?

@Samwilson You're right! Sorry I missed this.

I think it will be difficult to both adopt the new configuration paradigm and support backward compatibility. In this case, I think it would be best to go with single, separate setting items instead of the way it is in the patch. This will avoid confusion for users migrating from version 2.x. From what I can tell, there is no way to provide objects instead of arrays in the new settings style.

@Samwilson I've submitted a new patch for single setting items. Let me know if this is better!

Change 472057 merged by jenkins-bot:
[mediawiki/extensions/GraphViz@master] Makes GraphViz configurable again

https://gerrit.wikimedia.org/r/472057

osorio-juan-microsoft closed this task as Resolved.Nov 14 2018, 12:53 AM