Page MenuHomePhabricator

Autoedit replaces existing multi-instance subobject with duplicate entries
Closed, ResolvedPublic


Our older stack with working autoedit:
MediaWiki 1.20.2
PHP 5.4.9 (apache2handler)
MySQL 5.5.28-log
Semantic Forms 2.7

Our new stack with broken autoedit:
MediaWiki 1.30.0
PHP 7.1.13 (fpm-fcgi)
MariaDB 10.2.12-MariaDB-10.2.12+maria~jessie-log
nginx 1.13.8
Page Forms 4.2.1 (624f2a9)

We are in the process of upgrading our Semantic MediaWiki stack. We took a database dump from the old system and imported it into the new system. All of our autoedit buttons appear to be broken in the new system. In our old build, we had set up autoedit buttons to write multiple-instance subobjects to a page using for example the Multi_Instance_Template like this:






In the new stack, adding a second Multi_Instance_Template subobject using an autoedit button first completely removes the existing subobject, and replaces it with duplicate copies of the newly entered subobject to make the page look like this:








I appears to be a bug. I have tried several versions of Page Forms (including master, 4.2, 4.2.1, 4.1) which show the same phenomenon. Has this issue already been reported?



Event Timeline

@Longphile - I don't think this has been reported before. I wouldn't be surprised if this got broken in version 3.4.2 (about two years ago), when a lot of the relevant code got refactored.

I downgraded to Semantic Forms 3.4.2-alpha (624f2a9) and autoedit does not create duplicate new entries in the newer stack with MW1.30.0. However, autoedit does replace existing multi-instance subobjects.

Hi, just curious if there is any potential solution for this? Otherwise, it seems like autoedit is not functional for multi-instance subobjects since version 3.4.2. I'm trying to test version 3.4.1 on MW 1.29.2 and MW 1.27.4. However, that version seems incompatible and throws an error in both. So unfortunately, I can't test autoedit function in 3.4.1 in the more recent MW builds.

@Longphile - I don't know; I haven't looked into it. Hopefully I or someone else can fix this before too long.

I did some testing on this and it appears that the regular formedit works fine. Why would autoedit use different page save logic from formedit instead of reusing the existing functions? also, if you can tell me which function in which file handles page edit/save on autoedit that would help a lot.

@EllipticMW - I think they both use the same code to do the actual saves, which is contained at /includes/PF_AutoeditAPI.php (which means that the file is somewhat misleadingly named). The issue is that the code to set the new wikitext for the page is totally different. With regular forms, it's just based on what's contained in the form - the code doesn't care what's currently on the page, and just overwrites it completely. With #autoedit, the code takes what's on the page and modifies it. (Assuming it's not a new page.) Anyway, it's different logic.

Which function handle setting of the wikitext?

I may have found part of the problem:


private function parseDataFromQueryString( &$data, $queryString )

It splits the query string on & (separator between form field = value), then splits each param on = (to get field/value pairs), but it doesn't have any special considerations for template[instance][field]. The helper function addToArray( &$array, $key, $value, $toplevel = true ) doesn't appear to have any support for the instance number either.

It's possible this gets mishandled somewhere else (whatever calls parseDataFromQueryString. Are there any other files I should inspect?

Does anyone have more information on the code path for an autoedit? Which function in particular edits the wikitext?

The call into MW to store the page is in class PFAutoeditAPI, line 448:

$status = $editor->internalAttemptSave( $resultDetails, $bot );

The target content is returned by $wgPageFormsFormPrinter->formHTML in class PFAutoeditAPI, line 933:

			list ( $formHTML, $targetContent, $generatedFormName, $generatedTargetNameFormula ) =
				$wgPageFormsFormPrinter->formHTML( $formContent, $isFormSubmitted, $pageExists, $formArticleId, $preloadContent, $targetName, $targetNameFormula );

Continued reading and studying the source code (thanks Foxtrott) and here's what I found so far:

line 901-902:

list( $formHTML, $targetContent, $generatedFormName, $generatedTargetNameFormula ) =

$wgPageFormsFormPrinter->formHTML( $formContent, $isFormSubmitted, $pageExists, $formArticleId, $preloadContent, $targetName, $targetNameFormula );

