Progress. Now how do we get more people actively interested?

Good question!

I might have some time this evening for this.

So, one way to do this is to permit only for the instance of GraphViz graph modifying any user to upload. Basically, for the job of rendering the graph, we'd grant upload to the current user. Is that a dreadfully hacky way to do this?

@Kghbln do you have an opinion? How does SemanticResultFormats handle this? (As far as I can see, it's just using $GLOBALS['wgParser'] and not doing anything for anonymous users.)

Personally I think that any extension should stick to the respective permission setting of "upload" for the respective user group.

I can imagine the SemanticResultFormats inherits the behaviour from the GraphViz extension so creating a graph is not permitted without the "upload" permission. That's what it actuallly does in the end. I am less than a novice coder so I cannot provide substantiated details.

Hm, but what about when the results of a semantic query change? (i.e. a long time later, after the upload-authorized user has left the page, and the semantic results are changed by someone else changing some other page). Does the graph regenerate? At the moment, in that situation in the Genealogy extension, an error message is shown.

Same behaviour for graphs generated with Semantic Result Formats. It only regenerates if a new result set is to be shown and after a user with upload permission purges the page. Otherwise one gets the error message as described. So I now see your issue.

Since the file containing the graph was validly generated in the first place the extension could check for the existence of a file of the same name in the same location since both location and name should not change. If a file already exists the extension could regenerate it using a special purpose user account e.g. "GraphRegenerateBot" created for this purpose which has the upload permission.

That does sound like the better option. And the name of that account could be configurable, or set by a system message.

This is certainly a GraphViz issue then, not a Genealogy one.

And the name of that account could be configurable, or set by a system message.

There are a couple of extensions registering accounts, probably the latter method to do it via a system message is the more recent and better solution.

This is certainly a GraphViz issue then, not a Genealogy one.

Indeed sounds like it.

If you are considering a semantic extension, maybe you should have another look at the option of installing the same semantic extensions as Familypedia has and copying or adapting its templates and forms and properties etc. I know that Sam said it might not be supported, which I presume means that WMF might not accept it as a project because of the complexity. The recent discussions here look fairly complex to me (as another barely-novice programmer)! Disadvantages with Familypedia's system probably include limits on the number of generations a tree can show - but those limits may be mostly because it is part of a huge wikifarm whose owners are unwilling to adapt for just a few sites.

Change 398784 had a related patch set uploaded (by Samwilson; owner: Samwilson):
[mediawiki/extensions/GraphViz@master] Regenerate graphs even when user is not permitted to upload

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

I've started some work on this, but it's not fully done yet.

Okay, if anyone feels like reviewing https://gerrit.wikimedia.org/r/#/c/398784 it's ready. Any suggestions for other tests would be good too.

@Welterkj commented on https://gerrit.wikimedia.org/r/#/c/398784 but I'm moving it here to include people who mightn't be watching that patch.

Sorry to be chiming in late but I wonder if this is the right design direction for this fix. Arguably, since GraphViz version 2.0.0, the extension no longer performs "uploads" since no file page is created corresponding to a graph image. So, I think removing the check for upload permission is reasonable and probably cleaner.

Is this correct? It creates uploads for me. I thought that was the big change that happened, and that this extension used to not create File pages for graphs but now does?

At a glance, it looks like removing the UploadBase::isAllowed call in UploadLocalFile::isUploadAllowedForUser would do the trick. However, UploadBase::isAllowed checks for both upload and edit permission. Has edit permission already been checked at this point in the execution of the extension? Maybe better to do the (possibly superfluous) edit permission check at this point just in case.

Good points about moving the check elsewhere (I think perhaps in isUploadAllowedForTitle() as well?). This would mean that a user who doesn't have upload rights would be able to create graph uploads, which we discussed above and decided against (although it was just me and @Kghbln, so hardly widespread consensus).

Perhaps this needs a rethink, and a move away from creating File pages for graphs?

GraphViz truly no longer performs uploads (i.e. creates file pages) since version 2.0.0. I'm puzzled that you say otherwise. Here is a tell-tale sign. Prior to version 2.0.0, a generated graph image would include a blue circle with an "i" inside in the lower right hand corner that would link to the file page associated with the graph. At version 2.0.0+, the little blue superimposed "info" icon is gone since there is no file page to link to.

In T100795#3091815, @Welterkj wrote:

I've written a fix that is actually a design change. The fix performs the upload of graph or message sequence chart images without creating a file page. This approach eliminates the need for dummy file pages and other awkward techniques. Creation of these file pages may be considered a feature of the extension that will be lost. On balance, they seem to have caused more pain than good (both in the implementation and in the user community) and I now think it is best to abandon that feature. The fix needs some cleanup and additional testing so stay tuned.

Background: When I re-wrote this extension for version 1.0.0 I made a conscious design decision to use core upload functionality in the most unadulterated way possible. However, such uploads necessarily created file pages which were extremely tricky to handle in the context of a parser function which the GraphViz extension essentially is. For example, I had to create "dummy" file pages at "safe" points (outside the parser function) so that I could upload real graph images on top of them when inside the parser function. The alternative, which I'm now taking, is to customize the upload so that graph images are stored in the wiki file repository and image properties are stored in the database but corresponding file pages are not created. A consequence of this is that users will lose the ability to directly manage (e.g. link or delete) graph images via the file page. The extension has logic to detect when a graph image is no longer in use and delete it (e.g. when a page with graph source is deleted). That will have to be sufficient.

In T176594#3894946, @Welterkj wrote:

At version 2.0.0+, the little blue superimposed "info" icon is gone since there is no file page to link to.

It's not linked anymore, but a new file still shows up in Special:FileList (e.g.) and is definitely created; see https://sandbox.semantic-mediawiki.org/wiki/Fichier:GraphVizAnonymous_digraph_example1_dot.png for example.

Interesting. When I wrote GraphViz version 2.0.0 for MW 1.27 there was no page created in the "File" namespace for graph images but sure enough I see it on your MW 1.30 installation.

Regardless, I think that the simplest and best solution is to remove the requirement of upload permission for graph image creation. There is a strong argument that anyone permitted to edit the wiki text graph source should have been able to generate the corresponding graph from the beginning (i.e. GraphViz version 1.0.0). The user is not supplying the image-- the extension is.

Is this still an issue?

Yep it is. I think the best solution might be to not upload graph images as actual wiki images (especially as it sounds like that was your intent originally?).

No, my idea is to remove the restriction that graph editors must have upload permission.

BTW, looking back at the task description, I don't know what the context is for "tree regeneration". Are you talking about a specific Semantic Results Format?

Okay, that sounds fine; let's just give the uploading user the required rights when they're uploading.

The tree generation referred to is to do with the Genealogy extension (that's where this issue started, but after the root cause was thought to be GraphViz, it turned there).

I wrote a draft of the fix today and got an anonymous user to create a graph. I have to do some homework verifying correctness before I'm ready to check it in for review.

As I got deeper into coding the "don't check for upload permission" change it got ugly. To be specific, UploadLocalFile::isUploadAllowedForTitle (GraphViz) currently calls UploadBase::verifyTitlePermissions (MediaWiki core) which checks whether the user can edit, upload and create the image AND if the user is allowed to overwrite an existing image. The overwrite check is done by calling the private method UploadBase::checkOverwrite. There is no way to bypass the upload permission check without also bypassing the overwrite check. The overwrite check looks very much like it should not be bypassed.

I've come around to @Samwilson's "3rd solution".

In T176594#3811833, @Samwilson wrote:

Oh, well the 3rd solution is that in fact the current set up is correct and that anon users should not be able to regenerate the graph. This means an upload-permitted user needs to go in and purge the page.

If we want a system user to do the generating of the graph, my patch https://gerrit.wikimedia.org/r/#/c/398784/ does that. The upload is done by a new 'GraphViz' user, whose name is defined by the 'graphviz-upload-user' system message — but probably this should be changed to make it a config variable instead?

My apologies. The fact that you went to the effort to code a fix after describing your "3rd solution" should have made it clear to me that the status quo was not acceptable.

Still, I'd like to take another pass at a more narrowly focused solution. I think temporarily granting upload permission to the current user, if she doesn't already have it, may work well. I would also add a new configuration control to the extension so that this permission grant may be enabled or disabled.

No worries! That sounds good.

I think adding the rights to the user is something like:

$user->mRights = array_merge(
	$user->getRights(),
	[ 'upload', 'reupload', 'edit', 'createpage' ]
);

but the other thing to note is that $wgEmailConfirmToEdit might be set, and so the user would also have to have an email address set. I'm not sure how this would work for the anon user.

If $wgEmailConfirmToEdit (and $wgEmailAuthentication​) is true, is an anonymous user allowed to edit wikitext?

As I read back over the comments here it sounds like the goal is to allow any user to regenerate a graph image that has a "pending changes" due to authorized edits of others (perhaps not directly to the graph wikitext but to properties that it depends on). Is that right?

I want to be careful not to inadvertently allow a user to edit a graph that is not authorized to do so.

Oops, of course, $wgEmailConfirmToEdit means there is no anonymous editing. But worth keeping in mind if adding upload permissions to an existing user.

But yes, you understand correctly: this is mostly about the situation in which the user viewing a page with a graph on it has no rights to upload files, and the graph is being generated by some extension that results in the graph source changing. This means that the graph needs to be regenerated, but can't be. The source text of the page is probably not changed at all (because that's not where the <graphviz> element is).

Drat, foiled again. My strategy was to create a graphUpdater group with the necessary permissions and add it on the fly to a user needing to upload a graph. However, I discovered you can't add a group to an anonymous user (my first test).

So, how about a hybrid approach? That is, temporarily add the necessary permissions to registered users but use the custom 'GraphViz' user for the anonymous case.

This could give rise to some strange scenarios. For example, a registered user is blocked from uploading (including graphs) but she can subvert that (for graphs) by simply not logging in.

Thoughts?

I slept on it and I think going with @Samwilson's fix is best (not the hybrid solution I described in my last post). The hybrid would just add code complexity and less predictable behavior in the extension.

Change 398784 merged by jenkins-bot:
[mediawiki/extensions/GraphViz@master] Regenerate graphs even when user is not permitted to upload

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

This seems to be working correctly, e.g. see the history of this file: https://tools.wmflabs.org/genealogy/wiki/File:Neferure_digraph_GenealogyTree_2ac38e1520047b7a1c82bbaab5cad870_dot.png