Summary:
dirname('x') returns '.', not '/'; this caused some issues for repositories with
files at the root.
There are some cases in the parsers where I should probably swap this out too
but I'll wait until I'm doing some more rigorous testing since that stuff is a
bit fragile and this fixes an immediate issue.
Test Plan: Ran unit tests. Viewed a file at root level in a test repository.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, nh
Differential Revision: 932
Summary: See T262. This creates the index on the Differential side which we need in order to execute this query efficiently on the Diffusion side.
Also renames "DiffusionGitPathIDQuery" to "DiffusionPathIDQuery", this query object has nothing to do with git.
Test Plan: Attached top-level and sub-level diffs to revisions and verified they populated the table with sensible data.
Reviewers: bmaurer, aravindn, fmoo, jungejason, nh, tuomaspelkonen, aran
CC:
Differential Revision: 931
Summary:
we use to only add X-Frame-Options for AphrontWebpageResponse.
There some security concern about it. Example of a drag-drop attack:
http://sites.google.com/site/tentacoloviola/. The fix is to add it to
all AphrontResponse.
Test Plan:
View page which disalble this option still works (like the
xhpast tree page); verify that the AphrontAjaxResponse contains the
X-Frame-Options in the header.
Reviewers: epriestley, benmathews
Reviewed By: epriestley
CC: nh, aran, jungejason, epriestley
Differential Revision: 926
Summary: Feedback from @makinde. These are easy (and necessary) to configure so
we might as well give the user a heads up.
Test Plan: Regenerated the documentation and read "Configuration Guide".
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 929
Summary: For reasons explained in the config I've omitted this from the default
action set, but it's trivial to support it. See D916.
Test Plan: Commented on a revision, was informed I could "!accept" in the email.
Used "!accept" to accept the revision.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 928
Summary: This method relies on 64-bit math being available, which isn't a safe
assumption. Use the builtin bc functions instead for arbitrarily large integers.
Test Plan: @skrul, can you apply this locally and let me know if it works?
Reviewers: skrul, hunterbridges, jungejason, nh, tuomaspelkonen, aran
Reviewed By: skrul
CC: aran, skrul, epriestley
Differential Revision: 912
Summary:
This is pretty straightforward, except:
- We need to request read/write access to the address book to get the account
ID (which we MUST have) and real name, email and account name (which we'd like
to have). This is way more access than we should need, but there's apparently no
"get_loggedin_user_basic_information" type of call in the Google API suite (or,
at least, I couldn't find one).
- We can't get the profile picture or profile URI since there's no Plus API
access and Google users don't have meaningful public pages otherwise.
- Google doesn't save the fact that you've authorized the app, so every time
you want to login you need to reaffirm that you want to give us silly amounts of
access. Phabricator sessions are pretty long-duration though so this shouldn't
be a major issue.
Test Plan:
- Registered, logged out, and logged in with Google.
- Registered, logged out, and logged in with Facebook / Github to make sure I
didn't break anything.
- Linked / unlinked Google accounts.
Reviewers: Makinde, jungejason, nh, tuomaspelkonen, aran
Reviewed By: aran
CC: aran, epriestley, Makinde
Differential Revision: 916
Summary: No actual parsing/import yet, but now you can define and pull Mercurial
repositories. I merged most of the local pull code so we can share it between
hg/git.
Test Plan:
- Created a new Mercurial repository to track Codeigniter off Bitbucket
- Edited / saved / etc.
- Launched the mercurial pull daemon, it pulled the repo. Killed and
relaunched, it updated the repo.
- Launched the git fetch deamon, it still works correctly.
Reviewers: Makinde, aran, jungejason, tuomaspelkonen
Reviewed By: Makinde
CC: aran, Makinde
Differential Revision: 793
Summary:
Changed the documentation to describe the project-subproject join table
instead of the task-project join table.
Test Plan:
none
Reviewers:
epriestley, cadamo
CC:
Differential Revision: 927
Summary:
The CSRF changes meant that we can't generate a file URI with just its PHID
anymore, and converted a mathematical function into a service call.
Unfortunately, this caused massive perf problems in some parts of the
application, critically handles, where loading N users became N single gets.
Derp derp derp. Remedy this by doing a single multiget. This substantially
improves performance of many interfaces, particularly the Maniphest task list.
I need to go through the rest of the PhabricatorFileURI callsites and get rid of
them, but I think this is the most substantive one.
Test Plan: Profiled Maniphest task list, queries went from >100 to a handful.
Explosion of multiderp. :/ Looked at some views with profile photos to verify
they still render accurately.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: aran
CC: aran
Differential Revision: 921
Summary: This will get fancier, but here's a basic interface for doing symbol
lookups. Still all pretty tentative.
Test Plan: Looked up various things, got some sensible results.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: tuomaspelkonen
CC: aran, tuomaspelkonen
Differential Revision: 900
Summary: See T315 for an extensive description of this feature. Adds the
descibed storage table.
Test Plan: Used phpsh to read/write symbol objects.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: tuomaspelkonen
CC: aran, epriestley, tuomaspelkonen
Differential Revision: 897
Summary: This got caught in the CSRF filter but is a safe write.
Test Plan: Pasted the URI for a picture of a goat into a diff, saw a goat.
Reviewers: aran, jungejason
Reviewed By: aran
CC: aran
Differential Revision: 910
Summary: See D902. As @abdul notes, a password input is probably more
appropraite here.
Test Plan: Mashed stuff into it, got bullets instead of text.
Reviewers: abdul, jungejason, nh, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, nh
Differential Revision: 913
Summary: phriction.edit allows you to omit the content string, meaning "don't
edit content". If you're also creating the page, we currently break in a
terrible, horrible, no-good, very-bad way because 'content' can't be null.
Default to empty string instead so phriction.edit creates an empty page instead
of a broken mess.
Test Plan: Called phriction.edit on a new page with no content.
Reviewers: skrul, jungejason, nh, tuomaspelkonen, aran
Reviewed By: skrul
CC: aran, skrul
Differential Revision: 920
Summary:
When Diffusion encounters an image file, it displays it as an
image, but when it encounters a PDF file, it currently shows only some
gibberish. This fixes that.
Test Plan:
I tried it. Embedding a large PDF in a data URL is a little
bit slow, but it works.
Reviewers: tuomaspelkonen, epriestley, gc3, waltermundt, jungejason, nh
Reviewed By: epriestley
CC: aran, tuomaspelkonen, epriestley, jaapweel
Differential Revision: 915
Summary:
We need to query the unit status in order to determine if
there are postponed unit tests to update after running "arc unit"
Test Plan:
1) set my conduit uri to a server running the new code
2) ensured unitStatus existed when retrieving a diff
Reviewers: epriestley
Reviewed By: epriestley
CC: dpepper, aran, epriestley
Differential Revision: 918
Summary:
@tomo ran into an issue where he had some non-SSL-only cookie or whatever, so
"Logout" had no apparent effect. Make sure "Logout" really works by destroying
the session.
I originally kept the sessions around to be able to debug session stuff, but we
have a fairly good session log now and no reprorted session bugs except for all
the cookie stuff. It's also slightly more secure to actually destroy sessions,
since it means "logout" breaks any cookies that attackers somehow stole (e.g.,
by reading your requests off a public wifi network).
Test Plan: Commented out the cookie clear and logged out. I was logged out and
given a useful error message about clearing my cookies.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: aran
CC: tomo, aran, epriestley
Differential Revision: 911
- Removed irrelevant csprintf(..)
- Updated code to use $repository->getRemoteURI()
- Updated code to use getRemoteCommandFuture(..) in Diffusion code
- Updated code to use $repository->getRemoteURI()
Summary: I still need to go through all the daemon and Diffusion code and change
the bare execx() calls to $repository->execxXXX() to actually make this work,
but we're getting close.
Test Plan: Configured repositories with various HTTP / SVN setups and ran the
test_connection.php script to verify keys were located and added and
username/password information was supplied.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, nh, jungejason
Differential Revision: 902
setup
Summary: See T481. We'll fail the pcntl test if we don't have this, in a
potentially confusing way. Test and detect missing 'php' explicitly before we
try the pcntl test, so we can give the user a better error message.
Test Plan: In setup mode, did a good run and then faked it to execute 'phpx'
instead to get a failure.
Reviewers: johnduhart, jungejason, tuomaspelkonen, aran
Reviewed By: tuomaspelkonen
CC: aran, epriestley, tuomaspelkonen
Differential Revision: 878
Summary: Added line number 1 for each image and added code to display the
comments for each image.
Test Plan: Adding an image in my local directory and create a revision for it.
Click line number 1, and the comment window prompts. Adding and save the
comment. The comment shows in the differential comment list and in the inline
comment. Submit the comment. Create more comments for the image and the
"Previous" and "Next" buttons all work well.
Reviewers: epriestley, jungejason
CC:
Differential Revision: 901
Summary: Previously, this code accidentally did not use the best URI. Instead,
use the best URI. It's the best, obviously.
Test Plan: Uploaded a binary file and then clicked the preview.
Reviewers: hunterbridges, jungejason, nh, tuomaspelkonen, aran
Reviewed By: tuomaspelkonen
CC: aran, tuomaspelkonen
Differential Revision: 905
Summary:
Move toward storing credentials in configuration so it's easier to get the
daemons working. This should eventually solve all the key juggling junk you have
to do right now.
This only gets us part of the way to actually using these credentials in the
daemons since I have to go swap everything for $repository->execBlah().
I tried to write a web "Test Connection" button but it was too much of a mess to
get git to work since git doesn't give you access to its SSH command and SSH has
a bunch of interactive prompts which you can't really do anything about without
it or a bunch of ~/.ssh/config editing. This is what Git recommends:
https://git.wiki.kernel.org/index.php/GitFaq#How_do_I_specify_what_ssh_key_git_should_use.3F
..but it's not a great match for this use case.
Test Plan:
- Only partial.
- Ran "test_connection.php" on a Git repo with and without SSH, and with and
without valid credentials. This part works properly.
- Ran "test_connection.php" on a public SVN repo, but I don't have private or
WEBDAV repos set up at the moment.
- Mercurial doesn't work yet.
- Daemons haven't been converted yet.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, abdul, nmalcolm, epriestley, jungejason
Differential Revision: 888
Summary:
These fields use auxiliary storage now. Migrate the data and get rid of the
columns in the main table.
- This might take a little while to run, although there are <500k rows so
probably not too long.
- Maybe grab a backup of the table first, if I screwed something up this will
delete the data in these fields.
Test Plan:
- Ran migration locally.
- Browsed Differential.
- Grepped for "revertPlan" and "blameRevision".
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason, epriestley
Differential Revision: 832
Summary:
Give users better errors and UI:
- For subpath SVN repositories, default the path to the subdirectory, not to
"/". This makes the home screen useful and things generally less confusing.
- For unparsed commits, show a more descriptive error message without the
"blah blah" silliness.
- For paths outside of the subpath parse tree, short circuit into an
appropriate error message.
- For foreign SVN stub commits (see D892), show an explicit message.
Test Plan: Looked at unparsed commits, subpath repositories, foreign stub
commits, and paths outside of the subpath parse tree. Received sensible error
messages.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 894
Summary: These queries are safe to run without a CSRF token, and we need them
for the query analyzer in DarkConsole.
Test Plan: "Analyze Query Plans" works again.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, epriestley, nh
Differential Revision: 895
Summary:
In password-based auth environments, there is now a user settings
panel to allow them to change their password.
Test Plan:
Click settings, choose password from the left:
* enter current password, new password (twice), log out, and log in with
new password
* enter current password, non-matching passwords, and get error
* enter invalid old password, and get error
* use firebug to change csrf token and verify that it does not save with
and invalid token
Changed config to disable password auth, loaded settings panel and saw
that password was no longer visible. Tried loading the panel anyway and
got redirected.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 890
references
Summary:
See T325. We tentatively support doing partial subdirectory parses in
Phabricator for Subversion, so you can elect to import only "trunk/local/" or
similar. We do this by importing only some of the commits (those commits which
affected that directory).
In Subversion, you can also "svn cp
svn+ssh://example.com/svnroot/trunk/foreign/example.c@13 local.c". This means
that commits which reference "trunk/local/" may themselves reference foreign
commits.
Currently, we break in this case and can't find the commit reference. Instead,
generate a foreign commit stub so we can at least point at some reasonable
object.
Test Plan: Successfully imported trunk/a/ of the test repo in T325 without
errors. Verified commit 3 in that repo is imported as a foreign stub.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason, epriestley
Differential Revision: 892
repositories
Summary:
This query isn't scoped correctly to the repository ID, so we may identify
commits from other repositories.
This causes a somewhat subtle issue since we only use it to manage file
copies/moves, so you end up with a file "copied from" the same revision in
another repository. I think the UI probably even renders correctly.
Once I finish T325 and better understand what's going on here, I'll see how much
work is involved in writing an SQL patch to fix this.
Test Plan: Parsed the test repo from T325 with the expected error.
Reviewers: jungejason, nh, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 891
Summary:
The differential panels at the top of the differential revision view page
were 2px smaller than the divs on the bottom of the page (everything below
the table of contents). This diff makes differential-panel 2px wider so it
matches.
Test Plan: viewed a differential revision and checked that the divs lined up
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 887
Summary: See T489. Provide slightly more detail so we can figure out if there's
a real issue here.
Test Plan:
Hit URIs like:
/differential/comment/preview/29/
/differential/comment/preview/29/?__ajax__=1
/differential/comment/preview/29/?__csrf__=1
..and got appropriate error messages.
Reviewers: jungejason
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 884
Summary:
oh god everyone hates this
revert revert
https://www.facebook.com/photo.php?fbid=787360256660&set=p.787360256660&type=1&theater
(I left the icons themselves since I have some plans to do other things with
them.)
Test Plan: I am not good at designer
Reviewers: ola, elynde, bh, ashwin, jungejason, kdelong, zrait, tomo, aran
Reviewed By: aran
CC: aran, epriestley, tomo
Differential Revision: 885
Summary:
I didn't realize createDiffDict was a public method when I
modified it, and I broke the API call in getrevision. This moves the
modification inside the method and reverts the method header back to
it's original form.
Test Plan: none
Reviewers: epriestley
Reviewed By: epriestley
CC: edward, aran, epriestley
Differential Revision: 883
Summary:
After D857, we try to attach local commit information to revisions. If this
information is available, display it on the revision.
Design on this is a little rough, I might try to combine this into the revision
update view or something like that since we're starting to take up a lot of real
estate for metadata.
Test Plan: Local diffed this and got some commit info.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, epriestley, jungejason
Differential Revision: 872
Summary: Allow Conduit method so they stop raising lint warnings. See D874.
Test Plan: Ran "arc lint" on conduit files and was no longer given frivolous
warnings.
Reviewers: nh, jungejason, tuomaspelkonen, aran
Reviewed By: nh
CC: aran, nh, epriestley
Differential Revision: 875
Summary: If the link text is a URI, just treat it as a nameable (and possibly
relative) URI link. See tasks.
Test Plan: Copy/pasted the doc example into Phriction, links worked.
Reviewers: skrul, hunterbridges, jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 882
don't show "Undo"
Summary: When a user hits "Reply" on an inline comment, doesn't type anything,
and then hits "Cancel", we incorrectly store the text of the comment the user is
replying to as the "original" text, and then detect that they've changed it when
they immediately cancel. Instead, store empty string as the original text.
Test Plan:
- Hit "Reply" and then "Cancel" on an inline comment. No undo now.
- Hit "Reply", typed some text, and then hit "Cancel". Got an undo which
restored my text.
Reviewers: tomo, jungejason, tuomaspelkonen, aran
Reviewed By: aran
CC: aran, tomo
Differential Revision: 879
Summary: HPHP has behaviorial differences from PHP which make this logic
problematic and we provide a good error message to users when there's a cookie
issue now, so unsplit the cookie logic and just clear the same cookie we'd
otherwise set, as per ssl / base domain.
Test Plan: Logged in and out of my local install.
Reviewers: jungejason
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 876
Summary:
Provide three Phriction methods for programmatic access to Phriction:
- phriction.info: get information about a document
- phriction.history: get change history for a document
- phriction.edit: create or update a document
I moved all the editing/creation logic into a new PhrictionDocumentEditor to
share code between the Conduit and Web edit pathways.
Test Plan: Got info and history via conduit. Edited and created new pages via
conduit and web.
Reviewers: hsb, jungejason, tuomaspelkonen, aran, hunterbridges
Reviewed By: hunterbridges
CC: skrul, aran, hunterbridges
Differential Revision: 866
Summary:
We need createlintresults because we are doing extended
static analysis offline, and thus we need to be able to update the
lint results associated with a diff. This is similar to
updateunitresults, but "create" is more accurate than "update" since
we never need to change existing lint results.
getdiffproperty is used by the client to ensure it isn't creating any
duplicates lint results. It's the symmetric operation to
setdiffproperty, which already exists.
Test Plan:
We have a new offline linter that I used to test. This
linter calls getdiffproperty on every run.
1. Tested updating an existing set of lint results by first running
"arc diff" with lint errors caught by the local linter, then later
running offline analysis which catches one other error and updates via
createlintresults. Ensured the differential lint results were as
expected.
2. Tested the creation of an entirely new diff property through
createlintresults. I first ran "arc diff --nolint" to skip all lint
results, then ran offline analysis which caught an error and updated
through createlintresults. Ensured differential lint results were as
expected.
Reviewers: epriestley
Reviewed By: epriestley
CC: dpepper, aran, mgummelt, jungejason, epriestley
Differential Revision: 868
Summary: Oops, I left this in from an earlier version and missed it since I was
mostly looking at Maniphest for testing. We already render this information in
the header, don't additionally render it under the comments.
Test Plan: derp derp, loaded any revision with sourced comments
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 871
Summary: Execute searches like the primary Maniphest task list. Let me know what
else you guys need from this API.
Test Plan: Executed "maniphest.info" and "maniphest.find"
Reviewers: jungejason, tuomaspelkonen, aran, nh
Reviewed By: nh
CC: blair, skrul, aran, jungejason, epriestley, nh, tuomaspelkonen
Differential Revision: 867
Summary:
When an object is updated, record the content source for the update. This mostly
isn't terribly useful but one concrete thing I want to do with it is let admins
audit via-email replies more easily since there are a bunch of options which let
you do hyjinx if you intentionally configure them insecurely. I think having a
little more auditability around this feature is generally good. At some point
I'm going to turn this into a link admins can click to see details.
It also allows us to see how frequently different mechanisms are used, and lets
you see if someone is at their desk or on a mobile or whatever, at least
indirectly.
The "tablet" and "mobile" sources are currently unused but I figured I'd throw
them in anyway. SMS support should definitely happen at some point.
Not 100% sure about the design for this, I might change it to plain text at some
point.
Test Plan: Updated objects and saw update sources rendered.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, epriestley, jungejason
Differential Revision: 844
Summary: This may make it easier to debug problems with CLI + pcntl
Test Plan: Added a syntax error to the script and got more useful output
Reviewers: johnduhart, jungejason, tuomaspelkonen, aran
Reviewed By: johnduhart
CC: aran, johnduhart
Differential Revision: 869
Summary: When a user stores the empty string in an auxiliary field, simply don't
store it, and delete it if it already exists.
Test Plan: Edited a revision with an empty "Quack" field, got an empty row in
the DB. Applied patch, edited empty again, row went away. Edited empty again,
still no row. Edited and put something in the field, got a row.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 865
Summary: "set -e" causes the script to exit if any command returns nonzero.
Without it, we incorrectly discard the failure code. At Facebook everything runs
in "set -e" or some equivalent so I never picked this up in testing.
Test Plan:
Added the hook to my svn local, it blocked bad commits and allowed good ones.
>>> orbital:~/devtools/svnroot $ svn commit -m "quaa"
Sending test.php
Transmitting file data .svn: Commit failed (details follow):
svn: Commit blocked by pre-commit hook (exit code 1) with output:
LINT ERRORS
This changeset has lint errors. You must fix all lint errors before you can
commit.
You can add '@bypass-lint' to your commit message to disable lint checks for
this commit, or '@nolint' to the file with errors to disable lint for that
file.
>>> Lint for test.php:
Error (XHP1) PHP Syntax Error!
This file contains a syntax error: XHPAST Parse Error: syntax error,
unexpected '}' on line 1
>>> 1 <?php asdddddd;!}}
Reviewers: svemir, jungejason, tuomaspelkonen, aran
Reviewed By: svemir
CC: aran, svemir, epriestley
Differential Revision: 864
Summary: cpiro reported a cache inconsistency issue from a push a while ago
which this should fix (see #?????), and we haven't sync'd in a while anyway.
Test Plan: Poked some interfaces very gently.
Reviewers: cpiro, cpojer, tomo, jungejason, tuomaspelkonen, aran
Reviewed By: tomo
CC: aran, epriestley, tomo, cpiro
Differential Revision: 859
Summary:
It is possible to view a comment that has no cache; when viewing such a comment
the request doesn't have a csrf token and there is no need for one, so we turn
off the write guard.
Test Plan:
loaded an old diff that had no cache, and the page loaded instead of throwing
an AphrontCSRFException.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 858
Summary: Quora wants to handle some moderation tasks with Phabricator, but want
to lower the barrier to entry for the install and let moderators adopt it
gradually. One request is to allow auth rules to be relaxed so we can auth based
on Reply-To to make things easier. This is insecure if configured but not really
a big deal and the patch isn't big or complicated.
Test Plan: Sent a test email with bogus "From" but valid "Reply-To". It was
rejected with this setting off, and allowed with this setting on.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 842
Summary: Some stack frames do not have file/line information, e.g. __autoload
triggers. Render these as "Internal".
Test Plan: Reloaded a trace with an internal __autoload() frame, got
"(Internal)" instead of ": 0" with warnings.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 843
Summary: Do JOIN in the protocol handler, after we receive 376 ("end of motd").
Test Plan: Ran bot, it joined a channel after receieving a 376 command.
Reviewers: moos3, codeblock, aran, jungejason, tuomaspelkonen
Reviewed By: moos3
CC: aran, moos3
Differential Revision: 855
Summary: Delete one line which has no effect.
Test Plan: Open revision page to make sure it still works.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 852
Summary: Instead of just saying a task is "Closed", say "Resolved", "Wontfix",
etc.
Test Plan: Looked at task list view, saw "Resolved", "Wontfix", etc.
Reviewers: skrul, hunterbridges, jungejason, tuomaspelkonen, aran
Reviewed By: skrul
CC: aran, skrul, jungejason, epriestley
Differential Revision: 851
Summary: If a user partially discovers a repository and then deletes it, the
timeline will have events from the old repository which this daemon won't be
able to parse.
Test Plan: @ajtrichards, can you apply this locally and restart your daemons
(##phd stop##, then relaunch them) and let me know if it fixes the issue?
Reviewers: ajtrichards, jungejason, tuomaspelkonen, aran
Reviewed By: ajtrichards
CC: aran, epriestley, ajtrichards
Differential Revision: 845
Summary:
When selecting children of a directory, it is possible that none of its
children exist anymore even though the directory still exists. After fetching
the children but before returning them, we should check whether there are any,
and if there are no children, set the reason as empty directory.
Test Plan:
In sandbox, browsed in diffusion to a directory that exists but has no
files and saw that it has a useful message instead of a vague exception.
Reviewers: epriestley, tuomaspelkonen, jungejason
Reviewed By: tuomaspelkonen
CC: aran, tuomaspelkonen
Differential Revision: 846
"user":"authenticationusername",
"pass":"thisuserspassowrd",
This will allow people with internal irc servers to use this if they control access from ldap for irc.
Summary:
This allows you to configure a single mailbox for all mail sent by phabricator,
so you
can keep a mailaddress like bugs@example.com and don't need a catchall on your
domain/subdomain.
Test Plan:
Enabled and disabled suffix. Saw mails generated have to correct prefix. Also
piped raw mails
into the scripts/mail/mail_handler.php and ensured comments went into
phabricator for both maniphest
and differential.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 815
Summary: This could be a lot fancier but let's see what else we need. Also fixed
some bugs with maniphest.info.
Test Plan: Used the Conduit web console to create some tasks with different
values.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason, epriestley
Differential Revision: 824
purposes
Summary:
Browsers send port numbers (like ":443" or proxy ports) in the Host header and
we'll currently reject them with a message like:
> Blah is configured on "x.y.com" but you are accessing it on "x.y.com:443".
Instead, examine only the host part.
Test Plan: Had my local listen on port 81 and accessed Phabricator before/after
the change; it now works without throwing.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, epriestley, abdul, jungejason
Differential Revision: 841
Summary: Open AphrontWriteGuard for user login.
Test Plan: verified that the user can log in.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, epriestley
Differential Revision: 840
Summary: When a user has bad cookies, try to clear everything and tell them they
might need to manually clear things.
Test Plan: Added "&& false" to the valid branch and got the exception message.
Reviewers: jungejason
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 839
Summary:
After D814 and D829, you should be able to implement this logic in the
didWriteRevision() method of the field.
Note that the attacher is still referenced in
ConduitAPI_differential_updatetaskrevisionassoc_Method. This method should
probably be moved to facebook/ since it's pretty Facebook-specific.
No rush on any of this, it's not hurting anything.
Test Plan:
- Hit differential.getcommitmessage
- Ran 'arc diff'
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 830
Summary:
In D758, I tightened the scope for which we issue cookies. Instead of setting
them on the whole domain we set them only on the subdomain, and we set them as
HTTPS only if the install is HTTPS.
However, this can leave the user with a stale HTTP cookie which the browser
sends and which never gets cleared. Handle this situation by:
- Clear all four <domain, https> pairs when clearing cookies ("nuke it from
orbit").
- Clear 'phsid' cookies when they're invalid.
Test Plan: Applied a hackier version of this patch to secure.phabricator.com and
was able to login with a stale HTTP cookie.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 838
Summary:
This commit lets users who are filing Maniphest tasks to attache files to them
right off the bat.
Test Plan:
{F3545}
and
{F3546}
Reviewers: epriestley, fmoo, aran
CC:
Differential Revision: 837
Summary:
Remove the blame revision, revert plan and lines fields from the default field
loadout. (After D829 this doesn't cause issues where we have bogus dictionary
entries.)
You should add these back to the Facebook configuration since Facebook wants
these fields. However, I want to keep the default stack very light and I never
saw a huge amount of value in these fields at Facebook so I don't think they
make the cut. Sorry, tomo. ;_;
Test Plan: Ran "arc diff" locally.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: aran
CC: aran, tomo, epriestley
Differential Revision: 831
Differential comments
Summary: If you @mention several users, at least one of which is already CC'd,
we unset all the CCs and don't attach the "Added CCs: ..." block to the comment.
Test Plan: @mentioned two users, one of whom was already CC'd.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason
Differential Revision: 827
Summary:
I think this is the last major step -- use the fields to parse commit messages,
not a hard-coded list of stuff. This adds two primary methods to fields, one to
get all the labels they'll parse (so we can do "CC" and "CCs" and treat them as
the same field) and one to parse the string into a canonical representation
(e.g., lookup reviewers and such).
You'll need to impelement the one block of task-specific stuff I removed in
Facebook's task field:
list($pre_comment) = split(' -- ', $data);
$data = array_filter(preg_split('/[^\d]+/', $pre_comment));
foreach ($data as $k => $v) {
$data[$k] = (int)$v;
}
$data = array_unique($data);
break;
Otherwise I think this is clean.
Test Plan:
- Called the conduit method with various commit messages, parsed fields/errors
seemed correct.
- "arc diff"'d this diff onto localhost, then updated it.
- "arc amend"'d this diff.
Reviewers: jungejason, tuomaspelkonen, aran
Reviewed By: jungejason
CC: aran, jungejason, epriestley
Differential Revision: 829
Summary: While I thought this was complicated, there was nothing subtle or
tricky here -- I just misnamed a variable.
Test Plan: Created a revision with default CCs, got CCs instead of nothing.
Reviewers: aran, jungejason, tuomaspelkonen
Reviewed By: aran
CC: aran
Differential Revision: 834
Summary:
deprecate generateProperties() from class
DifferentialRevisionDetailRenderer. Custom fields now provides a much
more powerful version of generateProperties().
Depends on D814.
Test Plan:
implemented facebook task field with custom field and
verified it worked.
Reviewers: epriestley, tuomaspelkonen
Reviewed By: epriestley
CC: aran, jungejason, epriestley
Differential Revision: 826
Summary:
When rendering commit messages, drive all the logic through field specification
classes instead of the hard-coded DifferentialCommitMessageData class. This
removes DifferentialCommitMessageData and support classes.
Note that this effectively reverts D546, and will cause a minor break for
Facebook (Task IDs will no longer render in commit messages generated by "arc
amend", and will not be editable via "arc diff --edit"). This can be resolved by
implementing the feature as a custom field. While I've been able to preserve the
task ID functionality elsewhere, I felt this implementation was too complex to
reasonably leave hooks for, and the break is pretty minor.
Test Plan:
- Made numerous calls to differential.getcommitmessage across many diffs in
various states, with and without 'edit' and with and without various field
overrides.
- General behavior seems correct (messages look accurate, and have the
expected information). Special fields like "Reviewed By" and "git-svn-id" seem
to work correctly.
- Edit behavior seems correct (edit mode shows all editable fields, hides
fields like "Reviewed By").
- Field overwrite behavior seems correct (overwritable fields show the correct
values when overwritten, ignore provided values otherwise).
Reviewed By: jungejason
Reviewers: jungejason, tuomaspelkonen, aran
CC: aran, jungejason
Differential Revision: 814
Summary:
Make the unhandled exception dialogs slightly more useful:
- Make them easier to read.
- Link to files from Phabricator libraries.
- Don't show traces by default.
- Show traces in development mode.
- Rename button from "Cancel" to "Close" and only show it for Ajax.
Test Plan: Rigged DirectoryHomeController to throw, loaded home page. Changed
stack trace setting in config. Clicked some files in the trace.
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen, codeblock
CC: aran, epriestley
Differential Revision: 823
Summary:
See T354. List every rule which has ever been applied in X-Herald-Rules, not
just the ones which most recently triggered.
Also some random fixes while I was debugging this:
- When conduit methods throw non-conduit exceptions, make sure they get
logged.
- Trigger the Facebook "tasks" backcompat block only if we were going to fail
(this should reduce the shakniess of the transition).
- Fix some log spew from the new field stuff.
Test Plan:
- Created a rule (ID #3) "No Zebras" which triggers for revisions without
"zebra" in the title.
- Created a revision without "zebra" in the title, got X-Herald-Rules: <2>,
<3>
- Updated revision to have "zebra" in the title, verified rule did not trigger
in Herald transcript.
- Verified X-Herald-Rules is still: <2>, <3>
Reviewed By: aran
Reviewers: aran, jungejason, tuomaspelkonen
CC: aran, epriestley
Differential Revision: 817
