Page MenuHomePhorge

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

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

Details

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 requested review of this revision.May 4 2024, 12:21
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.

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.