@labster, @Vedmaka - any thoughts on this? Is there some version of MediaWiki where this code actually works?

Thank you, @SomeRandomDeveloper, for the patch, and all the analysis! This is an improvement to the code, even outside of fixing dark mode.

@Paladox - thank you for the fix! (And @Nikerabbit - thank you for the bug report.) I assume this can be closed now.

That's interesting that you didn't see any error messages on Special:Packages. I see (well, saw) two problems in the code:

• On line 28, MediaWikiServices::getNamespaceInfo() should be MediaWikiServices::getInstance()->getNamespaceInfo() (this is what broke the page for me)
• On line 41, $namespace === (string)(int)$namespace is called without $namespace having been set.

This might be the same sort of bug as T377307 - some newlines in the form definition might be helpful. Or maybe you just need an {{{end template}}} tag?

What versions of Page Forms and Semantic MediaWiki are you running? Most importantly, does your SMW code already include that fix?

@Huajing - I tried this out on the Vector skin (with MW 1.43) and it displays fine for me, more or less. Could this problem be due to the BlueSpice skin you are using? Have you tried it with any other skin?

Great! Of course, the underlying bug remains.

Okay - that's surprising, but good to hear! If it does come back up, it would seem to make sense to file it as an SMW bug report.

Did you mean that the problem happens when class="formtable" is set, or when it's not set? I can't reproduce the problem either way, by the way - or when width="100%" is set. Still it would be good to try to isolate the problem on your wiki.

My guess is that you've uncovered a bug in Cargo - and that the validation code is getting confused due to the fact that both the Roller_Coasters and Roller_Coaster_Past_Locations tables contain a field named "Coord". Obviously that should not be a problem, but apparently it is. Maybe it's rare for two different tables that both contain a coordinates field to get joined on (and even rarer for both fields to be named the same thing), which would explain why this hasn't come up before.

Where is height: 100%; coming from?

It's great that we were able to diagnose the issue. That's a fair point, though... let me close this bug report, and create a new one for that.

I can't reproduce this problem. What skin are you using? Or does it happen with every skin?

@Jongfeli - I can't replicate this problem, at least with MW 1.43.0-alpha. Do you have the following two lines in LocalSettings.php?

Okay - it's good to know that having a single newline works. I'll rename this bug report, now that it's clearer what is going on.

Okay, thanks. The fact that VisualEditor is still not working for you when the warnings go away would seem to indicate that this is two, or three, separate bugs. I'll just focus on the "string offset" warning, since that is the title of this task - if you want help with the VisualEditor or WikiEditor issues, please create separate tasks for them.

@Jongfeli - sorry for the long delay. I can't tell if this is one bug, or more than one. Does this warning occur for every from, or just one of them? It sounds like the problem is an unclosed "{{{", but I'm not sure.

@BertrandGorge - thanks for the fix! I checked in this code - I assume this issue can be closed now.

Alright. It looks like two easy solutions exist: removing $substring from the getMaxValuesToRetrieve() call, or removing the setLimit() call altogether. Have you tried either one on your wiki? Do you know for sure if either one works? Is one better than the other?

What about just removing that $query->setLimit() line altogether? Does it work in that case? Or does a limit need to be set?

I assume this is fixed now.

Okay, I was able to reproduce the problem by adding that $wgHiddenPrefs line - and then fixed it by adding the additional $ignoreHidden argument. So I just checked this fix in. Thanks for finding the solution! Hopefully this works for you.

Would that change work? Looking at the code, it looks like getIntOption() just calls intval() on the output of getOption(). Which would set $cw to 0 instead of null - which would get rid of this error message, but is not ideal either.

Great!

Okay, I was wondering why the lack of smartParse() would cause HTML to get over-escaped, but your explanation made more sense. Thanks for figuring out the version issue. I just checked in what I think is a fix - please let me know if it works for you, in place of the smartParse() changes.

@Taylan - sorry for the delay. What you are advocating for is basically a return to the default behavior - which was changed in 2021 with this commit:

@Paladox - thanks for pointing this out. It turned out to be unnecessary code anyway. I think this is fixed now.

Okay, hopefully it works across all versions now!

Sorry about the problem; I think is now fixed. Feel free to re-open if not.

Great!

By the way, there is now a newer version of Page Forms, 5.8.1. If you can try this with the newer code, that would be helpful.

Actually - having looked more at the code, I may have found a fix that works for all MW versions. Could you try putting in the following change?

Okay. That's unfortunate - I just tried, and it it looks like, with MW 1.43, the exact opposite problem happens: getParser() works fine, but using getParserFactory() leads to that "Call to a member function addModules() on null" error. Evidently there was some rearrangement of the setting of ParserOutput between MW 1.42 and 1.43. I don't know what the best solution for this is.

Hi - what version of Cargo? For what it's worth, the current/old code works fine me, on MW 1.43.

@BlankEclair - thank you for the analysis, and the fix!

@BlankEclair - thanks for reporting/analyzing this issue, and sorry about the long delay. I believe this is finally fixed now.

Done! Thank you for the patch. Feel free to cherry-pick it to whatever branches you want, and I will approve it.

