Page MenuHomePhabricator

Implement PhabricatorApplicationTransactionInterface on ManiphestTask
ClosedPublic

Authored by epriestley on Jul 5 2014, 10:00 PM.
Tags
None
Referenced Files
F12845632: D9838.id23926.diff
Fri, Mar 29, 12:27 AM
F12845631: D9838.id23626.diff
Fri, Mar 29, 12:27 AM
F12845629: D9838.id23600.diff
Fri, Mar 29, 12:27 AM
F12845627: D9838.diff
Fri, Mar 29, 12:27 AM
Unknown Object (File)
Sun, Mar 17, 4:51 AM
Unknown Object (File)
Tue, Mar 12, 12:45 AM
Unknown Object (File)
Wed, Mar 6, 9:23 PM
Unknown Object (File)
Feb 6 2024, 5:51 PM
Subscribers
Tokens
"The World Burns" token, awarded by joshuaspence.

Details

Summary

Ref T5245. A very long time ago I had this terrible idea that we'd let objects react to edges being added and insert transactions in response.

This turned out to be a clearly bad idea very quickly, for like 15 different reasons. A big issue is that it inverts the responsibilities of editors. It's also just clumsy and messy.

We now have PhabricatorApplicationTransactionInterface instead, which mostly provides a cleaner way to deal with this.

Implement PhabricatorApplicationTransactionInterface, implicitly moving all the attach actions (task/task, task/revision, task/commit, task/mock) to proper edge transactions.

The cost of this is that the inverse edges don't write transactions -- if you attach an object to another object, only the object you were acting on posts a transaction record. This is sort of buggy anyway already. I'll fix this in the next diff.

Test Plan

Attached tasks, revisions and mocks to a task, then detached them.

Diff Detail

Repository
rP Phabricator
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

epriestley retitled this revision from to Implement PhabricatorApplicationTransactionInterface on ManiphestTask.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added reviewers: joshuaspence, chad, btrahan.
This revision is now accepted and ready to land.Jul 6 2014, 5:02 PM
epriestley edited edge metadata.
  • Rebase since I'm moving through here.
src/applications/search/controller/PhabricatorSearchAttachController.php
61

LogicException perhaps?

Do you have any specific reasoning for using the SPL Exception subclasses over bare Exception?

I feel like we "should" be using them (and we do in some cases) but I can't imagine a case where it would ever actually matter. I think that if anything is catching specific subclasses of these exceptions, it's almost certainly written wrong (maybe with a weak argument for it being OK in unit tests). And if the additional information imparted by RangeException vs OutOfRangeException is helpful in diagnosing things, the exception message probably isn't helpful/detailed enough.

I also just don't know the classes very well, and don't have a consistent mental mechanism for selecting between them. For example, if a caller passes a numeric argument which is too large, is that OutOfBounds, OutOfRange, Logic, Length, InvalidArgument, Domain, BadMethodCall, Runtime, or UnexpectedValue? I feel like I could make arguments of approximately equal strength for each of those. Or maybe it's ErrorException?

If there's no concrete advantage to throwing the SPL exceptions, I'd rather just say "throw Exception for everything that you don't expect callers to recover from". If we have some reason to distinguish between RuntimeException ("roughly: as executed, code is wrong, but maybe fine when executed in other ways") and LogicException ("roughly: as written, code is always wrong") then maybe admitting those two types would make sense, but I'm not sure even that distinction is valuable.

Personally, I think it is a matter of "why not". I'll admit that I don't see any huge advantages, but it is free to implement. I think that LogicException versus RuntimeException is an important distinction in most cases ("the code is wrong" versus "something bad happened").

Yes, the distinction between the classes is often a little fuzzy.

Yeah, I guess I worry that what we're getting for "free" isn't useful. Silly example, but you could imagine MessageStartsWithLetterAException, MessageStartsWithLetterBException, etc., or similar, which would also be free but very obviously not valuable. Although at least those would be unambiguous!

Offhand, can you imagine (or, ideally, point out) a case in this codebase where we'd get better behavior by distinguishing between Logic and Runtime exceptions?

(My first thought is maybe reacting to task failure in the daemons, but I can't actually think of anything we'd do differently by distinguishing between these cases.)

(I would call that "cheap" but not "free" as you have to implement the exception classes).

Offhand, I can't think of any examples where there would be real benefits. But I also can't think of any disadvantages.

http://ralphschindler.com/2010/09/15/exception-best-practices-in-php-5-3 is a pretty good read.

http://ralphschindler.com/2010/09/15/exception-best-practices-in-php-5-3 is a pretty good read.

The arguments there aren't too compelling to me, mostly because I can't actually imagine any reasonable code that would ever catch any of the exceptions underneath Runtime/Logic exception -- except maybe unit tests, but even that doesn't feel great to me.

It's also hard for me to imagine any project with more than 1-2 contributors maintaining consistency between all the different flavors of exception, which further makes catching these useless. If a condition which is essentially identical is sometimes raised as OutOfRange and sometimes as OutOfBounds and sometimes as InvalidArgument and sometimes as UnexpectedValue (because different authors wrote them, or the same author wrote them on different days), nothing of use can be generalized.

I'm inclined to think we should never use any of these exceptions until we can come up with a concrete use case for them. If we do, we should choose a simple, unambiguous set of rules for when they're used. If we start kind of guessing now, we'd have to undo all that work later if we come up with some meaningful distinction between exception classes.

To me, at least, this is also more likely a runtime exception, not a logic exception (it would arise because the user tweaked the URL to pull up the attach dialog for an object which does not implement ApplicationTransactions). On the other hand, maybe an argument in a URL is a "logical" exception. If the difference meant anything, it would be obvious.

There is a lot of ambiguity with some of the exceptions, I'll admit. However, at least some of the SPL exceptions I do agree with:

  • InvalidArgumentException: argument is not of the expected type.
  • UnexpectedValueException: a value does not match with a set of values (e.g. assert_instances_of). The runtime version of InvalidArgumentException.
  • BadFunctionCallException: some arguments are missing (e.g. incomplete calls to *sprintf).

Also, I did introduce the use of some of these exceptions in D9788... happy to revert that though if you wish. I agree that there's little real value from using these exceptions other than being "more correct", so ultimately I'm not overly fussed either way.

I don't think we need to revert them unless we're in the code anyway (it's remotely possible doing so would break some third-party caller, and the benefit is very slight), but in the absence of any concrete benefit from their use I think we should stop introducing new ones. They seem moot at best and might be a mess we have to go undo later if we develop a meaningful reason to throw different types of exceptions.

epriestley updated this revision to Diff 23926.

Closed by commit rPace1feb70220 (authored by @epriestley).