Page MenuHomePhabricator

Modernize task/revision edges and write inverse transactions
ClosedPublic

Authored by epriestley on Jul 5 2014, 10:30 PM.

Details

Summary

Ref T5245. See some discussion in D9838.

When we attach object A to object B, we'd like to write transactions on both sides but only write the actual edges once.

To do this, allow edge types to shouldWriteInverseTransactions(). When an edge type opts into this, have editors apply the inverse transactions before writing the edge. These inverse transactions don't actually apply effects, they just show up in the transaction log.

Test Plan

Attached and detached revisions from tasks, saw transactions appear on both sides of the operation.

Diff Detail

Repository
rP Phabricator
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

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

See some discussion in T5245.

Er, some discussion in D9838, rather.

  • Remove stray debugging code.
joshuaspence edited edge metadata.

Some minor nitpicks.

src/applications/differential/edge/DifferentialEdgeTypeRevisionHasTask.php
3 ↗(On Diff #23603)

The name of this class seems wrong. I somewhat expect a class which extends PhabricatorEdgeType to be named *EdgeType, so perhaps DifferentialRevisionHasTaskEdgeType is a better name?

5–9 ↗(On Diff #23603)

This feels sort of strange... do we need a function and a constant?

src/applications/maniphest/edge/ManiphestEdgeTypeTaskHasRevision.php
3 ↗(On Diff #23603)

As above, maybe this should be named ManiphestTaskHasRevisionEdgeType?

This revision now requires changes to proceed.Jul 6 2014, 5:21 PM

The naming (ApplicationEdgeTypeThing) is mostly for consistency with the similar existing ApplicationPHIDTypeThing classes. I don't really have much of a specific justification for it. One advantage is that it avoids hard-to-parse names when an application and object have the same name, like PhabricatorProjectProjectHasObjectEdgeType or PhabricatorDashboardsDashboardHasWidgetEdgeType. In almost all callsites both of these objects are just used as XYZ::BLAHCONST, and it's maybe a little easier to see the important part if it's at the end? This is all pretty fluff, but these classes sort of feel distinct enough in their usage to me to justify the convention adjustment.

I'm not opposed to changing them if that's unpersuasive, but we should change ApplicationPHIDType classes too, and will end up with some ugly ones (PhabricatorProjectProjectPHIDType).

The method-and-constant is similarly because we do that in PHIDType. The reasoning there is roughly that it's nice to have a method so we can mpull(), but also nice to have a constant so usage is slightly more clear in common cases. Defining all the constants in a regular way is a little more greppable. In theory, we could implement the methods using reflection (there's no static:: until PHP 5.3.0), but that feels maybe-too-magical?

I'm also fine with changing this, but we should change PHIDType too if we do.

epriestley edited edge metadata.
  • Fix argument order to "remove" stings.

The naming (ApplicationEdgeTypeThing) is mostly for consistency with the similar existing ApplicationPHIDTypeThing classes. I don't really have much of a specific justification for it. One advantage is that it avoids hard-to-parse names when an application and object have the same name, like PhabricatorProjectProjectHasObjectEdgeType or PhabricatorDashboardsDashboardHasWidgetEdgeType. In almost all callsites both of these objects are just used as XYZ::BLAHCONST, and it's maybe a little easier to see the important part if it's at the end? This is all pretty fluff, but these classes sort of feel distinct enough in their usage to me to justify the convention adjustment.

I'm not opposed to changing them if that's unpersuasive, but we should change ApplicationPHIDType classes too, and will end up with some ugly ones (PhabricatorProjectProjectPHIDType).

Ultimately it's up to you I guess (you maintain the code base), but my personal thoughts are that:

  • ManiphestTaskHasRevisionEdgeType is preferable to ManiphestEdgeTypeTaskHasRevision, mainly because it reads nicer. It also "namespaces" better. If you ever moved to PHP 5.3 and adopted namespacing (unlikely but still a valid point I think), then ManiphestTaskHasRevisionEdgeType maps nicely to something like \Phabricator\Maniphest\TaskHasRevisionEdgeType.
  • Personally, I don't think that the application needs to prefix the class name in some (all?) cases. For example, I would prefer PhabricatorDashboardHasWidgetEdgeType and PhabricatorWidgetBelongsToDashboardEdgeType to PhabricatorDashboardsDashboardHasWidgetEdgeType and PhabricatorDashboardsWidgetBelongsToDashboardEdgeType (respectively). This is probably, however, not inline with your current naming conventions.
  • PHIDs are global, so PhabricatorProjectPHIDType seems unique enough to me. Again, this is probably not inline with your current naming conventions.

As I said, it's ultimately your call... but these are my thoughts. Oh, and +1 on consistency (i.e. if you were to change the naming convention then you should also change the ApplicationPHIDType classes).

The method-and-constant is similarly because we do that in PHIDType. The reasoning there is roughly that it's nice to have a method so we can mpull(), but also nice to have a constant so usage is slightly more clear in common cases. Defining all the constants in a regular way is a little more greppable. In theory, we could implement the methods using reflection (there's no static:: until PHP 5.3.0), but that feels maybe-too-magical?

Hmm yeah. Admittedly, I didn't notice that getEdgeConstant was an instance method (i.e. non-static) and I always forgot that Phabricator supports PHP 5.2. I guess, given the circumstances, the duplication between the constant and the method is tolerable. Mayyybbeee we could rename it to EDGE_CONST though?

src/infrastructure/edges/constants/PhabricatorLegacyEdgeType.php
24 ↗(On Diff #23627)

Maybe change this in the original diff (D9837).

Reducing ProjectProject to Project feels a little bit worse (inconsistent / special cased) to me than the ugliness of the double name.

I think there's probably at least an argument somewhere for \Phabricator\Maniphest\EdgeType\TaskHasRevision as a namespace, if we ever did do namespaces. I'm not sure if that argument's very strong or good. I think it's probably mostly moot anyway -- even if we bump the minimum version up to a version with namespaces, my inclination is probably not to use them (except maybe putting everything in a global \Phabricator namespace) -- I don't think they'd really solve any problems.

An alternative would be to make all names hierarchical (DifferentialControllerRevisionView, DifferentailEditorRevision) but a lot of them lose a lot of meaning.

Anyway, I'll rename these for consistency with all other non-PHIDType classes, and we can deal with PHIDType later.

Let me see if I can use reflection to get rid of the subclass method without creating too much of a mess.

epriestley edited edge metadata.
  • Rename ApplicationEdgeTypeThingRelationshipThing to ApplicationThingRelationshipThingEdgeType.
  • Remove getEdgeConstant() methods, which are now implemented with reflection.
joshuaspence edited edge metadata.
joshuaspence added inline comments.
src/applications/differential/customfield/DifferentialManiphestTasksField.php
45

You need to change this to DifferentialRevisionHasTaskEdgeType

54

As above.

src/applications/differential/editor/DifferentialTransactionEditor.php
257

As above.

1253

As above.

src/applications/differential/event/DifferentialHovercardEventListener.php
31

As above.

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
129

I wonder if this should be isInverseEdgeEditor(), for consistency with PhabricatorLegacyEdgeType::shouldPreventCycles()

This revision now requires changes to proceed.Jul 8 2014, 12:38 AM
epriestley edited edge metadata.
  • Fix edge constants.
  • getByConst() -> getByConstant()

I didn't do getIs...() -> is...(). This is a bit complex/fuzzy, but on Lisk objects if you call a property isX, you get a get a generated getter and setter getIsX()/setIsX(). Very, very long ago, we generated an isX() getter in this special case, but removed it at some point as the unpredictable magic of "is" doing something unusual didn't seem worth the slightly nicer method names. So we have a bunch of getIsX() and setIsX() anyway, and it seems desirable over special casing.

Elsewhere, in most cases, I think we use getIs/setIs or getShould/setShould when the property behaves like a property (has a getter and setter), and omit them when it performs computation or reveals some intrinsic value of the object. Basically, if a "set" method exists, the getter is "get". If no "set" method exists because the value is a result of the meaning of the object, the getter is "is/should". Not sure this is really followed, but maybe that's an okayish rule? If we don't like that, I think the next best rule would be "always use get".

(Some of this is probably a consequence of me having an editor macro which generates get/set methods from properties, so if I generate the methods by writing a property they'll default to get/set, while if I generate the method by just writing it out it won't.)

In this case, I'm mostly just being consistent with the existing getIsHeraldEditor() method here. We could rename these or reexamine the rule or whatever else, but I think it's probably best left for later.

btrahan edited edge metadata.

I kind of did a bare review - as opposed to make sure all existing feedback is implemented - and it looks good to me.

shipitquick

I'll take a look at this now.

(why is this still in "Needs Review"? @joshuaspence "rejected prior diff" + @btrahan "accepted" == "Needs Review"?)

Rejects are sticky. The assumption is you might be objecting to something significant and want to sign off on it before it moves forward.

If you were away or on vacation or something, I could edit the revision and remove you as a reviewer, which would move it to "Accepted" (one accept + no rejects), but also leave a more clear audit trail in case I was being sneaky about it.

(There's some discussion somewhere of introducing a "soft reject", maybe, and some discussion of making comments act like a "very soft reject", to smooth out some use cases.)

This revision is now accepted and ready to land.Jul 17 2014, 10:35 PM
epriestley updated this revision to Diff 23927.

Closed by commit rP533e799c5f7e (authored by @epriestley).