From 65c955746c5953e8747e27ce69fe1666581be82c Mon Sep 17 00:00:00 2001 From: Derk-Jan Hartman Date: Thu, 15 Apr 2021 22:54:58 +0200 Subject: [PATCH] Split TimeCorrection parser into separate class The parsing of the timecorrection useroption was split over multiple classes. Combine into a single class and add some testcases. Change-Id: I2cadac00e46dff2bc7d81ac2f294ea2ae4e72f47 --- includes/MWTimestamp.php | 62 +---- .../preferences/DefaultPreferencesFactory.php | 2 +- includes/preferences/TimezoneFilter.php | 50 +--- includes/user/UserOptionsManager.php | 13 +- includes/user/UserTimeCorrection.php | 252 ++++++++++++++++++ tests/common/TestSetup.php | 3 +- .../includes/user/UserOptionsManagerTest.php | 5 +- .../includes/user/UserTimeCorrectionTest.php | 131 +++++++++ 8 files changed, 417 insertions(+), 101 deletions(-) create mode 100644 includes/user/UserTimeCorrection.php create mode 100644 tests/phpunit/unit/includes/user/UserTimeCorrectionTest.php diff --git a/includes/MWTimestamp.php b/includes/MWTimestamp.php index 786e86a0991..d92e69fbc30 100644 --- a/includes/MWTimestamp.php +++ b/includes/MWTimestamp.php @@ -24,6 +24,7 @@ use MediaWiki\MediaWikiServices; use MediaWiki\User\UserIdentity; +use MediaWiki\User\UserTimeCorrection; use Wikimedia\Timestamp\ConvertibleTimestamp; /** @@ -82,62 +83,21 @@ class MWTimestamp extends ConvertibleTimestamp { * @return DateInterval Offset that was applied to the timestamp */ public function offsetForUser( UserIdentity $user ) { - global $wgLocalTZoffset; - $option = MediaWikiServices::getInstance() ->getUserOptionsLookup() ->getOption( $user, 'timecorrection' ); - $data = explode( '|', $option, 3 ); - - // First handle the case of an actual timezone being specified. - if ( $data[0] == 'ZoneInfo' ) { - try { - $tz = new DateTimeZone( $data[2] ); - } catch ( Exception $e ) { - $tz = false; - } - - if ( $tz ) { - $this->timestamp->setTimezone( $tz ); - return new DateInterval( 'P0Y' ); - } - - $data[0] = 'Offset'; + $value = new UserTimeCorrection( + $option, + $this->timestamp, + MediaWikiServices::getInstance()->getMainConfig()->get( 'LocalTZoffset' ) + ); + $tz = $value->getTimeZone(); + if ( $tz ) { + $this->timestamp->setTimezone( $tz ); + return new DateInterval( 'P0Y' ); } - - $diff = 0; - // If $option is in fact a pipe-separated value, check the - // first value. - if ( $data[0] == 'System' ) { - // First value is System, so use the system offset. - if ( $wgLocalTZoffset !== null ) { - $diff = $wgLocalTZoffset; - } - } elseif ( $data[0] == 'Offset' ) { - // First value is Offset, so use the specified offset - $diff = (int)$data[1]; - } else { - // $option actually isn't a pipe separated value, but instead - // a comma separated value. Isn't MediaWiki fun? - $data = explode( ':', $option ); - if ( count( $data ) >= 2 ) { - // Combination hours and minutes. - $diff = abs( (int)$data[0] ) * 60 + (int)$data[1]; - if ( (int)$data[0] < 0 ) { - $diff *= -1; - } - } else { - // Just hours. - $diff = (int)$data[0] * 60; - } - } - - $interval = new DateInterval( 'PT' . abs( $diff ) . 'M' ); - if ( $diff < 1 ) { - $interval->invert = 1; - } - + $interval = $value->getTimeOffsetInterval(); $this->timestamp->add( $interval ); return $interval; } diff --git a/includes/preferences/DefaultPreferencesFactory.php b/includes/preferences/DefaultPreferencesFactory.php index 33791d34fbe..15e00ca4655 100644 --- a/includes/preferences/DefaultPreferencesFactory.php +++ b/includes/preferences/DefaultPreferencesFactory.php @@ -1713,7 +1713,7 @@ class DefaultPreferencesFactory implements PreferencesFactory { $timestamp = MWTimestamp::getLocalInstance(); // Check that the LocalTZoffset is the same as the local time zone offset - if ( $localTZoffset == $timestamp->format( 'Z' ) / 60 ) { + if ( $localTZoffset === $timestamp->format( 'Z' ) / 60 ) { $timezoneName = $timestamp->getTimezone()->getName(); // Localize timezone if ( isset( $timeZoneList[$timezoneName] ) ) { diff --git a/includes/preferences/TimezoneFilter.php b/includes/preferences/TimezoneFilter.php index b73aa53a0d9..7864bc12a7f 100644 --- a/includes/preferences/TimezoneFilter.php +++ b/includes/preferences/TimezoneFilter.php @@ -20,8 +20,7 @@ namespace MediaWiki\Preferences; -use DateTimeZone; -use Exception; +use MediaWiki\User\UserTimeCorrection; class TimezoneFilter implements Filter { @@ -36,50 +35,9 @@ class TimezoneFilter implements Filter { * @inheritDoc */ public function filterFromForm( $tz ) { - $data = explode( '|', $tz, 3 ); - switch ( $data[0] ) { - case 'ZoneInfo': - $valid = false; - - if ( count( $data ) === 3 ) { - // Make sure this timezone exists - try { - // @phan-suppress-next-line PhanNoopNew - new DateTimeZone( $data[2] ); - // If the constructor didn't throw, we know it's valid - $valid = true; - } catch ( Exception $e ) { - // Not a valid timezone - } - } - - if ( !$valid ) { - // If the supplied timezone doesn't exist, fall back to the encoded offset - return 'Offset|' . intval( $tz[1] ); - } - return $tz; - case 'System': - return $tz; - default: - $data = explode( ':', $tz, 2 ); - if ( count( $data ) == 2 ) { - $data[0] = intval( $data[0] ); - $data[1] = intval( $data[1] ); - $minDiff = abs( $data[0] ) * 60 + $data[1]; - if ( $data[0] < 0 ) { - $minDiff = -$minDiff; - } - } else { - $minDiff = intval( $data[0] ) * 60; - } - - # Max is +14:00 and min is -12:00, see: - # https://en.wikipedia.org/wiki/Timezone - # 14:00 - $minDiff = min( $minDiff, 840 ); - # -12:00 - $minDiff = max( $minDiff, -720 ); - return 'Offset|' . $minDiff; + if ( $tz === UserTimeCorrection::SYSTEM ) { + return $tz; } + return ( new UserTimeCorrection( $tz ) )->toString(); } } diff --git a/includes/user/UserOptionsManager.php b/includes/user/UserOptionsManager.php index bdff30811e2..44934203f0c 100644 --- a/includes/user/UserOptionsManager.php +++ b/includes/user/UserOptionsManager.php @@ -47,7 +47,8 @@ class UserOptionsManager extends UserOptionsLookup { * @internal For use by ServiceWiring */ public const CONSTRUCTOR_OPTIONS = [ - 'HiddenPrefs' + 'HiddenPrefs', + 'LocalTZoffset', ]; /** @var ServiceOptions */ @@ -551,6 +552,16 @@ class UserOptionsManager extends UserOptionsLookup { // Replace deprecated language codes $options['language'] = LanguageCode::replaceDeprecatedCodes( $options['language'] ); + // Fix up timezone offset (Due to DST it can change from what was stored in the DB) + // ZoneInfo|offset|TimeZoneName + if ( isset( $options['timecorrection'] ) ) { + $options['timecorrection'] = ( new UserTimeCorrection( + $options['timecorrection'], + null, + $this->serviceOptions->get( 'LocalTZoffset' ) + ) )->toString(); + } + // Need to store what we have so far before the hook to prevent // infinite recursion if the hook attempts to reload options $this->originalOptionsCache[$userKey] = $options; diff --git a/includes/user/UserTimeCorrection.php b/includes/user/UserTimeCorrection.php new file mode 100644 index 00000000000..04a432521ae --- /dev/null +++ b/includes/user/UserTimeCorrection.php @@ -0,0 +1,252 @@ + + */ + +namespace MediaWiki\User; + +use DateInterval; +use DateTime; +use DateTimeZone; +use Exception; + +/** + * Utility class to parse the TimeCorrection string value. + * + * These values are used to specify the time offset for a user and are stored in + * the database as a user preference and returned by the preferences APIs + * + * The class will correct invalid input and adjusts timezone offsets to applicable dates, + * taking into account DST etc. + * + * @since 1.37 + */ +class UserTimeCorrection { + + /** + * @var string (default) Time correction based on the MediaWiki's system offset from UTC. + * The System offset can be configured with wgLocalTimezone and/or wgLocalTZoffset + */ + public const SYSTEM = 'System'; + + /** @var string Time correction based on a user defined offset from UTC */ + public const OFFSET = 'Offset'; + + /** @var string Time correction based on a user defined timezone */ + public const ZONEINFO = 'ZoneInfo'; + + /* @var DateTime */ + private $date; + + /* @var bool */ + private $valid; + + /* @var string */ + private $correctionType; + + /* @var int Offset in minutes */ + private $offset; + + /* @var DateTimeZone|null */ + private $timeZone; + + /** + * @param string $timeCorrection Original time correction string + * @param DateTime|null $relativeToDate The date used to calculate the time zone offset of. + * This defaults to the current date and time. + * @param int $offset An offset in minutes (default 0) + */ + public function __construct( + string $timeCorrection, + DateTime $relativeToDate = null, + int $offset = 0 + ) { + $this->date = $relativeToDate ?? new DateTime(); + $this->offset = $offset; + $this->valid = false; + $this->parse( $timeCorrection ); + } + + /** + * Get time offset for a user + * + * @return string Offset that was applied to the user + */ + public function getCorrectionType() : string { + return $this->correctionType; + } + + /** + * Get corresponding time offset for this correction + * Note: When correcting dates/times, apply only the offset OR the time zone, not both. + * @return int Offset in minutes + */ + public function getTimeOffset() : int { + return $this->offset; + } + + /** + * Get corresponding time offset for this correction + * Note: When correcting dates/times, apply only the offset OR the time zone, not both. + * @return DateInterval Offset in minutes as a DateInterval + */ + public function getTimeOffsetInterval() : DateInterval { + $offset = abs( $this->offset ); + $interval = new DateInterval( "PT{$offset}M" ); + if ( $this->offset < 1 ) { + $interval->invert = 1; + } + return $interval; + } + + /** + * The time zone if known + * Note: When correcting dates/times, apply only the offset OR the time zone, not both. + * @return DateTimeZone|null + */ + public function getTimeZone() : ?DateTimeZone { + return $this->timeZone; + } + + /** + * Was the original correction specification valid + * @return bool + */ + public function isValid() : bool { + return $this->valid; + } + + /** + * Parse the timecorrection string as stored in the database for a user + * or as entered into the Preferences form field + * + * There can be two forms of these strings: + * 1. A pipe separated tuple of a maximum of 3 fields + * - Field 1 is the type of offset definition + * - Field 2 is the offset in minutes from UTC (optional for System type) + * - Field 3 is a timezone identifier from the tz database (only required for ZoneInfo type) + * - The offset for a ZoneInfo type is unreliable because of DST. + * After retrieving it from the database, it should be recalculated based on the TZ identifier. + * Examples: + * - System + * - System|60 + * - Offset|60 + * - ZoneInfo|60|Europe/Amsterdam + * + * 2. The following form provides an offset in hours and minutes + * This currently should only be used by the preferences input field, + * but historically they were present in the database. + * TODO: write a maintenance script to migrate these old db values + * Examples: + * - 16:00 + * - 10 + * + * @param string $timeCorrection + */ + private function parse( string $timeCorrection ) { + $data = explode( '|', $timeCorrection, 3 ); + + // First handle the case of an actual timezone being specified. + if ( $data[0] === self::ZONEINFO ) { + try { + $this->correctionType = self::ZONEINFO; + $this->timeZone = new DateTimeZone( $data[2] ); + $this->offset = floor( $this->timeZone->getOffset( $this->date ) / 60 ); + $this->valid = true; + return; + } catch ( Exception $e ) { + // Not a valid/known timezone. + // Fall back to any specified offset + } + } + + // If $timeCorrection is in fact a pipe-separated value, check the + // first value. + switch ( $data[0] ) { + case self::OFFSET: + case self::ZONEINFO: + $this->correctionType = self::OFFSET; + // First value is Offset, so use the specified offset + $this->offset = (int)( $data[1] ?? 0 ); + // If this is ZoneInfo, then we didn't recognize the TimeZone + $this->valid = isset( $data[1] ) && $data[0] === self::OFFSET; + return; + case self::SYSTEM: + $this->correctionType = self::SYSTEM; + $this->valid = true; + return; + } + + // $timeCorrection actually isn't a pipe separated value, but instead + // a colon separated value. This is only used by the userinput of the preferences + // but can also still be present in the Db. (but shouldn't be) + $diff = 0; + $data = explode( ':', $timeCorrection, 2 ); + if ( count( $data ) >= 2 ) { + // Combination hours and minutes. + $diff = abs( (int)$data[0] ) * 60 + (int)$data[1]; + if ( (int)$data[0] < 0 ) { + $diff *= -1; + } + } elseif ( ctype_digit( $data[0] ) ) { + // Just hours. + $diff = (int)$data[0] * 60; + } else { + // We really don't know this. Fallback to System + $this->correctionType = self::SYSTEM; + return; + } + + // Max is +14:00 and min is -12:00, see: + // https://en.wikipedia.org/wiki/Timezone + if ( $diff >= -12 * 60 && $diff <= 14 * 60 ) { + $this->valid = true; + } + // 14:00 + $diff = min( $diff, 14 * 60 ); + // -12:00 + $diff = max( $diff, -12 * 60 ); + + $this->correctionType = self::OFFSET; + $this->offset = $diff; + } + + /** + * Note: The string value of this object might not be equal to the original value + * @return string a timecorrection string representing this value + */ + public function toString() : string { + switch ( $this->correctionType ) { + case self::ZONEINFO: + if ( $this->timeZone ) { + return "ZoneInfo|{$this->offset}|{$this->timeZone->getName()}"; + } + // If not, fallback: + case self::SYSTEM: + case self::OFFSET: + default: + return "{$this->correctionType}|{$this->offset}"; + } + } + + public function __toString() { + return $this->toString(); + } +} diff --git a/tests/common/TestSetup.php b/tests/common/TestSetup.php index f2f3cf49dd9..be72793c064 100644 --- a/tests/common/TestSetup.php +++ b/tests/common/TestSetup.php @@ -30,7 +30,7 @@ class TestSetup { global $wgMainStash, $wgChronologyProtectorStash; global $wgObjectCaches; global $wgLanguageConverterCacheType, $wgUseDatabaseMessages; - global $wgLocaltimezone, $wgLocalisationCacheConf; + global $wgLocaltimezone, $wgLocalTZOffset, $wgLocalisationCacheConf; global $wgSearchType; global $wgDevelopmentWarnings; global $wgSessionProviders, $wgSessionPbkdf2Iterations; @@ -78,6 +78,7 @@ class TestSetup { // Assume UTC for testing purposes $wgLocaltimezone = 'UTC'; + $wgLocalTZOffset = 0; $wgLocalisationCacheConf['class'] = TestLocalisationCache::class; $wgLocalisationCacheConf['storeClass'] = LCStoreNull::class; diff --git a/tests/phpunit/includes/user/UserOptionsManagerTest.php b/tests/phpunit/includes/user/UserOptionsManagerTest.php index d49b884c31c..74443c15d74 100644 --- a/tests/phpunit/includes/user/UserOptionsManagerTest.php +++ b/tests/phpunit/includes/user/UserOptionsManagerTest.php @@ -20,7 +20,10 @@ class UserOptionsManagerTest extends UserOptionsLookupTest { return new UserOptionsManager( new ServiceOptions( UserOptionsManager::CONSTRUCTOR_OPTIONS, - new HashConfig( [ 'HiddenPrefs' => [ 'hidden_user_option' ] ] ) + new HashConfig( [ + 'HiddenPrefs' => [ 'hidden_user_option' ], + 'LocalTZoffset' => 0, + ] ) ), $this->getDefaultManager( $langCode, $defaultOptionsOverrides ), $services->getLanguageConverterFactory(), diff --git a/tests/phpunit/unit/includes/user/UserTimeCorrectionTest.php b/tests/phpunit/unit/includes/user/UserTimeCorrectionTest.php new file mode 100644 index 00000000000..1d48fd1f197 --- /dev/null +++ b/tests/phpunit/unit/includes/user/UserTimeCorrectionTest.php @@ -0,0 +1,131 @@ +isValid() ); + } + + public function provideTimeCorrectionExamples() { + return [ + [ '', 'System|0', false ], + [ 'bogus', 'System|0', false ], + [ 'ZoneInfo', 'Offset|0', false ], + [ 'ZoneInfo|bogus', 'Offset|0', false ], + [ 'ZoneInfo|120|Africa/Johannesburg|bogus', 'Offset|120', false ], + [ 'ZoneInfo|120|Unknown/Unknown', 'Offset|120', false ], + // Africa/Johannesburg has not DST + [ 'ZoneInfo|0|Africa/Johannesburg', 'ZoneInfo|120|Africa/Johannesburg', true ], + [ 'ZoneInfo|120|Africa/Johannesburg', 'ZoneInfo|120|Africa/Johannesburg', true ], + // Deprecated timezone name + [ 'ZoneInfo|330|Asia/Calcutta', 'ZoneInfo|330|Asia/Calcutta', true ], + // Timezone identifier with space in name + [ 'ZoneInfo|-420|America/Dawson_Creek', 'ZoneInfo|-420|America/Dawson_Creek', true ], + [ 'System', 'System|0', true ], + [ 'System|0', 'System|0', true ], + [ 'System|120', 'System|0', true ], + [ '2:30', 'Offset|150', true ], + [ '02:30', 'Offset|150', true ], + [ '+02:30', 'Offset|150', true ], + [ '0230', 'Offset|840', false ], + [ '2', 'Offset|120', true ], + [ '14:00', 'Offset|840', true ], + [ '-12:00', 'Offset|-720', true ], + [ '15:00', 'Offset|840', false ], + [ '-13:00', 'Offset|-720', false ], + [ '2:30:40', 'Offset|150', true ], + [ '2:30bogus', 'Offset|150', true ], + [ '2.50', 'System|0', false ], + [ 'UTC-8', 'System|0', false ], + ]; + } + + /** + * @covers \MediaWiki\User\UserTimeCorrection::__construct + * @covers \MediaWiki\User\UserTimeCorrection::__toString + * @covers \MediaWiki\User\UserTimeCorrection::isValid + * @dataProvider provideServerTZoffsetExamples + * + * @param int $serverOffset + * @param string $input + * @param string $expected + * @param bool $isValid + */ + public function testServerOffset( int $serverOffset, string $input, string $expected, bool $isValid ) { + $value = new UserTimeCorrection( $input, null, $serverOffset ); + self::assertEquals( $expected, (string)$value ); + self::assertEquals( $isValid, $value->isValid() ); + } + + public function provideServerTZoffsetExamples() { + return [ + [ 120, '', 'System|120', false ], + [ 120, 'bogus', 'System|120', false ], + [ 120, 'System', 'System|120', true ], + [ 120, 'System|120', 'System|120', true ], + [ -120, 'System|-120', 'System|-120', true ], + [ 840, 'System|840', 'System|840', true ], + ]; + } + + /** + * @covers \MediaWiki\User\UserTimeCorrection::__construct + * @covers \MediaWiki\User\UserTimeCorrection::__toString + * @covers \MediaWiki\User\UserTimeCorrection::isValid + * @dataProvider provideDSTVariations + * + * @param DateTime $date Date/time to which the correction would apply + * @param string $input + * @param string $expected + * @param bool $isValid + */ + public function testDSTVariantions( DateTime $date, $input, $expected, $isValid ) { + $value = new UserTimeCorrection( $input, $date ); + self::assertEquals( $expected, (string)$value ); + self::assertEquals( $isValid, $value->isValid() ); + } + + public function provideDSTVariations() { + // Amsterdam observes DST. Johannesburg does not + return [ + // phpcs:disable Generic.Files.LineLength.TooLong + [ new DateTime( '2020-12-01' ), 'ZoneInfo|60|Europe/Amsterdam', 'ZoneInfo|60|Europe/Amsterdam', true ], + [ new DateTime( '2020-12-01' ), 'ZoneInfo|120|Europe/Amsterdam', 'ZoneInfo|60|Europe/Amsterdam', true ], + [ new DateTime( '2020-12-01' ), 'ZoneInfo|120|Africa/Johannesburg', 'ZoneInfo|120|Africa/Johannesburg', true ], + [ new DateTime( '2020-06-01' ), 'ZoneInfo|60|Europe/Amsterdam', 'ZoneInfo|120|Europe/Amsterdam', true ], + [ new DateTime( '2020-06-01' ), 'ZoneInfo|120|Europe/Amsterdam', 'ZoneInfo|120|Europe/Amsterdam', true ], + [ new DateTime( '2020-06-01' ), 'ZoneInfo|120|Africa/Johannesburg', 'ZoneInfo|120|Africa/Johannesburg', true ], + // phpcs:enable + ]; + } + + /** + * @covers \MediaWiki\User\UserTimeCorrection::getTimeOffset + * @covers \MediaWiki\User\UserTimeCorrection::getTimeOffsetInterval + * @covers \MediaWiki\User\UserTimeCorrection::getTimeZone + */ + public function testAccessors(): void { + $value = new UserTimeCorrection( 'ZoneInfo|120|Africa/Johannesburg' ); + self::assertEquals( 120, $value->getTimeOffset() ); + self::assertEquals( 120, (int)$value->getTimeOffsetInterval()->format( '%i' ) ); + self::assertEquals( 'Africa/Johannesburg', $value->getTimeZone()->getName() ); + } +}