@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.)
hq.recaptime.dev/wiki/Phorge
phorge
phabricator
Load all diff properties in revision view
Summary:
We need to load all properties with some prefix in one field.
We can't merge them in one property because there will be a race condition for update (we don't have API for load+update+save).
Instead of providing API for this and complicating the code even more, just load everything unconditionally.
It shouldn't waste much bandwith or memory because we use most of the properties anyway.
It also looked overengineered to me.
Test Plan: Displayed revision with fields using diff properties.
Reviewers: epriestley, royw
Reviewed By: royw
CC: aran, royw, Korvin
Differential Revision: https://secure.phabricator.com/D3676
+12
-82
+10
-54
src/applications/differential/controller/DifferentialRevisionViewController.php
+10
-54
src/applications/differential/controller/DifferentialRevisionViewController.php
···
82
82
$repository);
83
83
}
84
84
85
-
list($aux_fields, $props) = $this->loadAuxiliaryFieldsAndProperties(
86
-
$revision,
87
-
$target,
88
-
$target_manual,
89
-
array(
90
-
'local:commits',
91
-
'arc:lint',
92
-
'arc:unit',
93
-
));
85
+
$props = id(new DifferentialDiffProperty())->loadAllWhere(
86
+
'diffID = %d',
87
+
$target_manual->getID());
88
+
$props = mpull($props, 'getData', 'getName');
94
89
90
+
$aux_fields = $this->loadAuxiliaryFields($revision);
95
91
96
92
$comments = $revision->loadComments();
97
93
$comments = array_merge(
···
139
135
140
136
$aux_phids = array();
141
137
foreach ($aux_fields as $key => $aux_field) {
138
+
$aux_field->setDiff($target);
139
+
$aux_field->setManualDiff($target_manual);
140
+
$aux_field->setDiffProperties($props);
142
141
$aux_phids[$key] = $aux_field->getRequiredHandlePHIDsForRevisionView();
143
142
}
144
143
$object_phids = array_merge($object_phids, array_mergev($aux_phids));
···
740
739
return array($changesets, $vs_map, $vs_changesets, $refs);
741
740
}
742
741
743
-
private function loadAuxiliaryFieldsAndProperties(
744
-
DifferentialRevision $revision,
745
-
DifferentialDiff $diff,
746
-
DifferentialDiff $manual_diff,
747
-
array $special_properties) {
742
+
private function loadAuxiliaryFields(DifferentialRevision $revision) {
748
743
749
744
$aux_fields = DifferentialFieldSelector::newSelector()
750
745
->getFieldSpecifications();
···
760
755
$revision,
761
756
$aux_fields);
762
757
763
-
$aux_props = array();
764
-
foreach ($aux_fields as $key => $aux_field) {
765
-
$aux_field->setDiff($diff);
766
-
$aux_field->setManualDiff($manual_diff);
767
-
$aux_props[$key] = $aux_field->getRequiredDiffProperties();
768
-
}
769
-
770
-
$required_properties = array_mergev($aux_props);
771
-
$required_properties = array_merge(
772
-
$required_properties,
773
-
$special_properties);
774
-
775
-
$property_map = array();
776
-
if ($required_properties) {
777
-
$properties = id(new DifferentialDiffProperty())->loadAllWhere(
778
-
'diffID = %d AND name IN (%Ls)',
779
-
$manual_diff->getID(),
780
-
$required_properties);
781
-
$property_map = mpull($properties, 'getData', 'getName');
782
-
}
783
-
784
-
foreach ($aux_fields as $key => $aux_field) {
785
-
// Give each field only the properties it specifically required, and
786
-
// set 'null' for each requested key which we didn't actually load a
787
-
// value for (otherwise, getDiffProperty() will throw).
788
-
if ($aux_props[$key]) {
789
-
$props = array_select_keys($property_map, $aux_props[$key]) +
790
-
array_fill_keys($aux_props[$key], null);
791
-
} else {
792
-
$props = array();
793
-
}
794
-
795
-
$aux_field->setDiffProperties($props);
796
-
}
797
-
798
-
return array(
799
-
$aux_fields,
800
-
array_select_keys(
801
-
$property_map,
802
-
$special_properties));
758
+
return $aux_fields;
803
759
}
804
760
805
761
private function buildSymbolIndexes(
+2
-20
src/applications/differential/field/specification/DifferentialFieldSpecification.php
+2
-20
src/applications/differential/field/specification/DifferentialFieldSpecification.php
···
677
677
}
678
678
679
679
/**
680
-
* Specify which diff properties this field needs to load.
681
-
*
682
-
* @return list List of diff property keys this field requires.
683
-
* @task load
684
-
*/
685
-
public function getRequiredDiffProperties() {
686
-
return array();
687
-
}
688
-
689
-
/**
690
680
* Parse a list of users into a canonical PHID list.
691
681
*
692
682
* @param string Raw list of comma-separated user names.
···
904
894
}
905
895
906
896
/**
907
-
* Get a diff property which this field previously requested by returning
908
-
* the key from @{method:getRequiredDiffProperties}.
897
+
* Get a property of a diff set by @{method:setManualDiff}.
909
898
*
910
899
* @param string Diff property key.
911
900
* @return mixed|null Diff property, or null if the property does not have
···
919
908
// context.
920
909
throw new DifferentialFieldDataNotAvailableException($this);
921
910
}
922
-
if (!array_key_exists($key, $this->diffProperties)) {
923
-
$class = get_class($this);
924
-
throw new Exception(
925
-
"A differential field (of class '{$class}') is attempting to retrieve ".
926
-
"a diff property ('{$key}') which it did not request. Return all ".
927
-
"diff property keys you need from getRequiredDiffProperties().");
928
-
}
929
-
return $this->diffProperties[$key];
911
+
return idx($this->diffProperties, $key);
930
912
}
931
913
932
914
}
-4
src/applications/differential/field/specification/DifferentialLintFieldSpecification.php
-4
src/applications/differential/field/specification/DifferentialLintFieldSpecification.php
-4
src/applications/differential/field/specification/DifferentialUnitFieldSpecification.php
-4
src/applications/differential/field/specification/DifferentialUnitFieldSpecification.php