$targetContent is the new wikitext of the page after the formedit (???). I guess at this after seeing Foxtrott's reference to line 448 (the internalAttemptSave).

The variable that needs to be tracked and fixed is $page_text in includes/PF_FormPrinter.php (function formHTML)

1556-1564 in PF_FormPrinter.php:

Hooks::run( 'PageForms::BeforeFreeTextSubst',

array( &$free_text, $existing_page_content, &$page_text ) );
$wiki_page->setFreeText( $free_text );
$page_text = $wiki_page->createPageText();

It doesn't look like PageForms::BeforeFreeTextSubst is defined anywhere in the extension. Doing a full global search over PageForms only finds BeforeFreeTextSubst in that specific place. This could be part of the error. Can anyone confirm this? I'm guessing that BeforeFreeTextSubst handles the previously mentioned wikitext substitution.

The call to Hook::run should be no problem. Hooks allow extensions to register functionality. In this case an extension on top of PF could use this hook to modify $free_text before it is added to $wiki_page. If nothing is registered the hook does nothing.

Some useful info for anyone who wants a quick way of debugging this:

I found out how to make the autoedit print what it would save to the wiki page instead of editing.

Comment out line 425:
$status = $editor->internalAttemptSave( $resultDetails, $bot );

Right below that, add:
$status = Status::newGood();
$status->value = EditPage::AS_HOOK_ERROR;

This cancels the page save and fakes a hook error (where "another extension" causes the autoedit page save to fail).

Right above line 931:
$editor = $this->setupEditPage( $targetContent );

$this->logMessage($targetContent );

This causes the new wikitext that would have been written to the page to be printed in addition to the regular feedback about a hook error.

Further preliminary testing suggests that the duplication and mishandling might have something to do with how newer versions of Page Forms add unneeded subobject instances through some modification of the input query string.

I'm testing with an autoedit that adds 1 instance of a multi-instance template. The page I'm testing on has no instances of it before the autoedit.

In /includes/PF_TemplateInForm.php, line 248:
$allValuesFromSubmit = $wgRequest->getArray( $query_template_name );

I get an array with 3 sub-arrays, keys are '0a', 'num', and '1'. That's for adding the first instance of the autoedit (for example: MyTemplate[1][A]=Foo&MyTemplate[1][B]=Bar&MyTemplate[1][C]=Baz). With that example, $allValuesFromSubmit ends up with 3 subobjects: the subobject linked off 'num' is always empty (interestingly enough that one gets specifically ignored (conditional on line 257 of PF_TemplateInForm.php). The '0a' and '1' instances are identical and I would guess that's where the duplication is coming from. Will test with having the conditional ignore both 'num' and '0a' and see what happens. So far it seems like it fixes the duplication problem for the simple example although it likely breaks something else.

Did more testing and it looks like the problem is due Page Forms mishandling how multi-instance templates (or lack of them on a page) are populate into a mock form.

From reading the source code, an autoedit "reads" the form used and the page contents, loads the templates on the page into mock form fields, edits the fields given in the query string, and saves the page. All of my testing so far indicates the problem is on the "load templates on page into mock form fields". If there are no instances of the subobject, I get duplication from the 0a and 1 keys. When I added a filter on PF_TemplateInPage to selectively ignore the 0a key on autoedits only (ignoring 0a globally caused regular form edit to malfunction), I consistently got only a single instance of the multi-instance template on an autoedit that should have added another instance. This would indicate that any existing instances of the multi-instance template are being ignored when the page is loaded into a mock "form".

Any ideas?

Found more possibly serious problems. -- lines 832-854
$preloadContent is unable to read the wikitext of the target page for editing in an autoedit content. I patched in:
$preloadContent = $this->getTextForPage( Title::newFromText( $targetName ) );
right before the if ( $preloadContent !== '' ) check and it came back as an empty string anyway.

Other tests in PF_FormPrinter.php indicate that the problem isn't so much a duplicate entry of the new template instance as a wipe-out of existing multi-instance templates because PageForms fails to read them. Additionally I haven't found any reliably way to manually read in the wikitext of the page and override this behavior.

This may be fixed now, with the latest code.

Yaron_Koren claimed this task.

I'm marking this as "Resolved" - feel free to re-open it if there are any problems.