Page MenuHomePhorge

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

Authored by aklapper on Jan 26 2024, 10:42.

Details

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

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?

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).

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

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? :)

Abandoning per last comment