Okay, that's too bad. I still think there's a chance that switching to the latest code will fix the problem - in large part because I can't reproduce this issue. Perhaps this will have to wait until the next version of Page Forms comes out in order to test it, though.

@Osnard - thank you for the fix!

I'm closing this, on the assumption that this was indeed fixed. If it's still an issue, of course feel free to re-open this task.

@Rye_Greenwood - thanks for pointing out the problem. I think this is fixed now.

@Virenerus - sorry for the delay, and the bug. I think there's a good chance that this recent commit fixes the problem:

Okay, good to know. Right, you could check for "instanceof TextContent" instead - presumably it's the same thing.

@Samwilson - thanks for pointing this out, or rather re-pointing this out. I don't know if you remember, but a few months ago you created a patch to remove the ContentHandler::getContentText() calls:

That's strange, then - those were my two guesses about what was going wrong. So, I have no idea. I'm glad you found a fix for it, but that particular code change should not be necessary. (And it's not necessary on my wiki.) Barring any further information, I don't know how to proceed on this one. Maybe someone else has some idea...

Okay, thanks for trying the update. I have two questions: does the category that you set the page to actually exist, i.e. has a blue link to it? And if you go to Special:CargoTables, does a replacement table row appear for _pageData?

I think this can be closed.

Thanks for the fix!

I'm happy to say that this is now completed! Many thanks to @Jayanthvikashs for all the work on accomplishing this. Thanks also to @Solaris22 for one element of it (T355950), and to @Rockingpenny4 for the valuable idea of being able to edit previous comments.

This was fixed with the changes in https://gerrit.wikimedia.org/r/1048022 .

Done!

@FrozenPlum - note that this bug is in the calendar display within Page Forms, which is a fairly obscure feature; my guess is that what you're using is instead the calendar query format in either Cargo or Semantic MediaWiki.

That's a somewhat old version of Cargo, and I think the handling of page data has changed since then. Could you try upgrading to the latest version?

That's good to hear!

Great!

It never ends! Thank you for your continued patience. I think I've fixed everything now with d60319b19027 and 59af70a2ec87, but I'm looking forward to finding out...

Thanks for pointing out these problems. Clearly I was over-optimistic before! I believe I've now taken care of the remiaining issues in dfe25479c450 and 8ca19783349d - though of course I could be wrong.

Sorry - by "JS-related issues", I meant issues where the fix needs to be in the JavaScript code, rather than in PHP.

Here's another change, which I think fixes issues 14 and 15, and also some new long-overdue i18n messages: 62b150dca2bb

@BlankEclair - thank you for this very detailed analysis, and the video, and the patch! This is extremely helpful. (And I had no idea that there were this many unescaped messages and other strings!) Unfortunately, I didn't see the patch until now - by which time I had already written up fixes on my own for most of these issues. Although in many cases our fixes are (as maybe could be expected) very similar to one another's, and in some cases your fixes are a little more elegant. Anyway, I'm going through the issues piecemeal, and here is the first of my changes:

@BlankEclair - thank you for the patch, the explanation, and that very illustrative example! I just checked in your fix, here:

@BlankEclair - thank you for this patch. Any security leak is bad, so I plan to check this fix in, but I just want to make sure I understand this problem, because it seems surprisingly minor. A malicious user can convince an administrator that the admin deleted or switched a Cargo table, where in actuality nothing was done?

I'm amazed that this never came up before! I guess people don't enter negative numbers that often. Anyway, @KGello - thanks for pointing out the problem.

@Samwilson - thank you for pointing out this problem. I believe it's fixed now...

Oh! That's quite surprising, given that (as you pointed out) the Postgres drilldown query checks for both blank and null.

Why do you need to change the PostgreSQL query at all? I thought it was only the MySQL/MariaDB query that was the issue.

Actually, thinking more about it now - unlike with #cargo_store, with the Lua equivalent, you really can tell whether a value was intended to be blank or null. So perhaps the value should not be changed - and the Special:Drilldown SQL query should just be changed to match the Postgres version, as you suggested.

Okay, this is interesting. I had forgotten this, but #cargo_store, when it takes in a blank value, changes it to null before storing it to the DB. It appears that, now that storage can be called from Lua, this code should be duplicated, or moved, somewhere else - around here in storeAllData() might make the most sense, since there is a loop that already cycles through all the field values.

Somehow, for you, this line:

@Rishi2108 - sorry for the delay. That's great to hear! Are there specific extensions you know about and are interested in? Or are there specific technologies (PHP, JavaScript, Vue, SQL) that you want more experience with?

Fixed in 5e94b1625258. Thanks for pointing this out! I had no idea you could put function names inside backticks.

@Jayanthvikashs - thank you for fixing this!

That does seem to work, yes! I'm indeed quite surprised. If you create a patch to fix the JS bug you talked about, I would be happy to check it in.

Yes, sounds good.

What do you mean by the "backend code" - the PHP code that displays the forms? If so, I would be surprised if the current code can handle parsing pages that contain two or more levels of nesting - though I could be wrong. Do you know definitely that this works?