Page MenuHomePhabricator

Non-list values containing a comma lead to error when $wgPageFormsUseDisplayTitle = true
Closed, ResolvedPublic

Description

Hi Yaron,

I just updated PageForms from version 4.2 to 4.3 on my 1.27.4 MediaWiki (PHP 7.1.9 fpm-fcgi).

In my articles I use the #arraymap function and I separate the values with a comma. Since the update when I try to form-edit pages, that have more than one value in that field, I get the following error:

HTML attribute value can not contain a list of values

Those are the two #arraymap calls I use:

{{#arraymap:{{{Weiterführend|}}}|,|@|*[[@]]|\n}}
{{#arraymap:{{{Kategorie|}}}|,|@|[[Kategorie:@]]| }}

I'm using SMW but not on those two fields. So this shouldn't be related to SMW.
The issue might be similar to https://phabricator.wikimedia.org/T128733.

Regards,
Stefan

Event Timeline

I also observe this behaviour in recent versions of PageForms. I didn’t find arraymap in our case, but I isolated our issue in the following field of the form, with a value of a file containing a comma (although the field should have a single value):

{{{field|Document|uploadable|property=Document|values from namespace=File}}}

I isolated the issue in 94a0d85329188085b31e40546f5e89de36c80b39, even if it sounds strange: it does not work with the commit and it works with the previous commit. I guess the issue from @Stefahn is not in arraymap but in the form which have a "values from namespace=Category".

More specifically, it seems to be related to the condition getUseDisplayTitle() in includes/PF_FormPrinter.php, in the sense it works when I disable only this condition.

Experiencing it with the following field:

{{{field|Cast of Characters|input type=text with autocomplete|mandatory|property=Log Characters|placeholder=A comma seperated list of the scene's Cast.|values from property=Log Characters|values from category=Characters|list|delimiter=,|size=118px}}}

when it is provided with > 1 value separated by commas (ie, it works with "4" but not with "4, 5") through external HTML form data. It has been reported as starting around the time this issue was opened.

Two temporary patches are:

  1. in the file includes/PF_FormPrinter.php of PageForms, add && false after $form_field->getUseDisplayTitle(), or
  2. set $wgPageFormsUseDisplayTitle to false.

@Yaron_Koren: is there some way to detect a field is specified as a single value, and hence no split should be done on this field?

Thanks @Seb35. Tested both fixes: Both work for me.
And yes, I also use values from namespace=Category in my form.

Hi everyone - sorry, I'm only looking into this now. Yes, looking at that code, it does seem like the code assumes that *every* field is a list field, and thus fails when non-list fields contain commas. The recent change to make $wgPageFormsUseDisplayTitle default to true just exposed that bug to many more users.

Am I correct in assuming that #arraymap is unrelated to this issue?

Also, @elifoster - just so you know, if you have both "values from property=" and "values from category=" in the field, only one will take effect. (I don't know which.)

Yaron_Koren renamed this task from Since version 4.3: "HTML attribute value can not contain a list of values" when using arraymap to Non-list values containing a comma lead to error when $wgPageFormsUseDisplayTitle = true.Apr 10 2018, 2:20 PM

Change 425296 merged by jenkins-bot:
[mediawiki/extensions/PageForms@master] Removed unnecessary delimiter setting - fix for T189928

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

I think this is fixed now in the latest code; can someone verify? There were a variety of changes made, so the fix might have messed up something else, though I hope not.

I tried ce534aa and 31cb78b, it does not fix the issue.

@Seb35 - sorry for the delay. What is the exact problem you're seeing?

Change 434273 merged by jenkins-bot:
[mediawiki/extensions/PageForms@master] Another attempted fix for T189928 - follow-up to ce534aac0362

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

@Seb35 - it would be great if you (or anyone else) could test with the latest code; this problem may have been fixed.

Yes, it works for me with the current master b7b2c72 containing the latter change. Thanks!