Avoid search exception for calendar events when using localized time formats
Needs RevisionPublic
Actions

Authored by aklapper on May 4 2024, 12:21.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15811: Search exception when AM/PM string is localized in Calendar application

Summary

Do not call formatTime which will return a localized time format (based on the translation setting of the user) which then fails to match the (English language) $translatable characters defined in PhutilTranslator.php, throwing a search exception when calendar items match the search query string.

Also update DateTime exception handling comment per changes in PHP 8.3:
https://www.php.net/manual/en/class.datemalformedstringexception.php

EXCEPTION: (PhutilAggregateException) All of the configured Fulltext Search services failed.
    - DateMalformedStringException: Failed to parse time string (2024-05-04 12:00 epp.) at position 17 (e): The timezone could not be found in the database at [<phorge>/src/infrastructure/cluster/search/PhabricatorSearchService.php:276]

Closes T15811

Test Plan

Please follow the verbose steps in T15811. In short: Install the Translations extension, set your language to a language which translated AM/PM in time strings, create a calendar event, use the global search to search for the calendar event title.

Diff Detail

Repository

rP Phorge

Branch

amPmCalExplode (branched from master)

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 1221
Build 1221: arc lint + arc unit

Event Timeline

aklapper created this revision.May 4 2024, 12:21

Owners added a reviewer: O1: Blessed Committers.May 4 2024, 12:21

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptMay 4 2024, 12:21

aklapper requested review of this revision.May 4 2024, 12:21

Harbormaster completed remote builds in B1221: Diff 1933.May 4 2024, 12:21

pppery subscribed.May 4 2024, 15:42

aklapper mentioned this in T15815: Make i18n string extraction ignore strings in test cases.May 7 2024, 20:36

valerio.bozzolan added inline comments.Jun 7 2024, 15:42

src/view/viewutils.php

128–140

Part of this function is useful. For example, this historical try-catch with human error is useful.

We should probably keep that.

To re-use this code, maybe it's nice to create a new function that just does the basic thing we need:

function phabricator_datetime_from_epoch($epoch) {

  // NOTE: Although DateTime takes a second DateTimeZone parameter to its
  // constructor, it ignores it if the date string includes timezone
  // information. Further, it treats epoch timestamps ("@946684800") as having
  // a UTC timezone. Set the timezone explicitly after constructing the object.
  try {
    $date = new DateTime('@'.$epoch);
  } catch (Exception $ex) {
    ...
  }

  $date->setTimezone($zone);

  return $date;
}

142

The above comment is also about do not forgetting the timezone set, that is probably important.

valerio.bozzolan added inline comments.Jun 7 2024, 15:43

src/view/form/control/AphrontFormDateControlValue.php
100	Probably this could become something like this if we create such function 🤔 Taking care of nice exception msg, timezone etc.

Visually marking this as "ongoing discussion for code changes" to keep historically nice error messages

This revision now requires changes to proceed.Jun 28 2024, 11:40

I think the behavior of not setting the timezone is probably correct, actually. GetFormattedDateFromParts below already constructs a date object again with the timezone set. So I think the timezone offset is being applied twice here.

(From reading the code, I don't have a means to test this)

And the date is read from the database so I don't think it can be malformed.