Summary:
This came up in discussions with both ccheever and fratrik so I prototyped a
"send screenshot to maniphest" feature, which needs this:
https://www.facebook.com/video/video.php?v=892599296749
Test Plan:
Sent screenshot to maniphest.
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: ccheever, fratrik, aran, epriestley
Differential Revision: 240
Summary:
ConduitAPIRequest::getValue call for optional arguments which are
not given added a line to the error log file.
Test Plan:
Tested that Conduit API calls were working from Conduit console.
Reviewed By: epriestley
Reviewers: jungejason, epriestley
Commenters: jungejason
CC: epriestley, aran, jungejason
Differential Revision: 236
Summary:
Sendmail isn't actually OK with passing ENV stuff via 'aliases', accept it as an
argument instead.
Test Plan:
Sent real email to a real server, got differential updates!
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 233
Summary:
Provides support for per-user x per-object unique reply-to email addresses, plus
SMTP integration.
This does not actually make Phabricator use these in outbound email.
Test Plan:
Used test console to validate in-Phabricator routing and handling.
Piped emails into the "mail_handler.php" script to validate mail parsing.
Configured sendmail and sent mail to Phabricator.
Technically I haven't conducted all parts of this test on the same machine since
I lost the will to configure more SMTP servers after configuring phabricator.com
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley, jungejason
Differential Revision: 226
Summary:
I never actually wrote this controller.
Test Plan:
Deleted a repository via web UI.
Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen, jungejason, aran
Commenters: aran
CC: aran, tuomaspelkonen, epriestley
Differential Revision: 231
Summary:
DifferentialChangesetParser currently takes the Changeset object to mean a bunch
of different and mutually conflicting things implicitly:
- Changeset ID is used to access the render cache.
- Changeset ID is also used to tell the ajax endpoint what to render when
clicking "show more".
- Changeset object has the actual changes.
- Changeset ID and "oldChangesetID" are used to choose where to show inline
comments and how to attach new ones.
This indirectly causes a bunch of problems, like T141 and T132. Move toward
making all these separate things explicit. I want to have the changeset object
only mean the actual changes to display.
Test Plan:
Looked at changesets and verified the render cache was accessed correctly (and
not accessed in other cases).
Reviewed By: tuomaspelkonen
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, epriestley, tuomaspelkonen
Differential Revision: 228
Summary:
This mode was fixed in D174 by grglr and is the best mode for almost all changes
once nonbroken, so make it the default.
This is also the mode which takes advantage of the render cache.
Test Plan:
Loaded a revision, got "Ignore All" as the default mode, and revision rendering
wasn't silly/broken.
Reviewed By: tuomaspelkonen
Reviewers: grglr, jungejason, tuomaspelkonen, aran
CC: aran, tuomaspelkonen
Differential Revision: 229
Summary:
That's not how tables work!
Test Plan:
Load maniphest, do not receive a zillion console warnings in Safari.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran
Differential Revision: 227
Summary:
Previously, Remarkup allowed you to paste in an image URI and get an inline
image. However, it did this by hotlinking the image which isn't so hot in an
open source product.
Restore this feature, but use image proxying instead. The existing image macro
code does most of the work.
There is a mild security risk depending on the network setup so I've left this
default-disabled and made a note about it. It should be safe to enable for
Facebook.
Test Plan:
Pasted in image and non-image links, got reasonable behavior. Verified proxying
appears to work. Verified that file:// shenanigans produce 400.
Reviewed By: tuomaspelkonen
Reviewers: aran, jungejason, tuomaspelkonen
Commenters: cpiro
CC: aran, cpiro, tuomaspelkonen
Differential Revision: 214
Summary:
Restores the old "pokedex" feature and allows easy definition of new macros.
Only good can come of this!
Critical feature!!
Test Plan:
nyancat
Reviewed By: tuomaspelkonen
Reviewers: aran, bh, tomo, jungejason, tuomaspelkonen
CC: aran, tuomaspelkonen
Differential Revision: 220
Summary:
The old query was effectively
SELECT DISTINCT revision.* FROM `differential_revision` revision
JOIN `differential_relationship` relationship ON
(relationship.revisionID = revision.id
AND relationship.objectPHID in
('PHID-USER-a113b9ae4ee9524d0a20'))
OR revision.authorPHID = 'PHID-USER-a113b9ae4ee9524d0a20'
LEFT JOIN `differential_viewtime` viewtime ON
viewtime.viewerPHID in ('PHID-USER-a113b9ae4ee9524d0a20')
AND viewtime.objectPHID = revision.phid
AND GREATEST(1304022277, IFNULL(viewtime.viewTime, 0)) <
revision.dateModified
ORDER BY dateModified DESC;
I'm not a db performance expert but it looks like the problem is that we
have to scan all revisions
mysql> EXPLAIN SELECT DISTINCT revision.* FROM `differential_revision`
revision JOIN `differential_relationship` relationship ON
(relationship.revisionID = revision.id AND relationship.objectPHID
in ('PHID-USER-a113b9ae4ee9524d0a20')) OR revision.authorPHID =
'PHID-USER-a113b9ae4ee9524d0a20' LEFT JOIN `differential_viewtime`
viewtime ON viewtime.viewerPHID in ('PHID-USER-a113b9ae4ee9524d0a20')
AND viewtime.objectPHID = revision.phid AND GREATEST(1304022277,
IFNULL(viewtime.viewTime, 0)) < revision.dateModified ORDER BY
dateModified DESC;
+----+-------------+--------------+-------+--------------------+------------+---------+-------+--------+------------------------------------+
| id | select_type | table | type | possible_keys | key |
key_len | ref | rows | Extra |
+----+-------------+--------------+-------+--------------------+------------+---------+-------+--------+------------------------------------+
| 1 | SIMPLE | revision | ALL | PRIMARY,authorPHID | NULL |
NULL | NULL | 254127 | Using temporary; Using filesort |
| 1 | SIMPLE | viewtime | ref | PRIMARY | PRIMARY |
66 | const | 17 | Distinct |
| 1 | SIMPLE | relationship | index | PRIMARY,objectPHID | objectPHID |
72 | NULL | 966900 | Using where; Using index; Distinct |
+----+-------------+--------------+-------+--------------------+------------+---------+-------+--------+------------------------------------+
The new query is a lot faster
mysql> EXPLAIN SELECT revs.* FROM ( (SELECT revision.* FROM
`differential_revision` revision WHERE revision.authorPHID in
('PHID-USER-a113b9ae4ee9524d0a20')) UNION (SELECT revision.* FROM
`differential_revision` revision JOIN differential_relationship rel WHERE
rel.revisionId = revision.Id AND rel.objectPHID =
'PHID-USER-a113b9ae4ee9524d0a20')) as revs LEFT JOIN `differential_viewtime`
viewtime ON viewtime.viewerPHID = 'PHID-USER-a113b9ae4ee9524d0a20' AND
viewtime.objectPHID = revs.phid WHERE GREATEST(1304022277,
IFNULL(viewtime.viewTime, 0)) < revs.dateModified ORDER BY revs.dateModified;
+----+--------------+------------+--------+--------------------+------------+---------+-----------------------------------------+------+--------------------------+
| id | select_type | table | type | possible_keys | key |
key_len | ref | rows | Extra
|
+----+--------------+------------+--------+--------------------+------------+---------+-----------------------------------------+------+--------------------------+
| 1 | PRIMARY | <derived2> | ALL | NULL | NULL |
NULL | NULL | 3021 | Using filesort
|
| 1 | PRIMARY | viewtime | ref | PRIMARY | PRIMARY |
66 | const | 17 | Using where
|
| 2 | DERIVED | revision | ref | authorPHID | authorPHID |
67 | | 1040 | Using where
|
| 3 | UNION | rel | ref | PRIMARY,objectPHID | objectPHID |
66 | | 3822 | Using where; Using
index |
| 3 | UNION | revision | eq_ref | PRIMARY | PRIMARY |
4 | phabricator_differential.rel.revisionID | 1 |
|
| NULL | UNION RESULT | <union2,3> | ALL | NULL | NULL
| NULL | NULL | NULL |
|
+----+--------------+------------+--------+--------------------+------------+---------+-----------------------------------------+------+--------------------------+
Test Plan:
Loaded differential updates with new query, made sure page loaded quickly. Ran
the query from the command-line, it took about .4 seconds.
Reviewed By: Girish
Reviewers: tuomaspelkonen, jungejason, Girish
Commenters: btrahan
CC: aran, btrahan, elynde, Girish
Differential Revision: 181
Summary:
The 'All Revisions and Reviews' Query takes about 2 seconds when I run
it from the mysql command-line:
SELECT revision.*
FROM `differential_revision` revision LEFT JOIN
`differential_relationship` relationship
ON revision.id = relationship.revisionID
AND relationship.relation = 'revw'
WHERE revision.authorPHID in ('PHID-USER-a113b9ae4ee9524d0a20') OR
relationship.objectPHID in ('PHID-USER-a113b9ae4ee9524d0a20')
GROUP BY revision.id ORDER BY dateModified DESC
2419 rows in set (2.05 sec)
This takes about 0.1-0.2 seconds. Just dug into this because I guess
phabricator is haven't a bunch of mysql timeouts.
I don't know what the hell I'm doing; this is just faster
Test Plan:
Loaded 'All Revisions and Reviews' in sandbox
http://phabricator.dev1577.snc6.facebook.com/differential/filter/related/
Made sure it had same results as the version in prod
https://phabricator.fb.com/differential/filter/related/
Still slow to generate all that html
Reviewed By: epriestley
Reviewers: epriestley, aran, tuomaspelkonen, jungejason
CC: aran, epriestley
Differential Revision: 182
Summary:
Moved this to the "phabricator-www" project.
Test Plan:
N/A
Reviewed By: scottmac
Reviewers: scottmac
CC: aran, scottmac
Differential Revision: 219
Summary:
- Provides an "all daemons" view to look at more than the first 15 daemons.
- Provides a "combined log" view with a large page size, to quickly look at
the log across all the daemons, making it easier to find issues when you have a
bunch of the same daemon and only one is having issues.
- When viewing the web console on the same host as a daemon, show whether it's
running or not.
Test Plan:
Clicked the various daemon log interfaces.
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran
Differential Revision: 215
Summary:
This isn't terribly elegant but it solves the problem without loss of
generality. We can pursue a more finessed solution later if it seems prudent.
Test Plan:
Created a revision matched by a blanket herald rule, and then commented on it.
Comment email had X-Herald-Rules header in it.
Reviewed By: aran
Reviewers: aran, tuomaspelkonen, jungejason
CC: aran
Differential Revision: 218
Summary:
The diff view page should point to the revision, if the diff has
already been attached to one. The form to select the revision was also
removed in this case.
Let's me know if it should be possible to reattach a diff to a different
revision.
Test Plan:
Tested that a new diff created with '--preview' option was not attached to
any revision. After attaching the diff manually, made sure that the diff view
page
showed the link to the revision correctly.
Reviewed By: epriestley
Reviewers: jungejason, epriestley
CC: aran, epriestley
Differential Revision: 216
Summary:
I only actually enabled it in Remarkup previously.
Test Plan:
Created a python diff, got syntax highlighted.
Reviewed By: aran
Reviewers: aran, tuomaspelkonen, jungejason
CC: aran, epriestley
Differential Revision: 202
Summary:
Orient potential contributors to stuff they should read first, the Facebook CLA,
how they can get started, and the general philosophy of the project.
Test Plan:
read the document
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen, davidrecordon
Commenters: davidrecordon
CC: aran, epriestley, davidrecordon
Differential Revision: 208
Summary:
Adds a pager control to the "Files" tool so you can page through files if there
are >100.
Test Plan:
Set page size to a smaller number, paged through files.
Reviewed By: aran
Reviewers: aran, tuomaspelkonen, jungejason
CC: aran
Differential Revision: 211
Summary:
Some users have had problems with the database initialization process, simplify
it by creating a new "initialize.sql" dump at v34.
I also populated this dump with the right landing screen (so all the tools
actually have links) and a default avatar.
Test Plan:
Dropped all databases, initialized according to documentation, ended up in a
good state with sensible defaults.
Reviewed By: jungejason
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran, jungejason
Differential Revision: 210
Summary:
there are several places we open an 'r' connection but use it
for writing. Fix them.
Test Plan:
ran parse_one_commit.php against one revision which executes
the code with problem. It used to throw exception. Now it works fine.
Reviewed By: Girish
Reviewers: tuomaspelkonen, Girish
Commenters: aran
CC: aran, Girish
Differential Revision: 213
Summary:
I pretty shortsightedly made sending a side effect of save() in the case that a
server is configured for immediate sending. Move this out, make it explicit, and
get rid of all the tangles surrounding it.
The web tool now ignores the server setting and only repsects the checkbox,
which makes far more sense.
Test Plan:
Sent mails from Maniphest, Differential, and the web console. Also ran all the
unit tests. Verified headers from Maniphest.
Reviewed By: rm
Reviewers: aran, rm
CC: tuomaspelkonen, rm, jungejason, aran
Differential Revision: 200
Summary:
See T129, some older git doesn't have %B and we can reasonably fake it with %s
and %b.
Test Plan:
Reparsed all of the Phabricator repository with this worker, commit messages
look fine.
Reviewed By: rm
Reviewers: rm, aran, jungejason, tuomaspelkonen
CC: aran, rm
Differential Revision: 209
Summary:
Currently you can still punch through Lisk isolation by calling
establishConnection(), and we do that all over the place. Rename getConnection()
to establishConnection() so that all existing callers are safe, and rename
establishConnection() to establishLiveConnection() so that it's not surprising
when this fails to stub in unit tests.
Not wedded to the name if anyone thinks "establishExternalConnection" or
something is clearer.
Test Plan:
Loaded site, browsed around, ran unit tests.
Reviewed By: aran
Reviewers: aran, tuomaspelkonen, jungejason
CC: aran
Differential Revision: 201
Summary:
Amazon SES does not allow us to set a Message-ID header, which means
that threads are incorrect in Mail.app (and presumably other applications
which respect In-Reply-To and References) because the initial email does not
have anything which attaches it to the rest of the thread. To fix this, never
rely on Message-ID if the mailer doesn't support Message-ID.
(In the Amazon SES case, Amazon generates its own Message-ID which we can't
know ahead of time).
I additionally used all the Lisk isolation from the other tests to make this
testable and wrote tests for it.
I also moved the idea of a thread ID lower in the stack and out of
DifferentialMail, which should not be responsible for implementation details.
NOTE: If you push this, it will cause a one-time break of threading for
everyone using Outlook since I've changed the seed for generating Thread-Index.
I feel like this is okay to avoid introducing more complexity here.
Test Plan:
Created and then updated a revision, messages delivered over Amazon
SES threaded correctly in Mail.app. Verified headers. Unit tests.
Reviewed By: rm
Reviewers: aran, tuomaspelkonen, jungejason, rm
Commenters: aran
CC: aran, rm, epriestley
Differential Revision: 195
Summary:
Allow Lisk to be put into process-isolated mode which establishes
only isolated connections. By default, put it into this mode when running
unit tests. Build some simple unit tests around object insertion and
updating.
NOTE: The one flaw in this is that $dao->establishConnection() still
punches through the isolation layer. I need to do an API change to fix this
though so I'm holding it for now. It will probably just rename getConnection()
to establishConnection() and then rename establishConnection() to something
scary like establishLiveExternalConnection().
Test Plan:
Ran unit tests.
Reviewed By: aran
Reviewers: aran, tuomaspelkonen, jungejason
CC: aran, epriestley
Differential Revision: 194
Summary:
This provides a new connection which doesn't connect to
anything, so effects can be isolated to the current process (for
unit testing).
Test Plan:
Ran unit tests.
Reviewed By: aran
Reviewers: aran, tuomaspelkonen, jungejason
CC: aran, epriestley
Differential Revision: 193
Summary:
Make it slightly less terrible. Make sure to use tables for layout.
Test Plan:
Looked at it.
Reviewed By: tomo
Reviewers: aran, tomo, tuomaspelkonen, jungejason
CC: aran, epriestley, tomo
Differential Revision: 196
Summary:
The correct name of this key is 'github.application-secret', not
'github.secret'. Make DarkConsole check that all the masked keys exist to
prevent this from happening again. This isn't super important since this
is just intended to protected against casual security lapses (taking a
screenshot with DarkCnosole's "Config" tab open, for instance) but it's easy
to check for so it seems worthwhile to get right.
Test Plan:
Loaded page without the actual config file change, got an exception.
Fixed the config, reloaded the page, good news goats (really trying to get this
to catch on since goats are adorable).
Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran
CC: aran
Differential Revision: 189
Summary:
In a basically reasonable configuration where you connect
with a non-privileged user from the web workflow, upgrade_schema.php
won't have enough privileges. Allow the user to override the normal
auth with -u and -p.
Test Plan:
Tried to do a schema upgrade with an underprivileged user,
got a useful error message instead of garbage.
Reviewed By: Girish
Reviewers: Girish, davidrecordon, jungejason, tuomaspelkonen, aran
CC: aran, epriestley, Girish
Differential Revision: 191
Summary:
This global 'const' syntax was introduced in PHP 5.3:
http://www.php.net/manual/en/language.constants.syntax.php
We're PHP 5.2.x elsewhere so just use define(). Made the constant a little more
specific too.
Test Plan:
Ran upgrade_schema.php script.
Reviewed By: Girish
Reviewers: tuomaspelkonen, Girish, davidrecordon
CC: jungejason, aran, epriestley, Girish
Differential Revision: 190
Summary:
DifferentialRevision stores this field as a dictionary but the
Editor incorrectly passed it to Herald as a raw value array. Ideally the
property should be called unsubscribedDict or something but I'm increasingly
thinking we're going to centralize subscriptions for Adjutant or some similar
system so I'm disinclined to pursue the schema change just yet. I provided an
explicit raw-value-oriented API, at least.
Test Plan:
With two accounts, A and B: created universal Herald CC rule with
user B, created a revision with user A, had user B unsubscribe, had user A
update the revision. User B was not resubscribed.
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: ola, aran, epriestley
Differential Revision: 188
Summary:
Thanks to erling for the report. This was XSSable, although you could
only get yourself.
Test Plan:
Made a comment like "</textarea><h1>" and edited it before and after
the patch. Proper behavior with this patch.
Reviewed By: aran
Reviewers: erling, jungejason, tuomaspelkonen, aran
CC: aran
Differential Revision: 187
Summary:
If you don't have a custom renderer, this variable does not get set
and emits a warning when you try to read it.
Test Plan:
Loaded page before and after change, warnings went away (lines
145 and 154 in old file).
Reviewed By: aran
Reviewers: tuomaspelkonen, aran
CC: jungejason, aran, epriestley
Differential Revision: 186
Summary:
Just minor bookkeeping, but the current regexp is too liberal and
will match things which can't possibly be revision hashes.
Test Plan:
Typed things which should and shouldn't be revision links, they
got handled properly.
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran
Differential Revision: 185
Summary:
This is evil, don't run with it enabled. Somehow got enabled on my
laptop?
Test Plan:
Loaded phabricator with magic_quotes_gpc enabled, was rebuffed.
Reviewed By: aran
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran
Differential Revision: 184
Summary:
This permits individual deployments to better configure their
database configuration, e.g. to allow more dynamic configuration that reacts
to database moves or master/slave replication.
Test Plan:
Browse
Reviewed By: epriestley
Reviewers: Girish, epriestley
CC: aran, epriestley
Differential Revision: 183
Summary:
Fixed buggy and incomplete logic for handling IGNORE ALL mode properly.
A subparser is used to parse the non-ws-ignoring changeset while a
ws-ignoring changeset is handed off to the original parser. At a later
step, the original parser queries the subparser for its lines of text
(which are formatted properly due to being in non-ws-ignoring mode) and
uses them to replace the text in the ws-ignoring diff.
Task ID: 549940
Test Plan:
-turn off caching temporarily (the cached view is still indented
improperly)
-visit http://phabricator.dev1943.facebook.com/D242591
-note aligned, but not completely highlighted, indentation right above the
comments complaining about
indentation issues
Reviewed By: aran
Reviewers: tuomaspelkonen, jungejason, aran
Commenters: epriestley
CC: aran, epriestley, grglr
Differential Revision: 174
Summary:
Add ability to define mysql slaves and then use that connection on 'r'
connection modes. 'w' connections go to the master server.
Test Plan:
- php -l and checkModule
- worked in my devbox
Reviewed By: jungejason
Reviewers: dpepper, tuomaspelkonen, jungejason
CC: jungejason, aran
Revert Plan:
sure
Differential Revision: 175
Summary:
This adds a new view to differential called Updates.
The high-level goal of Updates is to enabled differential to be
effectively used without email notifications. I've tried doing things
like automatically deleting differential emails where I'm in the 'to'
line since they show up on the main diffential page but then there's
always the chance an important diff flies by without me seeing it. Also,
sometimes someone comments on a diff post-commit but differential
doesn't surface those diffs.
I re-created a test db on my devserver using mysqldump to get data on
revs > 230000 so I would have some test data. We need to add a simple
viewtime table but I didn't want to do that in production. Here's the
table:
CREATE TABLE differential_viewtime (
viewerPHID varchar(64) not null,
objectPHID varchar(64) not null,
viewTime int unsigned not null,
PRIMARY KEY (viewerPHID, objectPHID)
);
Issues:
-Once we turn this on, all diffs will be 'unviewed'. What do you think
about a 'Clear All' button or something?
-Maybe we should add a pager
This feature would be insanely useful, let me know what you think.
Test Plan:
Loaded Updates in my sandbox
http://phabricator.dev1577.snc6.facebook.com/differential/filter/updates/
Clicked a diff, then went back, made sure diff disappeared from Updates
list
Reviewed By: tuomaspelkonen
Reviewers: epriestley, jungejason, tuomaspelkonen
Commenters: epriestley
CC: epriestley, elynde, tuomaspelkonen
Differential Revision: 169
Summary:
Take Tuomas's first diff. Sorry, my bad on diff feedback.
Test Plan:
Load D240353 without any options and make sure 'ignore-trailing'
selected.
Reviewed By: tuomaspelkonen
Reviewers: tuomaspelkonen
CC: epriestley, aran, tuomaspelkonen
Differential Revision: 172
Summary:
'ignore-all' is very confusing, especially for Python, because the indentation
is incorrect.
Test Plan:
Tested loading D240353 without any options and made sure 'ignore-trailing' was
selected and the diff looked correct.
Reviewed By: aran
Reviewers: jungejason, simpkins, aran, dpepper
Commenters: dpepper
CC: grglr, aran, dpepper, tuomaspelkonen
Differential Revision: 171
Summary: add a conduit method to enable querying revisions' phid from
their revision_IDs, and another one to update the task->revision assoc.
Test Plan: for querying revision_phid method, tested empty, one, and two
revisions in the query. For the one to update the task->revision assoc,
I have another diff in facebook which verified it add and remove assoc
correctly.
Reviewers: tuomaspelkonen, epriestley
CC:
Differential Revision: 165
