Page MenuHomePhabricator

Added a `token_token` table in anticipation of some data-driven tokens
ClosedPublic

Authored by jcox on Sep 28 2016, 2:41 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Jun 25, 1:10 PM
Unknown Object (File)
Fri, Jun 17, 12:00 AM
Unknown Object (File)
Thu, Jun 9, 11:41 AM
Unknown Object (File)
Wed, Jun 8, 5:33 PM
Unknown Object (File)
Tue, Jun 7, 1:33 AM
Unknown Object (File)
May 30 2022, 12:53 AM
Unknown Object (File)
May 27 2022, 4:13 PM
Unknown Object (File)
Apr 15 2017, 2:36 AM

Details

Summary

Ref T11217. This just adds the table that we'll store tokens in. It doesn't make use of the table at all yet. This is mostly pulled from this diff (D16178). Specifically I mostly followed Evan's instructions related to the token table here: D16178#189120.

Test Plan

I ran ./bin/storage upgrade successfully and there were no schema errors.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jcox retitled this revision from to Added a `token_token` table in anticipation of some data-driven tokens.
jcox updated this object.
jcox edited the test plan for this revision. (Show Details)
epriestley added a reviewer: epriestley.

Couple of possible things inline, but this looks pretty good to me.

resources/sql/autopatches/20160928.tokentoken.sql
12

This should maybe be nullable since I think builtin tokens probably won't use an actual File for their data (we can just read it directly off disk) but we can fix that easily later and I'm not 100% sure it will actually be how things turn out.

src/applications/tokens/storage/PhabricatorTokensToken.php
25–27

(Column doesn't exist?)

98–100

Let's just omit this for now -- we don't have a mailKey, and I think interacting with tokens via email seems a little silly / not worth building, at least for now. I could be wrong and maybe emailing datboi@whatever.phabricator.com will be all the rage in a few months, but doesn't feel like the highest-value interaction here.

112–117

I think this isn't right -- it's loading copies of itself by a nonexistent column. Probably intended to delete PhabricatorTokenGiven instead (replace new self() with new PhabricatorTokenGiven())?

I think we could reasonably delete the File by filePHID, if filePHID is set too.

126

This is ultimately a product question but maybe creating a token shouldn't permanently auto-subscribe you to it. We don't really have a hard-and-fast rule for this but I think the soft rule is "if you own the object", and a creator's ownership claim on a token feels a little tenuous (e.g., compared to a Differential Revision, which is clearly strongly-owned).

We recently backed off on auto-subscribe on Phame blogs, and this ownership feels similarly soft to me.

We can easily tweak this later, though.

This revision is now accepted and ready to land.Sep 28 2016, 2:56 PM
jcox marked 4 inline comments as done.Sep 28 2016, 3:13 PM
jcox added inline comments.
src/applications/tokens/storage/PhabricatorTokensToken.php
126

What does it even mean to be subscribed to a token? I can't think of a scenario where I would want that.

jcox edited edge metadata.

Fixed issues brought up in CR

Gonna go ahead and say no one should be automatically subscribed to tokens

This revision was automatically updated to reflect the committed changes.