Fixed moment timezone configuration. #5916
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jorgefilipecosta
added
the
[Type] Bug
youknowriad
reviewed
Mar 31, 2018
date/index.js
Outdated
| offsets: [ offsets ], | ||
| }; | ||
| momentLib.tz.add( momentLib.tz.pack( momentTimezone ) ); | ||
| if ( settings.timezone.offset ) { |
There was a problem hiding this comment.
I can't test now but I recall the special test was here for special timezones (like UTC)
There was a problem hiding this comment.
Hi @youknowriad, you are totally right this code had a bug.
The offset can be 0 in which the condition would return false and the code would not be executed. I removed the condition as if we don't initialize the timezone we have errors. I added an or to use 0 if we are in the presence of some undefined case (should never happen because get_option( 'gmt_offset', 0 ); should always return something.
When we use moment.tz.zone( settings.timezone.string ) moment returns us an object which contains the offsets property with a large array, but also returns arrays of the same sizes for untils and abbrs. When timezone was passed back to the moment (to configure it) we only passed the offsets array and we used an array with a null value for untils and an array with 'WP' for the abbrs. This made moment logic not work correctly. If besides offsets we also passback untils and abbrs arrays things work as expected. Given that our logic worked correctly when the timezone contains just the offset (a UTC value was set as timezone instead of a city), there is no need to maintain two logics because if we select a city as timezone we have access to offset anyway which was computed on the server using get_option( 'gmt_offset', 0 ); So we are changing the moment timezone configuration to rely just on the offset.
jorgefilipecosta
force-pushed
the
fix/moment-timezone-configuration
branch
from
1d838bf to
7cb4021
Compare
youknowriad
approved these changes
Apr 2, 2018
Closed
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When we use moment.tz.zone( settings.timezone.string ) moment returns us an object which contains the offsets property with a large array, but also returns arrays of the same sizes for untils and abbrs. When timezone was passed back to the moment (to configure it) we only passed the offsets array and we used an array with a null value for untils and an array with 'WP' for the abbrs. This made moment logic not work correctly. If besides offsets we also passback untils and abbrs arrays things work as expected.
Given that our logic worked correctly when the timezone contains just the offset (a UTC value was set as timezone instead of a city), there is no need to maintain two logics because if we select a city as timezone we have access to offset anyway which was computed on the server using get_option( 'gmt_offset', 0 );
So we are changing the moment timezone configuration to rely just on the offset.
Fixes: #5874
How Has This Been Tested?
Set your wordpress timezone settings to a city e.g: "Lisbon".
Create a new post, write something and verify the Publish button says "Publish...", publish the post see the pre/pos publish panel works as expected.
Schedule a post to be posted after 3 minutes and after a long time verify we have now a "Schedule..." button and the schedule flow works as expected.