Summary:
Port PHPMailer options. Also:
- Don't show values on config lists if they're masked (this is mostly for passwords, to prevent them from being idly/accidentally disclosed).
- Don't show "default" icon -- just show an icon if the value has been customized. This makes it easier to pick out custom values.
Test Plan: Looked at / edited mailer values.
Reviewers: codeblock, btrahan
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4441
Summary:
The flow was off in some cases. This should mitigate it.
Adds a Unicode musical note character at the start of the rap which,
when clicked, embeds a song in the background, hides it and plays it.
Currently the song is Gangsta's Paradise (karaoke version) but if you'd
prefer a different song, I can probably change it with only a few weeks
of work.
This doesn't respect the "embed youtubes" preference because you have to
click something to embed it, so it's your own fault your referers are
getting leaked.
For now, does the simplest thing and doesn't loop it. If it turns out
people are spending a lot of time on this page, we should look into
doing something like youtuberepeater.
Not trying to make this share code with the existing Youtube embedding
stuff - I think they're doing different enough things that solving them
both in the same way would be more code.
Test Plan:
Clicked the note in Firefox. Clicked the note in Chrome.
Considered clicking the note in Safari and Internet Explorer.
Reviewers: epriestley
Reviewed By: epriestley
CC: miorel, HarrisonW, aran, Korvin
Differential Revision: https://secure.phabricator.com/D4437
Summary:
- The order of operations for file changes is wrong. We need to wrap them in a table, then render the changeset talbe around that table.
- Line data was being set to late, so renderShield() got the wrong line count and rendered empty diffs behind, e.g., generated changes.
Test Plan: Looked at an image change with property changes. Clicked "show file contents" on a generated file.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D4432
Summary: One setting.
Test Plan:
- Looked at the setting in the web interface.
- Waved to it
- Introduced myself
- Had a nice conversation
- Made a new friend
- Turns out `phriction.enabled` is a nice guy.
Reviewers: epriestley, chad
Reviewed By: chad
CC: aran, Korvin
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4436
Test Plan:
Made getTokenStatus() return the four legal values and an
illegal one, looked at the linked account page and confirmed that the
rap rendered and had correct meter
Reviewers: epriestley
Reviewed By: epriestley
CC: miorel, aran, Korvin
Differential Revision: https://secure.phabricator.com/D4433
Summary: Still working through basic re-design. Adds the ability to re-use panel view without the background.
Test Plan: Viewed Diffusion and Differential in Chrome, FF, Safari.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4430
Summary: You can't interact with them yey, but do a less awful job of rendering them.
Test Plan: {F29204}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D4426
Summary:
- Add a query parameter to select the diff renderer.
- When we populate diffs, use the 1-up renderer if the device isn't a desktop. Don't switch renderers once the page has loaded since it will be a tremendous mess. We can add an "always use 1-up" preference later.
- Tweak JX.Device so we avoid a race condition, where `JX.Device.getDevice()` may not be valid when other behaviors run.
- Implement enough of the 1-up HTML renderer to provide a vague hint that it actually works.
- Fix a couple of bugs with primitive construction.
Test Plan:
{F29168}
{F29169}
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D4423
Summary: The "line count" is always the same as count($this->getOldLines()), and somewhat misleading since it's really "number of rows in the two-up view". D4421 removed the only (or only remaining?) call.
Test Plan: looked at some diffs
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D4422
Summary:
This is a half-step toward one-up and test renderers. This is mostly a structural change, and has no user-facing impact. It splits the rendering hierarchy like this:
- Renderer (more methods are abstract than before)
- HTML Renderer (most HTML stuff has moved down from the base to here)
- HTML 1-up (placeholder only -- not yet a functional implementation)
- HTML 2-up (minimal changes, uses mostly old code)
- Test Renderer (unit-testable renderer base, implements text versions of the HTML stuff)
- Test 1-up (selects 1-up mode for test rendering)
- Test 2-up (selects 2-up mode for test rendering)
Broadly, I'm trying to share as much code as possible by splitting rendering into more, smaller stages. Specifically, we do this:
- Combine the various sorts of inputs (changes, context, inlines, etc.) into a single, relatively homogenous list of "primitives". This happens in the base class.
- The primitive types are: old (diff left side), new (diff right side), context ("show more context"), no-context ("context not available") and inline (inline comment).
- Possibly, apply a filtering/reordering step to the primitives to get them ready for 1-up rendering. This mostly removes information, and does a small amount of reordering. This also happens in the base class.
- Pass the primitives to the actual renderer, to convert them into HTML, text, or whatever else. This happens in the leaf class.
The primitive implementation is not yet complete (it doesn't attach as much information to the primitives as it should -- stuff like coverage and copies), but covers the basics.
The existing HTMLTwoUp renderer does not use the primitive path; instead, it still goes down the old path.
Test Plan: Ran unit tests, looked at a bunch of diffs.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D4421
Summary:
First pass at testing out a dark sidebar everywhere. Wanting feedback with real test time before implementing before commiting.
Thoughts
- Aligns with Mobile, Tablet UI experience.
- Creates 'application' feel on Desktop.
- Begins to make Phabricator feel like a branded UI.
Cons
- Probably contensious visually.
TODO:
- Update diff view sidebar.
- Make breadcrumbs appear above content area, not above nav.
- Change background texture on crumbs to match table headers.
Test Plan: Testing Nav with fellow co-workers.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4427
Summary: If there are items (like labels) without keys, we incorrectly select the first item with no key in response to a `selectFilter(null)` call.
Test Plan: Called `selectFilter(null)`, didn't get a selected label.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4428
Test Plan: Get feed.query with a Phriction story
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4417
Summary: This removes all calls to addSpacer and the method. We were applying it inconsistently and it was causing spacing issues with redesigning the sidenav. My feeling is we can recreate the space in CSS if the design dictates, which would apply it consistently.
Test Plan: Go to Applications, click on every application.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4420
Test Plan: Went to /differential.
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4415
Summary: I changed this a long time ago probably without knowing that this format is usable in Remarkup.
Test Plan: Viewed revision and task.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4413
Summary: FF treats input height different than other browsers, removes that case.
Test Plan: Test inputs in FF, Chrome, Safari.
Reviewers: vrana
Reviewed By: vrana
CC: aran, epriestley
Differential Revision: https://secure.phabricator.com/D4406
Summary:
- Move `prettyPrintJSON()` and make it static.
- Use it from `PhabricatorSetupIssueView`
- Update other `config/` places that use it to call it from the new class.
This fixes a bug in `PhabricatorSetupIssueView` which showed up if the value
was an array and couldn't be rendered by `phutil_escape_html()`.
Test Plan:
- Rendered some config options.
- Went to /config/issue/config.unknown.phame.skins/ without error.
Reviewers: epriestley, btrahan, chad
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4411
Summary:
Fixes T181. I actually have no idea what the original issue in T181 was, but this fixes several problems:
- The code for figuring out whitespace-only changes was extremely confusing and probably buggy (the code for figuring out unchanged files was equally confusing but probably less buggy).
- When rendering a whitespace shield, we did not offer a "Show Changes" link. Instead, show the "Show Changes" link: we can always render the content beneath this link.
- When clicking "Show Changes", we used the current whitespace mode, which might result in a diff with no changes. Instead, force "show all" whitespace mode.
- We never offered a "show changes" link for unchanged files. Instead, offer this link if we can service it.
- When clicking "Show Changes", we pierced the shield but didn't force file content, which would fold the entire file even if it was available. Instead, force the file content.
Test Plan: Generated whitespace-only and no-change diffs. Clicked shield links, or verified no shield link available in no-change-with-no-content. Generated an "@generated" change, verified shield worked correctly.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T181, T2009
Differential Revision: https://secure.phabricator.com/D4407
Summary:
Simplify whitespace-ignoring diffing:
- Currently, we generate two diffs (one ignoring whitespace, one normal) and then copy the //text content// from the normal one to the whitespace-ignoring one.
- Instead, copy the //change types// from the ignoring one to the normal one.
- This is cheaper and much simpler.
- It means that we have the right change types in reparseHunksForSpecialAttributes(), and can simplify some other things.
Simplify whitespace changes, unchanged files, and deleted file detections:
- Currently, we do this inline with a bunch of other stuff, in the reparse step.
- Instead, do it separately. This is simpler.
Simplify intraline whitespace handling:
- Currently, this is a complicated mess that makes roughly zero sense.
- Instead, do it in reparse in a straightforward way.
Partially fix handling of files changed only by changing whitespace.
Partially fix handling of unchanged files.
Test Plan:
- Ran unit tests.
- Created context-less diffs, verified they rendered reasonably.
- Generated a diff with prefix whitespace, suffix whitespace, intraline whitespace, and non-whitespace changes. Verified changes rendered correctly in "ignore most" and "show all" modes.
- Verified unchanged files and files with only whitspace changes render with the correct masks.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D4398
Summary:
Depends on D4392.
I don't plan on adding these to changesets.
Test Plan: Added `$message['locations'] = array(array('line' => 10));` in the code and displayed a revision with lint problems.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4394
Summary:
This is useful for writing feedback - what did I work on with someone?
This creates `AND IN (...)` and not `AND AND AND` if more participants are specified.
User may not expect it but whatever, it works the same for the most common case of 1 extra participant.
It would be nice to have this also for other filters but it would by way harder.
Test Plan: Displayed my revisions with some specific reviewer and some elses revisions with me as reviewer.
Reviewers: epriestley
Reviewed By: epriestley
CC: wez, aran, Korvin
Differential Revision: https://secure.phabricator.com/D4408
Summary:
I want to check the work of several people (bootcampers) at once.
This implements the OR filter required for that.
In the next diff, I plan to implement also the AND filter which can be useful too.
Test Plan:
Searched for several people, clicked all filters.
Searched for one person, verified that pretty URL is still created.
Reviewers: epriestley
Reviewed By: epriestley
CC: wez, aran, Korvin
Differential Revision: https://secure.phabricator.com/D4404
Summary:
My claims that this worked on D4183 were incorrect:
- Make it work.
- Then use it.
- Also make cancel URI not cancel to the same page.
Test Plan: Clicked "Create Document", got a workflow on-page dialog instead of a page transition.
Reviewers: codeblock
Reviewed By: codeblock
CC: aran
Differential Revision: https://secure.phabricator.com/D4403
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: Normalizes the font-size for readability, consistent padding to the header, more prominence on the date dividers.
Test Plan: Reviewed in sandbox. http://phab1.pushlabs.net/feed/
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4402
Summary:
- Remove 'missingNew', etc. It's impossible for a diff to popluate these, as far as I can tell (I can't generate such a diff, or find any which generate it).
- Add unit tests.
Test Plan: Unit tests, viewed a diff with some missing context.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D4393
Summary:
Fixes the two-level nav issue introduced by D4376.
(My claim that this page is device ready in the code is something of a lie, but it's fairly close.)
(@chad, this could use an icon at some point, or you can point me at which one you want and I can take a stab at slicing it.)
Test Plan: Looked at feed; saw it not-broken. Also checked public feed (which should just merge at some point).
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4381
Summary: Simple change to stop exceptions from being thrown based on the diff in D4146.
Test Plan: arc call-conduit repository.create
Reviewers: epriestley, btrahan
Reviewed By: btrahan
CC: vrana, aran, Korvin
Maniphest Tasks: T2268
Differential Revision: https://secure.phabricator.com/D4342
Summary:
This pushes the rendering of feed stories out of IRCDifferentialNotificationHandler and into feed.query
Also fixes bug in feed.query that broke attachment of related objects to stories.
Test Plan: echo '{"view": "text", "limit": 10}' | arc call-conduit feed.query
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, codeblock
Differential Revision: https://secure.phabricator.com/D4390
Summary:
T2255 lists it as "???" and we don't have a "Misc" category and it seems silly
to make it for one option, so stick this in core for now.
Test Plan: Went to the setting page and saw the bool options.
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4389
Test Plan: Quick view of each option in the web interface.
Reviewers: epriestley, btrahan, chad
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4386
Summary: D4351 swapped this from a map to a list, and then stopped it from getting to the ChangesetParser so it didn't make it to the Renderer.
Test Plan: Ran "arc diff", copy/pasted it into the web UI. Saw a diff with "context not available" blocks in between missing contexts.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D4384
Summary: Adds a slight blue border, radius, and shadow to inputs and textareas (small). Better padding for legibility as well.
Test Plan: Review a number of forms
Reviewers: epriestley, vrana, btrahan
Reviewed By: btrahan
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4383
Summary: This won't win any awards, but makes User and Project profile pages significantly less broken in the wake of D4376.
Test Plan:
{F28858}
{F28859}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4382
Summary: I forgot to "set it" from the changeset parser.
Test Plan: made a diff with silly whitespace changes. toggled the various whitespace modes and got appropriate results
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D4378
Summary: introducing a new friend called DifferentialHunkParser. Sort of like the DifferentialChangesetParser but works with hunks only. tried to grab hunk parsing type things from across the code base and move them into this new class.
Test Plan: unit tests and played around in Differential a bit.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D4351
Summary: Font size globally is 13px, but form elements were 12px and harder to read. This helps readability and consistency with form elements.
Test Plan: Checked various forms, left comments.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4377
Summary: Move all navs to use the newer-style, darker, textured look. I'm //pretty// sure this doesn't break anything.
Test Plan: Looked at a bunch of apps.
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4376
Summary: Missed these in D4357.
Test Plan: Meh.
Reviewers: epriestley, chad
Reviewed By: chad
CC: aran, Korvin
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4371
Summary:
Rather than throwing if we don't `setOptions()`, let's just default to `true`
and `false`.
Test Plan:
Removed a `setOptions()` call temporarily and saw options default to
`true` / `false`.
Reviewers: epriestley, btrahan, chad
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4368
Summary:
Ref T2296. If a file upload fails (e.g., too large), we read the textarea from the "Create New Diff" interface. This means we show the user an error like "empty diff" when we should show them an error like "file upload failed, patch is too large". This is part of the issue in T2296, which features a 2.5MB diff.
Instead, check if a file was specified, so we'll raise a better error.
Test Plan: Tried to upload a large patch, got a "file is too large" error instead of an empty-diff-related error.
Reviewers: btrahan, vrana, codeblock
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2296
Differential Revision: https://secure.phabricator.com/D4369
Summary:
Fixes T2273. We currently discard logs, service calls, etc., for daemons, but not for other scripts. However, other scripts may be long-running or issue a large body of service calls (e.g., `bin/search index --all`). We never retrieve this information from scripts (it is used to build darkconsole; in scripts, we echo it immediately under --trace), so discard it immediately to prevent these scripts from requiring a large amount of memory.
(When the daemons load `__init_script__.php` they end up calling this code, so this doesn't change anything for them. They hit another ServiceProfiler discard along the daemon pathways in libphutil, but the call is idempotent.)
Test Plan: Ran `bin/search index --all` and saw increasing memory usage before this patch, but steady memory usage after this patch.
Reviewers: btrahan, vrana, codeblock
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2273
Differential Revision: https://secure.phabricator.com/D4364
Summary:
Fixes T2290. Older versions of PHP (prior to PHP 5.4) raised a warning if you tried to combine empty arrays. (Newer versions don't, which is why I missed this in testing, although I may also not have tried sending empty mail.)
If mail has no recipients, we reach this with an empty array. Just skip the function body and return immediately, the result is empty array.
(You can get mail with no recipients in various valid ways, currently by, e.g., commenting on a Macro with no subscribers.)
Test Plan: Sent mail with zero, nonzero recipients. Received the nonzero recipient mail. Verified on php.net that this is a version issue.
Reviewers: codeblock, btrahan
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2290
Differential Revision: https://secure.phabricator.com/D4360
Summary: If the repository has no lint information, we don't set a 'lint' key, but try to access it unconditionally later on.
Test Plan: Looked at an SVN repository browse view, saw no errors.
Reviewers: vrana, btrahan, codeblock
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2281
Differential Revision: https://secure.phabricator.com/D4359
Summary:
This implements most/all of the difficult parts of Diviner on top of Phabricator instead of as standalone components. See T988. In particular, here are the things I want to fix:
**Performance** The Diviner parser works in two stages. The first stage breaks source files into "Atoms". The second stage renders atoms into a display format (e.g., HTML). Diviner currently has a good caching story on the first step of the pipeline, but zero caching in the second step. This means it's very slow, even for a fairly small project like Phabricator. We must re-render every piece of documentation every time, instead of only changed documentation. Most of this diff concerns itself with addressing this problem. There's a fairly large explanatory comment about it, but the trickiest part is that when an atom changes, other atoms (defined in other places) may also change -- for example, if `class B extends A`, editing A should dirty B, even if B is in an entirely different file. We perform analysis in two stages to propagate these changes: first detecting direct changes, then detecting indirect changes. This isn't completely implemented -- we need to propagate 'extends' through more levels -- but I believe it's structurally correct and good enough until we actually document classes.
**Inheritance** Diviner currently has a very weak story on inheritance. I want to inherit a lot more metas/docs. If an interface documents a method, we should just pull that documentation in to every implementation by default (implementations can still override it if they want). It can be shown in grey or something, but it should be desirable and correct to omit documentation of a method implementation when you are implementing a parent. Similarly, I want to pull in inherited methods and @tasks and such. This diff sets up for that, by formalizing "extends" relationships between atoms.
**Overspecialization** Diviner currently specializes atoms (FileAtom, FunctionAtom, ClassAtom, etc.). This is pretty much not useful, because Atomizers (which produce the atoms) need to be highly specialized, and Renderers/Publishers (which consume the atoms) also need to be highly specialized. Nothing interesting actually lives in the atom specializations, and we don't benefit from having them -- it just costs us generality in storage/caches for them. In the new code, I've used a single Atom class to represent any type of atom.
**URIs** We have fairly hideous URIs right now, which are very cumbersome For in-app doc links, I want to provide nice URIs ("/h/notfications" or similar) which are stable redirects, and probably add remarkup for it: !{notifications} or similar. This diff isn't related to that since it's too premature.
**Search** Once we have a database generation target, we can index the documentation.
**Design** Chad has some nice mocks.
Test Plan: Ran `bin/diviner generate`, `bin/diviner generate --clean`. Saw appropriate graph propagation after edits. This diff doesn't do anything very useful yet.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T988
Differential Revision: https://secure.phabricator.com/D4340
Summary: See discussion in D4355, this fixes reversed bool logic.
Test Plan:
- Quickly viewed in the web interface to make sure it didn't break anything.
- Saved `ldap.auth-enabled` with correct boolean value in the db.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4357
Test Plan: HHVM currently core dumps so I didn't test it.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4343
Test Plan: Looked at them in the web UI.
Reviewers: epriestley, chad, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4355
Test Plan: Looked at the setting and available options from the dropdown.
Reviewers: epriestley, chad
Reviewed By: chad
CC: aran, Korvin
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4354
Summary: Also provide a way to update old files metadata.
Test Plan: Create a revision which includes a image file. Check whether the widht, height metadata exists. Run `scripts/files/manage_files.php metadata --all` to update previously uploaded files.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2101
Differential Revision: https://secure.phabricator.com/D4347
Summary:
Adds the translations group as per T2255. Currently `translation.override` is
`wild` -- it should be changed to dict<string, string> when that exists.
Also fixes a small bug from D4326 which caused "class" types to not ever
validate.
Test Plan:
- Looked at the settings.
- Successfully saved a setting relating to classes.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4350
Summary: this can happen if you have Phabricator and email lists co-mingling such that Phabricator receives an email multiple times. we can prevent this from then spamming everyone or otherwise taking the action multiple times by storing a message id hash and dropping the message if we have more than one message that matches.
Test Plan: simulated sending the same email multiple times on the command line. noted only the first one made it through.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T1726
Differential Revision: https://secure.phabricator.com/D4328
Summary:
This replaces D4175 and makes it so phabot doesn't message anyone.
The reasons for this are twofold:
- It was possible to get information from the bot, by private messaging it, even
if the bot was only in a +i channel (on a public network) -- meaning that if
someone knew the nickname of the bot, they could obtain e.g. ticket names
or diff titles.
- The other time it messaged people was when you typed e.g. "somenick: T123".
Most times when this is triggered, it's done so on accident.
See discussion on the old revision (D4175).
Test Plan:
15:29:33 ::: Irssi: Starting query in quartz with cb-phabot
15:29:38 <relrod> T2
(nothing back)
and
15:29:21 <@relrod> rublets: T1
15:29:21 < cb-phabot> T1: asdfasdf (Priority: Needs Triage) - http://local.elrod.me/T1
Reviewers: epriestley, btrahan, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4339
Summary: Bring these over. Also sort the group list.
Test Plan: Viewed config.
Reviewers: btrahan, codeblock, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4338
Summary:
If you go to `/rXnnnn` in Git, we expand the hash. If you go to `/rXnnnn` in Mercurial, we give you a confusing error message.
Reconcile Mercurial behavior with Git. Fixes T2265.
Test Plan: Viewed partial hash, full hash commit in Diffusion. Viewed very short hash, got reasonable behaviors.
Reviewers: btrahan, tido
Reviewed By: tido
CC: aran
Maniphest Tasks: T2265
Differential Revision: https://secure.phabricator.com/D4330
Summary:
- Ports MySQL settings to PHP.
- Removes "mysql.retries" -- this existed only because Magic Numbers Are Bad, but there is no concievable reason it should ever be set to anything other than 3.
- Introduced "Hidden" config, which isn't visible from the web (for SaaS, we'll just mark anything with secret keys as "hidden").
- Introduced "Masked" config, which will be masked in darkconsole once that gets updated.
- "Hidden" implies "Masked" and "Locked".
- Moved "storage.default-namespace" here -- it probably makes more sense than core; this was my bad in T2255.
- Put cancel button back for hidden/locked config.
- Introduce 'class' config type.
Test Plan: Viewed MySQL options. None are editable.
Reviewers: codeblock, btrahan
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4326
Summary:
- Adds the rest of the group as per T2255.
- Adds a pht() around the `$developer_warning` in `PhabricatorStandardPageView`.
Test Plan:
- Viewed new config options.
- Triggered a fake warning to make sure I didn't break error callouts.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4333
Summary: Added all the "Security" group options listed in T2255.
Test Plan:
- Looked at all the options.
- Tested validation on `security.alternate-file-domain`
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4334
Summary: these existed once, are no more, and don't get cleaned up in the current code path
Test Plan: storage destroy --dryrun -- noted the correct database names
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2237
Differential Revision: https://secure.phabricator.com/D4329
Summary: Refs T2255 and takes care of the "EXTENDING PHABRICATOR" group thereof.
Test Plan: Looked at each of the new options.
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4325
Summary: See title.
Test Plan: Checked that the strings still rendered.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4327
Summary: This config section is weak (poorly documented) and inconsistent (keys with "_" instead of "-") but I'm going to keep punting on improving it until after T1536.
Test Plan: Loaded, examined LDAP config.
Reviewers: btrahan, codeblock
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4322
Summary: We interpret "size" as "size => true", and "true == 'full'", so we hit the wrong branch in the switch(). String cast explicitly.
Test Plan: Typed `{Fnnn, size}`; saw it render as a thumb instead of full.
Reviewers: btrahan, codeblock
Reviewed By: codeblock
CC: frozendevil, aran
Differential Revision: https://secure.phabricator.com/D4323
Summary:
- Move GC options into PHP.
- Remove the "run at" and "run for" options. The GC daemon doesn't actually do any table scans, is very gentle, and runs for like 3 seconds per day in any normal install. Just limit it to running once every 4 hours when it's caught up and call it a day.
Test Plan: Edited GC options.
Reviewers: btrahan, codeblock
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4321
Summary: Some config shouldn't reasonably be edited from the web interface because it immediately torpedoes the install if you make a mistake. Block edits to "locked" config.
Test Plan: Tried to edit locked config, got denied. Viewed locked config on edit and list screens.
Reviewers: codeblock, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4320
Summary:
Add validation for timezones, since date_default_timezone_set() returns a usable error code.
Note that we could also list all the timezones using timezone_identifiers_list(), but the list is enormous (many hundreds of entries) and impossible to use (~160 entries in "America" alone). I listed the likely US values as examples but left it as a string input text field.
Test Plan: Tried to save an invalid setting. Saved a valid setting.
Reviewers: codeblock, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4318
Summary:
Refs #2255 and completes the first group ("CORE") in @epriestley's comment
thereof.
Test Plan: Saw the new options appear in the list and save correctly.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4317
Summary: This is more or less a copy of the validation which lives in `webroot/index.php` right now, but I don't want to wipe that out just yet because there's no way for normal users to see this new validation.
Test Plan: Tried to set "phabricator.base-uri" to crazy nonsense, was harshly rebuffed.
Reviewers: codeblock, btrahan
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4316
Summary:
- Allows us to implement setup warnings for edits which don't go through the web UI, e.g. "you edited a config file and set value X to something goofy".
- Allows us to implement more sophisticated validations, beyond basic type checks (e.g., "phabricator.base-uri" must be a URI).
- Fixes T358 (or, close enough -- fixes it for all options which have been migrated as per T2255.
Test Plan: Set "darkconsole.enabled" to "xyz" in my config, observed setup warning. Added fake validation, observed web UI edit error.
Reviewers: codeblock, btrahan
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2255, T358
Differential Revision: https://secure.phabricator.com/D4315
Summary: Use ApplicationTransactions in Config to create an edit history. Resolves T2256.
Test Plan: {F28477}
Reviewers: btrahan, codeblock
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2256
Differential Revision: https://secure.phabricator.com/D4314
Summary:
- When viewing a config list, show the current effective value.
- Add an icon showing default vs nondefault values.
Test Plan: {F28475}
Reviewers: btrahan, codeblock
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4313
Summary: Show the value for all loaded configuration sources.
Test Plan:
{F28469}
{F28470}
{F28471}
Reviewers: btrahan, codeblock
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4312
Summary: Show example config values to the user when available.
Test Plan:
{F28465}
{F28466}
Reviewers: btrahan, codeblock
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2221, T2255
Differential Revision: https://secure.phabricator.com/D4311
Summary: Also improve behavior for the "unknown config" warning.
Test Plan: Looked at configs, went through unknown config workflow.
Reviewers: btrahan, codeblock
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4310
Summary:
- Add a "developer" option group.
- Add an "access log" option group.
- Render the types "bool", "int" and "string" in a more tailored way.
- Add a config check for dead config. Right now this serves as a "TODO" list of things that need to be migrated.
Test Plan: Looked at config options, setup issues. Edited bool, int, string options.
Reviewers: codeblock, btrahan
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4308
Summary: This is obsolete with the 'device' => true stuff, which is more granular and generally better.
Test Plan: grep
Reviewers: btrahan, codeblock
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2255
Differential Revision: https://secure.phabricator.com/D4309
Summary:
- When a setup issue is nonfatal (i.e., a warning), instruct the user to edit the value from the web UI instead of using `bin/config`.
- When the user edits configuration in response to a setup issue, send them back to the issue when they're done.
- When an issue relates to PHP configuration, link to the PHP documentation on configuration.
- Add new-style setup check for timezone issues.
Test Plan: Mucked with my timezone config, resolved the issues I created.
Reviewers: codeblock, btrahan, vrana
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2221, T2228
Differential Revision: https://secure.phabricator.com/D4298
Summary: Ref T2255. Ref T2221. Lay the groundwork to move configuration into PHP, so we can show descriptions in the web UI, do typechecking, disable application options when an application is uninstalled, etc.
Test Plan:
{F28421}
{F28420}
{F28422}
Reviewers: codeblock, btrahan, vrana
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2221, T2255
Differential Revision: https://secure.phabricator.com/D4306
Summary: Some time long in the past, this was renamed to unsavedHunks. Fixes T2249.
Test Plan: Ran `destroy_revision.php D20`, got a successful destruction.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2249
Differential Revision: https://secure.phabricator.com/D4304
Summary:
- Fixes T2257. We wrote a 0-length file (erroneously?) and currently throw when retrieving it. This also happens if you intentionally upload an empty file. I'm not sure what happened with the image: we check for errors during the write, so its existence implies S3 told us the write was successful and then lost the data. Since this is a one-off, I'm not too worried about it. The indistinguishable case of an actually empty file is fixed, at least.
- Writes to a directory like "phabricator/ab/cd/efgh" instead of "phabricator/abcdefgh". When I had to go look for the file on S3 it took a few minutes of scrolling since the web interface isn't very fast. Make it so a file can be located by navigating through pieces of the hash.
Test Plan: Viewed an empty file, no fatal. Viewed the file from T2257 locally, no fatal (no data either, but it's gone). Uploaded a file, saw a nice path.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2257
Differential Revision: https://secure.phabricator.com/D4303
Summary: We have enough z-index rules that they're fairly hard to visualize with "git grep". Consolidate them. Then fix T2253 (missing z-index on left menu background).
Test Plan: Made a Differential window really narrow, then scrolled it horizontally.
Reviewers: btrahan, chad, ender
Reviewed By: chad
CC: aran
Maniphest Tasks: T2253
Differential Revision: https://secure.phabricator.com/D4302
Summary:
This is basicaly a light version of D4286. The major problem with D4286 is that it's a huge leap and completely replaces the setup process in one step.
Instead, I want to do this:
- Add the post-setup warnings (yellow bar with "6 unresolved warnings...").
- Copy all setup checks into post-setup warnings (so every check has an old-style check and a new-style check).
- Run that for a little bit and make sure it's stable.
- Implement fatal post-setup checks (the red screen, vs the yellow bar).
- Run that for a little bit.
- Nuke setup mode and delete all the old checks.
This should give us a bunch of very gradual steps toward the brave new world of simpler setup.
Test Plan:
- Faked APC setup failures, saw warnings raise.
- Verified that this runs after restart (get + set).
- Verified that this costs us only one cache hit after first-run (get only).
Reviewers: btrahan, codeblock, vrana, chad
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2228
Differential Revision: https://secure.phabricator.com/D4295
Summary:
See discussion in T2221. Before we can move configuration to the database, we have a bootstrapping problem: we need database credentials to live //somewhere// if we can't guess them (and we can only really guess localhost / root / no password).
Some options for this are:
- Have them live in ENV variables.
- These are often somewhat unfamiliar to users.
- Scripts would become a huge pain -- you'd have to dump a bunch of stuff into ENV.
- Some environments have limited ability to set ENV vars.
- SSH is also a pain.
- Have them live in a normal config file.
- This probably isn't really too awful, but:
- Since we deploy/upgrade with git, we can't currently let them edit a file which already exists, or their working copy will become dirty.
- So they have to copy or create a file, then edit it.
- The biggest issue I have with this is that it will be difficult to give specific, easily-followed directions from Setup. The instructions need to be like "Copy template.conf.php to real.conf.php, then edit these keys: x, y, z". This isn't as easy to follow as "run script Y".
- Have them live in an abnormal config file with script access (this diff).
- I think this is a little better than a normal config file, because we can tell users 'run phabricator/bin/config set mysql.user phabricator' and such, which is easier to follow than editing a config file.
I think this is only a marginal improvement over a normal config file and am open to arguments against this approach, but I think it will be a little easier for users to deal with than a normal config file. In most cases they should only need to store three values in this file -- db user/host/pass -- since once we have those we can bootstrap everything else. Normal config files also aren't going away for more advanced users, we're just offering a simple alternative for most users.
This also adds an ENVIRONMENT file as an alternative to PHABRICATOR_ENV. This is just a simple way to specify the environment if you don't have convenient access to env vars.
Test Plan: Ran `config set x y`, verified writes. Wrote to ENVIRONMENT, ran `PHABRICATOR_ENV= ./bin/repository`.
Reviewers: btrahan, vrana, codeblock
Reviewed By: codeblock
CC: aran
Maniphest Tasks: T2221
Differential Revision: https://secure.phabricator.com/D4294
Summary:
* When we restored to the default value, we did, in fact delete the row from the
database, but then a few lines later down, we saved it again. This patch causes
the controller to return early on delete, like it was supposed to do to begin
with.
* When checking the user's input value for `null` (since PHP's JSON encoder will
return `null` on failure), check the value that the user gave, not the value
that we default to (which is often `null` anyway). Oops.
Test Plan:
* Saved an empty text field and saw the delete work properly and NOT get
re-added.
* Put `null` in the text field, and saved successfully.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4300
Summary:
The placeholder property remains null despite it is set to a custom
value.
Test Plan:
Use a custom placeholder in `AphrontFormTokenizerControl`
```
id(new AphrontFormTokenizerControl())
->setPlaceholder('My custom placeholder...')
->setDatasource('/typeahead/common/projects/');
```
The placeholder should be set to "My custom placeholder..."
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4297
Summary:
As mentioned by @epriestley in an inline on D4290, we should show what happens
if the user leaves the box blank.
Test Plan: Went to edit a setting and saw the default below the text box.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, asherkin
Differential Revision: https://secure.phabricator.com/D4293
Summary:
This is somewhat clowny, particularly in how it handles JSON encode/decode, but
I've commented why I did things the way I did. The goal is to store minified JSON
but show pretty-printed JSON where possible, to the user editing it.
Test Plan:
* Went to /config/ and saw a list of keys from the `default` config.
* Clicked on one of them, submitted the default value successfully.
* Changed the value to invalid JSON and got a decent error.
* Changed the value to valid JSON and checked the DB to confirm it saved.
* Confirmed the DB values were minified.
* Confirmed the user-facing values were pretty-printed where they could be.
* Confirmed that PHIDs were getting assigned properly and that isDeleted
properly defaulted to false/0.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2246
Differential Revision: https://secure.phabricator.com/D4290
Summary:
This replaces D4042 and gives you the option to search for existing Phrication
documents only.
Test Plan:
# Reindexed ALL THE THINGS, saw existing documents show up in search in all cases.
# Deleted a document and saw it only show up when "Open and Closed Documents" are
shown.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4289
Summary: Currently, we have a configuration stack for unit tests, but they're built in to `PhabricatorEnv`. Pull them out and formalize them, so we can add more configuration sources (e.g., database).
Test Plan: Ran unit tests, web requests, scripts. This code had fairly good existing test coverage.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2223, T2221
Differential Revision: https://secure.phabricator.com/D4284
Summary:
In the past, we did some additional magic on `$response_string` (adding profiling headers? Or DarkConsole?), so we could not share the pathway with HTTPSink. We no longer do this; share the pathways.
Also remove error handler initialization (duplicated in PhabricatorEnv), and move $sink initialization earlier. My general goal here is to allow PhabricatorSetup to emit a normal Response object and share as much code as possible with normal pages.
Test Plan: Loaded page.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2228
Differential Revision: https://secure.phabricator.com/D4285
Summary:
We have a bunch of code duplication now between __init_script__.php and webroot/index.php. Consoldiate these methods and move them into PhabricatorEnv.
Merge PhabricatorRequestOverseer into PhabricatorStartup.
Test Plan: Loaded page, ran script. Wiped PHABRICATOR_ENV; loaded page, ran script; got errors.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2223
Differential Revision: https://secure.phabricator.com/D4283
Summary:
We have a lot of mess to get through before we can load libphutil and enter Phabricator code properly. Move it to a dedicated class.
I'm probably going to merge PhabricatorRequestOverseer into this, although the check that lives there now is kind of weird. It also does not really need to be a pre-load check and could be handled better.
I stopped shoving stuff in here once I got to ENV stuff, I'm going to tackle that next.
Test Plan: Ran phabricator normally; introduced fatals and misconfigurations. Grepped for changed symbols.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, asherkin
Maniphest Tasks: T2223
Differential Revision: https://secure.phabricator.com/D4282
Summary:
See T2062. This cache allows us to essentially implement this sort of block:
if (this_code_has_not_run_since_the_last_server_restart()) {
...
}
This will let us do setup checks automatically (i.e., without a specialized setup mode) without imposing hundreds of milliseconds of `git submodule status` and similar checks on every page load, even if an install does not have APC.
Broadly, the major goals here are:
- Reduce user errors and support costs related to misconfiguration (e.g., failure to update submodules).
- Simplify setup and configuration (remove 'phabricator.setup', remove/reduce PHABRICATOR_ENV).
- Move as much configuration to the web as possible (required for SaaS).
Test Plan:
Added this block to webroot/index.php:
$cache = PhabricatorCaches::getSetupCache();
$result = $cache->getKeys(array('x'));
if (empty($result['x'])) {
phlog('Cache miss + set.');
$cache->setKeys(array('x' => 'y'));
} else {
phlog('Cache hit.');
}
Verified it used APC correctly.
Disabled APC and verified it degraded to a reasonable disk-based behavior.
If we miss both of these we end up with no actual caching, but that's the best we can do. This code will also run too early in setup for it to be appropriate to raise exceptions out of this pathway -- later on, we can raise a warning that APC is not installed.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2227, T2062
Differential Revision: https://secure.phabricator.com/D4281
Test Plan: Clicked on next month, didn't see year 20121.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4277
Summary:
This fixes two separate issues:
# `getTextStatus()` is used for machine readable data in handles and user.info method. Broken since D3810.
# Status may contain date. Broken since beginning but masked by the fact that CSS ignores unknown class names.
Test Plan:
Displayed revision with reviewer away.
Called `user.addstatus`.
Edited status in calendar.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
CC: nh, aran, Korvin
Differential Revision: https://secure.phabricator.com/D4275
Summary:
The search indexing API has several problems right now:
- Always runs in-process.
- It would be nice to push this into the task queue for performance. However, the API currently passses an object all the way through (and some indexers depend on preloaded object attributes), so it can't be dumped into the task queue at any stage since we can't serialize it.
- Being able to use the task queue will also make rebuilding indexes faster.
- Instead, make the API phid-oriented.
- No uniform indexing API.
- Each "Editor" currently calls SomeCustomIndexer::indexThing(). This won't work with AbstractTransactions. The API is also just weird.
- Instead, provide a uniform API.
- No uniform CLI.
- We have `scripts/search/reindex_everything.php`, but it doesn't actually index everything. Each new document type needs to be separately added to it, leading to stuff like D3839. Third-party applications can't provide indexers.
- Instead, let indexers expose documents for indexing.
- Not application-oriented.
- All the indexers live in search/ right now, which isn't the right organization in an application-orietned view of the world.
- Instead, move indexers to applications and load them with SymbolLoader.
Test Plan:
- `bin/search index`
- Indexed one revision, one task.
- Indexed `--type TASK`, `--type DREV`, etc., for all types.
- Indexed `--all`.
- Added the word "saboteur" to a revision, task, wiki page, and question and then searched for it.
- Creating users is a pain; searched for a user after indexing.
- Creating commits is a pain; searched for a commit after indexing.
- Mocks aren't currently loadable in the result view, so their indexing is moot.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: 20after4, aran
Maniphest Tasks: T1991, T2104
Differential Revision: https://secure.phabricator.com/D4261
Summary:
See discussion in D4204. Facebook currently has a 314MB remarkup cache with a 55MB index, which is slow to access. Under the theory that this is an index size/quality problem (the current index is on a potentially-384-byte field, with many keys sharing prefixes), provide a more general index with fancy new features:
- It implements PhutilKeyValueCache, so it can be a component in cache stacks and supports TTL.
- It has a 12-byte hash-based key.
- It automatically compresses large blocks of data (most of what we store is highly-compressible HTML).
Test Plan:
- Basics:
- Loaded /paste/, saw caches generate and save.
- Reloaded /paste/, saw the page hit cache.
- GC:
- Ran GC daemon, saw nothing.
- Set maximum lifetime to 1 second, ran GC daemon, saw it collect the entire cache.
- Deflate:
- Selected row formats from the database, saw a mixture of 'raw' and 'deflate' storage.
- Used profiler to verify that 'deflate' is fast (12 calls @ 220us on my paste list).
- Ran unit tests
Reviewers: vrana, btrahan
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4259
Summary: This data structure is a `dict<int, list<Comment>>` now, where the `int` is the line number.
Test Plan:
- Created a diff changing an image.
- Added inline comments on the left and right sides of the diff.
- Saw some exceptions and general sadness.
- Applied patch.
- Reloaded page.
- Everything worked great.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4264
Summary: D4270 missed a spot I think.
Test Plan: Called it.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4271
Summary: D4249 missed a spot I think.
Test Plan: no more fatal
Reviewers: vrana, epriestley
Reviewed By: vrana
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4270
Summary: basically when we're on a page we have a URIPath so set that and pass it over. I wasn't super duper happy with this but it seems to be the best way to pass this data.
Test Plan: verified the og:url was correct on my dev instance of phacility blog for both the main blog and individual post view
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Differential Revision: https://secure.phabricator.com/D4269
Summary: this makes it more sensical when you hit "share" from a bookmarklet or cut and paste a link into FB, basically by having post-specific data when sharing a post.
Test Plan: looked at generated HTML on my test blog
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Differential Revision: https://secure.phabricator.com/D4266
Summary: Sets any event text color to white, reguardless if its an anchor or not.
Test Plan: Have events from multiple people in the calendar, view events.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4263
Summary:
When previewing, save drafts. When loading objects, restore drafts if they are available.
Depends on: D665
Test Plan:
- Viewed a Mock.
- Typed text into the comment box.
- Reloaded the page.
- Text still there.
- Hit submit, got my comment.
- Reloaded the page.
- Draft correctly deleted.
- Repeated for Macros.
Reviewers: btrahan, chad, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2104
Differential Revision: https://secure.phabricator.com/D4252
Summary:
Implements previews for Macros and Pholio.
(Design is nonfinal -- kind of split the difference between `diff_full_view.png`, laziness, and space concerns. Next couple diffs will add more stuff here.)
Test Plan: {F28055}
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran, vrana
Maniphest Tasks: T2104
Differential Revision: https://secure.phabricator.com/D4246
Summary: Does this seem reasonable? It's a bit more compact than digest() (6 bits / byte instead of 4 bits / byte) and 72 bits of entropy @ 12 bytes instead of 128 bits of entropy @ 32 bytes. I feel like it's important to preserve the printability, though, and this seemed like a fairly good balance of concerns.
Test Plan: unit tests
Reviewers: vrana
Reviewed By: vrana
CC: aran, yemao932
Differential Revision: https://secure.phabricator.com/D4253
Summary: Fixes T2210. Recently, we require unique keys on menu items, but it's currently possible in Maniphest to save the same custom query under multiple names. Avoid exploding in this case (we'll hide the duplicates). This isn't a great fix, but makes Maniphest usable again.
Test Plan: Saved the same query twice, laoded page, got exception, applied patch, loaded page, saw duplicate query stripped.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2210
Differential Revision: https://secure.phabricator.com/D4247
Summary: We use "D<id>" for revisions.
Test Plan: Looked at revision.
Reviewers: btrahan, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4258
Summary: 'cuz new fluid layouts require the westerlyness. Looks like D4126 started the N and W implementation but didn't finish it...? note I had to do the shifting of the 5 pixels in javascript; using the CSS didn't work for me in chrome.
Test Plan: uiexample, and hoping it goes well when deployed in prod for differential case
Reviewers: epriestley
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T2211
Differential Revision: https://secure.phabricator.com/D4257
Summary: This is used in every other view.
Test Plan: Browsed around.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4248
Summary:
I messed up D4244 (and didn't test it properly) - this change should load
the raw content so we can use it.
Test Plan: use the conduit console and actually test the paste conduit methods
Reviewers: vrana, epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4245
Summary:
The caching introduced by rP7e37eb48273eef87e6e6811703fb5d85a3e07a81
broke the use of PhabricatorPaste::getContent() for forking/editing
pastes and downloading pastes with arc paste. I think this fixes all
the appropriate callsites.
Test Plan: fork a paste in the web ui
Reviewers: vrana, epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4244
Summary:
See f5c2a2ab4b (commitcomment-2333247)
Copy of working implementation from PHPMailerLite.
Also expose the SSL/TLS options.
Test Plan: Switched to this mailer, configured Gmail SMTP, sent email. Verified email arrived intact.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran, mbeck
Differential Revision: https://secure.phabricator.com/D4239
Summary:
- Original command is in SSH_ORIGINAL_COMMAND, not normal argv.
- Use PhutilShellLexer to parse it.
- Fix a protocol encoding issue with ConduitSSHWorkflow. I think I'm going to make this protocol accept multiple commands anyway because SSH pipes are crazy expensive to build (even locally, they're ~300ms).
Test Plan: With other changes, successfully executed "arc list --conduit-uri=ssh://localhost:2222".
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T550
Differential Revision: https://secure.phabricator.com/D4232
Summary:
- Build "sshd-auth" (for authentication) and "sshd-exec" (for command execution) binaries. These are callable by "sshd-vcs", located [[https://github.com/epriestley/sshd-vcs | in my account on GitHub]]. They are based on precursors [[https://github.com/epriestley/sshd-vcs-glue | here on GitHub]] which I deployed for TenXer about a year ago, so I have some confidence they at least basically work.
- The problem this solves is that normally every user would need an account on a machine to connect to it, and/or their public keys would all need to be listed in `~/.authorized_keys`. This is a big pain in most installs. Software like Gitosis/Gitolite solve this problem by giving you an easy way to add public keys to `~/.authorized_keys`, but this is pretty gross.
- Roughly, instead of looking in `~/.authorized_keys` when a user connects, the patched sshd instead runs `echo <public key> | sshd-auth`. The `sshd-auth` script looks up the public key and authorizes the matching user, if they exist. It also forces sshd to run `sshd-exec` instead of a normal shell.
- `sshd-exec` receives the authenticated user and any command which was passed to ssh (like `git receive-pack`) and can route them appropriately.
- Overall, this permits a single account to be set up on a server which all Phabricator users can connect to without any extra work, and which can safely execute commands and apply appropriate permissions, and disable users when they are disabled in Phabricator and all that stuff.
- Build out "sshd-exec" to do more thorough checks and setup, and delegate command execution to Workflows (they now exist, and did not when I originally built this stuff).
- Convert @btrahan's conduit API script into a workflow and slightly simplify it (ConduitCall did not exist at the time it was written).
The next steps here on the Repository side are to implement Workflows for Git, SVN and HG wire protocols. These will mostly just proxy the protocols, but also need to enforce permissions. So the approach will basically be:
- Implement workflows for stuff like `git receive-pack`.
- These workflows will implement enough of the underlying protocol to determine what resource the user is trying to access, and whether they want to read or write it.
- They'll then do a permissons check, and kick the user out if they don't have permission to do whatever they are trying to do.
- If the user does have permission, we just proxy the rest of the transaction.
Next steps on the Conduit side are more simple:
- Make ConduitClient understand "ssh://" URLs.
Test Plan: Ran `sshd-exec --phabricator-ssh-user epriestley conduit differential.query`, etc. This will get a more comprehensive test once I set up sshd-vcs.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T603, T550
Differential Revision: https://secure.phabricator.com/D4229
Summary: in the re-factor quest this got broken. Turns out NewLines (aka $this->new in the parser) gets updated right up until the very end for showing text changes so instead pass over the total line count to the renderer to get this codepath right.
Test Plan: showed content of some generated files in diffusion and all was well
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2204
Differential Revision: https://secure.phabricator.com/D4237
Summary:
Because of the way PhabricatorOwnersPackage works, the code to save package
changes when parsing commit changes was raising a few undefined index errors,
and was throwing an exception trying to call a method on null (because not
all of the phids related to the package had their handles loaded). This fix
doesn't load the missing handles, it just avoids trying to use them.
Test Plan:
./scripts/repository/reparse.php on a commit with path changes that triggered
a package change
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4236
Summary: Some fallout from D4191.
Test Plan: Sent "D12" in IRC and got a response.
Reviewers: epriestley, vrana, zeeg
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4235
Summary: Continue work started at D3601.
Test Plan:
Commented declaration `AphrontController::$request`, saw exception.
Brought it back, didn't see exception.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4233
Summary:
Load the data for daemon worker tasks when viewing them, and present
the information in a useful way. This defaults to printing the json data,
but for some classes of worker it will also link to the corresponding
object, to make debugging problems with workers easier.
Test Plan:
load /daemon/task/NNN for a CommitParserWorker and a MetaMTAWorker, and
see the addition of a data field with useful content and link.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4226
Summary: This is probably not the most useful app to have work on mobile, but get the log view to do something fairly sensible.
Test Plan: Looked at all Drydock views in mobile.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D4224
Test Plan: Called it on a diff with postponed linters and no messages.
Reviewers: mgummelt, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4223
Summary: its a bit confusing but "newer" posts are the "previous" page and "older" posts are the "next" page. this is because newer posts are those with higher ids. also make the title be the title of the post if we have an actual post.
Test Plan: set page limit to 5 and got somewhat sensical results (note this pagination seems to break with my test data set where there's fun gaps in the contiguity of the ids in a given blog) viewed an actual post and noted the page title was the post title
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4222
Summary: The logs bits still need some work but add crumbs/lists to everything else. Also build a propery DrydockResourceQuery.
Test Plan: Looked at lease list/detail; resource list/detail.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D4221
Summary: arc incorrectly passes a "user" parameter to differential.createrevision (long ago, we respected it, I think). After D4191 this fatals. Provide a stub call until the next version bump.
Test Plan: inspection
Reviewers: vrana, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4220
Summary: Minor updates to Drydock things to make them work better. In particular, after this patch working copies are correctly allocated or reused.
Test Plan: Ran "reparse.php --harbormaster <derp derp>", saw reuse of working copies when unleased resources were avilable.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D4216
Summary:
This is very preliminary and doesn't actually do anything useful. In theory, it uses Drydock to check out a working copy and run tests. In practice, it's not actually capable of running any of our tests (because of complicated interdependency stuff), but does check out a working copy and //try// to run tests there.
Adds various sorts of utility methods to various things as well.
Test Plan: Ran `reparse.php --harbormaster --trace <commit>`, observed attempt to run tests via Drydock.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015, T1049
Differential Revision: https://secure.phabricator.com/D4215
Summary:
This feature isn't necessarily intentional, but might as well make it work better.
Fixes T2201.
Test Plan: Verified that ##[[link | `name`]]## renders in blue.
Reviewers: chad, btrahan, vrana
Reviewed By: chad
CC: aran
Maniphest Tasks: T2201
Differential Revision: https://secure.phabricator.com/D4217
Summary: I plan to use this in Arcanist.
Test Plan:
$ echo '{}' | arc call-conduit x
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4192
Summary: Wide content is currently allowed to push too far to the right, overlapping icons. Prevent it from doing so.
Test Plan:
Before:
{F27906}
After:
{F27907}
After, Mobile:
{F27908}
Reviewers: chad, codeblock, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4212
Summary:
- If a Paste has very long lines, we show far too much text in the summary, because we wrap the lines. Instead, overflow them.
- If a Paste has very long unbroken lines (MMMMM...), they extend past the page. Instead, add a scrollable container.
Test Plan:
{F27868}
{F27869}
{F27870}
Reviewers: btrahan, codeblock, chad
Reviewed By: codeblock
CC: aran
Differential Revision: https://secure.phabricator.com/D4211
Summary:
Modernizes file uploads. In particular:
- Adds a mobile menu, with an "Upload File" item.
- Adds crumbs to the list view, detail view and upload view.
- Adds "Upload File" action to crumbs.
- Moves upload file to a separate page.
- Removes the combined upload file + recent files page.
- Makes upload file use a normal file control by default (works on mobile).
- Home page, file list and file upload page are now global drop targets which accept files dropped anywhere on them. Dragging a file into the window shows a mask and an instructional message.
- User education on this is a little weak but I think that's a big can of worms?
- Fixes a bug where dropping multiple files into a Remarkup text area produced bad results (resolves T2190).
T879 is related, although it's specifically about Maniphest. I've declined to make global drop targets yet there because there are multiple drop targets on the page with different meanings. That UI needs updating in general.
@chad, do we have an "upload" icon (counterpart to "download")?
Test Plan: Uploaded files in Maniphest, Differential, Files, and from Home. Dragged and dropped multiple files into Differential. Used crumbs, mobile.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2190
Differential Revision: https://secure.phabricator.com/D4200
Summary:
D4188 adds a preview of Paste contents to the list, but gets slow for large lists if you have Pygments installed since it has to spend ~200ms/item highlighting them. Instead, cache the highlighted output.
- Adds a Paste highlighting cache. This uses RemarkupCache because it has automatic GC and we don't have a more generalized cache for the moment.
- Uses the Paste cache on paste list and paste detail.
- Adds a little padding to the summary.
- Adds "..." if there's more content.
- Adds line count and language, if available. These are hidden on Mobile.
- Nothing actually uses needRawContent() but I left it there since it doesn't hurt anything and is used internally (I thought the detail view did, but it uses the file content directly, and must for security reasons).
Test Plan:
{F27710}
- Profiled paste list, saw good performance and few service calls.
- Viewed paste.
- Viewed raw paste content.
Reviewers: codeblock, btrahan, chad
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4204
Summary:
Use modern elements for file detail view.
For the "Technical Details", maybe we should implement a disclosure triangle element? I'd guess there are other cases we could use it.
This makes two small practical changes:
- We show "Delete File" even if you can't delete it; I'm going to align this properly with CAN_EDIT shortly.
- We no longer show the feature discovery hint about using "arc download".
Test Plan:
{F27179}
{F27180}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4199
Summary: Update to new UI stuff, prepare for mobile.
Test Plan: {F27167}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4198
Summary:
This adds a "body" field to `PhabricatorObjectItemView` which lets you optionally
add more information to the list view. It then uses this new field to show
samples of pastes in the paste list view.
Test Plan:
{F27147}
{F27148}
Reviewers: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4188
Summary: Include task ID and class when raising this exception. I took a brief stab at doing this generically, but (a) we specifically raise this exception outside of normal try/catch because we can't follow normal recovery rules for it and (b) we don't have a reasonable PhutilProxyException or similar right now which would preserve stack traces, and don't have builtin exception nesting support until PHP 5.3.
Test Plan: Faked this exception, verified we get more information in the logs.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2193
Differential Revision: https://secure.phabricator.com/D4205
Summary: Most cells in Diffusion were aligning text to the top, and the sigil height is fixed. This lines all up on center.
Test Plan: Tested diffusion with new cell alignment, clicked around to other tables. Looks good in Chrome.
Reviewers: epriestley, asherkin, btrahan
Reviewed By: btrahan
CC: aran, Korvin
Maniphest Tasks: T2189
Differential Revision: https://secure.phabricator.com/D4210
Summary: This tones down the glow and highlight for a cleaner hover experience.
Test Plan: chrome, firefox
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4207
Summary:
Add a `setViewer()` so that PHID loading doesn't throw.
Also remove a period from a button because none of the others have one.
Test Plan: Saved a comment successfully.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4202
Summary: I think I forgot this on my icon update
Test Plan: run celerity
Reviewers: epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4203
Summary: Added 6px to the left and right of each TD on the revision history table.
Test Plan: Used Chrome and a table with many revisions.
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4197
Summary:
- Fixes a bug where dragging a flexible left nav and then pressing "f" to hide it didn't properly reset the content panel's left margin.
- Fixes a bug where D4185 removed "white-space: nowrap;" accidentally (from @asherkin)
Test Plan:
- Dragged bar, then pressed "f".
- Can't repro the nowrap thing for some reason.
iiam
Reviewers: asherkin, vrana
Reviewed By: vrana
CC: aran
Differential Revision: https://secure.phabricator.com/D4194
Summary: Permit the forcible release of Drydock leases. The implementation isn't very exciting for now.
Test Plan: Released leases via web and CLI.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D4181
Summary:
Fixes glitches in the side nav. Resolves T1828. Resolves T2156.
- Elastic scrolling (T2156): in Safari on OSX, using a scroll touch on the trackpad to scroll up past the top of the document caused newer-style side menus to scroll down, leaving a visible whitespace bar.
- Whitespace glitch: Particularly in Safari, scrolling down the document quickly from the top caused the top menu to scroll away before the side menu rose to meet it. Use a fixed background color bar that extends under the menu so this doesn't happen.
- Use of "!important": use CSS better so we don't need to "!important" things.
- Dark Console (T1828): Instead of hard-coding the top position, determine it dynamically by looking at where the content is. This also fixes the menu overlapping with the red "there are errors on this page" development bar.
- General "fixed" glitchiness: don't use fixed-position for menu content other than flexible (draggable) menus.
Test Plan:
- Viewed and scrolled menus in Paste. Opened and closed DarkConsole. Switched devices.
- Viewed and scrolled flexible menus in Differential and Diffusion. Opened and closed DarkConsole. Switched devices.
Reviewers: vrana, chad, btrahan
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1828, T2156
Differential Revision: https://secure.phabricator.com/D4185
Summary: Add a WIKI block to the switch in PhabricatorObjectHandleData->loadObjects().
Test Plan: Went to /feed/ and saw my wiki edit stories.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4180
Summary: See discussion in T2014. Aligns this element more closely with @chad's `frame_v3.psd` mock, and implements the icon/label element. Removes "details".
Test Plan: {F27062} {F27063} {F27064} {F27065}
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2014
Differential Revision: https://secure.phabricator.com/D4179
Summary: most awesome is that differential-primary-pane no longer has a place in diffusion. less awesome is fixing the zebra striping on differential "Local Commits" view and making the font size of one of the table headers match the others.
Test Plan: looks good!
Reviewers: epriestley, chad
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4177
Summary: we don't always have a diff so instead set an explicit title in the controller.
Test Plan: no more fatals. grepped carefully for every call site and tested them all
Summary: basically made the header elements for the individual panes and cleaned up the panes to make it look okay. tried to copy colors from @chad 's mocks.
Test Plan: looks good to me
Reviewers: epriestley, chad
Reviewed By: chad
CC: aran, Korvin
Maniphest Tasks: T2004
Differential Revision: https://secure.phabricator.com/D4174
Summary:
If something that doesn't belong to any field appears in the commit message
below the differential revision field, it gets included as part of the
value for the field, which can mess up parsing.
Test Plan:
called differential.parsecommitmessage on a commit whose differential
revision field wasn't being parsed earlier (it had a line of dashes two
lines below the Differential Revision: line).
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4171
Summary:
Right now, Drydock gives out multiple leases to the same working copy and gives out leases to working copies with repository "P" in them when the user requested some other repository.
Add two callbacks:
- `canAllocateLease()` - allows a blueprint to reject a lease on a resource because of a fundamental incompatibility, like "it's a working copy with Phabricator in it, but the lease wants a working copy with Javelin in it".
- `shouldAllocateLease()` - allows a blueprint to reject a lease on a resource because of resource limits, like "only one active lease can own a working copy at a time".
Also cleaned up various other things.
Test Plan:
After implementing the callbacks, Drydock has the correct behavior:
- It gives multiple leases on `localhost`, but only one lease per working-copy resource.
- It does not grant leases on resources with repository X to requests for repository Y.
Ran `bin/drydock lease --type working-copy --repositoryID 12` and similar repeatedly and verified results in the web console.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D4166
Summary:
- Remove "Diffusion" crumb (redundant with application icon)
- Put "All Repositories" in its place on the landing page.
- Render the repository crumb as "rX" instead of "X Repository".
Test Plan: Looked at various Diffusion pages.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4173
Summary:
upgrades are CrumbsView, HeaderView, PropertyListView, and ActionListView. I had to modify CrumbsView stuff a bit to handle the "advanced" diffusion crumbs.
Quirks fixed include making file tree view show up in diffusion, the page not have extra space when the file tree is hidden, links no longer breaking once you visit files (since without the change the files always got "/" appended and thus 404'd), and a differential quirk where it read "next step:" and that colon is a no no,
Test Plan: played around in diffusion and differential
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin, chad
Maniphest Tasks: T2048, T2178
Differential Revision: https://secure.phabricator.com/D4169
Summary:
Clicking "show details" of a task description change in Maniphest currently throws an exception about the markup engine.
Since we don't actually need the engine an alternate fix would be "if ($this->markupEngine) { $renderer->setMarkupEngine($this->markupEngine); }" but we have one at the ready so just provide it. This should become part of the Transactions stuff anyway.
Test Plan: Clicked "show details".
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4167
Summary:
This adds a configuration option for repositories to be marked as managed
by phabricator, which is then used by the pullLocal daemon to try to recover
from some errors it runs into.
Test Plan: Set a bogus uri for a repo, saw that the pullLocal daemon fixed it.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D3680
Summary: Changes the tan table with the girly blue header color.
Test Plan: Chrome
Reviewers: vrana, epriestley, btrahan
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2176
Differential Revision: https://secure.phabricator.com/D4164
Summary: Show attributes on the view pages.
Test Plan: {F26985} {F26986}
Reviewers: btrahan, chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2015
Differential Revision: https://secure.phabricator.com/D4165
Summary: wanted to play with some policy stuff as its been a bit. Turns out you can't edit questions so this is very silly "so long as you are a user you can view it" policy. also sorry if you have a diff or twelve out for this in your sandbox(es).
Test Plan: loaded up ponder and clicked about
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2113
Differential Revision: https://secure.phabricator.com/D4163
Summary:
When a user submits an action with no effect (like an empty comment, an "abandon" on an already-accepted revision, or a "close, resolved" on a closed task) we want to alert them that their action isn't effective. These warnings fall into two general buckets:
- User is submitting two or more actions, and some aren't effective but some are. Prompt them to apply the effective actions only.
- A special case of this is where the only effective action is a comment. We provide tailored text ("Post Comment") in this case.
- User is submitting one action, which isn't effective. Tell them they're out of luck.
- A special case of this is an empty comment. We provide tailored text in this case.
By default, the transaction editor throws when transactions have no effect. The caller can then deal with this, or use `PhabricatorApplicationTransactionNoEffectResponse` to provide a standard dialog that gives the user information as above. For cases where we expect transactions to have no effect (notably, "edit" forms) we just continue on no-effect unconditionally.
Also fix an issue where new, combined or filtered transactions would not be represented properly in the Ajax response (i.e., return final transactions from `applyTransactions()`), and translate some strings.
Test Plan:
- Submitted empty and nonempy comments in Macro and Pholio.
- Submitted comments with new and existing "@mentions".
- Submitted edits in both applications.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T912, T2104
Differential Revision: https://secure.phabricator.com/D4160
Summary: Currently, AphrontProxyResponse is expected to build a string. This prevents some response types (like Dialog) from being proxied, because they have special rules. Instead, make proxy responses reduce into a non-proxied response so it's possible to proxy any type of response and hit all the normal rules for it.
Test Plan: Built a proxied DialogResponse on top of this.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2104, T912
Differential Revision: https://secure.phabricator.com/D4159
Summary:
For transaction interfaces, I want to prompt the user when they take an action that has no effect, e.g.:
Action Has No Effect
You can not close this task, because someone else has already closed it.
Do you want to post your comment anyway?
[Cancel] [Post Comment]
We already do this for Differential, but it's all hard-coded. T912 is an open task for fixing this for Maniphest.
To do this in a general way, I want to embed the entire request in the dialog as hidden inputs, then add a "__continue__" key and resubmit the form. The endpoint will read this key the second time through and apply what effects it can (e.g., just post a comment).
This adds a mechanism for getting all the request data, minus "magic" like __dialog__ and __csrf__. We need to jump through some hoops because of how PHP encodes arrays.
Test Plan: Ran unit tests, built "no effect" dialogs on top of this.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T912, T2104
Differential Revision: https://secure.phabricator.com/D4158
Summary: pull out some more stuff from the TwoUp renderer that's generically useful. also clean up $xhp variable and add a get method for the $coverage. Finally, fix T2177 while I'm in here.
Test Plan: played around with Differential. Also opened things up in Firefox to verify T2177.
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2009, T2177
Differential Revision: https://secure.phabricator.com/D4161
Summary: I thought I'd already implemented this, but hadn't. Implement a "USER" policy -- a USER phid means only that user has the capability.
Test Plan: Looked at macros as a user other than the comment owner.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4155
Summary: D4153 made these render with newlines between items; use commas instead.
Test Plan: {F26950}
Reviewers: btrahan, chad, vrana
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4162
Summary:
Git adds it automatically.
I don't like this solution much because there could be other unknown fields appended to the end of the commit message which will break parsing.
Test Plan:
Reparsed commit ending with:
> Differential Revision: ...
> Conflicts: ...
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4157
Summary: actions are still a bit messy - unsatisfactory icons (T2013 will help!)
Test Plan: viewed diffs - they look good
Reviewers: epriestley, vrana
Reviewed By: epriestley
CC: chad, aran, Korvin
Maniphest Tasks: T2007
Differential Revision: https://secure.phabricator.com/D3904
Summary:
For text like "MMM", make the right parts of the element scroll.
Also fixed a couple of 1px issues here and there.
Test Plan: Added, viewed UIExamples.
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4152
Summary:
When possible, render application transactions via Ajax. Instead of reloading the page when the response returns, append new transactions to the transaction view.
Scroll the window to the new transactions, animate them in, and clear the form to make this interaction feel reasonable.
When editing transactions, fade them in but do not scroll to them (i.e., don't disrupt the user's position).
Test Plan: Edited and appended transactions via Ajax. Observed fade in animations and scroll behavior. Clicked anchors to verify proper anchor accounting.
Reviewers: vrana, btrahan, chad
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1960
Differential Revision: https://secure.phabricator.com/D4151
Summary: When possible, replace the edited or deleted transaction inline using Ajax.
Test Plan: Repeatedly edited and deleted transactions. Clicked their anchors to verify anchors were correctly preserved.
Reviewers: vrana, btrahan, chad
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1082
Differential Revision: https://secure.phabricator.com/D4150
Summary:
Allows you to edit or delete comments in appplications which support ApplicationTransactions.
UI/UX stuff:
- The dialogs are rough but I want to do a dialog design pass more generally, @chad has some mocks.
- When you add new mentions via edit, they don't currently count as mentions. I'm not sure what I want to do about this.
- When you edit or delete a comment, we do not publish any notifications about it. I think this is reasonable.
- I didn't separate "delete" out versus "edit"; I assume it will be reasonably intuitive that deleting all the text deletes effectively deletes the comment. I also want to discourage deletion somewhat (we still show the transaction, just show that the comment has been deleted).
Test Plan:
Transaction view, note "Edit" and "Edited" links:
{F26914}
Edit view, has some design issues but I want to do a pass on dialogs in general:
{F26915}
History view:
{F26913}
Reviewers: vrana, btrahan, chad
Reviewed By: vrana
CC: aran
Maniphest Tasks: T1082
Differential Revision: https://secure.phabricator.com/D4149
Summary: Aligns more styles to the `diff_full_view.png` mock.
Test Plan: {F26859}
Reviewers: chad, btrahan
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4142
Summary:
Suhosin has about 50 options for filtering input variables, doucmented here:
http://www.hardened-php.net/suhosin/configuration.html
The default behavior of Suhosin is to drop the variable entirely if it violates any of the rules, then continue with the request. It doesn't affect 'php://input' and doesn't drop other variables, so it evades existing detection, and we can't figure out that it's happened at runtime. We could add blanket checks (Suhosin enabled + suhosin.filter.action set to nothing means this may happen, and will be undetectable if it does happen) but can't tailor a check or recovery to this specific problem.
Instead, raise a better error in the specific case where we encounter this, which is Conduit calls of "arc diff" of files over 1MB (the default POST limit). In these cases, Suhosin drops the variable entirely. If there is no 'params', scream. We never encounter this case normall (`arc`, including `arc call-conduit`, always sends this parameter) although other clients might omit it. The only exception is the web console with `conduit.ping`, which submits nothing; make it submit something so it keeps working.
See also https://github.com/facebook/phabricator/issues/233#issuecomment-11186074
Test Plan: Brought up a Debian + Suhosin box, verified the behavior of Suhosin, made requests with and without 'params'.
Reviewers: btrahan, vrana
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4144
Summary: Publish feed stories, including from Pholio. Actual stories are somewhat garbage but it's all display-time; I'm going to do a pass on feed in general.
Test Plan: {F26832}
Reviewers: btrahan, chad, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2104
Differential Revision: https://secure.phabricator.com/D4140
Summary:
- Adds mail support to the generic transaction construct.
- Restores mail support to Pholio (now much improved; the mails are actually useful).
Test Plan: Updated a Pholio mock, got mail.
Reviewers: btrahan, chad, vrana
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2104
Differential Revision: https://secure.phabricator.com/D4139
Summary: I got rid of the "#4" and just linked the timestamps.
Test Plan: {F26826}
Reviewers: chad
Reviewed By: chad
CC: aran
Maniphest Tasks: T2104
Differential Revision: https://secure.phabricator.com/D4138
Summary:
Split Pholio's transaction implementation into generic and application-specific parts. Moves us toward generic transactions, with support for:
- Editing and deleting comments.
- Setting visibility of individual comments (I'm not a fan of this feature but we'll see).
I want to move everything to a more generic piece of infrastructure but there's very little they can share right now so adding transactions to, e.g., Paste or Macros (T2157) means massive amounts of similar code.
Tons of work left to do here, but I think it basically works. Here's a screenshot:
{F26820}
Test Plan: Made transactions in Pholio.
Reviewers: btrahan, vrana, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2104
Differential Revision: https://secure.phabricator.com/D4136
Summary: This is to reduce number of calls from Arcanist.
Test Plan: Called it from web interface.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4146
Summary: Minor issue from D4117, user doesn't get passed down so inline comments are missing their "reply" link.
Test Plan: Looked at Differential, saw reply link.
Reviewers: btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4145
Summary: The dark highlight was bothering me, I'm assuming it probably bothered others but hadn't come up yet.
Test Plan: Hover reload hover reload hover.
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4143
Summary:
Currently, code blocks inside Phabricator remarkup are constrained to ~80col. There's no technical or design reason for this -- it looks and works fine without the restriction. Join the new world of fluid widths.
(Note that //inline// comments can get a bit whacky in some cases with large code blocks, but we have no restrictions there currently, so this doesn't break anything.)
Test Plan: {F26814}
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Maniphest Tasks: T2005
Differential Revision: https://secure.phabricator.com/D4135
Summary:
Support SMTP as the mailer and user could turn on SMTP authentication if needed.
Import PHPMailer as PHPMailerLite doesn't support SMTP.
Make class PhabricatorMailImplementationPHPMailerAdapter final.
Test Plan: N/A
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2139
Differential Revision: https://secure.phabricator.com/D4063
Summary:
D4117 (which is otherwise awesome :)) requires you to `setMarkupEngine()` and
Phriction's diff rendering wasn't changed to call that with a
`PhabricatorMarkupEngine`.
Test Plan: Went to a Phriction diff page and saw it render correctly.
Reviewers: epriestley, btrahan, vrana
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4132
Summary: This updates spacing and colors on the calendar page.
Test Plan: View calendar, make an event, view it again.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4130
Summary: Adds a darker hover color for mousing over the logo.
Test Plan: Mouse over logo, feel the gentle hover wash over me like a light spring rain.
Reviewers: epriestley, btrahan
Reviewed By: btrahan
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4128
Summary: Adds little arrows in CSS to the tooltipcs.
Test Plan: Tested UIExamples and Remarkup Box
Reviewers: epriestley, btrahan
Reviewed By: btrahan
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4126
Summary: we need to render left and right* classes as appropriate, plus colspan for the right
Test Plan: made inline comments and it was no longer borked
Reviewers: vrana, epriestley, chad
Reviewed By: chad
CC: aran, Korvin
Maniphest Tasks: T2005
Differential Revision: https://secure.phabricator.com/D4131
Summary:
- Align the icon and the logo
- Have text links show underline on hover
Test Plan: Viewed the fruits of my efforts
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Differential Revision: https://secure.phabricator.com/D4125
Summary: I think we've sorted out enough of the problems with these to turn them on for everyone. The real-time component remains configuration-dependent.
Test Plan: Turned off "notification.enabled", still saw notifications.
Reviewers: btrahan, chad
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4120
Summary:
- Gets about 25% of the way toward @chad's notification mocks.
- YES: Hover states, entire notification is a click target, border, header, footer.
- NO: Profile pictures (lazy), timestamps (want to refactor time code before introducing a new formatting style), app icons (they'd look funny without timestamps I think)
- Deletes some old files.
- Mostly trying to get this good enough to turn on by default.
Test Plan: Looked at notifications. Clicked some notifications.
Reviewers: chad, btrahan
Reviewed By: btrahan
CC: aran
Differential Revision: https://secure.phabricator.com/D4119
Summary:
Currently we have two different feed story classes, one for notifications and one for feed stories. However, we never actually do anything different with them -- the notification is always the same as the feed story, just shown differently. Delete the notification special case to reduce the amount of code we have supporting feed and notifications.
This is a precursor to @chad's notification designs.
Test Plan: Viewed notifications and feed, saw exactly the same result before and after the patch (but less, simpler code).
Reviewers: btrahan
Reviewed By: btrahan
CC: chad, aran
Differential Revision: https://secure.phabricator.com/D4114
Summary:
basically did my darnedest to pull out a TwoUp rendering view. Made a base class for the rendering views with "old" and "new" terminology rather than "left" and "right.
Future revisions will finish cleaning up the terminology within the DifferentialChangesetParser itself and more of the ideas within T2009.
Test Plan: been playing with differential all day
Reviewers: epriestley
Reviewed By: epriestley
CC: vrana, chad, aran, Korvin
Maniphest Tasks: T2009
Differential Revision: https://secure.phabricator.com/D4117
Summary: we were catching a specific exception; just catch all exceptions
Test Plan: viewed repository tool home page
Reviewers: epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2155
Differential Revision: https://secure.phabricator.com/D4118
Summary: inline comments eat up all 3 tds and no code coverage should be shown.
Test Plan: verified in FIREFOX that inline comments looked good
Reviewers: vrana, epriestley
Reviewed By: epriestley
CC: aran, Korvin
Maniphest Tasks: T2005
Differential Revision: https://secure.phabricator.com/D4115
Summary:
- Remove unused CSS rules and non-sprite images.
- Sprite the logo.
Test Plan: Looked at site, looked good.
Reviewers: chad
Reviewed By: chad
CC: aran
Differential Revision: https://secure.phabricator.com/D4113
