Page MenuHomePhabricator

Upgrading: TYPE_MANIPHEST_WILLEDITTASK and TYPE_MANIPHEST_DIDEDITTASK are deprecated and will be removed
Closed, ResolvedPublic

Description

At HEAD after fb3c1834 (December 9), these events no longer fire. Week 49 is the last stable release to include them.

Long ago, I implemented an "event" system in order to provide extensibility for some operations. The system isn't very good, and we've found better solutions to these problems since the original implementation. I would like to reduce the size of this system over time and eventually remove it.

We have gradually reduced the size of the system. Continuing in this direction, we are deprecating the TYPE_MANIPHEST_WILLEDITTASK and TYPE_MANIPHEST_DIDEDITTASK events today and will remove them in a future version of Phabricator.

If you do not have custom event listeners, you are not affected. This is an advanced feature which very few installs are likely to be affected by.

The event system suffers from a number of irreparable problems that are fundamental to the architecture (for example, poor observability), but the biggest general issue is that modern modular extensions are much more powerful than events. Retaining events leaves us with two separate systems. We'd rather just have one way to do things.

If you currently use these events, you can replace your handlers with one of these approaches:

  • For event handlers which modify the task or adjust transactions, a Herald action is probably the best replacement. See below for details.
  • For event handlers which publish notifications into remote systems, see discussion in T5462: How do I publish Phabricator events into remote systems?.
  • We are not aware of any other types of event handlers in the wild. If you have something which isn't covered here, you may need to fork. You can likely call your code from ManiphestTransactionEditor, either via applyInitialEffects() or applyFinalEffects().

Writing a Herald Action

In some cases, writing a Herald action is the best replacement for event behavior. In the simplest case, you'll write an action which has an effect like "Run MyCompany Special Rules", then write a Herald rule like this:

When:
[ Always ]
Take these actions:
[ Run MyCompany Special Rules ]

This will cause your action to always run. Your action can execute complex logic internally and decide whether or not to actually do anything, so this general case is broadly equivalent to the old event handler behavior.

Depending on what your handler does, it may also make sense to create actions which work more like real actions and move some of the logic into Herald itself. This may be a little more powerful and more flexible.

Herald actions fire in a phase between the WILLEDIT and DIDEDIT events: the primary transactions have already applied, but no notifications have been published about them yet. You can apply additional transactions which will be turned into part of the same transaction group for the purposes of mail, feed, etc.

Here's an example of how to write such an action (which just counts the number of transactions which applied):

1<?php
2
3/**
4 * Example of how to use a HeraldAction instead of an event listener.
5 */
6final class CountTransactionsHeraldAction
7 extends HeraldAction {
8
9 const ACTIONCONST = 'custom.transactions.count';
10 const DO_COUNT = 'do.count';
11
12 public function getHeraldActionName() {
13 return pht('Count task transactions');
14 }
15
16 public function getActionGroupKey() {
17 return HeraldUtilityActionGroup::ACTIONGROUPKEY;
18 }
19
20 public function supportsObject($object) {
21 // You can modify this method to let your rule run on other types of
22 // objects that support Herald, like Differential revisions.
23 return ($object instanceof ManiphestTask);
24 }
25
26 public function supportsRuleType($rule_type) {
27 switch ($rule_type) {
28 case HeraldRuleTypeConfig::RULE_TYPE_GLOBAL:
29 case HeraldRuleTypeConfig::RULE_TYPE_OBJECT:
30 case HeraldRuleTypeConfig::RULE_TYPE_PERSONAL:
31 return true;
32 }
33 }
34
35 public function applyEffect($object, HeraldEffect $effect) {
36 $adapter = $this->getAdapter();
37
38 // NOTE: $object is the object being edited. If you didn't change the
39 // implementation of `supportsObject()`, it's a ManiphestTask.
40
41 // Transactions which were just applied.
42 $applied_xactions = $adapter->getAppliedTransactions();
43
44 $comment_text = pht(
45 'This edit applied %s transactions.',
46 phutil_count($applied_xactions));
47
48 $comment = $adapter->newTransaction()
49 ->getApplicationTransactionCommentObject()
50 ->setContent($comment_text);
51
52 $xaction = $adapter->newTransaction()
53 ->setTransactionType(PhabricatorTransactions::TYPE_COMMENT)
54 ->attachComment($comment);
55
56 // Use queueTransaction() to queue up transactions to be applied.
57 $adapter->queueTransaction($xaction);
58
59 // This makes the effect observable in the Herald transcript.
60 $this->logEffect(self::DO_COUNT);
61 }
62
63 public function getHeraldActionStandardType() {
64 // You can change this to give your action an editable value in the UI,
65 // like how "Add Subscribers: ..." allows you to choose subscribers to add.
66 // This might let you make a more modular/flexible action.
67 return self::STANDARD_NONE;
68 }
69
70 public function renderActionDescription($value) {
71 return pht('Count task transactions');
72 }
73
74 protected function getActionEffectMap() {
75 return array(
76 self::DO_COUNT => array(
77 'icon' => 'fa-sort-numeric-asc',
78 'color' => 'violet',
79 'name' => pht('Counted transactions.'),
80 ),
81 );
82 }
83
84 protected function renderActionEffectDescription($type, $data) {
85 switch ($type) {
86 case self::DO_COUNT:
87 return pht('Added comment.');
88 }
89 }
90}

