What's the advantage of this over just using constants? That is, in terms of actual usage, typing Class::VALUE seems far better to me than Class::VALUE().

This has toArray(), but we could easily use reflection to provide get_all_class_constants(), get_all_class_constants_starting_with_prefix(...), or get_all_class_constants_matching_pattern(...) or whatever. Maybe something like:

final class PhutilEnum extends Phobject implements Iterator {

  public function __construct($class_name, $prefix = null) {
    // ...
  }

  // ...

}

Then:

foreach (new PhutilEnum('SomeClass', 'STATUS_') as $const => $value) { 
  // Maps all STATUS_X constants.
}

Maybe slightly better would be doing a thing like this and implicit prefix matching:

foreach (new PhutilEnum('SomeClass::STATUS') as $const => $value) { 
  // Maps all STATUS_X constants.
}

Then it would hit grep for SomeClass::STATUS, which is sort of nice.

Ohhh, this is for typehinting.

Typehinting enums would be nice, but my gut reaction is that this is a lot of weirdness to add to solve a problem we don't really have -- I can't recall the last time we had an issue which typehinting of constants would have solved. I think X::Y() is super weird, and I'm not sure that we

A lot of our constants are also dynamic (for example, Maniphest status constants are completely user-defined). This approach doesn't preclude tackling that, too, but it would move us into the realm of typed strings rather than mere enums.

The existing typed strings we have (PhutilCommandString, PhutilSafeHTML) do seem clearly valuable, but they're also doing a lot more than just providing typechecking, and are generally invisible to the user.

The cases I can come up with where this sort of checking is most valuable are largely API endpoints in front of transactions (e.g., future ApplicationEditor Conduit API endpoints) but I think most of the code in handling those will be tailoring the error messages: we don't want to raise type checking errors to users in those cases, and generally can't just catch any typechecking error and turn it into a useful message to the user because we won't be certain the user caused the problem.

In D14481#162747, @epriestley wrote:

Ohhh, this is for typehinting.

Typehinting enums would be nice, but my gut reaction is that this is a lot of weirdness to add to solve a problem we don't really have -- I can't recall the last time we had an issue which typehinting of constants would have solved. I think X::Y() is super weird, and I'm not sure that we

A lot of our constants are also dynamic (for example, Maniphest status constants are completely user-defined). This approach doesn't preclude tackling that, too, but it would move us into the realm of typed strings rather than mere enums.

The existing typed strings we have (PhutilCommandString, PhutilSafeHTML) do seem clearly valuable, but they're also doing a lot more than just providing typechecking, and are generally invisible to the user.

The cases I can come up with where this sort of checking is most valuable are largely API endpoints in front of transactions (e.g., future ApplicationEditor Conduit API endpoints) but I think most of the code in handling those will be tailoring the error messages: we don't want to raise type checking errors to users in those cases, and generally can't just catch any typechecking error and turn it into a useful message to the user because we won't be certain the user caused the problem.

Yeah, I think that the primary advantage here would be typehinting. Largely this was motivated by me working on D14480, during which I realized that the PhabricatorTransactions class is rather messy and inflexible. Specifically, it wasn't clear how to add TYPE_REVEAL_POLICY.

<?php

final class PhabricatorTransactions extends Phobject {

  const TYPE_COMMENT       = 'core:comment';
  const TYPE_SUBSCRIBERS   = 'core:subscribers';
  const TYPE_VIEW_POLICY   = 'core:view-policy';
  const TYPE_EDIT_POLICY   = 'core:edit-policy';
  const TYPE_JOIN_POLICY   = 'core:join-policy';
  const TYPE_EDGE          = 'core:edge';
  const TYPE_CUSTOMFIELD   = 'core:customfield';
  const TYPE_BUILDABLE     = 'harbormaster:buildable';
  const TYPE_TOKEN         = 'token:give';
  const TYPE_INLINESTATE   = 'core:inlinestate';
  const TYPE_SPACE         = 'core:space';

  const COLOR_RED          = 'red';
  const COLOR_ORANGE       = 'orange';
  const COLOR_YELLOW       = 'yellow';
  const COLOR_GREEN        = 'green';
  const COLOR_SKY          = 'sky';
  const COLOR_BLUE         = 'blue';
  const COLOR_INDIGO       = 'indigo';
  const COLOR_VIOLET       = 'violet';
  const COLOR_GREY         = 'grey';
  const COLOR_BLACK        = 'black';


  public static function getInlineStateMap() {
    return array(
      PhabricatorInlineCommentInterface::STATE_DRAFT =>
        PhabricatorInlineCommentInterface::STATE_DONE,
      PhabricatorInlineCommentInterface::STATE_UNDRAFT =>
        PhabricatorInlineCommentInterface::STATE_UNDONE,
    );
  }

}

