Page MenuHomePhabricator

Provide "almanac.binding.search" and "almanac.binding.edit"
ClosedPublic

Authored by epriestley on Apr 11 2018, 1:40 PM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Dec 19, 12:48 AM
Unknown Object (File)
Mon, Dec 9, 6:29 PM
Unknown Object (File)
Mon, Dec 9, 6:29 PM
Unknown Object (File)
Mon, Dec 9, 6:24 PM
Unknown Object (File)
Mon, Dec 9, 5:49 PM
Unknown Object (File)
Thu, Dec 5, 4:36 AM
Unknown Object (File)
Mon, Dec 2, 11:20 PM
Unknown Object (File)
Mon, Dec 2, 11:20 PM
Subscribers
None

Details

Summary

Depends on D19338. Ref T13120. Ref T12414. These are the last of the new API methods.

This stuff still doesn't work:

  • You can't actually enable/disable bindings yet. I want to take a look at the use cases and consider changing "disabled" to "status", or providing a different way to solve the problem.
  • You can't edit properties via the API. I expect to enable this for all AlmanacPropertyInterface objects with an extension in a future change.
Test Plan
  • Searched for bindings via API.
  • Viewed binding web UI for API methods.
  • Created bindings via API.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

amckinley added inline comments.
src/applications/almanac/xaction/AlmanacBindingServiceTransaction.php
40

Just out of curiosity, why isn't this executeOne()?

This revision is now accepted and ready to land.Apr 11 2018, 3:57 PM

executeOne() raises an immediate policy exception if the object exists but can't be seen, which is less specific ("You can't see a service.") than the error we'll be able to raise ("You can't see the service you tried to bind to.") if we continue.

This pattern is sort of "executeOne(), but return null instead of raising a policy exception if the object exists and can't be seen". Possibly it should be more of a first-class part of the API like executeOneMaybeOrMaybeNot(). There's setRaisePolicyExceptions() but it has some weird behavior that makes it not quite the same, I think. This is relatively rare overall and I don't have a great name for it so I haven't gotten as far as actually adding it to the API.

Conceptually, it would also be kind of nice to differentiate more clearly between "invalid" and "restricted". The API currently makes this tough. An executeOneMaybe() could perhaps return some kind of object wrapping the result that had methods like isRestricted(), isNonexistent(), etc. But I'm not sure if we'd realistically write out all the branches of error messages every time. Still, it might be nice if we had the option more readily.

This revision was automatically updated to reflect the committed changes.