Summary:
Ref T13000. The new approach for dumping database-by-database means that we don't get CREATE DATABASE or USE statements, which makes importing the dump again inconvenient.
Manually stitch these into the dump.
Test Plan:
- Used `bin/storage dump --namespace ...` to dump a smaller local instance.
- Used `bin/storage destroy --namespace ...`, to destroy the namespace, then inported the dump cleanly.
- Verified that each CREATE DATABASE statement appears only once.
- Verified that `bin/storage renamespace --live` can correctly process this file.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13000
Differential Revision: https://secure.phabricator.com/D18707
Summary: Noticed a couple of typos in the docs, and then things got out of hand.
Test Plan:
- Stared at the words until my eyes watered and the letters began to swim on the screen.
- Consulted a dictionary.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D18693
Summary:
Ref T13000. This marks each table as either "data" (normal data), "cache" (automatically rebuilt, no need to ever dump) or "index" (can be manually rebuilt).
By default, `bin/storage dump` dumps data and index tables, but not cache tables.
With `--no-indexes`, it dumps only data tables. Indexes can be rebuilt after a restore with `bin/search index --all ...`.
Test Plan:
- Ran `--no-indexes` and normal dumps with `--trace`, verified that cache and index (former case) or cache only (latter case) tables were dumped with `--no-data`.
- Verified dump has the same number of `CREATE TABLE` statements as before the changes.
- Reviewed persistence tags in the web UI (note Ferret engine tables are "Index"):
{F5210886}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13000
Differential Revision: https://secure.phabricator.com/D18682
Summary:
Ref T13000. This allows us to be more selective about which tables we dump data for, to reduce the size of backups and exports. The immediate goal is to make large `ngrams` tables more manageable in the cluster, but this generally makes all backups and exports faster and easier.
Here, tables are dumped one at a time. A followup change will sometimes add the `--no-data` flag, to skip dumping readthrough caches and (optionally) rebuildable indexes.
Test Plan: Compared a dump from `master` and from this branch, found them to be essentially identical. The new dump has a little more header information in each section. Verified each contains the same number of `CREATE TABLE` statements.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13000
Differential Revision: https://secure.phabricator.com/D18679
Summary: See PHI118. Enables hovercards to support peeking at tags and other details if you, e.g., create numerous identical subtasks of each task.
Test Plan: {F5210816}
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18681
Summary:
Ref T13000. This adds support for tracking "common" ngrams, which occur in too many documents to be useful as part of the ngram index.
If an ngram is listed in the "common" table, it won't be written when indexing documents, or queried for when searching for them.
In this change, nothing actually writes to the "common" table. I'll start writing to the table in a followup change.
Specifically, I plan to do this:
- A new GC process updates the "common" table periodically, by writing ngrams which appear in more than X% of documents to it, for some value of X, if there are at least a minimum number of documents (maybe like 4,000).
- A new GC process deletes ngrams that have been added to the common table from the existing indexes.
Hopefully, this will pare down the ngrams index to something reasonable over time without requiring any manual tuning.
Test Plan:
- Ran some queries and indexes.
- Manually inserted ngrams `xxx` and `yyy` into the ngrams table, searched and indexed, saw them ignored as viable ngrams for search/index.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13000
Differential Revision: https://secure.phabricator.com/D18672
Summary: Despite how I (and everyone else?) pronounce it, it is spelled with an "a". See PHI38.
Test Plan: Googled both spellings.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18622
Summary:
Ref T12819. In D18581, I corrected one bug (ngram selection for terms) but introduced a minor new bug. We now pass `' query '` (term corpus with boundary spaces) to the stemmer, but it bails out on this since English words don't start with spaces.
Trim these extra boundary spaces off before invoking the stemmer.
The practical effect of this is that searching for non-stem variations of a word ("detection") now finds stemmed variations again ("detect"). Prior to fixing this bug, the stem could find longer variations but not the other way around.
Test Plan: Searched for "detection", found results matching "detect" after patch (and saw same results for "detect" and "detection").
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18593
Summary:
Ref T12819. These render the little "Searched For: X, Y, U V" hint about how something was parsed.
(This might get a "substring" color or "title only" color or something in the future.)
Test Plan: {F5178807}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18589
Summary:
Ref T12819. For queries like `v0.2`, we would incorrectly search for ngrams including `0.2`, but this is only a substring ngram: the term corpus splits this into `v0` and `2`, so `0.2` is not in the ngrams table.
When executing term queries, search for term ngrams instead. This makes "v0.2" work properly again.
Test Plan: Searched for "v0.2", found a task with "v0.2" in the title.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18581
Summary:
Ref T12819. This worked right in a non-cluster environment, but `bin/storage upgrade` iterates over each master in a partitioned cluster environment.
Tweak the API so `bin/storage analyze` targets a single host but `bin/storage upgrade` can hit all the masters.
Test Plan: Will run `bin/storage upgrade` in production again. Ran `upgrade` and `analyze` locally, still work fine.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18576
Summary:
Ref T12819. Normallly "ANALYZE TABLE" is like sprinkling magic pixie dust on the database and hoping it will make "good vibes" that cause it to go faster, but in at least some concrete cases with the ngrams tables there really was a key cardinality issue which ANALYZE TABLE corrected, fixing bogus query plans.
Add `bin/storage analyze` to analyze all tables, and make `bin/storage upgrade` run it after adjustment if `--no-adjust` is not specified, and make `bin/storage adjust` run it always.
This runs in a couple seconds and should never hurt anything, so it should be fine to sprinkle lots of pixie dust into the `bin/storage` workflow.
Test Plan: Ran `bin/storage analyze`. Ran `bin/storage upgrade`, saw analyze run. Totally felt great vibes and really aligned chakras on the database.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18573
Summary:
See brief discussion in D18554. All the index tables are the same for every application (and, at this point, seem unlikely to change) and we never actually pass these objects around (they're only used internally).
In some other cases (like Transactions) not every application has the same tables (for example, Differential has extra field for inline comments), and/or we pass the objects around (lots of stuff uses `$xactions` directly).
However, in this case, and in Edges, we don't interact with any representation of the database state directly in much of the code, and it doesn't change from application to application.
Just automatically define document, field, and ngram tables for anything which implements `FerretInterface`. This makes the query and index logic a tiny bit messier but lets us delete a ton of boilerplate classes.
Test Plan: Indexed objects, searched for objects. Same results as before with much less code. Ran `bin/storage upgrade`, got a clean bill of health.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18559
Summary:
Ref T12819. Currently, under the Ferret engine, we query each application's index separately and then aggregate the results.
At the moment, results are aggregated by type first, then by actual rank. For example, all the revisions appear first, then all the tasks.
Instead, surface the internal ranking data from the underlying query and sort by it.
Test Plan: Searched for "A B" with a task named "A B" and a revision named "A". Saw task first. Broadly, saw mixed task and revision order in result sets.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18551
Summary:
Ref T12819. I started trying to get individual engines to drive these constraints (e.g., `ManiphestTaskQuery` can do most of the work) but this is a big pain, especially since most engines don't support "any owner" or "no owner", and not everything has an owner, and so on and so on. Going down this path would have meant a huge pile of stub functions everywhere, I think.
Instead, drive these through the main engine using the fulltext document table, which already has everything we need to apply these constraints in a uniform way.
Also tweak some parts of query construction and result ordering.
Test Plan: Searched for documents by author, owner, unowned, any owner, tags, subscribers, fulltext in global search. Got sensible results without any application-specific code.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18550
Summary:
See PHI57. For example, a query for "ios, only()" finds tags tasked with iOS, exactly, and no other tags.
I called this "only()" instead of "exact()" because we use the term/function "Exact" elsewhere with a different meaning, e.g. in Differential.
Test Plan:
Basic query for a tag:
{F5168857}
Same query with "only", finds tasks tagged with only that tag:
{F5168858}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18543
Summary: Ref T12819. "Relevance" here just means "how many of your search terms are present in the title?" but that's about the best we can do anyway.
Test Plan: Indexed tasks "A B", "A Z", "Z B", and "Z Z" (all with "A B" in comments). Searched for "A B". Got results ranked in the listed order, with "A B" as the most relevant hit for query "A B".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18539
Summary: Ref T12819. Move these out of the core engine into the Ferret engine. In the future different applications can define different functions, like "summary:..." or whatever. This may get more formalization when I possibly do "author:" and such some time down the road.
Test Plan: Searched for "title:...". Searched for "dog:...", got a useful error.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18536
Summary:
Ref T12819. Push more of the magic `' '` stuff into the engine and simplify calls to ngram construction.
Also fixes a bug where a task with title "apple banana" and description "cherry doughnut" could match query "banana cherry" by separating separate term segments with newlines instead of spaces.
Test Plan:
- Indexed some objects.
- Searched (term, substring, quoted terms).
- Viewed index in database.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18534
Summary: Ref T12819. Earlier I separated some ngram code into an "ngram engine" hoping to share it across the simple Ngrams stuff and the full Ferret stuff, but they actually use slightly different rules. Just pull more of this stuff into FerretEngine to reduce the number of moving pieces and the amount of code duplication.
Test Plan: Searched for terms, rebuilt indexes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18533
Summary: See PHI46. The `core:` function means "find results in either the title or body, but not other auxiliary fields like comments".
Test Plan: Searched for text present in the title (yes), body (yes), and comments (no) with the `core:...` prefix.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18514
Summary:
Ref T12819. Some of the extensions "enrich" the document (adding more fields or relationships), while others "index" it (insert it into some kind of index for later searching).
Currently, these are all muddled under a single "index" phase. However, the Ferret extension cares about fields and relationships which other extensions may add.
Split this into two phases: "enrich" adds fields and relationships so other extensions can read them later if they want. "Index" happens after the document is built and has all the fields and relationships.
The specific problem this solves is that comments may not have been added to the document when the Ferret extension runs. By moving them to the "enrich" phase, the Ferret engine will be able to see and index comments.
Test Plan: Ran `bin/search index ...`, grepped for `indexFulltextDocument`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18513
Summary:
Ref T12819. Adds (hacky, hard-coded) field support (for now, only for "title").
I've written this so `title:quick ferret` is the same as `title:quick title:ferret`. I think this is what users probably mean.
You can do the other thing as `ferret title:quick`, or `title:quick all:ferret`.
Test Plan: Searched for `title:x`, `title:"x"`, `title:~"x"`, etc. Searched for "garbage:y", got an exception since that's not a recognized function. Searched for `title:x y`, saw both do title search.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18503
Summary:
Ref T12819. Supports negating search terms, e.g. "apple -honeycrisp".
When negating a term, we're a little more strict about what can match (that is, what can //prevent// a document from being returned) since it's easy for a user to type "apple -honeycrisp -honey -crisp -crispies -olcrispers -honeyyums" to keep refining their search, but hard/impossible to split apart an overboard term.
Test Plan:
- Ran `apple -smith`, `apple -"granny smith"`, etc.
- Verified `phone -tact` does not exclude `phone contact`.
- (In theory, `phone -~tact` would, but the parser currently doesn't support this, and I'm not champing at the bit to add support.)
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18502
Summary:
Ref T12819. Distinguishes between "term" queries and "substring" queries, and tries to match them correctly most of the time. For example:
- `example` matches "example", obviously.
- `~amp` matches "example", but `amp` does not.
- `examples` matches "example" through stemming.
- `"examples"` does not match "example" (quoted text does not stem).
- `"an examp"` does not match "an example" (quoted text is still term text).
- `~"an examp"` matches "an example" (quoted, substring-operator text uses substring search).
Test Plan: Ran searches similar to the above, they seemed to do what they should.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18500
Summary:
Ref T12819. Depends on D18492. Instead of passing a raw query into the Query layer, parse it first.
This allows the query layer to figure out which parts should be substring vs term match, and would allow the SearchEngine layer to do `author:...` eventually by picking it out before sending it to the Ferret engine.
Test Plan: Ran some Ferret queries. They work like before, except that nonsense like `-+"quack"` raises an exception now.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819
Differential Revision: https://secure.phabricator.com/D18499
Summary:
Ref T12819. I gave this stuff a sweet code name because all the terms related to "fulltext" and "search" already mean 5 different things. It, uh, ferrets out documents for you?
I'm building this to work a lot like the existing ngram index, which seems to work pretty well. If this sticks, it will auto-resolve the join issue (in T12443) by letting us do the entire thing locally in a JOIN and thus dodge a lot of mess.
This index gets built alongside other indexes, but only shows up in the UI if you have prototypes enabled. If you do, it appears under the existing fulltext field in Maniphest. No existing functionality is affected or disrupted.
NOTE: The query engine half of this is still EXTREMELY primitive, and this probably performs worse than the existing field for now. If this doesn't show obvious signs of being awful on `secure` I'll improve that in followup changes.
Test Plan:
Indexed my tasks, ran some simple queries, got the results I wanted, even for queries "ko", "k", "v0.1".
{F5147746}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12819, T12443
Differential Revision: https://secure.phabricator.com/D18484
Summary:
Ref T2543. Further consolidates status management into DifferentialRevisionStatus.
One change I'm making here is internally renaming "CLOSED" to "PUBLISHED". The UI will continue to say "Closed", at least for now, but this should make the code more clear because we care about "is closed, exactly" vs "is any closed status (closed, abandoned, sometimes accepted)". This distinction is more obvious as `isClosed()` vs `isPublished()` than, e.g., `isClosedWithExactlyTheClosedStatus()` or something. I think "Published" is generally more clear, too, and more consistent with modern language (e.g., "pre-publish review" replacing "pre-commit review" to make it more clear what we mean in Git/Mercurial).
I've removed the IN_PREPARATION status since this was just earlier groundwork for "Draft" and not actually used, and under the newer plan I'm trying to just abandon `ArcanistDifferentialRevisionStatus` entirely (or, at least, substantially).
Test Plan:
- Viewed revisions.
- Viewed revision list.
- Viewed revisions linked to a task in Maniphest.
- Viewed revision graph of dependencies in Differential.
- Grepped for `COLOR_STATUS_...` constants.
- Grepped for removed method `getRevisionStatusIcon()` (no callsites).
- Grepped for removed method `renderFullDescription()` (one callsite, replaced with just building a `TagView` inline).
- Grepped for removed method `isClosedStatus()` (no callsites after other changes).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T2543
Differential Revision: https://secure.phabricator.com/D18340
Summary: This word is not spelled properly.
Test Plan: Read the word.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D18250
Summary: Ref T12845. Converts the cluster and project config options to the new stuff; this is mostly just shifting boilerplate around.
Test Plan: Edited, deleted, and mangled these options from the web UI and CLI.
Reviewers: chad, amckinley
Reviewed By: amckinley
Maniphest Tasks: T12845
Differential Revision: https://secure.phabricator.com/D18166
Summary:
Fixes T12867. Also:
- Simplify the code a little.
- Stop mutating this on text/mobile -- there's no inherent value in the "youtu.be" link so I think this just changes the text the user wrote unnecessarily.
Test Plan: {F5013804}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12867
Differential Revision: https://secure.phabricator.com/D18149
Summary:
Ref T12855. PHP7 introduced "Throwables", which are sort of like super exceptions. Some errors that PHP raises at runtime have become Throwables instead of old-school errors now.
The major effect this has is blank pages during development under PHP7 for certain classes of errors: they skip all the nice "show a pretty error" handlers and
This isn't a compelete fix, but catches the most common classes of unexpected Throwable and sends them through the normal machinery. Principally, it shows a nice stack trace again instead of a blank page for a larger class of typos and minor mistakes.
Test Plan:
Before: blank page. After:
{F5007979}
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T12855
Differential Revision: https://secure.phabricator.com/D18136
Summary:
Ref T12733. When a revision has unsubmitted checkmarks:
- Color the banner yellow.
- Show them in the "X unsubmitted" count.
- Make the "X unsubmitted" button cycle between all drafts (written but unpublished comments) and "draft done" (checked but unsubmitted "Done" checkbox comments).
Test Plan:
- Checked a "Done" box, saw "1 unsubmitted" and yellow banner.
- Clicked "5 unsubmitted" repeatedly, saw it cycle through all unsubmitted comments and checkboxes.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D18127
Summary:
Ref T12733. This paves the way for a separate "hide" operation which completely hides things.
(I didn't extend this to the server side because that would require schema changes and the new "hide" state is client-only.)
Test Plan: Collapsed and expanded inlines, viewed tooltips.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D18126
Summary: Adds a very basic list of all inline comments, threaded, and their status. Kept this a little simpler than the mock, mostly because sorting here feels a little strange given threads would be all over the place. Not sure sorted is needed in practice anyways. I'd probably lean towards just adding a JS checkbox to hide certain rows if needed in the future.
Test Plan:
Test various commenting structures:
- Leave Comment
- Update Diff
- Leave new comment
- Reply to comment
- Reply to comment as revision author
- Mark items as done
- Update diff again
{F4996915}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D18112
Summary:
Fixes T12803. An install is having difficulty diagnosing mail failures, and one component is that permanent task failures aren't reaching the log.
It's reasonable to send these to the log even when "phd.verbose" is off. See T12803 for a rough review of when we generate these failrues today.
Test Plan:
- Faked some exceptions.
- Got a result in the log (P2058) with `phd.verbose` turned off.
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T12803
Differential Revision: https://secure.phabricator.com/D18106
Summary:
Fixes T12806. Ref T12733.
- Don't count synthetic (lint) comments as anything.
- When you begin writing an inline then cancel it, don't count it as anything.
- When we would show "0 / X", just show "X".
Test Plan:
- Viewed a diff with synthetic comments, no button.
- Wrote, then cancelled an inline. No "X comments".
- Clicked / unlicked "Done", saw "X" -> "1 / X".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12806, T12733
Differential Revision: https://secure.phabricator.com/D18103
Summary:
Fixes T12753. See that task for reproduction instructions.
We add a `GROUP BY` clause to queries with an "ANCESTOR" edge constraint only if the constaint has more than one PHID, but this is incorrect: the same row can be found twice by an ANCESTOR query if task T is tagged with both "B" and "C", children of "A", and the user queries for "tasks in A".
Instead, always add GROUP BY for ANCESTOR queries.
Test Plan:
- Followed test plan in T12753.
- Saw proper paging controls after change.
- Saw `GROUP BY` in DarkConsole.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12753
Differential Revision: https://secure.phabricator.com/D18012
Summary: The tag/shade stuff changed, so purge older markup (like Diviner documents).
Test Plan: {F4972666}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17998
Summary: Ref T12733. Shows a comment snippet when hovering inlines in the objective list.
Test Plan: {F4968490}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12733
Differential Revision: https://secure.phabricator.com/D17980
Summary:
Add important objectives (like waygates and quest markers) to the minimap.
This also probably fixes @cspeckmim's bug with the {key @} keyboard shortcut.
Test Plan:
(This is probably easier to undestand if you `arc patch` + click around.)
{F4966037}
Reviewers: chad, amckinley
Reviewed By: chad
Subscribers: cspeckmim
Differential Revision: https://secure.phabricator.com/D17955
Summary: Our `local.json` configuration file contains various secrets, including database usernames and passwords. As such, we recently changed the permissions on this file from `0644` to `0640`. After doing so, however, I constantly forget to run commands with `sudo`. This is made worse by the fact that `PhabricatorConfigLocalSource` seems to simply ignore `local.json` is it isn't readable, whereas throwing an `Exception` would have saved me a lot of debugging.
Test Plan:
```name=Before
> /usr/local/src/phabricator/bin/config get mysql.pass
{
"config": [
{
"key": "mysql.pass",
"source": "local",
"value": null,
"status": "unset",
"errorInfo": null
},
{
"key": "mysql.pass",
"source": "database",
"value": null,
"status": "error",
"errorInfo": "Database source is not configured properly"
}
]
}
```
```name=After
> /usr/local/src/phabricator/bin/config get mysql.pass
[2017-05-16 21:49:26] EXCEPTION: (FilesystemException) Path '/usr/local/src/phabricator/conf/local/local.json' is not readable. at [<phutil>/src/filesystem/Filesystem.php:1124]
arcanist(head=stable, ref.master=3c4735795a29, ref.stable=20ad47f27331), phabricator(head=stable, ref.master=3dae9701298f, ref.stable=fcebaa5097f3), phutil(head=stable, ref.master=a900d7b63e95, ref.stable=d02cc05931b0)
#0 Filesystem::assertReadable(string) called at [<phutil>/src/filesystem/Filesystem.php:39]
#1 Filesystem::readFile(string) called at [<phabricator>/src/infrastructure/env/PhabricatorConfigLocalSource.php:25]
#2 PhabricatorConfigLocalSource::loadConfig() called at [<phabricator>/src/infrastructure/env/PhabricatorConfigLocalSource.php:6]
#3 PhabricatorConfigLocalSource::__construct() called at [<phabricator>/src/infrastructure/env/PhabricatorEnv.php:195]
#4 PhabricatorEnv::buildConfigurationSourceStack(boolean) called at [<phabricator>/src/infrastructure/env/PhabricatorEnv.php:95]
#5 PhabricatorEnv::initializeCommonEnvironment(boolean) called at [<phabricator>/src/infrastructure/env/PhabricatorEnv.php:75]
#6 PhabricatorEnv::initializeScriptEnvironment(boolean) called at [<phabricator>/scripts/init/lib.php:22]
#7 init_phabricator_script(array) called at [<phabricator>/scripts/init/init-setup.php:11]
#8 require_once(string) called at [<phabricator>/scripts/setup/manage_config.php:5]
```
Reviewers: #blessed_reviewers, joshuaspence
Reviewed By: joshuaspence
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17917
Summary:
Ref T12616. Fixes T11648. Currently, we snug up replies with a negative margin (from T10563) but this throws off the anchor highlighting.
Instead:
- Remove padding from these dolumns.
- Use margins on the stuff inside them instead.
- Less margins for replies.
- Less margins for collapsed comments.
- Show some text for collapsed comments.
Test Plan:
{F4960890}
{F4960891}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616, T11648
Differential Revision: https://secure.phabricator.com/D17913
Summary:
Fixes T8420. Now that hidden inlines no longer fold into a big clump, anchors can just jump to them in a normal way.
Move the anchors up a smidge so thing work.
Test Plan: Clicked an anchor pointed at a hidden inline, ended up in the right place.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8420
Differential Revision: https://secure.phabricator.com/D17910
Summary:
Ref T12616. Fixes T12715. I suspect these are very rarely used. (I think you tried to get rid of them before but I pushed back since we couldn't really offer great alternatives at the time?)
Now that the code is in a better place:
- Click an inline's header (just the colored part) to select it with the keyboard selection cursor.
- Click again to deselect it.
- You can use "n" and "p" to jump to comments, so "click + n" is the same as the old "V" action.
- This also makes it easier to swap between keyboard and mouse workflows, since you can jump into things with the keyboard at any inline.
Also, make "Reply" render more consistently.
Test Plan:
- Did all that stuff, things seemed to work OK.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12715, T12616
Differential Revision: https://secure.phabricator.com/D17908
Summary:
Ref T12616. This moves "reply" to the new stuff and deletes DifferentialInlineEditor, which no longer does anything.
(This breaks some keyboard shortcuts, but I'll rebase D17859 shortly.)
Test Plan: Replied to inlines; things seemed to work properly.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12616
Differential Revision: https://secure.phabricator.com/D17894
