Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

email.utils.formatdate uses unreliable time.timezone constant #67121

Closed
mitya57 mannequin opened this issue Nov 24, 2014 · 23 comments
Closed

email.utils.formatdate uses unreliable time.timezone constant #67121

mitya57 mannequin opened this issue Nov 24, 2014 · 23 comments
Labels
stdlib Python modules in the Lib dir topic-email type-bug An unexpected behavior, bug, or error

Comments

@mitya57
Copy link
Mannequin

mitya57 mannequin commented Nov 24, 2014

BPO 22932
Nosy @warsaw, @abalkin, @rbtcollins, @bitdancer, @4kir4, @berkerpeksag, @mitya57
Files
  • issue22932.patch
  • issue22932_v2.patch
  • issue22932_v3.patch
  • issue22932_test.patch
  • issue22932_combined.diff
  • issue22932_combined_v2.diff
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2015-07-31.20:23:56.997>
    created_at = <Date 2014-11-24.16:17:20.017>
    labels = ['type-bug', 'library', 'expert-email']
    title = 'email.utils.formatdate uses unreliable time.timezone constant'
    updated_at = <Date 2015-07-31.20:23:56.995>
    user = 'https://github.com/mitya57'

    bugs.python.org fields:

    activity = <Date 2015-07-31.20:23:56.995>
    actor = 'rbcollins'
    assignee = 'none'
    closed = True
    closed_date = <Date 2015-07-31.20:23:56.997>
    closer = 'rbcollins'
    components = ['Library (Lib)', 'email']
    creation = <Date 2014-11-24.16:17:20.017>
    creator = 'mitya57'
    dependencies = []
    files = ['37266', '37394', '37402', '37675', '37683', '37721']
    hgrepos = []
    issue_num = 22932
    keywords = ['patch']
    message_count = 23.0
    messages = ['231608', '231609', '231610', '231611', '231613', '231614', '231615', '232347', '232378', '232386', '232388', '232392', '233091', '233841', '233856', '233901', '233905', '233934', '233996', '234001', '234113', '247768', '247769']
    nosy_count = 8.0
    nosy_names = ['barry', 'belopolsky', 'rbcollins', 'r.david.murray', 'akira', 'python-dev', 'berker.peksag', 'mitya57']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue22932'
    versions = ['Python 3.4', 'Python 3.5', 'Python 3.6']

    @mitya57
    Copy link
    Mannequin Author

    mitya57 mannequin commented Nov 24, 2014

    The value of time.timezone may be wrong sometimes (see http://bugs.python.org/issue22752), so I think the email library should not use it:

    $ python3 -c "from email.utils import formatdate; print(formatdate(localtime=True))"
    Mon, 24 Nov 2014 19:16:32 +0400
    $ date -R
    Mon, 24 Nov 2014 19:16:32 +0300

    Using something from datetime module may be a better solution.

    @mitya57 mitya57 mannequin added the stdlib Python modules in the Lib dir label Nov 24, 2014
    @abalkin
    Copy link
    Member

    abalkin commented Nov 24, 2014

    See also bpo-665194.

    @abalkin
    Copy link
    Member

    abalkin commented Nov 24, 2014

    I was able to reproduce the problem on a Mac as follows:

    $ TZ=Europe/Moscow date "+%c %z"
    Mon Nov 24 19:27:51 2014 +0300
    $ TZ=Europe/Moscow python3 -c "from email.utils import formatdate; print(formatdate(localtime=True))"
    Mon, 24 Nov 2014 19:28:03 +0400

    @abalkin
    Copy link
    Member

    abalkin commented Nov 24, 2014

    "Using something from datetime module" works as expected:

    $ TZ=Europe/Moscow python3 -c "from datetime import datetime, timezone; print(datetime.now(timezone.utc).astimezone())"
    2014-11-24 19:30:27.141542+03:00

    @abalkin abalkin added the type-bug An unexpected behavior, bug, or error label Nov 24, 2014
    @mitya57
    Copy link
    Mannequin Author

    mitya57 mannequin commented Nov 24, 2014

    This patch fixes the issue for me.

    @bitdancer
    Copy link
    Member

    email.utils.format_datetime uses datetime. formatdate wasn't touched, for backward compatibility reasons. If it has an actual bug we can fix it.

    If it can be converted to use datetime without sacrificing backward compatibility, that's also fine with me.

    Frankly I don't know enough at the moment to know if the proposed patch is correct. Alexander?

    @abalkin
    Copy link
    Member

    abalkin commented Nov 24, 2014

    The proposed patch will not work on platforms that don't support tm_gmtoff. A proper fix may look like this:

    dt = datetime.fromtimestamp(timeval, timezone.utc)
    if localtime:
      dt = dt.astimezone()
    return format_datetime(dt, usegmt)

    @mitya57
    Copy link
    Mannequin Author

    mitya57 mannequin commented Dec 9, 2014

    Here is the updated patch based on Alexander’s suggestion.

    Works fine, the only difference with previous behavior is that the zero time zone is now +0000 instead of -0000.

    @bitdancer
    Copy link
    Member

    That's a bug (you will note that it causes a test failure in the email test suite). -0000 means "this date is in UTC, but that may or may not be the local timezone of the message". +0000 means "this date really is in the UTC timezone". utils.format_datetime uses +0000 if and only if the datetime is an aware UTC datetime.

    @mitya57
    Copy link
    Mannequin Author

    mitya57 mannequin commented Dec 9, 2014

    Thanks, I did not know that. Here is version 3.

    @bitdancer
    Copy link
    Member

    Yeah, only someone who has studied the RFCs would know that detail. I have no idea whether or not any email clients/processors actually *use* that distinction for anything, and I doubt there are very many humans who do :).

    @abalkin
    Copy link
    Member

    abalkin commented Dec 9, 2014

    v3 looks good to me. BTW, I knew that the sign of TZ 0000 was significant, but would have to look up the RFC to recall what the significance was. Thanks, David, for the explanation.

    @bitdancer
    Copy link
    Member

    Is there any way to write a test for this?

    @mitya57
    Copy link
    Mannequin Author

    mitya57 mannequin commented Jan 11, 2015

    This patch adds a test case for formatdate() function.

    Before applying issue22932_v3.patch it fails, after applying that patch it succeeds.

    Sorry for the delay caused by holidays.

    @4kir4
    Copy link
    Mannequin

    4kir4 mannequin commented Jan 11, 2015

    @mitya57: Please, combine the code changes, tests, docs into a single rietveld-compatible patch (hg diff); read devguide and http://bugs.python.org/issue13963

    Make sure "review" link appears on the right near the patch. Example:
    http://bugs.python.org/issue22798

    @bitdancer
    Copy link
    Member

    The tests fail for me the same way both before and after the code patch:

    ======================================================================
    FAIL: test_formatdate (test.test_email.test_utils.FormatDateTests)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/home/rdmurray/python/p34/Lib/test/support/__init__.py", line 1525, in inner
        return func(*args, **kwds)
      File "/home/rdmurray/python/p34/Lib/test/test_email/test_utils.py", line 145, in test_formatdate
        self.assertEqual(string, 'Mon, 01 Dec 2014 15:00:00 -0000')
    AssertionError: 'Mon, 01 Dec 2014 14:00:00 -0000' != 'Mon, 01 Dec 2014 15:00:00 -0000'
    - Mon, 01 Dec 2014 14:00:00 -0000
    ?                   ^
    + Mon, 01 Dec 2014 15:00:00 -0000
    ?                   ^

    ======================================================================
    FAIL: test_formatdate_with_localtime (test.test_email.test_utils.FormatDateTests)
    ----------------------------------------------------------------------

    Traceback (most recent call last):
      File "/home/rdmurray/python/p34/Lib/test/support/__init__.py", line 1525, in inner
        return func(*args, **kwds)
      File "/home/rdmurray/python/p34/Lib/test/test_email/test_utils.py", line 157, in test_formatdate_with_localtime
        self.assertEqual(string, 'Mon, 01 Dec 2014 18:00:00 +0300')
    AssertionError: 'Mon, 01 Dec 2014 18:00:00 +0400' != 'Mon, 01 Dec 2014 18:00:00 +0300'
    - Mon, 01 Dec 2014 18:00:00 +0400
    ?                             ^
    + Mon, 01 Dec 2014 18:00:00 +0300
    ?                             ^

    @mitya57
    Copy link
    Mannequin Author

    mitya57 mannequin commented Jan 13, 2015

    Can it be that you have outdated tzdata?

    @bitdancer
    Copy link
    Member

    Yes, that's possible. I will check.

    @bitdancer
    Copy link
    Member

    Upgrading the timezone data results in passed tests. Without the fix, only one of the tests fails. Is this intentional?

    This, however, brings up the issue that the tests may fail on the buildbots, which may also not have up to date timezone data :(

    @abalkin
    Copy link
    Member

    abalkin commented Jan 14, 2015

    Maybe Dmitry can come up with the "skipif" logic that will test for up-to date tzinfo and skip the test if it is not. Otherwise we can try to come up with a test case which is sufficiently far in the past.

    @mitya57
    Copy link
    Mannequin Author

    mitya57 mannequin commented Jan 16, 2015

    Now the patch works with tzdata >= 2011k, which is quite old.

    @david: yes, with the unpatched version only one of the tests should fail.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 31, 2015

    New changeset fa8b76dfd138 by Robert Collins in branch '3.4':
    Issue bpo-22932: Fix timezones in email.utils.formatdate.
    https://hg.python.org/cpython/rev/fa8b76dfd138

    New changeset 94b43b36e464 by Robert Collins in branch '3.5':
    Issue bpo-22932: Fix timezones in email.utils.formatdate.
    https://hg.python.org/cpython/rev/94b43b36e464

    New changeset 479787100b91 by Robert Collins in branch 'default':
    Issue bpo-22932: Fix timezones in email.utils.formatdate.
    https://hg.python.org/cpython/rev/479787100b91

    @rbtcollins
    Copy link
    Member

    Applied to 3.4 and up. Thanks for the patch!

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    stdlib Python modules in the Lib dir topic-email type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    3 participants