To put it into effect, write a rule like this:

Here's an example of it in action:

Like other Herald actions, it will appear in the transcript:

Broadly, all of the upstream Herald actions are also implemented like this, so you can look at other HeraldAction subclasses to figure out how to do things they do.


See T9851 for some additional discussion.

Event Timeline

epriestley updated the task description. (Show Details)
epriestley raised the priority of this task from to Normal.
epriestley added a subscriber: epriestley.

โ†’ โ†’

What are you doing which can't be covered by Herald?

Krenair added a subscriber: Krenair.Dec 1 2015, 8:21 PM
robla added a subscriber: robla.Dec 1 2015, 8:32 PM

/me patiently awaits @20after4's response, after being directed to this ticket from Wikimedia's Phabricator instance

My recollection, at least, is that Herald wasn't originally suitable because of T6367. Prior to that task, an "attacker" could write a rule like this:

When [this is a security issue], [send me an email].

Getting your name on the mail recipient list didn't do a separate policy check, so this would let you keep tabs on the initial filing of security issues.

After T6367, policy changes applied by Herald should be correctly checked prior to mail delivery, so the above rule no longer allows an attacker to snoop on stuff they don't otherwise have access to. I think the behavior can now safely be applied as a Herald action.

I'm hoping to be able to obsolete this entirely via T9132 soon anyway, which we're within striking distance of bringing to Maniphest, although it is entangled with the WMF security behavior. Specifically, the "New Task" button should soon be able to be turned into a "New Security Issue" / "New Other Task" dropdown (see example in D14584 of the behavior in Paste), and the "New Security Issue" form can have all of the relevant fields prefilled and locked, or prefilled and completely hidden.

Broadly, we're in no rush to get rid of these events -- definitely no need to panic. I just wanted to push things in the right direction after T9851 was filed, which originally asked for the scope of these events to be substantially expanded.

What are you doing which can't be covered by Herald?

What we are doing is restricting task policies _before_ someone has a chance to add themself to the CC list by way of a herald rule like "always: cc me" attached to all maniphest tasks. We need to preempt them from adding themself to CC if the herald rule decides to restrict the task's view/edit policy.

I believe if you include a transaction which sets the CCs to the current CCs, it is guaranteed to run after any personal rules and will entirely undo the effects of any sneaky rules anyone might write.