PhabricatorPHIDType on the other hand, is much more extensible and more natural to work with, so maybe we would be better creating a PhabricatorTransactionType class instead? Or maybe you don't care about this as much as I do in the upstream.

In D14481#162783, @joshuaspence wrote:

PhabricatorPHIDType on the other hand, is much more extensible and more natural to work with, so maybe we would be better creating a PhabricatorTransactionType class instead? Or maybe you don't care about this as much as I do in the upstream.

I started fleshing this out to see what it would look like... https://secure.phabricator.com/differential/diff/35032/

Transactions work like they do mostly because we'd have even more boilerplate code if every transaction type had its own class, and they already feel pretty boilerplate-heavy. Possibly we should do that anyway.

The expectation is that you'll add application-specific transactions to the <TheApplication>Transaction class, not the root PhabricatorTransactions class. For example, see ManiphestTransaction, which defines 11 transaction types:

const TYPE_TITLE = 'title';
const TYPE_STATUS = 'status';
const TYPE_DESCRIPTION = 'description';
const TYPE_OWNER  = 'reassign';
const TYPE_PRIORITY = 'priority';
const TYPE_EDGE = 'edge';
const TYPE_SUBPRIORITY = 'subpriority';
const TYPE_PROJECT_COLUMN = 'projectcolumn';
const TYPE_MERGED_INTO = 'mergedinto';
const TYPE_MERGED_FROM = 'mergedfrom';
const TYPE_UNBLOCK = 'unblock';

These could be 11 classes instead, which would be cleaner in some sense, but methods like this would be split across multiple classes:

public function getActionStrength() {
  switch ($this->getTransactionType()) {
    case self::TYPE_TITLE:
      return 1.4;
    case self::TYPE_STATUS:
      return 1.3;
    case self::TYPE_OWNER:
      return 1.2;
    case self::TYPE_PRIORITY:
      return 1.1;
  }

  return parent::getActionStrength();
}

Maybe this is still worthwhile overall, but XTransaction classes feel really boilerplate-heavy already and requiring a class per transaction type is likely to exacerbate that. That said, it's possible that other things would get easier, and it would make things much more extensible, and improve consistency in general.

We should wait until T9132 in any case but I don't think it's unreasonable to look at making this stuff more consistent after that, even if we end up with a bit more boilerplate.

That diff looks broadly reasonable as a starting point. Ideally we'd also move all the code specific to each transaction type into those classes from PhabricatorApplicationTransaction (and, to some degree, PhabricatorApplicationTransactionEditor) and then do the same to all application transaction types. Hopefully at least slightly easier after T9132.

One thing that would vaguely be nice in the distant future is to allow extensions to write their own transactions. This would likely be nicer if we used subclasses instead of constants.

Yeah -- there's no meaningful way to do that right now, and no meaningful way we can provide a way to do that, and transactions like TYPE_BUILDABLE feel very hacked-in.

In D14481#162798, @epriestley wrote:

These could be 11 classes instead, which would be cleaner in some sense, but methods like this would be split across multiple classes:

public function getActionStrength() {
  switch ($this->getTransactionType()) {
    case self::TYPE_TITLE:
      return 1.4;
    case self::TYPE_STATUS:
      return 1.3;
    case self::TYPE_OWNER:
      return 1.2;
    case self::TYPE_PRIORITY:
      return 1.1;
  }

  return parent::getActionStrength();
}

With subclasses, wouldn't this just become...

public function getActionStrength() {
  return $this->getTransactionType()->getActionStrength();
}

With subclasses, wouldn't this just become...

In Editor or whatever, yes, but the actual numbers (1.1, 1.2, etc) would be spread out across 4 classes, so we'd have 4x as much "public function whatever()", $old = $this->getOldValue();, $new = $this->getNewValue();, etc., and the individual values would no longer be as easy to find in one place (since they'd be in 4 separate files, rather than collected in one place).

This is not a big deal, and I think prior splits like EdgeType and PHIDType were clearly for the best even though they also created some of this extra boilerplate. Transactions are just going to generate relatively more duplicate boilerplate after splitting, I think.

Stuff like figuring out the values easily is also tractable by building better tools, like the recent Module views, so you can go there to see the information collected across classes.

Should I submit a diff or just file a ticket as something that we will come back to eventually?

Maybe just stick it in Maniphest for now.

In D14481#162805, @epriestley wrote:

Maybe just stick it in Maniphest for now.

Filed T9789.