Page MenuHomePhabricator

Modernize Project/Object edges
ClosedPublic

Authored by epriestley on Jul 6 2014, 2:43 PM.
Tags
None
Referenced Files
F13997367: D9849.id23640.diff
Thu, Oct 24, 4:02 AM
F13980221: D9849.id23931.diff
Sat, Oct 19, 9:07 AM
F13968853: D9849.id23931.diff
Wed, Oct 16, 11:05 PM
F13968852: D9849.id23640.diff
Wed, Oct 16, 11:05 PM
F13968851: D9849.id23630.diff
Wed, Oct 16, 11:05 PM
F13968849: D9849.diff
Wed, Oct 16, 11:05 PM
Unknown Object (File)
Sun, Oct 13, 1:54 PM
Unknown Object (File)
Wed, Oct 9, 1:55 PM
Subscribers

Details

Summary

Ref T5245. Updates the project/object edge to use a modern class definition. Moves further toward real edges.

Test Plan

Added projects to some objects, viewed transactions in transaction record.

Diff Detail

Repository
rP Phabricator
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

epriestley retitled this revision from to Modernize Project/Object edges.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added reviewers: joshuaspence, chad, btrahan.

This looks good. Again there are a couple of minor issues which are carried over from previous diffs.

src/applications/differential/customfield/DifferentialProjectsField.php
38

As in D9848, this feels awkward.

101

As above.

src/applications/diffusion/controller/DiffusionRepositoryController.php
234

As above.

src/applications/diffusion/controller/DiffusionRepositoryEditBasicController.php
76

As above.

src/applications/diffusion/controller/DiffusionRepositoryEditMainController.php
268

You get the idea...

src/applications/project/edge/PhabricatorProjectEdgeTypeObjectHasProject.php
3 ↗(On Diff #23620)

As in D9839, I'm not a fan of the class name.

6–11 ↗(On Diff #23620)

As in D9839, this feels sorta awkward.

42 ↗(On Diff #23620)

As in D9839, I'm not convinced that we need a $total_count parameter.

78–83 ↗(On Diff #23620)

As in D9839, the parameter order is wrong.

89 ↗(On Diff #23620)

As above.

src/applications/project/edge/PhabricatorProjectEdgeTypeProjectHasObject.php
3 ↗(On Diff #23620)

As above.

6–10 ↗(On Diff #23620)

As above.

joshuaspence edited edge metadata.
This revision now requires changes to proceed.Jul 7 2014, 12:09 AM
src/applications/differential/customfield/DifferentialProjectsField.php
101

This is maybe an argument against using objects. This is getting serialized and must be a scalar.

Or we need to build a lot more "stuff" around edge transactions, which might not be terrible, but they only feel a touch clumsy to me right now (and mostly during construction).

epriestley edited edge metadata.
  • Fix removal string.
joshuaspence edited edge metadata.

This looks good except maybe the class names (see discussion in D9839)

This revision is now accepted and ready to land.Jul 7 2014, 4:57 AM
epriestley edited edge metadata.
  • Rename classes to "*EdgeType".
  • Remove obsolete methods.
epriestley updated this revision to Diff 23931.

Closed by commit rP33120e377a63 (authored by @epriestley).