mysociety / fixmystreet

This is mySociety's popular map-based reporting platform: easy to install in new countries and regions
http://fixmystreet.org/
Other
506 stars 235 forks source link

[Bexley] Use force_arrayref on Whitespace site contracts #5063

Closed chrismytton closed 1 month ago

chrismytton commented 1 month ago

Fixes these errors that are appearing in the logs:

Caught exception in FixMyStreet::App::Controller::Waste->process_report_data "Not an ARRAY reference at /data/vhost/www.fixmystreet.com/fixmystreet-2024-07-10T10-44-33/perllib/FixMyStreet/Cobrand/Bexley/Waste.pm line 1015.", referer: /waste/10011843743/report

Basecamp todo

codecov[bot] commented 1 month ago

Codecov Report

Attention: Patch coverage is 0% with 2 lines in your changes missing coverage. Please review.

Project coverage is 82.61%. Comparing base (70826f7) to head (4d827dc).

Files Patch % Lines
perllib/Integrations/Whitespace.pm 0.00% 2 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #5063 +/- ## ========================================== - Coverage 82.62% 82.61% -0.01% ========================================== Files 395 395 Lines 30872 30873 +1 Branches 4892 4892 ========================================== - Hits 25509 25507 -2 - Misses 3917 3920 +3 Partials 1446 1446 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

chrismytton commented 1 month ago

@nephila-nacrea I have to admit I don't fully understand the issue, I think it's related to the XML library we're using, so feels like maybe it would be possible to fix it at a lower level so that single item lists are always returned as lists, but I imagine there's a lot more nuance to it than that!

dracos commented 1 month ago

There's two separate XML things here. We use XML::Simple in a few places, and that has configuration arguments as to whether it turns single entries into hashes or keeps them as arrays (either everywhere, or you can specify element names) which we do use. But SOAP::Lite has its own handling of this - https://github.com/redhotpenguin/perl-soaplite/blob/66be2ded5b45e36763b2501a3d1f25e804fcf331/lib/SOAP/Lite.pm#L2421-L2439 - where you can see it always starts with a hash (or single scalar etc), then if it gets another child turns it into an array. That seems a bit harder to override, hence our force_arrayref. In this particular case, if it's always the case for every call, it seems like it would make sense to refactor it into the central Whitespace call function - pass in the results value, and do it there, and then you'll know it's always necessary - if that makes sense.