Summary:
If the actual commit message has a duplicate field and we shouldAutoclose it then the commit message parser fails.
Put the error in `$errors` instead.
Test Plan:
Reparsed commit with duplicate field in message.
Tried to `arc diff` message with duplicate field.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3470
Summary:
Including view in permalink seemed like the correct solution to me.
Instead, I delete it all the time when copying somewhere else.
Test Plan:
Clicked on the link.
Changed view on permalink.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3453
Summary:
Owners packages might include repositories that are no longer tracked
(or checked out locally), and there's no reason why we need a working copy
to generate a diffusion link. Instead of displaying a red error box, this
diff allows the owners tool to render correctly.
Test Plan:
viewed /owners/view/all/ and /owners/package/395/ and verified no error box
for not having a checkout of the repository.
Reviewers: epriestley, vrana, wez
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3452
Summary:
In the commit message parser worker, it tries to get relationships for a
revision without first loading them. (D3444 introduced the change.) This
is where the get happens, so I'm adding the load here - maybe it should be
added in the CommitMessageParserWorker instead?
Test Plan: Made this change, and my taskmaster daemons stopped failing.
Reviewers: epriestley, vrana
Reviewed By: vrana
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3448
Summary:
This is mostly ripped from D2721, but doesn't implement the T945 part.
After we parse a commit message, give DifferentialFieldSpecifications an opportunity to react to the message as well (e.g., by updating related objects).
Test Plan: Impelmented a var_dump() body, ran `reparse.php` on a commit.
Reviewers: vrana, 20after4, btrahan, edward
Reviewed By: 20after4
CC: aran
Maniphest Tasks: T945, T1544
Differential Revision: https://secure.phabricator.com/D3444
Summary:
Without this, the https redirect doesn't work if you're not logged in, because
the login check in willBeginExecution happens before the redirect controller
can redirect. This also has the unpleasant effect of the login page on http
(when it should have redirected to https) not having any css or js.
Test Plan:
With the https redirect enabled, loaded phabricator without being logged in,
and saw that I got redirected to the https login page instead of seeing a
http login page.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3438
Summary: AphrontSideNavView is oldschool and I want to kill it.
Test Plan: Viewed Ponder, clicked both nav options.
Reviewers: pieter, vrana, btrahan
Reviewed By: pieter
CC: aran
Differential Revision: https://secure.phabricator.com/D3430
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:
We have lots of info about unit tests.
This allows linking them from Unit field.
Test Plan: Monkey patched `$test['link']`, clicked on it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3434
Summary:
I've replaced all `id(new PhabricatorObjectHandleData(...))->loadHandles()` by `$this->loadViewerHandles(...)`.
Lint caught one usage in a static method.
Test Plan: Displayed revision with sporadic author.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3432
Summary: I've done D3432 in the hope that it will fix also this...
Test Plan: Blamed sporadic author.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3433
Summary: Rehash of D3411. In cgi/fcgi setups we have no idea if the request is HTTP or HTTPS as far as I can tell, so make this config-triggered again. Also handle @vrana's "off" case.
Test Plan: Set this flag, observed redirect to https when `$_SERVER['HTTPS']` was absent.
Reviewers: nh, vrana
Reviewed By: nh
CC: aran
Differential Revision: https://secure.phabricator.com/D3420
Summary:
See discussion in D3340. Some configurations set only a search attribute because their records are indexed by username (this is probably not quite the correct LDAP terminology). Other configurations use one attribute to search and a different attribute to select usernames.
After D3340, installs which set only a search attribute broke. Instead, fall back to the search attribute if no username attribute is present.
Test Plan: Successfully logged in on my test slapd.
Reviewers: yunake, voldern, briancline
Reviewed By: voldern
CC: aran
Differential Revision: https://secure.phabricator.com/D3406
Summary: It would be best to add this everywhere at once but I'm too lazy for it.
Test Plan: Displayed package list and detail and revision comment with away users.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3419
Summary:
Currently, if some page /x/ has a grandchild at /x/y/z/ but /x/y/ does not exist, we insert a ghost entry for it in the "Document Hierarchy". However, we don't sort the hierarchy properly after inserting ghosts, so they always appear at the bottom and in an unuseful order.
Instead, sort children again after any ghost insertions.
Test Plan: Created /x/a/, /y/a/, /z/a/, verified ghosts sorted incorrectly before this patch and correctly afterward.
Reviewers: btrahan, vrana
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1750
Differential Revision: https://secure.phabricator.com/D3418
Summary: Replaces the full names after D3413.
Test Plan: See screenshot.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3414
Summary:
There is basically no reason for anyone to ever use the uncollapsed mode for more than the first 2 minutes of using the tool.
Delete all code related to collapse/expand.
(I'm going to add tooltips next.)
Also move the drag bar a few pixels to the right, so it does not overlap with the scrollbar on the "local" nav if there is one.
Test Plan: Viewed in desktop/tablet/phone modes.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3413
Summary:
See some discussion in D1673.
- There's a concrete (if minor) problem with this in Firefox with wrapping search.
- People complain about how we're stealing all their pixels.
- There isn't much of a functional purpose to it since all the operations are fairly rare.
- This addresses the aesthetic purpose of the fixed-position nav (not making the side nav ugly) by making the side nav scroll up 44px and then stop.
Test Plan: Scrolled in desktop, tablet modes.
Reviewers: vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D3412
Summary: We need to do something here.
Test Plan: None.
Reviewers: epriestley, nh
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3415
Summary:
If the phabricator.base-uri is set up with https and phabricator receives
a request that isn't over ssl, we will issue a redirect response to the user
to force them to use https.
Test Plan:
With this disabled, verified that pages still load correctly over http.
Enabled it; verified I get redirected to the same path but on https when I
make an http request; verified https requests get served without a redirect.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3411
Summary: Previously, the identification string was thrown at the server long before you were connected, I've moved this to the end of the motd raw, and now errthangz gud
Test Plan: Register an account for your bot to use, give your bot the correct nick and password, then watch
Reviewers: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D3410
Summary: This isn't very obvious, provide some more specific instructions.
Test Plan: Followed the instructions on my Windows machine, got a working `php`.
Reviewers: btrahan, vrana, jungejason
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D2555
Summary:
Some fields require displayed diff (e.g. Lines), some require last manual diff (Lint, Unit).
Lint requires both - it needs to find if the line with the error is displayed or not.
Test Plan: Displayed committed diff with lint errors, clicked on them, got last manual diff at the correct line.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3331
Summary:
Ball is more obvious and visible than I thought.
Delete the status word and display until date in title.
Also display the until date in revision list.
Also display near future dates with DoW instead of year.
Test Plan: Displayed revision and revision list with away reviewers.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3407
Summary:
We have some complaints on this feature:
- It's not clear that the displayed code is context from gap.
- It would be better if the context would be displayed with its real indentation.
- It's not clear how far the context is from the displayed code.
- Links revealing gap aren't on consistent place.
This solves all these problems and introduces new one:
It now seems that the reveal links works only with the left side.
Anyway, I think that this is better overall.
I don't want to put the context on a separate line to not waste space.
Test Plan: Displayed various contexts, revealed context.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, bh, jwatzman
Differential Revision: https://secure.phabricator.com/D3404
Summary: the former is self explanatory. the latter is necessary for installations that require email verification. since many system agents are given bogus email address there can become a problem where these accounts can't be verified
Test Plan: created system agent account from scratch. edited user and toggled system agent accountness. created system agent with unverified email address and verified it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1656
Differential Revision: https://secure.phabricator.com/D3401
Summary:
I have a chain of revisions sometimes.
I want to see also revisions depending on me.
Test Plan: Displayed revision in the middle of chain.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3399
Summary: this is useful for certain workflows, typically where the reviewer is a gatekeeper of sorts who does the acutal commit. Special thanks to D2900 which made this relatively brain-dead to code up.
Test Plan: set to "true" in my local development environment and verified test user "xerxes" could close my stuff
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1732
Differential Revision: https://secure.phabricator.com/D3398
Summary: said differently, if the user included another to address or one or more cc's, don't send the error message email.
Test Plan: played around in the metamta test console and verified that blank replies generated the error handler.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1643
Differential Revision: https://secure.phabricator.com/D3345
Summary:
Currently, CelerityController extends AphrontController, not PhabricatorController. (I think I imagined Celerity being somewhat stand-alone and didn't want to create a dependency.)
This creates a concrete problem if a static resource is missing, since we throw an exception, but the higher-level exception handlers depend on the User existing in order to show an appropriate response page. This is the only controller which doesn't extend PhabricatorController, and it doesn't seem worthwhile to make a weird edge case out of it.
Specific repro case is:
- Remove `externals/javelin/` (or forget to run `git submodule update --init`).
- Load a static resource.
- Get "[Rendering Exception] Argument 1 passed to PhabricatorMainMenuView::setUser() must be an instance of PhabricatorUser, null given, called in /services/apache/phabricator/phabricator/src/view/page/PhabricatorStandardPageView.php on line 435 and defined"
Test Plan:
- Followed above steps, no more fataling.
- Verified this is the only weird controller.
Reviewers: voldern, vrana, btrahan
Reviewed By: voldern
CC: aran
Differential Revision: https://secure.phabricator.com/D3389
Summary:
We have `max_allowed_packet` 1 GiB but our replication dies if the query is longer than unknown value (it dies with 293 MB long query).
Anyway, there's no reason why we should not save the cache if you have small `max_allowed_packet`.
Test Plan: Lowered `$size` to 100, deleted cache from DB, displayed changeset, verified issued queries in DarkConsole, verified DB.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3390
Summary:
I like systems that just work. It is possible to store files larger than max_allowed_packet in MySQL and we shouldn't demand it.
It also fixes a problem when file was smaller than `storage.mysql-engine.max-size` but its escaped version was larger than `max_allowed_packet`.
Test Plan: Reduced the size to 5e4, uploaded 90 kB file, checked the queries in DarkConsole, downloaded the file.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3392
Summary: Also allow left nav to hide.
Test Plan:
# Resize left nav.
# Shrink browser width to switch device.
# Increase the width again.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3383
Summary: In my haste, I forgot a trailing ?
Test Plan: Try both "Where is Derp?" and "Where in the world is Derp?"
Reviewers: epriestley
CC: aran
Differential Revision: https://secure.phabricator.com/D3387
Summary: We recently opted for 'security.alternate-file-domain' and we have some hotlinks to the original domain.
Test Plan: Enabled 'security.alternate-file-domain', observed redirect.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3380
Summary:
This does a few things:
- Allows you to flag pastes. This is straightforward.
- Allows Applications to register event listeners.
- Makes object action lists emit a 'didrenderactions' event, so other applications can add more actions. The Flags application injects its action in this way. This should generally make it much easier to add actions to objects when we add new applications, with less code duplication and better modularity. We have a really hacky version of this in Differential that I want to get rid of in lieu of this more general approach. I'm going to make object lists do the same thing, so any application can jump in and add stuff.
Test Plan: Flagged and unflagged pastes. Viewed home page, differential, flags list.
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D3377
