Add optional start and end times to calendar entries - #268
Conversation
|
Moin, habs mir jetzt angesehen.
LG 😊 |
|
Alle drei Punkte Punkte angegangen, bitte kritisch prüfen.
Dazu kommt ein zweiter Branch, da steckt eine Entscheidung drin, die über den Kalender hinausgeht. 1. ZeitzoneDer Parser hat die Property-Parameter bisher komplett verworfen ( Kurios daran: die Der Parser behält die Parameter jetzt unter einem eigenen Schlüssel und beherrscht alle drei RFC-5545-Formen: Wichtig war, die Umrechnung vor der Datumsermittlung zu machen, nicht erst bei der Anzeige: die Zone entscheidet auch über den Kalendertag. 2. Verlorener letzter TagDer Code hat Bei mir wurde aus Ausnahme ist ein Ende glatt um Mitternacht, das weiter zum Vortag zählt. Sonst bekäme ein Termin von 18:00 bis 00:00 fälschlich noch einen Eintrag am Folgetag. 3. EndzeitpunktIch habe deinen zweiten Vorschlag genommen, nicht den ersten — das Enddatum zu sperren hätte Punkt 2 für Termine aus der Quelle ja nicht gelöst.
Angezeigt wird Dein Beispiel habe ich einmal komplett durch den reparierten Import geschickt. 12.08. 18:00 bis 14.08. 19:00 ergibt jetzt:
Beide Fehler auf einmal erledigt: die 18:00 stehen richtig da, und der 14. ist wieder dabei — mit der Endzeit statt einer leeren Zeile. Die Zeitzone kommt nicht mehr aus der php.iniDas ist der Teil, den ich dir erklären möchte, bevor du den Code liest. Beim Nachstellen bin ich darauf gestoßen, dass Eine Umgebungsvariable als Grundlage fand ich deshalb zu wackelig. Sie muss im Deployment gesetzt sein, und es gibt eine Falle: Die Zone liegt jetzt als Feld auf Das passt auch zu der Richtung, die in Nebenbei habe ich das Docker-Setup gegengeprüft, weil ich es nicht raten wollte. Dort war der SAPI-Bruch nie das Problem: Zweiter Branch:
|
|
Danke für die Überarbeitung. Ich musste das ganze jetzt einmal in Ruhe durchdenken.
Eine optionale Endzeit finde ich grundsätzlich sinnvoll und passend zur Rückmeldung über mehrtägige Termine. Die genannten Randfälle sollten aber vor dem Merge geklärt werden. Ich bin auch gerade dabei ein paar Informationen und Regeln für Agenten und Menschen zu erstellen. Bitte berücksichtige daher die Konventionen aus der kommenden AGENTS.md, insbesondere hinsichtlich Controller-/Service-Aufteilung, zentraler Formatierung, Migrationen, Dokumentation sowie Unit- und Functional-Tests. Wenn du Fragen hast, melde dich einfach. |
Entries were all-day only. A collection date or a maintenance appointment usually has an hour attached, and showing nothing left the reader to guess. Both times are optional and nullable, so every existing row stays exactly what it was: an all-day entry. They are separate TIME columns rather than a widened DATETIME date, because every consumer keys days by Y-m-d - sync reconciliation, the table decorations, the reminder query - and each would otherwise have to strip a time component it never asked for. On a period the start time sits on the first day and the end time on the last, with the days between running all day. That keeps one row per day while still letting the closing day say when the event stops. One rule needs stating because a time without a date is ambiguous: an end of 00:00 is midnight *closing* the day, not opening it. CalendarEntryTimeRules is the single place that knows this, so the ICS import, the form and the validation cannot contradict each other - an imported "18:00 - 00:00" has to be an entry the edit form accepts. Validation and the day-by-day creation move out of the controller into CalendarEntryService, which returns violations naming the form field they belong on; the controller only maps them onto the form. The time label moves off the entity into CalendarEntryTimeFormatter, reachable from Twig via a filter, so mail and PDF templates can use it later and the times are formatted through Intl in the viewer's locale rather than by hand. Ordering within a day now falls back to the end time, so the closing day of a multi-day entry sorts at its own hour instead of among the all-day entries.
The parser threw away each property's parameters and read the value as whatever it happened to say. RFC 5545 knows three forms and only one of them was handled: a trailing Z is UTC, a TZID parameter names the zone, a bare value is local time. A Google export publishes UTC instants, so its 13:00 appointment was read and stored as 11:00. Values are now resolved to the instant they denote and expressed in the application timezone, which is PHP's date.timezone - the same single source Doctrine hydrates zone-less DATETIME columns in. Conversion happens before the dates are derived, because the zone decides the calendar day as much as the clock: 22:30Z belongs to the next day here. Multi-day events lost their closing day. DTEND is exclusive only for all-day events; when it carries a time it is the moment the event stops, so its day is still covered - unless it stops exactly at midnight, which belongs to the day before. "Aug 14 13:00 - Aug 16 14:00" now yields all three days. An event whose period cannot be read is discarded instead of filed under a guessed day: no DTSTART, an unparseable DTEND, or a DTEND not after DTSTART. Equal values are rejected too - feeds write them both for a zero-length event and, wrongly, for a single all-day one, and picking one reading would invent a period the source never stated. Discarded events are counted and reported in the sync result, the same way recurring ones already are, so nothing disappears silently. The date arithmetic moves into IcsEventSpanResolver, where it can be tested without a database and where the sync service is left with reconciliation alone.
e62751f to
4b695e4
Compare
|
Hi, alle Punkte überarbeietet, der Branch ist neu auf Zeitzone raus (Punkt 1 und 10)OK hast recht: Die Einstellung, die Migration, das Formularfeld, die Settings-Karte und Der Zwei-Stunden-Versatz, den du gemeldet hattest, kam ohnehin nicht daher: der Damit fällt auch der Folge-Branch weg, der die Zone prozessweit gesetzt hätte – Import rechnet um, speichert lokal (Punkt 2)UTC- und TZID-Werte werden in die konfigurierte PHP-Zone umgerechnet und dann Mitternacht: eine zentrale Regel (Punkt 3)Ich habe mich gegen das Verwerfen der Endzeit und gegen das Klemmen auf
Konkret:
Einen Fall regele ich bewusst nicht, weil er beim Testen an echten Daten Ich habe darauf verzichtet, das zu unterdrücken, weil es strukturell derselbe Ungültige Zeiträume werden verworfen (Punkt 4)
Damit das nicht still passiert, werden verworfene Termine gezählt und dem Nutzer Sortierung des Abschlusstags (Punkt 5)Die Sortierung fällt jetzt auf die Endzeit zurück Controller entlastet (Punkt 6)Validierung und das Anlegen mehrtägiger Einträge liegen in Formatierung zentral (Punkt 7)
Eine Migration (Punkt 8)Die drei Migrationen sind zu Tests (Punkt 9)Neu:
Die Sync-Tests sind um die neuen Fälle ergänzt und pinnen die Zeitzone jetzt
Ein Hinweis noch: PHPStan lässt sich auf |
|
Danke für die ausführliche Überarbeitung. Das sieht für mich deutlich runder aus 😊 Einen Fehler würde ich vor dem Merge noch korrigieren: Bei einem manuell angelegten mehrtägigen Eintrag mit Endzeit 00:00 wird auf dem letzten Tag ein alleinstehendes „– 00:00“ gespeichert. Diesen Eintrag lehnt das Bearbeitungsformular anschließend unverändert ab. Da das Bis-Datum einschließlich gilt und 00:00 als Tagesende verstanden wird, sollte der letzte Tag in diesem Fall ohne Endzeit ganztägig gespeichert werden. Bitte ergänze dafür auch einen Unit- oder Functional-Test. Sekunden müssen aus meiner Sicht nicht unterstützt werden. Passend zum Formular wäre es aber sauber, importierte Uhrzeiten konsequent auf Minuten zu normalisieren. Dann wird beispielsweise 00:00:30 bewusst zu 00:00, statt möglicherweise einen zusätzlichen ganztägigen Kalendertag zu erzeugen. Bitte aktualisiere außerdem noch die eigentliche PR-Beschreibung. Sie beschreibt weiterhin den alten Stand mit verworfenen Parametern, nur einer Startzeit und der vorherigen Migration. Danach wäre der PR aus meiner Sicht bereit für den merge. 👍 Alex |
… seconds A manually created period ending at 00:00 stored a lone "- 00:00" on its closing day - an entry the edit form then refused to save unchanged. The ICS import already avoided that, which is the actual defect: the rule sat in IcsEventSpanResolver rather than in CalendarEntryTimeRules, so only one of the two creation paths followed it. It moves to endTimeForClosingDay(), which both paths now call. A midnight end on a day that has no start time of its own says nothing an all-day entry does not already say, so nothing is stored. A single day keeps its "18:00 - 00:00", where the end time is the only thing stating when the entry stops. Imported times are normalised to whole minutes. The form works in minutes, so a feed's seconds could not be represented anyway - and 00:00:30 in particular was midnight to the rules but not to the day loop, which then covered an extra calendar day for those thirty seconds. Normalising before the checks also means an event shorter than a minute is treated as the zero-length period it becomes, rather than slipping through.
MeisterAdebar
changed the title
2 participants
Calendar entries can now carry an optional start and end time. Left empty they
stay all-day entries, which is what every entry was before and still is by
default.
Storage
Both times live in their own nullable
TIMEcolumns rather than wideningdatetoDATETIME. Everything downstream keys days byY-m-d- syncreconciliation, the reservation table decorations, the reminder query - and
each of them would otherwise have to strip a time component it never asked
for.
nullalready says "all day", and that is exactly what existing rows saywithout touching them.
Migration
Version20260816120000adds both columns,down()drops them again.On an entry spanning several days the start time sits on the first day and the
end time on the last, with the days between carrying neither - they genuinely
run all day. That keeps one row per day while still letting the closing day say
when the entry stops, instead of showing nothing and leaving the reader to
guess.
One rule for midnight
A time stored without a date is ambiguous in exactly one place, and both halves
of the application have to read it the same way or they contradict each other:
CalendarEntryTimeRulesis the only place that knows this. The ICS import, theentry form and the validation all ask it, so an imported
18:00 - 00:00is byconstruction an entry the edit form accepts.
Two consequences fall out of the same rule:
00:00is rejected: a day that runs to its end and has nobeginning is an all-day entry, and
- 00:00would read as ending at theday's start.
whether it came from a feed or from the form.
Calendar sync
CalendarEntrySyncServicenow reads RFC 5545 date-times properly. All threeforms occur in the wild and only one was handled before: a trailing
Zis UTC,a
TZIDparameter names the zone, a bare value is local time. A Google exportpublishes UTC instants, so its 13:00 appointment was read and stored as 11:00.
Values are resolved to the instant they denote and expressed in the application
timezone - PHP's
date.timezone, the same single source Doctrine hydrateszone-less
DATETIMEcolumns in. The conversion happens before the days arederived, because the zone decides the calendar day as much as the clock:
20260814T223000Zis the 15th inEurope/Berlin.Imported times are normalised to whole minutes, matching what the form can
express.
Multi-day events used to lose their closing day.
DTENDis exclusive only forall-day events; when it carries a time it is the moment the event stops, so its
day is still covered - unless it stops exactly at midnight, which belongs to
the day before.
Aug 14 13:00 - Aug 16 14:00now yields all three days.Events that are discarded
An event whose period cannot be read is dropped rather than filed under a
guessed day: no
DTSTART, an unparseableDTEND, aDTENDnot afterDTSTART, or a span beyond a year. Equal values are rejected too - feeds writethem both for a zero-length event and, wrongly, for a single all-day one, and
picking one reading would invent a period the source never stated.
Discarded events are counted and reported after the sync, the same way
recurring ones already are, so nothing disappears silently.
The date arithmetic lives in
IcsEventSpanResolver, where it is testablewithout a database and where the sync service is left with reconciliation
alone.
Manual entries
Validation and the day-by-day creation of a period moved out of the controller
into
CalendarEntryService. It returns violations naming the form field theybelong on; the controller only maps them onto the form.
Display and ordering
The time label moved off the entity into
CalendarEntryTimeFormatter, reachablefrom Twig through a filter, so mail and PDF templates can use it later. Times go
through
IntlDateFormatterin the viewer's locale rather than a hand-writtenformat string.
Ordering within a day falls back to the end time (
COALESCE(e.time, e.endTime)),so the closing day of a multi-day entry sorts at its own hour instead of among
the all-day entries. Entries with no time at all still sort first.
Deliberate limits
stored and silently ignored.
an entry on the following day showing
- 00:11. That is correct - the eventoccupies those minutes - but it is worth knowing. Suppressing it would need a
duration threshold, and any number there would be arbitrary; the only clean
boundary is the
00:00case above, which covers zero minutes.startAt/endAton the entity andis a separate change.
Tests
tests/Unit/CalendarEntryTimeRulesTest.php- the midnight rule and everyvalid and invalid combination of times
tests/Unit/CalendarEntryServiceTest.php- validation and how a period issplit across days
tests/Unit/CalendarEntryTimeFormatterTest.php- label and localetests/Unit/IcsEventSpanResolverTest.php- UTC, TZID, local time, the zonedeciding the day, exclusive
DTEND, midnight, minute normalisation, andevery discard case
tests/Functional/CalendarEntryFormTest.php- manual entry, periods,midnight, rejected input and ordering
The existing sync tests are extended for the new cases and pin the timezone
themselves, so they no longer depend on the
php.iniof whoever runs them.