Page MenuHomePhabricator

Make edge types modular
ClosedPublic

Authored by epriestley on Jul 5 2014, 9:42 PM.

Details

Summary

Ref T5245. I want to add a new capability to edge types, which is a good opportunity to move away from PhabricatorEdgeConfig, which isn't modular.

This is basically the same as the modularization of PHID types, which has worked well. Add PhabricatorEdgeType and provide an adaption layer for the existing code.

This has no runtime changes, except the fixed edge constant.

Test Plan

Ran var_dump(PhabricatorEdgeType::getAllTypes()) and got reasonable looking output.

Diff Detail

Repository
rP Phabricator
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

epriestley updated this revision to Diff 23599.Jul 5 2014, 9:42 PM
epriestley retitled this revision from to Make edge types modular.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added reviewers: joshuaspence, chad, btrahan.
joshuaspence edited edge metadata.Jul 6 2014, 2:15 PM

Looks pretty good, some minor inlines. I haven't quite finished looking at this, but so far so good.

src/infrastructure/edges/constants/PhabricatorEdgeConfig.php
81

For consistency:

-/* !!! STOP !!! STOP !!! STOP !!! STOP !!! STOP !!! STOP !!! STOP !!! STOP !! */
+/* !!! STOP !!! STOP !!! STOP !!! STOP !!! STOP !!! STOP !!! STOP !!! STOP !!! */
86

As above.

99

array_diff() takes array parameters, so $exclude may as well have an array type hint.

