Page MenuHomePhorge
Differential D25526

Avoid exception in revision timeline when left diff does not exist
AbandonedPublic
Actions

Authored by aklapper on Jan 26 2024, 10:42.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Apr 27, 03:29
Unknown Object (File)
Thu, Apr 25, 22:39
Unknown Object (File)
Wed, Apr 17, 18:22
Unknown Object (File)
Wed, Apr 17, 06:55
Unknown Object (File)
Mon, Apr 15, 16:57
Unknown Object (File)
Thu, Apr 4, 01:41
Unknown Object (File)
Thu, Apr 4, 01:34
Unknown Object (File)
Sat, Mar 30, 02:11
View All 26 Files
Subscribers
Cigaryno
Matthew
speck
tobiaswiese
valerio.bozzolan

Details

Reviewers
valerio.bozzolan
Group Reviewers
O1: Blessed Committers
Maniphest Tasks
T15638: RuntimeException: "Undefined index" when left diff does not exist in DifferentialRevisionTimelineEngine.php
Summary

The $view_data array passed in $left = idx($view_data, 'left') can obviously be null as there is a if (!$view_data) check a few lines above.
Thus make idx() silently return 0 as default value which does not fix the unknown root cause but still seems better than returning an empty array which inevitably leads to a crash.

Closes T15638

Test Plan

Unclear.

Diff Detail

Repository
rP Phorge
Branch
idxExceptionDifferentialTimeline (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 1043
Build 1043: arc lint + arc unit

Event Timeline

aklapper created this revision.Jan 26 2024, 10:42
Owners added a reviewer: O1: Blessed Committers.Jan 26 2024, 10:42
Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptJan 26 2024, 10:42
Harbormaster completed remote builds in B1043: Diff 1664.Jan 26 2024, 10:42
aklapper requested review of this revision.Jan 26 2024, 10:42
speck subscribed.Jan 26 2024, 14:19
speck added inline comments.
src/applications/differential/engine/DifferentialRevisionTimelineEngine.php
16

Could you comment to clarify the behavior here? Is using the same diff for left and right appropriate when left isn’t present? And presumably right doesn’t need the same check because right should always exist in a diff?

aklapper added inline comments.Feb 6 2024, 10:44
src/applications/differential/engine/DifferentialRevisionTimelineEngine.php
16

I do not know if that would be appropriate... Right now I'm only after avoiding an exception. I'd personally also assume that right should always exist in a diff (if the code doesn't turn out to be more broken).

valerio.bozzolan requested changes to this revision.Mar 10 2024, 17:07

This seems reasonable but better to find a test plan. I also tried, without success :(

(Maybe time to follow our heart and debug this in WMF production - ?)

This revision now requires changes to proceed.Mar 10 2024, 17:07
aklapper added a comment.Mar 12 2024, 12:45

As Wikimedia uninstalled Differential I cannot further debug in downstream.
We can either decline the ticket and its patch for now (if someone runs into this again, they could reopen or file a new task), or could get the patch in (setting a default value) without a test plan to have more robust code. Shrug.
Opinions? :)

aklapper abandoned this revision.Mon, Apr 1, 08:40

Abandoning per last comment

Revision Contents
Changeset List

PathSizePackages
src/
applications/
differential/
engine/
2 linesO1: Blessed Committers

Diff 1664

View Options

src/applications/differential/engine/DifferentialRevisionTimelineEngine.php

Loading...
Content licensed under Creative Commons Attribution-ShareAlike 4.0 (CC-BY-SA) unless otherwise noted; code licensed under Apache 2.0 or other open source licenses. · CC BY-SA 4.0 · Apache 2.0