Summary: Ref T4029. this diff makes the pertinent database changes AND adds the migration script. This is important to get the data backend straightened away before we fully ship T4029. Next diff will expose the edit controls for these policies and whatever else work is needed to get that part done right.
Test Plan: made sure the lone project page on my wiki had a project with restrictive view policy. Post migration verified correct policy applied to this lone project page AND most open policy applied to the others
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4029
Differential Revision: https://secure.phabricator.com/D10814
Summary:
Ref T4029. Long live PhrictionTransactionEditor...! this means that all existing functionality runs 100% through the modern transactions + editor framework. this diff does a few things in sum
- kills the old editor
- moves conduit-based edits to new editor
- moves stubbing out documents to new editor
- deletes moving of wiki docs for projects functionality... (T4021#59511 is a better bigger battle plan here.)
Test Plan: edited a phriction document via conduit and it worked. created a new phriction document /that/was/deep/ and verified ancestral documents were properly stubbed out. changed a project name and noted no wiki page moves.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T4029
Differential Revision: https://secure.phabricator.com/D10792
Summary:
Ref T4029. Much like D10756, D10761 this does the bare minimum to get things in there. I have a sticky with "TODOs" about moving the error-checking business logic into the editor in all three cases.
Up next - policy...
Test Plan: moved a document and it worked! verified no feed story. verified both documents involved looked good
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: epriestley, Korvin
Maniphest Tasks: T4029
Differential Revision: https://secure.phabricator.com/D10763
Summary:
Ref T4029. Much like D10756 this does the bare minimum to get things in there. I have a sticky with "TODOs" about moving the error-checking business logic into the editor in both cases.
Up next - move actions...
Test Plan: deleted a document and it worked! verified proper feed story.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: shadowhand, Korvin, epriestley
Maniphest Tasks: T4029
Differential Revision: https://secure.phabricator.com/D10761
Summary:
This implements as little as possible to stick a working transactions + editor codepath in the basic create / edit flow. Aside from the transaction tables, this also required adding a mailKey to a phrictionDocument.
Future work would include adding more transactions types for things like "move" and all the pertinent support. Even future work is to add things like policies which will work easily in the transaction framework. Ref T4029.
Test Plan:
- made a wiki doc
- edit a wiki doc
- had someone subscribe to a wiki doc and edited it
For all three, the edits worked, a reasonable email was sent out, and feed stories were generated.
- made a wiki doc at a /location/like/this
document "stubs" were made as expected in /location and /location/like
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: chad, Korvin, epriestley
Maniphest Tasks: T4029
Differential Revision: https://secure.phabricator.com/D10756
Summary:
Ref T1191. Now that the whole database is covered, we don't need to do as much work to build expected schemata. Doing them database-by-database was helpful in converting, but is just reudndant work now.
Instead of requiring every application to build its Lisk objects, just build all Lisk objects.
I removed `harbormaster.lisk_counter` because it is unused.
It would be nice to autogenerate edge schemata, too, but that's a little trickier.
Test Plan: Database setup issues are all green.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley, hach-que
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10620
Summary:
Ref T1191. For most text columns, we either don't care if "a" and "A" are the same, or we expect them to be different (for example: keys, domains, secrets, etc). Default text columns to the `_bin` collation so they are compared by strict character value. This is safer in cases where we aren't sure.
For some text columns, we allow the user to sort by the column in the UI (like Maniphest task titles) or we do care that "A" and "a" are the same (for example: project names). Introduce a new class of virtual data types, the "sort..." types, to cover these columns. These are like the "text..." types but use sorting collations which treat "A" and "a" the same.
Test Plan:
- Made an effort to identify all columns where the UI relies on database collation.
- Ran `bin/storage adjust` and cleared all warnings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: beng, epriestley
Maniphest Tasks: T1191
Differential Revision: https://secure.phabricator.com/D10602
Summary:
Ref T1191. Ref T6203. While generating expected schemata, I ran into these columns which seem to have sketchy nullability.
- Mark most of them for later resolution (T6203). They work fine today and don't need to block T1191. Changing them can break the application, so we can't autofix them.
- Forgive a couple of them that are sort-of reasonable or going to get wiped out.
Test Plan: Saw 94 remaining warnings.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: hach-que, epriestley
Maniphest Tasks: T1191, T6203
Differential Revision: https://secure.phabricator.com/D10593
Summary: Ref T5655. Rename `PhabricatorPHIDType` subclasses for clarity (see discussion in D9839). I'm not too keen on some of the resulting class names, so feel free to suggest alternatives.
Test Plan: Ran unit tests.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin, hach-que
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D9986
Summary: Ref T5655. The `PhabricatorDestructibleInterface` interface is misspelled as `PhabricatorDestructableInterface`. Fix the spelling mistake.
Test Plan: `grep`. Seeing as this interface is fairly recent, I don't expect that this would cause any widespread breakages.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: epriestley, Korvin
Maniphest Tasks: T5655
Differential Revision: https://secure.phabricator.com/D9988
Summary: Allow `PhrictionDocument` to be permanently deleted with `./bin/remove destroy`.
Test Plan:
Deleted a Phriction document with `./bin/remove` and verified that the database was in the expected state.
```
> ./bin/remove destroy PHID-WIKI-auj57rauigvcqvv5feh6
IMPORTANT: OBJECTS WILL BE PERMANENTLY DESTROYED!
There is no way to undo this operation or ever retrieve this data.
These 1 object(s) will be completely destroyed forever:
- PHID-WIKI-auj57rauigvcqvv5feh6 (PhrictionDocument)
Are you absolutely certain you want to destroy these 1 object(s)? [y/N] y
Destroying objects...
Destroying PhrictionDocument PHID-WIKI-auj57rauigvcqvv5feh6...
Permanently destroyed 1 object(s).
```
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin
Differential Revision: https://secure.phabricator.com/D9976
Summary: I'm pretty sure that `@group` annotations are useless now... see D9855. Also fixed various other minor issues.
Test Plan: Eye-ball it.
Reviewers: #blessed_reviewers, epriestley, chad
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, Korvin, hach-que
Differential Revision: https://secure.phabricator.com/D9859
Summary: Ref T4029. When checking the view policy of a document, require the viewer to also be able to see all of the ancestors.
Test Plan:
- Hard-coded `/x/y/` to "no one".
- Checked that `/x/y/` is not visible.
- Checked that `/x/y/z/` is not visible.
- Checked that `/x/`, `/x/q/`, etc., are still visible.
- Tested project pages and sub-pages for project visibility.
Reviewers: btrahan
Reviewed By: btrahan
Subscribers: epriestley
Maniphest Tasks: T4029
Differential Revision: https://secure.phabricator.com/D9199
Summary:
There are quite a few tests in Arcanist, libphutil and Phabricator that do something similar to `$this->assertEqual(false, ...)` or `$this->assertEqual(true, ...)`.
This is unnecessarily verbose and it would be cleaner if we had `assertFalse` and `assertTrue` methods.
Test Plan: I contemplated adding a unit test for the `getCallerInfo` method but wasn't sure if it was required / where it should live.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Differential Revision: https://secure.phabricator.com/D8460
Summary:
Ref T4379. I want project subscriptions to work like this (yell if this seems whacky, since it makes subscriptions mean somethign a little different for projects than they do for other objects):
- You can only subscribe to a project if you're a project member.
- When you're added as a member, you're added as a subscriber.
- When you're removed as a member, you're removed as a subscriber.
- While you're a member, you can optionally unsubscribe.
From a UI perspective:
- We don't show the subscriber list, since it's going to be some uninteresting subset of the member list.
- We don't show CC transactions in history, since they're an uninteresting near-approximation of the membership transactions.
- You only see the subscription controls if you're a member.
To do this, I've augmented `PhabricatorSubscribableInterface` with two new methods. It would be nice if we were on PHP 5.4+ and could just use traits for this, but we should get data about version usage before we think about this. For now, copy/paste the default implementations into every implementing class.
Then, I implemented the interface in `PhabricatorProject` but with alternate defaults.
Test Plan:
- Used the normal interaction on existing objects.
- This has no actual effect on projects, verified no subscription stuff mysteriously appeared.
- Hit the new error case by fiddling with the UI.
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Maniphest Tasks: T4379
Differential Revision: https://secure.phabricator.com/D8165
Summary: See title. Fixes T1809.
Test Plan:
verified each type that has flaggable interface still can be flagged
verified that new custom query filter works
Reviewers: epriestley
Reviewed By: epriestley
CC: Korvin, epriestley, aran
Maniphest Tasks: T1809
Differential Revision: https://secure.phabricator.com/D7392
Summary:
Ref T603. This cleans up an existing callsite in the policy filter, and opens up some stuff in the future.
Some policy objects don't have real PHIDs:
PhabricatorTokenGiven
PhabricatorSavedQuery
PhabricatorNamedQuery
PhrequentUserTime
PhabricatorFlag
PhabricatorDaemonLog
PhabricatorConduitMethodCallLog
ConduitAPIMethod
PhabricatorChatLogEvent
PhabricatorChatLogChannel
Although it would be reasonable to add real PHIDs to some of these (like `ChatLogChannel`), it probably doesn't make much sense for others (`DaemonLog`, `MethodCallLog`). Just let them return `null`.
Also remove some duplicate `$id` and `$phid` properties. These are declared on `PhabricatorLiskDAO` and do not need to be redeclared.
Test Plan: Ran the `testEverythingImplemented` unit test, which verifies that all classes conform to the interface.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7306
Summary:
Ref T603. Adds clarifying text which expands on policies and explains exceptions and rules. The goal is to provide an easy way for users to learn about special policy rules, like "task owners can always see a task".
This presentation might be a little aggressive. That's probably OK as we introduce policies, but something a little more tempered might be better down the road.
Test Plan: See screenshot.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T603
Differential Revision: https://secure.phabricator.com/D7150
Summary:
Ref T3023
Token support for Phriction Documents, Ponder Questions, and Phame Blogs
Test Plan: Token notifications and visual display seems to be working for the above types
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, AnhNhan
Maniphest Tasks: T3023
Differential Revision: https://secure.phabricator.com/D5862
Summary:
Fixes T2694
added edge infrastructure for Phriction
added mail subject prefix option for Phriction
added messy mail support for subscribers
adds edges to the phriction db, along with the subscriber interface
which gives us subscriptions for free.
simple display of subscribers, adequate to the current design and
sufficient fallbacks for exceptional cases. @chad may
be mailed about that one more UI element may be added to his redesign
mail support is messy. not generic at all. only sends to subscribed non-authors.
Test Plan:
tried out all kinds of stuff. applied patch, subscribed, unsubscribed with multiple
accs. verified proper
edited documents, verified that mail was sent in MetaMTA. Verified
contents, tos and stuff by looking into the db, comparing PHIDs etc.
functional testing per serious MTA (that is, AWS SES) worked wonderfully.
Here's how the subscription list looks like:
{F36320, layout=link}
Reviewers: epriestley, chad, btrahan
Reviewed By: epriestley
CC: hfcorriez, aran, Korvin
Maniphest Tasks: T2686, T2694
Differential Revision: https://secure.phabricator.com/D5372
Conflicts:
src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php
Summary:
Companion for D5284. Move all the query logic to a policy-aware query class. In particular:
- Currently, anyone can view and edit a project's wiki documents. For callsites using this query class, you must be able to view or edit the project to view or edit its documents.
- There's some very sketchy logic with the content/document joins. This cleans that up.
- This cleans up loading projects by moving it inside the query. We need to do this anyway to perform policy checks.
Test Plan: Viewed active/all/updated. Set page size to 2, verified pager works.
Reviewers: AnhNhan, chad
Reviewed By: AnhNhan
CC: aran
Differential Revision: https://secure.phabricator.com/D5285
Summary:
Ref T2632. Fixes T1466.
Currently, we normalize slugs (and thus Phriction URIs and canonical project names) to a small number of latin characters. Instead, blacklist a few characters and permit everything else (including utf8 characters).
When generating Phriction URIs, encode any utf8 characters. This means we render URIs encoded, but browsers handle this fine and display them readably in the URI and address bar, etc.
The blacklisted characters are mostly for practical reasons: \x00-\x19 are control characters, `#%?` are meaningful in URIs, `+` is sometimes configured to be interprted as space by apache, etc., `<>\\` are just silly, `&= ` are largely cosmetic.
This allows some silly stuff, like generating URIs with zero-width spaces and RTL markers in them. Possibly we should go blacklist those characters at some point.
Depends on: D5191
Test Plan: {F34402}
Reviewers: AnhNhan, chad, vrana
Reviewed By: chad
CC: aran
Maniphest Tasks: T1466, T2632
Differential Revision: https://secure.phabricator.com/D5192
Summary: Scan all phriction app files for text to pht
Test Plan: Use phriction in ALL CAPS, seems reasonably usable.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4862
Summary:
Okay, just a forewarning, this is horrible right now.
But I think it's an okay enough start that it's worth sending what I've done.
This rips out the old Phriction breadcrumb UI and replaces it with the new one
that other apps have been starting to use.
Test Plan: Looked at a bunch of Phriction pages.
Reviewers: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4183
Summary:
This commit doesn't change license of any file. It just makes the license implicit (inherited from LICENSE file in the root directory).
We are removing the headers for these reasons:
- It wastes space in editors, less code is visible in editor upon opening a file.
- It brings noise to diff of the first change of any file every year.
- It confuses Git file copy detection when creating small files.
- We don't have an explicit license header in other files (JS, CSS, images, documentation).
- Using license header in every file is not obligatory: http://www.apache.org/dev/apply-license.html#new.
This change is approved by Alma Chao (Lead Open Source and IP Counsel at Facebook).
Test Plan: Verified that the license survived only in LICENSE file and that it didn't modify externals.
Reviewers: epriestley, davidrecordon
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2035
Differential Revision: https://secure.phabricator.com/D3886
Summary:
I want to implement a `{P123}` rule to embed pastes, but we need viewers everywhere before it will work with privacy.
This is not exhaustive; many Remarkup callsites haven't been converted to `PhabricatorMarkupInterface` yet.
Test Plan: Looked at Maniphest, Differential, Diffusion, Phriction; added markup, made edits and hit previews.
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3428
Summary:
The immediate issue this addresses is T1366, adding a rendering cache to Phriction. For wiki pages with code blocks especially, rerendering them each time is expensive.
The broader issue is that out markup caches aren't very good right now. They have three major problems:
**Problem 1: the data is stored in the wrong place.** We currently store remarkup caches on objects. This means we're always loading it and passing it around even when we don't need it, can't genericize cache management code (e.g., have one simple script to drop/GC caches), need to update authoritative rows to clear caches, and can't genericize rendering code since each object is different.
To solve this, I created a dedicated cache database that I plan to move all markup caches to use.
**Problem 2: time-variant rules break when cached.** Some rules like `**bold**` are time-invariant and always produce the same output, but some rules like `{Tnnn}` and `@username` are variant and may render differently (because a task was closed or a user is on vacation). Currently, we cache the raw output, so these time-variant rules get locked at whatever values they had when they were first rendered. This is the main reason Phriction doesn't have a cache right now -- I wanted `{Tnnn}` rules to reflect open/closed tasks.
To solve this, I split markup into a "preprocessing" phase (which does all the parsing and evaluates all time-invariant rules) and a "postprocessing" phase (which evaluates time-variant rules only). The preprocessing phase is most of the expense (and, notably, includes syntax highlighting) so this is nearly as good as caching the final output. I did most of the work here in D737 / D738, but we never moved to use it in Phabricator -- we currently just do the two operations serially in all cases.
This diff splits them apart and caches the output of preprocessing only, so we benefit from caching but also get accurate time-variant rendering.
**Problem 3: cache access isn't batched/pipelined optimally.** When we're rendering a list of markup blocks, we should be able to batch datafetching better than we do. D738 helped with this (fetching is batched within a single hunk of markup) and this improves batching on cache access. We could still do better here, but this is at least a step forward.
Also fixes a bug with generating a link in the Phriction history interface ($uri gets clobbered).
I'm using PHP serialization instead of JSON serialization because Remarkup does some stuff with non-ascii characters that might not survive JSON.
Test Plan:
- Created a Phriction document and verified that previews don't go to cache (no rows appear in the cache table).
- Verified that published documents come out of cache.
- Verified that caches generate/regenerate correctly, time-variant rules render properly and old documents hit the right caches.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T1366
Differential Revision: https://secure.phabricator.com/D2945
Summary:
- `kill_init.php` said "Moving 1000 files" - I hope that this is not some limit in `FileFinder`.
- [src/infrastructure/celerity] `git mv utils.php map.php; git mv api/utils.php api.php`
- Comment `phutil_libraries` in `.arcconfig` and run `arc liberate`.
NOTE: `arc diff` timed out so I'm pushing it without review.
Test Plan:
/D1234
Browsed around, especially in `applications/repository/worker/commitchangeparser` and `applications/` in general.
Auditors: epriestley
Maniphest Tasks: T1103
Summary: Last of the big final patches. Left a few debatable classes (12 out of about 400) that I'll deal with individually eventually.
Test Plan: Ran testEverythingImplemented.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran, epriestley
Maniphest Tasks: T795
Differential Revision: https://secure.phabricator.com/D1881
Summary:
Provide tighter integration between Projects and Phriction. Partly, I have most
of a rewrite for the Projects homepage ready but it's not currently possible to
publish feed stories about a project so all the feeds are empty/boring. This
partly makes them more useful and partly just provides a tool integration point.
- When you create a project, all the wiki pages in projects/<project_name>/*
are associated with it.
- Publish updates to those pages as being related to the project so they'll
show up in project feeds.
- Show a project link on those pages.
This is very "convention over configuration" but I think it's the right
approach. We could provide some sort of, like, "@project=derp" tag to let you
associated arbitrary pages to projects later, but just letting you move pages is
probably far better.
Test Plan:
- Ran upgrade scripts against stupidly named projects ("der", " der", " der
", "der (2)", " der (2) (2)", etc). Ended up with uniquely named projects.
- Ran unit tests.
- Created /projects/ wiki documents and made sure they displayed correctly.
- Verified feed stories publish as project-related.
- Edited projects, including perfomring a name-colliding edit.
- Created projects, including performing a name-colliding create.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: aran, epriestley, btrahan
Maniphest Tasks: T681
Differential Revision: 1231
Summary:
- Add a "delete" operation. Delete is just a special edit which removes the
page from indexes and shows a notice that the document has been deleted.
- When a user deletes all the content on a page, treat it as a delete.
- When a conduit call deletes all the content on a page, treat it as a delete.
- Add page status to Conduit.
- Add change type field to history.
- Added a couple of constants to support a future 'move' change, which would
move content from one document to another.
Test Plan:
- Verified deleted pages vanish from the document index (and restoring them
puts them back).
- Verified deleted pages show "This page has been deleted...".
- Created, edited and deleted a document via Conduit.
- Deleted pages via "delete" button.
- Deleted pages via editing content to nothing.
Reviewers: btrahan, jungejason
Reviewed By: btrahan
CC: skrul, aran, btrahan, epriestley
Maniphest Tasks: T680
Differential Revision: 1230
Summary:
Add a new column to PhrictionContent called 'comment' or 'description' or
something
Add an optional field to the Phriction document editing interface that
allows you to add a comment
Test Plan:
Run the sql patch to modify phriction_content table
Create a new wiki page in Phriction
Type in words in description field and save the page
Visit this page and click "Edit Page" button
The content in the desciption field is saved
Reviewed By: epriestley
Reviewers: epriestley, hsb, codeblock
Commenters: codeblock
CC: aran, codeblock, hwang, epriestley
Differential Revision: 709
Summary: Pretty much ripped from D636, but somewhat simplified. Lists all the
documents in the system.
Test Plan: Looked at both of the views, seems to work correctly.
Reviewed By: hsb
Reviewers: hsb, codeblock, jungejason, tuomaspelkonen, aran
CC: aran, hsb, epriestley
Differential Revision: 645
Summary: Provide a (mostly useless, currently) table of document edits.
Test Plan: Looked at document history for several of my high-quality sandbox
wiki pages.
Reviewed By: hsb
Reviewers: hsb, codeblock, jungejason, aran, tuomaspelkonen
CC: aran, hsb
Differential Revision: 644
Summary:
This is another chunk of D636, I just simplified it a bit and added slugs.
When you go to a page like /w/pokemon/, it allows you to create or edit the
page.
Title vs slug stuff is a little funky but I think mostly-reasonable.
Test Plan: Created and edited /w/, /w/pokemon/, etc.
Reviewed By: hsb
Reviewers: hsb, codeblock, jungejason, aran, tuomaspelkonen
CC: aran, hsb
Differential Revision: 643
Summary:
Basically a copy/paste of parts of D636, but with two changes:
- Fully separate the index table ("document") from the content table
("content"). I think this will be a cleaner solution in the long run.
- Build slugs into the document structure.
This doesn't do anything useful, it just normalizes slugs and lays some
groundwork.
Test Plan:
- Visited various /w/ pages and saw them normalize correctly.
- Verified the DAO works by inserting dummy rows.
Reviewed By: codeblock
Reviewers: hsb, codeblock, jungejason, aran, tuomaspelkonen
CC: aran, codeblock, epriestley
Differential Revision: 638
