Don't crash on blank PO-Revision-Date / POT-Creation-Date headers - #1293
Don't crash on blank PO-Revision-Date / POT-Creation-Date headers#1293agu2347 wants to merge 1 commit into
Conversation
Sorry, something went wrong.
Some tools (e.g. Poedit) can leave the PO-Revision-Date header blank in a generated .po file instead of omitting it or using the conventional 'YEAR-MO-DA HO:MI+ZONE' placeholder. Parsing such a file crashed with: ValueError: time data '' does not match format '%Y-%m-%d %H:%M' because _parse_datetime_header() was called unconditionally with the blank value. Guard both call sites (po-revision-date and, for the same reason, pot-creation-date) to skip parsing when the header value is blank, keeping the previous/default value instead -- the same treatment already given to the 'YEAR-MO-DA HO:MI+ZONE' placeholder value just above, and to a blank Language header elsewhere in this same method. Added a regression test that parses a message block with both headers blank and asserts: no exception is raised, the catalog's creation_date/revision_date fall back to their defaults rather than being corrupted, and no literal 'None' leaks into the round-tripped mime_headers output. Confirmed the test fails with the original ValueError without the fix, and passes with it. Also verified end-to-end with an actual .po file parse + write round-trip (using an installed release build with CLDR data, since a shallow clone here can't build that data), and confirmed normal (non-blank) header values are completely unaffected. Ran the full existing test_catalog.py and test_pofile.py suites: all pass, no regressions. Fixes python-babel#1219
|
Thanks for the pointer -- I read through the #1222 discussion. To make sure this PR doesn't repeat the concern raised there ("I don't think we should be silently ignoring malformed dates" / silent data loss on unparseable values): #1222 was about any unparseable date string being silently swallowed, discarding a value the user actually wrote. This PR is narrower and only triggers on a genuinely blank header value (POT-Creation-Date: / PO-Revision-Date: with nothing after the colon) -- something Poedit itself sometimes writes. In that case there's no user-authored value to lose; falling back to the existing default (or leaving the previous value, for revision-date) doesn't discard any real information the way silently accepting-and-mangling an actually malformed non-blank date would. Any other malformed-but-non-blank value still goes through _parse_datetime_header unchanged and will raise/behave exactly as before -- this PR doesn't touch that path. Happy to adjust the approach further if this distinction doesn't fully address the earlier concern. |
Sorry, something went wrong.
|
Heads up for maintainers: there are currently three open PRs addressing this exact problem (blank POT-Creation-Date/PO-Revision-Date headers from tools like Poedit, crash in _parse_datetime_header, ref. #1219):
Notes on the differences:
It would be great if @akx (or whoever picks this up) can choose a preferred approach so two of the three can be closed — happy to re-review whichever direction wins. All three reference the same motivating case, and all include tests, which is nice to see. |
Sorry, something went wrong.
Fixes #1219.
Some tools (e.g. Poedit) can leave the PO-Revision-Date header blank in a generated .po file instead of omitting it or using the conventional YEAR-MO-DA HO:MI+ZONE placeholder. Parsing such a file crashes with:
because _parse_datetime_header() is called unconditionally with the blank value, as reported in the issue.
Fix: guard both call sites (po-revision-date and, for the same reason, pot-creation-date) to skip parsing when the header value is blank, keeping the previous/default value instead -- the same treatment already given to the YEAR-MO-DA HO:MI+ZONE placeholder value right above it, and to a blank Language header elsewhere in this same method (self._set_locale(value or None)).
Testing: added a regression test that parses a message block with both headers blank and asserts: no exception is raised, creation_date/revision_date fall back to their defaults rather than being corrupted, and no literal None leaks into the round-tripped mime_headers output. I confirmed the test fails with the original ValueError without the fix, and passes with it.
I also verified end-to-end with an actual .po file parse + write round-trip (using an installed release build with CLDR data, since a shallow clone here can't build that data on its own), confirming no None leaks into the output and that normal (non-blank) header values are completely unaffected. Ran the full existing test_catalog.py and test_pofile.py suites: all pass, no regressions.