Ref T11217. Bare bones structure to make Tokens an application like Badges.
Details
Details
- Reviewers
epriestley - Maniphest Tasks
- T11217: Make Tokens modular / real application
All the basic wiring works. Create Token, Edit Token, Archive Token, See list of Tokens.
Diff Detail
Diff Detail
- Repository
- rP Phabricator
- Branch
- master
- Lint
Lint Passed Severity Location Code Message Advice src/applications/tokens/controller/PhabricatorTokenViewController.php:79 XHP16 TODO Comment - Unit
Tests Passed - Build Status
Buildable 12804 Build 16308: Run Core Tests Build 16307: arc lint + arc unit
Event Timeline
Comment Actions
Do we need these capabilities?
- Per-token edit policies.
- Commenting on tokens.
- Mail interaction with tokens.
Instead of per-token edit policies, we could have a single "Manage Tokens" permission, like Macros. Offhand, I can't think of a reasonable reason to give a token a custom edit policy, so this seems like it might be needless complexity.
Token commenting and mail interactions also seem low-value / not worth the complexity to me, although maybe there are good reasons to have them that I'm not thinking of. If there's some kind of issue with a token, I'd generally expect opening a task about it to be reasonable/sufficient.
It looks like the new edge types aren't used yet, but I'm not sure what they're intended for. They probably are not a good fit for tokens:
- Edges connect two PHIDs (<src, dst>) but tokens connect three PHIDs (<objectPHID, userPHID, tokenPHID>).
- (It's possible to store the third PHID in edge data today, but I want to remove edge data in T10967.)
- Edges uniquely connect two PHIDs: src and dst can be connected only once. But objects can be connected to a token multiple times: several users can award the same token to an object. As written, the edges won't work when users A and B both give an object a cactus token: the edge will be ignored when user B tries to re-add it, just like how you can't subscribe the same user to a task over and over again.
I suspect edges aren't a good fit for anything related to tokens.
I think the biggest issue here is that if we give users the ability to create and edit their own tokens, they should be able to edit and disable (or subscribe to, comment on, flag, make API calls against, etc.) the builtin tokens in the same way. We can do this by creating real objects for builtin tokens.
Offhand, there are several objects that already work something like this:
- RepositoryURIs have a mixture of builtin and custom URIs, all of which are editable.
- Global Settings sort of do this.
- EditEngine forms sort of do this.
- Builtin files sort of do this.
None of these are particularly simple or good examples, though. I'd probably approach things roughly like this, starting from master.
- Add Token (the storage class).
- Give it a builtinKey VARCHAR(32) which is nullable. For user-created tokens, this will be null. For builtin tokens, this will hold the builtin key identifying the token.
- Add TokenQuery.
- Implement willExecute(), and have it rebuild tokens if they need to be rebuilt.
- For now, always rebuild tokens (if (true) { ... }).
- In the future, we'll put a cache on top of this.
- To rebuild tokens, select all the builtinKey values from the database.
- Then compare them to the available builtin tokens.
- Create real objects for any missing builtins, then save them.
- Give them an application creatorPHID, I guess.
- Then continue, executing query logic normally.
Then migrate all of the existing controller and storage tables to use PHIDs instead of token constants, so this piece of logic is the only thing using builtinKey. Once that's done, you should be able to put this change on top of it and get builtin tokens which work just like user-defined tokens.
To fix the if (true) { ... } thing, we can build a cache key like this. This cache key can double as a key for the new CSS we have Celerity generate dynamically which inlines token images in data URIs.
- Select all of the builtin keys. This will let us know we have to rebuild the CSS after builtins change.
- Select the maximum ID from the token transaction table. This will let us know we have to rebuild the CSS after user tokens change.
- Smush all of that together and hash it.
- Store it in the general cache.
Then to use it:
- Build builtin tokens if the cache does not exist (and the install is not in readonly mode), then populate the cache.
- When saving any token edit with the Editor, dirty the cache.
To make the CSS work, we need to do this:
- Query the cache on every page.
- Rebuild the tokens if necessary.
- Construct a new type of resource URI which ends up in some kind of Celerity controller which knows how to build the CSS properly.
- When that controller receives the request, it should recompute the cache value and only respond with a cacheable response if there's a match.
This is not ideal from a performance viewpoint: it will add an extra cache query to every page (to check if we need to regenerate tokens) and an extra (cacheable) HTTP request (to fetch the token CSS). This creates an extra request because we can't easily package the dynamic token CSS with the static normal CSS.
The cache hit can be mitigated by bundling global cache hits (spaces, etc), discussed in T10078.
The request hit can be mitigated by making Celerity smarter about bundling and letting it bundle dynamic and static resources together. However, this gets tricky because the URI to the resource must change when any tokens change, and we need to have some way to let Celerity figure that out.
So that probably needs to look something like:
- Dynamic resources are some new type of Celerity object.
- They have normal resource names (require('normal-token-css')) and look like normal resources, but aren't.
- They know how to generate their own versions dynamically.
- They know how to generate their own content dynamically.
That's probably reasonable; I don't see any real issues with it offhand. I'm not sure how involved implementation will be.
Overall, I'd rather see this pathway forward:
- Turn tokens into real objects.
- Dynamically generate the token CSS, with packaging support, remove the sprite map, convert token images to builtin files instead (resources/builtin/).
- Then pursue this, on a clean foundation.
| resources/sql/autopatches/20160625.tokens.1.sql | ||
|---|---|---|
| 5 | Maybe just LONGTEXT? We can always summarize in the UI... | |
| src/applications/tokens/application/PhabricatorTokensApplication.php | ||
| 54 | This isn't a "default policy" like "Default View Policy" or "Default Edit Policy"; instead, it controls who is allowed to create tokens. | |
| src/applications/tokens/editor/PhabricatorTokenEditor.php | ||
| 114–115 | TYPE_IMAGE needs to verify that the image PHID exists, and that the viewer has permission to see it. Otherwise, you can do this to view any file whether you have permission to see it or not:
If we're going to use data URIs, it looks like they have a maximum length of 32KB in IE. I'm not sure what (if anything) we should do about that. Some possibilities might be:
It would be nice for TYPE_NAME to enforce the length restriction with an explicit error. If you try to create a 65+ character token name I think you'll get an unhelplful exception from MySQL. It would be nice for TYPE_FLAVOR to enforce the length restriction with an explicit error, or to remove the restriction. | |
| 190 | When the IMAGE is updated, the Editor should emit the new image from extractFilePHIDs(). Otherwise, it won't get attached to the token and other users won't be able to see it. With modular transactions, this got simpler -- see PhabricatorPasteContentTransaction for an example. | |
| src/applications/tokens/query/PhabricatorTokenSearchEngine.php | ||
| 70–77 | You can just omit this method. | |
| 92 | Slightly simpler as $token->getViewURI(), I think. | |
| 122 | I think we have this issue elsewhere, but this probably isn't translatable. For example, in Spanish, I think it has two gender variants:
| |
| src/applications/tokens/storage/PhabricatorTokensToken.php | ||
| 37–39 | (I don't think any query actually uses this key.) | |
| 62 | This is sketchy, although probably fine in a rough cut, but maybe leave a TODO. | |
| 137–141 | This has no effect, and is the same as $this->delete(). However, this probably should remove all PhabricatorTokenGiven rows for the token, and should somehow regenerate all PhabricatorTokenCount rows for affected objects. Neither of these tables are edge tables, so they won't be handled automatically. | |
| 199 | The most useful thing we could return here is probably image with filePHID and viewURI? (Or maybe this is images with a list of different sizes, although presumably we'll just use one size and tell users to upload 2x.) | |
| 202 | I've been inching toward providing richer result sets for values like "status" -- for example, Maniphest returns the constant, but also the human-readable name, color, etc. Although it's unlikely that we'll add colors or other special things to this, we do have at least two values we can return (constant, human-readable name). And if we return a dictionary instead of a single string, we can add more stuff later more easily. One alternative is to return only the constant and then provide a separate query for getting details about available statuses, but that feels a little clumsy/YAGNI for simple applications like this. Another alternative is to provide extra information later as an attachment instead of on the main response. That's very tacked-on, but doesn't break backward compatibility. A third alternative is to break backward compatibility if we want to add more data later. | |
| src/applications/tokens/storage/PhabricatorTokensTransaction.php | ||
| 3 | I think new applications like this should probably use ModularTransactions now -- they may run into some issues, but it seems like they're in good shape so far. D16111 has old/new for Paste, although it has a lot of other stuff too. | |
| 38 | These null create cases are no longer possible in modern EditEngine code, and can be removed from new object types. | |
| 51–59 | This should render a prose diff. | |
| 101 | For consistency, include "from X to Y"? | |
| src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php | ||
| 93 | We already have a database named token, so we should rename it rather than adding a new tokens database. | |
Comment Actions
You should be able to move it off like this:
# Check out your changes on master, if you aren't already here. $ git checkout master # Create a new branch based on the current state ("copy changes to a branch"). $ git checkout -b tokens # Go back to master. $ git checkout master # Reset master to the upstream, erasing your changes here. $ git reset --hard origin/master # Later, go back to your branch to work on your changes. $ git checkout tokens
Comment Actions
So instead of edges, I need an Awards table, similar to Badges? Building this is a little over my head, but a sufficient challenge for weekend downtime. It might take me some time.
Comment Actions
Yeah, but the table already exists -- phabricator_token.token_given. I don't think that table need any changes.
Comment Actions
Here's a more detailed approach that might be easier to tackle than the vague one above:
Database
First, a database already exists. We should not create a second one, since then we'd end up with two similarly-named databases (token and tokens) with very similar data. Instead, we should either use the existing one as-is or rename it. Decide if you want to rename the database from token to tokens.
If you want to rename it, this is probably a lot of work, and we've never really done this before so there's nothing to reference. There was a sort of soft rename of xhpastview to xhpast, but xhpastview never really existed. T8476 has some discussion of this. D13223 and D13224 are earlier efforts in this vein. Neither ever landed, but they discuss some of the difficulty of doing this. One piece of good news is that if you do pursue this, it may be easier than either of those because there are probably (?) no migrations which affect these tables. The general difficulty here is that old migrations need to know that they have to use the old database name, which is complicated.
If you don't want to rename it, you can still use whatever class names you want. The only real cost is that you may have to implement getTableName() in any new storage classes to specify their table names explicitly.
I'd suggest not bothering with renaming it, although this is the best time to do it if we are going to, since it will get harder in the future.
Create the Token Table
Once the database is sorted one way or another, start with PhabricatorTokensToken from this revision. Make it extend the existing PhabricatorTokenDAO, not a new PhabricatorTokensDAO. You can either keep the class name and implement getTableName() as return 'token_token';, or you can rename the class to PhabricatorTokenToken.
The fields are OK as-is, except you should make these changes:
- Add builtinKey, a nullable varchar(32) field.
- Add a unique key on this field (key_builtin).
I'd suggest considering these changes:
- Remove editPolicy (one "manage" policy for all tokens instead?)
- Remove mailKey (no mail interaction with tokens?)
- For simplicity, you may not want to implement PhabricatorApplicationTransactionInterface yet, since you have to add a bunch of other stuff to add that.
This could land with just the .sql + Token object.
Add the Token Query
Rename TokenQuery to OldTokenQuery or BuiltinTokenQuery or something, as in this revision. Rename all callsites to use the new class name. We'll throw it away later.
Add a new TokenQuery, as here, and SearchEngine, ListController, and ViewController, and PHIDType and maybe a couple other things. Probably don't add any of the edit stuff yet: we'll make reads work properly first.
You should be able to manually add rows to the database with INSERT and see them show up in the list and detail views, now.
Generate Builtin Tokens
Implement the stuff described above to generate builtin tokens in willExecute() of the new Query. Roughly, it will look like this:
if (true) { // TODO: Implement caching so we can do this less often. $in_database = queryfx_all(..., 'SELECT builtinKey FROM tokens ...'); $in_code = id(new BuiltinTokenQuery())->loadEverything(); $need_to_create = array_diff($in_code, $in_database); foreach ($need_to_create as $token) { id(new Token()) ->setThings(...) ->setBuiltinKey(...) ->save(); } }
This should make the List and View stuff populate, so you can see all the builtin tokens. Don't worry about the cache stuff for now.
Editing
Now you can do all the editing/create stuff, pretty much as written in this revision.
That will leave the Celerity stuff. It's probably easier for me to just figure out how to build that than try to describe it, but it shouldn't be too tricky, I think. I can get that working and probably do the cache stuff too.
Once those work, you can migrate all the queries in the UI to the new stuff and we'll be good to go.