src/infrastructure/edges/constants/PhabricatorEdgeType.php
86–91 ↗(On Diff #23599)

I think that this should be:

    return pht(
      '%s removed %s edge(s) from %s: %s.',
      $actor,
-      $object,
+      $rem_count,
-      $rem_count,
+      $object,
      $rem_edges);
src/infrastructure/edges/constants/PhabricatorLegacyEdgeType.php
6 ↗(On Diff #23599)

s/PhabricatorEdgeConfig/@{class:PhabricatorEdgeConfig}/ for prettier documentation IIRC.

I'm not sure that PhabricatorEdgeType belongs in src/infrastructure/edges/constants/PhabricatorEdgeType.php... I think src/infrastructure/edges/PhabricatorEdgeType.php would be more appropriate.

src/infrastructure/edges/constants/PhabricatorEdgeConfig.php
81

Disregard this... there was method to your madness.

86

derp

99

It's not clear to me (from the context of this diff alone... it will possibly become clearer in a later diff) what this function is used for. Unless it becomes clear in a later diff, perhaps a docblock comment would help.

epriestley added inline comments.Jul 6 2014, 4:34 PM
src/infrastructure/edges/constants/PhabricatorEdgeConfig.php
81

Haha, I was pretty sad about this. Maybe I can come up with some alternate art which fits more cleanly.

99

Yeah, this could be more clear. In future diffs, I start replacing stuff like this:

$inverse = PhabricatorEdgeConfig::getInverse($const);

...with this:

$type_obj = PhabricatorEdgeType::getByConst($const);
$inverse = $type_obj->getInverseEdgeConstant();

For that to work, we either need to convert every edge type up front (which is a lot of tedious work, and creates some level of unnecessary risk) or expose all the existing data through as PhabricatorEdgeTypes.

This method enables the latter, by building legacy objects for all the existing edge types, so the migration can take place gradually but new code can pretend the migration is already complete because the old types look like they're new types.

src/infrastructure/edges/constants/PhabricatorEdgeType.php
86–91 ↗(On Diff #23599)

Oh, good catch. I probably copy/pasted this like five places by now too. I'll clean those up.

joshuaspence requested changes to this revision.Jul 6 2014, 4:39 PM
joshuaspence edited edge metadata.

Some minor stuff.

src/infrastructure/edges/constants/PhabricatorEdgeType.php
24–34 ↗(On Diff #23599)

Would the $add_edges argument be better off as an array? Same goes for the other functions.

50 ↗(On Diff #23599)

$total_count == $add_count + $rem_count right? If so, we don't need to pass this in explicitly.

72–77 ↗(On Diff #23599)

To save on code duplication, perhaps just return $this->getTransactionAddString($action, $object, $add_count, $add_edges).

86–91 ↗(On Diff #23599)

As above... To save on code duplication, perhaps just return $this->getTransactionRemoveString($action, $object, $rem_count, $rem_edges).

94–112 ↗(On Diff #23599)

As above.

src/infrastructure/edges/constants/PhabricatorLegacyEdgeType.php
86–117 ↗(On Diff #23599)

The same probably applies here... maybe just call the transaction-counterpart.

This revision now requires changes to proceed.Jul 6 2014, 4:39 PM
joshuaspence added inline comments.Jul 6 2014, 5:21 PM
src/infrastructure/edges/constants/PhabricatorEdgeType.php
72–77 ↗(On Diff #23599)

Oh actually, never mind... the strings are different.

86–91 ↗(On Diff #23599)

Nevermind.

94–112 ↗(On Diff #23599)

Nevermind.

Based on the usage in D9841, perhaps the $add_count and $rem_count parameters should have a PhutilNumber type hint? Hmm

None of the strings are typed because:

  • Actor will eventually be a "gendered HTML node" or "gendered string", neither of which exist yet.
  • total_count is distinct from add_count / rem_count because they're PhutilNumbers, and you can't add PhutilNumber + PhutilNumber. We could typehint and then do "new PhutilNumber($add_count->getNumericValue() + $rem_count->getNumericValue())" or something, but it's possible we might want to make these something else in the future, and that's more complex than just doing the math outside the function and passing it in.
  • The edge lists are strings, and might be "translated list" or something in the future. We could take arrays and do implode, but then we'd lose the ability to use a non-comma separator in Chinese or handle RTL languages specially, for example, and end up with more code duplication in the child functions. I don't know that we'll care about separating lists with anything other than comma, but I'm basically just trying to limit the amount of logic in subclasses, even if it's trivial and probably stable.
  • total_count is distinct from add_count / rem_count because they're PhutilNumbers, and you can't add PhutilNumber + PhutilNumber.

Not having come across PhutilNumber before, why do we use it over an ordinary number?

Mostly, it lets us render "123,456.78" in a locale-aware way (for instance, 123.456,78 in some locales). Sometimes you don't want that (e.g., T1234 should not be rendered as T1,234) but sometimes you do (epriestley added 9,876 projects...). We needed some way to tell pht() which is which, and PhutilNumber seemed cleanest.

In theory we could use different escapes (%d vs %n or something) but then we couldn't use native sprintf() internally, which would increase the overall cost of pht() by a lot -- currently, we can just examine the arguments to make decisions about the string, and do not need to parse the string ourselves.

epriestley updated this revision to Diff 23625.Jul 7 2014, 12:25 AM
epriestley edited edge metadata.
  • Make the STOP banner align cleanly.
  • Fix the argument order for the feed removal string.
  • Typehint $exclude.
  • Explain getLegacyEdgeWhatevers().
  • Move EdgeType out of constants/ to type/.
  • Link EdgeConfig in documentation.
joshuaspence accepted this revision.Jul 7 2014, 12:27 AM
joshuaspence edited edge metadata.
joshuaspence added inline comments.
src/infrastructure/edges/constants/PhabricatorEdgeConfig.php
81

Hooray!

This revision is now accepted and ready to land.Jul 7 2014, 12:27 AM
joshuaspence added inline comments.Jul 7 2014, 2:04 AM
src/infrastructure/edges/constants/PhabricatorEdgeConfig.php
101

Pluralize "edge".

src/infrastructure/edges/type/PhabricatorEdgeType.php
105

Not sure how pht handles pluralisation but s/edge/edge(s)/.

143

I prefer to throw more specific exceptions where possible... Perhaps LogicException?

151

As above.

174

As above.

185

As above.

207

I'm not a huge fan of this method name.

epriestley updated this revision to Diff 23634.Jul 7 2014, 3:31 PM
epriestley edited edge metadata.
  • Move the method name fix here from D9839.
epriestley updated this revision to Diff 23635.Jul 7 2014, 4:01 PM
  • Implement getEdgeConstant() using ReflectionClass.
joshuaspence added inline comments.Jul 7 2014, 4:07 PM
src/infrastructure/edges/type/PhabricatorEdgeType.php
15

Maybe make this final?

epriestley added inline comments.Jul 7 2014, 4:08 PM
src/infrastructure/edges/type/PhabricatorEdgeType.php
15

We can't until we get rid of LegacyEdgeType, but it would make sense afterward. I'm also not 100% sure this works yet, need to rebase a couple of diffs on top of it. :P

joshuaspence added inline comments.Jul 7 2014, 4:12 PM
src/infrastructure/edges/type/PhabricatorEdgeType.php
15

Ah of course. Mark with a TODO?

epriestley updated this revision to Diff 23636.Jul 7 2014, 4:28 PM
  • Add TODO.
joshuaspence added inline comments.Jul 8 2014, 12:30 AM
src/infrastructure/edges/constants/PhabricatorEdgeConfig.php
101

s/edge/edges/

106

"That objects" is repeated.

src/infrastructure/edges/type/PhabricatorEdgeType.php
16–28

Maybe we should check here that EDGECONST is a positive integer as well.

71

s/edge/edge(s)/

118

As above.

156–160

Oh that's right, you do it here. I wonder if there's any benefit doing it in getEdgeConstant.

joshuaspence added inline comments.Jul 8 2014, 12:37 AM
src/infrastructure/edges/type/PhabricatorEdgeType.php
220

Maybe rename to getByConstant for consistency with getInverseEdgeConstant()

  • Rename getByConst() to getByConstant().
  • Move EDGECONST type check to the first place we can make it.
  • Fix typography stuff.
epriestley closed this revision.Jul 17 2014, 10:40 PM
epriestley updated this revision to Diff 23925.

Closed by commit rP7afb770cbe07 (authored by @epriestley).