The problem is, we add a rule that includes "subscribers can view" to the policy - so even if it prevents mail from being sent, by the end of the herald processing, the user has gained 'can view' on the private task (by way of being CC'd) even though they should have never been able to CC themself in the first place.

I know it's a stupid use case but we specifically require subscribers to be able to view, but people shouldn't be able to subscribe themselves.

I believe if you include a transaction which sets the CCs to the current CCs, it is guaranteed to run after any personal rules and will entirely undo the effects of any sneaky rules anyone might write.

hmm. so I would cache the CC list and then reset it?

I'm not 100% sure it actually works, but you'd just do this:

// Get the CCs before any Herald rules apply.
$current_ccs = PhabricatorSubscribersQuery::loadSubscribersForPHID($task_phid);

// Create a transaction which just sets the CCs to the current value.
$force_ccs = $adapter->newTransaction()
  ->setTransactionType(PhabricatorTransactions::TYPE_SUBSCRIBERS)
  ->setNewValue(
    array(
      '=' => array_fuse($current_ccs),
    ));

// Queue the transaction.
$adapter->queueTransaction($force_ccs);

Most of the time, that won't do anything (it will set the CCs to the current CCs, and be dropped during transaction application). But if personal rules include "Add CCs" actions, it theoretically should just overwrite them and undo them (global rules should be guaranteed to execute after personal rules).

The logic for merging transactions is a little complicated and this situation never occurs naturally anywhere in the application, so it's possible that we merge a + transaction into an = transaction by taking the sum of the two transactions instead of ignoring the +. I can fix this if that's the case -- we should just take the = transaction and ignore the + / - stuff. Then you could swap this to an action, we could do a clean cut over to EditEngine (T9132), and you could (possibly, maybe with a little more work) swap this to a separate form.

But maybe it's simpler to just wait until the form thing is ready. You might be able to swap directly to that and throw most of this stuff away entirely.

In any case, I'm sure we can find a reasonable pathway forward here that minimizes the amount of work you have to do and doesn't require forking.

How about this:

  • For now, don't touch anything.
  • Once T9132 is close to working for Maniphest, we can look at it in more detail and see if it gets you 100% of the way to where you need to be. I think it will (or can be made to get you there), and that's simplest if it does (you can just cut over to it and throw this stuff away).
  • If it's not quite there, we can figure out which pieces still need to be in Herald (or whatever) and I can probably make adjustments (like the CC merging logic) to support the behaviors you want.

@epriestley: FWIW I don't mind doing the work, and I would love to throw away all the hacky stuff we have right now.

Also, I didn't realize the + / - / = transactions worked that way, that actually makes a lot of sense... I'll try it!

And thanks for the guidance, it's much appreciated as always.

Quick update here: ApplicationEditor/EditEngine have caught up with this, so the current plan is to indirectly remove this event in at least some cases (currently: changes made from the "Comments" area) after the stable cut on Saturday. You can upgrade to this week's stable safely but we should make sure things are in a good place before you move beyond that.

Once that's cut and deployed, D14659 + D14663 (and whatever else I do today) will land and be deployed to this server and I'll write a bunch of contextual documentation, and we can figure out if that stuff as-is is good enough for your use case or not. If it is, maybe you just upgrade whenever and throw your old stuff away at your leisure.

If it isn't and we can't easily fix it or easily get Herald in place, I'll just put the events back for now in a hacky way and we can kick the can down the road a little until we have more time to figure out how to move forward. However, if things are easy we might not need to do this.

Upshot:

  • Upgrading to stable this week (after Saturday) is fine, but be careful about upgrading beyond that.
  • New stuff should be available on this server and in master after the release cut on Saturday and we can figure out how exactly to move forward from there once it's clickable and everyone can actually bang on it.

Actually, you probably also don't really care about removing the comment event anyway, since it can't change policies and you can't comment on objects when creating them, and I think your listener doesn't actually end up doing anything special for the actions that are available? So this might be mostly moot anyway, but in an ideal world I'll get through the main "edit" event shortly too.

epriestley moved this task from Backlog to vNext on the Maniphest board.Dec 4 2015, 7:06 PM

The basics of the ApplicationEditor stuff are now live on this install, and T9908 has some discussion of the direction I think we can pursue for the "Security" use case.

epriestley updated the task description. (Show Details)Dec 9 2015, 3:05 PM
epriestley closed this task as Resolved.Feb 5 2016, 7:59 PM
epriestley claimed this task.

The WMF-specific aspects of this are covered on the WMF install in https://phabricator.wikimedia.org/T125104, and that looks like it has a promising path forward that doesn't rely on this event. We haven't seen other outstanding unresolved issues and this has been in the wild for about two months now, so I'm going to assume the best and close it out.