The extension currently lacks input validation for the JSON, and therefore throws type errors if the data structure is not provided correctly: https://issue-tracker.miraheze.org/T13560 @SamanthaNguyen Would you be willing to review and merge a patch that improves input validation in the extension? SomeRandomDeveloper (talk) 16:23, 16 May 2025 (UTC)

Hi @SomeRandomDeveloper, sorry that happened! Admittedly it's been a minute since I've maintained this extension.

Sure, if you have a patch I can look at (I don't see any on Gerrit), then send a link and I will try to review in the next few days. If not, I can write the patch as well to fix this. Thank you! SamanthaNguyen (talk) 16:33, 16 May 2025 (UTC)

Hi, thank you for the quick response! I haven't submitted a patch yet since some extensions don't have an active maintainer anymore, leading to fixes like this staying unmerged for a long time, so I wanted to make sure that there is somebody who is familiar with the code base and able to merge it.

I could write a simple patch that just skips entries with invalid values so exceptions don't occur anymore. If you have a more advanced solution in mind (e.g. with error reporting), it would probably be better if you could write a patch if you have time, since I am not too familiar with the codebase. SomeRandomDeveloper (talk) 18:19, 16 May 2025 (UTC)

No worries, totally get it. I'll work on a patch and keep you in the loop :-) SamanthaNguyen (talk) 18:34, 16 May 2025 (UTC)

Thanks! SomeRandomDeveloper (talk) 18:41, 16 May 2025 (UTC)

Thanks for your patience! I just had some time to look into it; my first thought was that the validation code I wrote back then *should* be working since I looked at the code and I had implemented JSON validation back then, but I still do see in the code where error handling can be more graceful, so I'm improving that.

My 2nd conclusion was that since at the time I wrote this extension, 1.34 was the latest stable version, so I had written it compatible for that. Since it's been so long as the latest stable is 1.43, it seems it also needs to be updated to get it fully working. I'm surprised that it has been as working as long as it has, to be honest. 😅 Will keep working on it and update you again in a bit! SamanthaNguyen (talk) 05:35, 19 May 2025 (UTC)

I did not even see the schema validation code before. I wonder why it didn't catch this error before the data is run through the PHP code? "type": "array" for the content item seems specific enough to me to not allow objects...

Also, I interestingly did not notice any compatibility issues (at least for basic navigation boxes like the one in the task I linked). The only thing I had to fix when testing locally were the class aliases for namespaced classes (e.g. Title and Html) that were removed in 1.44, since my testing environment is running 1.45. SomeRandomDeveloper (talk) 12:31, 19 May 2025 (UTC)

Thanks, the observations re:compatibility ended up pretty much matching what you said; I recently wrote and merged a patch to bring it up to par for 1.43. I also found the root issue. With MediaWiki's content handler system, Content (in 1.34) had a method that could be overridden via prepareSave(). This was hard-deprecated in 1.38, replaced by ContentHandler->validateSave(), and eventually Content->prepareSave() was removed in 1.42. Patch will be available soon-ish, working on adding back contextual error messages and a few other things. SamanthaNguyen (talk) 18:00, 19 May 2025 (UTC)

Thank you! Note that using namespaced classes like MediaWiki\Title\Title may require the MW requirement of the extension to be raised to 1.40, since these namespaces were introduced in that version. SomeRandomDeveloper (talk) 18:22, 19 May 2025 (UTC)

Hi @SomeRandomDeveloper, I implemented Gerrit change 1147916. This restores the original behavior of the extension, so that:

• When you go a navigation in the Navigation namespace, it'll now render again as a navigation box (not a JSON table). It'll also render as a navigation box when you click "Show preview" while editing (when the JSON is valid syntax and conformant).
• Contextual error messages now show up when there's a validation error, instead of showing a stack trace.

This is what the render looks like:

For context, these error messages are internally generated by justinrainbow/json-schema so it's more developer-oriented; I still agree however that these can be much more user-friendly, but I wanted to get it to a state where at the minimum the extension is usable/not broken as IMO that's the most important, and will improve the extension in follow-up patches.

Would you be willing to test the patch locally? (will also need to run composer update since I synced the json-schema package version with MediaWiki's). FWIW I'm running a local MediaWiki 1.43 instance, but the extension should still be fine in later versions. If you think it's fine, I'll merge the patch so that Miraheze can pull the latest update.

Thanks again for reporting the bug! For future requests/bug reports etc, feel free to continue to report on this page. (While technically WMF's Phabricator would be ideal, I'm not very active on there at the moment.)

Edit: Wanted to let you know I ended up going ahead by merging the patch; I didn't want to add further delay. Again, happy for any feedback if you'd like to test locally. SamanthaNguyen (talk) 22:40, 20 May 2025 (UTC)

Awesome! Sorry for the delay on my side. I tested it locally (on MW 1.43 and master) and I did not experience the bug anymore. Of course, the error messages could be improved, but they're way more explanatory than a TypeError ;)

One last thing, would it be possible to backport the fix to REL1_43 and REL1_44? Miraheze (and probably the majority of third party users) uses the release branches of most extensions, so without a backport this change wouldn't be available before 1.45 is branched. This should probably be possible without merge conflicts, given that there aren't any other relevant commits that are on master but not on the release branches. SomeRandomDeveloper (talk) 17:21, 23 May 2025 (UTC)

Done, the bug fix has been back ported to both branches.

FWIW regarding branches: When I pull extensions, I personally use release branches for WMF-maintained extensions and master branch for most other extensions. It's also been a while since I've done MediaWiki development actively, so maybe it's different for others (but tbh, I've assumed the opposite for a while, instead thinking most people would do the same thing I described). I think at a glance, Category:Extensions with release branches compatibility policy is mostly maintained by WMF, so maybe the reason it's that way could be that the majority of extensions people use are by WMF? I'm not sure.

Either way, no worries. I hadn't listed a compatibility policy on Extension:StructuredNavigation so that's on me. With developing MediaWiki extensions specifically, I follow developing on master + only guaranteeing on latest stable version since that's the most time-effective for me. I might reconsider changing the compatibility policy in the future so it's not as strict, just at the moment I'll be sticking with master.

I do think it's cool that Miraheze uses my extension though, never knew that until a few days ago :-) I'd be fine focusing the extension's compatibility solely on 1.44 whenever Miraheze moves to that. SamanthaNguyen (talk) 23:54, 23 May 2025 (UTC)

Yeah, the release branches are often a problem because they always exist for every extension repository hosted by WMF. If they didn't exist in the first place for extensions that don't use them, backports like this wouldn't be required.

Anyways, thanks again for fixing the bug. The extension hasn't been updated yet on Miraheze, but hopefully it will be soon so the problem won't occur anymore. SomeRandomDeveloper (talk) 00:54, 24 May 2025 (UTC)