Summary: Splitting these up to re-use in Config as a stop gap.
Test Plan: Visit welcome, install, and quick start on guides app
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16478
Summary: Fixes T11569. This fixes a known bad `setIcon()`. I also looked for more calls to `setIcon()` without success, and stubbed `setIcon()` so we're in good shape even if more exist.
Test Plan:
- Grepped for `setIcon(` and manually inspect all 1,004 callsites to look for calls on `PHUIObjectItemView` objects.
- Grepped for "high risk" callsites (`setIcon` in file after `PHUIObjectItemView`) and re-examined them. I identified these files with this command:
```
git ls-tree -r --name-only HEAD | xargs pcregrep -i -M -H -c --files-with-matches -o 'PHUIObjectItemView(.|\n)*setIcon'
```
There might be some more clever way to do that.
- Since this only identified the callsites I already knew about and I don't have a ton of confidence that I didn't miss any, I put a stub in place that logs a deprecation warning. I'll file a followup to go clean these up in a month or so if the logs are clean.
- Loaded Nuance, saw it work but warn.
- Changed Nuance to use `setStatusIcon()`, loaded Nuance, no more fatal.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11569
Differential Revision: https://secure.phabricator.com/D16477
Summary: This adds status icons, locked, hidden, editable, customized, to the list of options in config. Makes it easier to read and assertain state.
Test Plan:
View a hidden, customized, editable, and locked.
{F1796320}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16475
Summary: Fixes T10679. Added a 'short url' field to the phurl link page and changed the "view url" button to link to the shortened version
Test Plan: Create a phurl link (or navigate to an existing one) and note that there is now a field for "Short URL". Also verified that the "Visit URL" button in the top right still works as intended
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley, yelirekim
Maniphest Tasks: T10679
Differential Revision: https://secure.phabricator.com/D16473
Summary:
Fixes T11555. Previously changing the extension of a paste wouldn't change the syntax highlight language. Now it does.
Also, feed items involving autodetect weren't rendering in a readable way.
Test Plan: Created a paste named `paste.php` and let it autodetect language. Then edited the paste to be named paste.rainbow. It should now be highlighted in raiinnboow
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley, yelirekim
Maniphest Tasks: T11555
Differential Revision: https://secure.phabricator.com/D16474
Summary: Ref T11559. This makes managing large numbers of repositories slightly easier.
Test Plan: {F1796119}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11559
Differential Revision: https://secure.phabricator.com/D16472
Summary: Fixes T11556. This was just missing an `implements ...`, which became necessary at some point even for classes that don't use much of the beahvior (ModularTransactions?).
Test Plan: Created a new test payment provider on a Phortune merchant.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11556
Differential Revision: https://secure.phabricator.com/D16471
Summary: Fixes T11541. `PhabricatorApplication::getIconURI()` has been returning only null for a while (I assume in preparation to remove it). I removed the method and all the remaining call sites.
Test Plan: Removed the method and then clicked around. Things didn't explode!
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, yelirekim
Maniphest Tasks: T11541
Differential Revision: https://secure.phabricator.com/D16470
Summary: Previously we collapsed all table search results, but the new UI doesn't need it. Remove unused methods and fix CSS.
Test Plan: Legalpad Signatures, Phortune Accounts.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16469
Summary: Fixes T11532. The language selection for pastes is now a typeahead that is backed by `pygments.dropdown-choices`. There is still a bit of weirdness around making "auto-detection" the default state. To actually select a different language, you first need to remove the "auto detect" option that is pre-populated in a new paste. Other than that, it works as intended.
Test Plan:
Create a new paste with a file extension that can be auto-detected.
Created a new paste and manually selected the language
Edited a paste and changed the language.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley, yelirekim
Maniphest Tasks: T11532
Differential Revision: https://secure.phabricator.com/D16463
Summary: Ref T11132, significantly cleans up the Config app, new layout, icons, spacing, etc. Some minor todos around re-designing "issues", mobile support, and maybe another pass at actual Group pages.
Test Plan: Visit and test every page in the config app, set new items, resolve setup issues, etc.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam, Korvin
Maniphest Tasks: T11132
Differential Revision: https://secure.phabricator.com/D16468
Summary: Ref T11132. This gets rid of the red bar for admins and instead shows a new menu item next to notifications/chat if there are unresolved configuration issues. Menu goes away if there are no issues. May move this later into the bell icon, but think think might be the right place to start especially for NUX and updates. Maybe limit the number of items?
Test Plan:
Tested with some, lots, and no config issues.
{F1790156}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11132
Differential Revision: https://secure.phabricator.com/D16461
Summary: I plan to reuse these styles with Config, maybe also Almanac, etc.
Test Plan: Review /guides/, see same styles.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16467
Summary:
Fixes T10423. Ref T11524. This changes `diffusion.rawdiffquery` to return a file PHID instead of a blob of data.
This is better in general, but particularly better for huge diffs (as in T10423) and diffs with non-utf8 data (as in T10423).
Test Plan:
- Used `bin/differential extract` to extract a latin1 diff, got a clean diff.
- Used `bin/repository reparse --herald` to rerun herald on a latin1 diff, got a clean result.
- Pushed latin1 diffs to test commit hooks.
- Triggered the the too large / too slow logic.
- Viewed latin1 diffs in Diffusion.
- Used "blame past this change" in Diffusion to hit the `before` logic.
Reviewers: chad
Reviewed By: chad
Subscribers: eadler
Maniphest Tasks: T10423, T11524
Differential Revision: https://secure.phabricator.com/D16460
Summary:
Ref T11524. Ref T10423. Earlier, I converted `diffusion.filecontentquery` to put the actual file content in Files, then return a PHID for the file, instead of trying to send the content over Conduit.
In T11524, we have a similar set of problems with diffs that contain non-UTF8 data (and, in T10423, diffs that are simply enormous).
I want to provide an API method to do the same sort of thing with diff output (like from `git diff`), so we call the method, it shoves the data in Files, and then we go pull it out of Files.
To support this, take the "shove the output of a Future into Files" logic and put it in a new base `FileFuture` query. This will let me make `RawDiffQuery` share the logic more easily.
Test Plan: Browsed Diffusion, ran `diffusion.filecontentquery` to fetch file content.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10423, T11524
Differential Revision: https://secure.phabricator.com/D16458
Summary: Fixes T11513. Previously the selector was just a giant dropdown which was just... just too much. Now there's a handy typeahead.
Test Plan:
Happy Path:
Go to `Settings -> Home Page -> Pin Application`, start typing in the form then select one of the options. Click on "Pin Application". The application should now be in the list.
Other paths:
- Type nothing into the box and submit, nothing should happen.
- Choose an application that is already pinned. The list should stay the same.
- Type nonsense into the box and submit, nothing should happen.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: chad, Korvin, epriestley, yelirekim
Maniphest Tasks: T11513
Differential Revision: https://secure.phabricator.com/D16459
Summary:
Fixes T11537. See that task for discussion.
Although we could accommodate these faithfully, it requires a huge migration and affects one repository on one install which was written with buggy tools.
At least for now, just replace out-of-32-bit-range epoch values with the current time, which is often somewhat close to the real value.
Test Plan:
- Following the instructions in T11537, created commits in 40,000 AD.
- Tried to import them, reproducing the "epoch" database issue.
- Applied the patch.
- Successfully imported future-commits, with some liberties around commit dates. Note that author date (not stored in an `epoch` column) is still shown faithfully:
{F1789302}
Reviewers: chad, avivey
Reviewed By: avivey
Maniphest Tasks: T11537
Differential Revision: https://secure.phabricator.com/D16456
Summary:
Fixes T9235. When the stars align, PHP 5.6 or newer emits a deprecation warning on startup about "always_populate_raw_post_data" which occurs too early for us to intercept and can break responses by adding garbage to the output.
These settings appear to be sufficient:
```
always_populate_raw_post_data = 1
display_errors = 1
display_startup_errors = 1
error_reporting = -1
```
Then make a request with an unusual content type:
```
$ curl -X POST -H "Content-Type: application/json" -d "{foo: bar}" http://phabricator.example.com/
```
This triggers the warning:
```
<br />
<b>Deprecated</b>: Automatically populating $HTTP_RAW_POST_DATA is deprecated and will be removed in a future version. To avoid this warning set 'always_populate_raw_post_data' to '-1' in php.ini and use the php://input stream instead. in <b>Unknown</b> on line <b>0</b><br />
<br />
...
```
To avoid this, just instruct administrators to set this value to "-1", which completely disables the feature and silences the warning.
Test Plan:
- Reproduced this issue by following the instructions above.
- Triggered the setup issue locally and read all the captivating prose:
{F1786911}
- Made the configuration change it directed me to, saw the setup issue resolve.
Reviewers: jcox
Reviewed By: jcox
Maniphest Tasks: T9235
Differential Revision: https://secure.phabricator.com/D16454
Summary: Ref T11132. This is a new default default (no dashboard) homepage. It offers (Diffs) (Tasks) (Repositories) in the main column and (Feed) in the side column. No NUX stuff, No logged out public view (upcoming diff). This should be complete, but unclear how to bucketize Differential.
Test Plan: Test new account's default homepage.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11132
Differential Revision: https://secure.phabricator.com/D16449
Summary: Ref T8628. Updates people controllers for handleRequest
Test Plan: Viewed the people list, viewed the activity logs, then went through the approval process for a new user account.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley, yelirekim
Maniphest Tasks: T8628
Differential Revision: https://secure.phabricator.com/D16451
Summary: Fixes T8850. Previously, if a user's preamble script mangled `$_SERVER['REMOTE_ADDR']` or somehow set it to `null`, the user would get errors when performing certain actions. Now those errors shouldn't occur, and instead the user will be warned that there is a setup issue related to their preamble script.
Test Plan: Create a preamble script that contains `$_SERVER['REMOTE_ADDR'] = null;` then navigate to /config/issue/. There should be a warning there about `REMOTE_ADDR` not being available.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, yelirekim, epriestley
Maniphest Tasks: T8850
Differential Revision: https://secure.phabricator.com/D16450
Summary: Ref T10951. This adds the application name as an attribute below the document type in the UI for doc type search.
Test Plan: Verify that the application name appears as an attribute on the document type results.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Maniphest Tasks: T10951
Differential Revision: https://secure.phabricator.com/D16446
Summary: Ref T10951. This diff removes uninstalled applications from the result set for DocumentType restults
Test Plan: Uninstall an application (diviner for example), then go to the document type search menu and ensure that the uninstalled application doesn't show up.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Maniphest Tasks: T10951
Differential Revision: https://secure.phabricator.com/D16445
Summary: Fixes T10999. Now MFA will be required for all email address related operations.
Test Plan: Ensure that adding and removing email addresses now requires you to enter high security mode.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Maniphest Tasks: T10999
Differential Revision: https://secure.phabricator.com/D16444
Summary: These were blank, from last week's shenanigans.
Test Plan: View homepage settings, see icons.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16447
Summary:
Ref T11522. This tries to reduce the cost of rewriting a repository by making handles smarter about rewritten commits.
When a handle references an unreachable commit, try to load a rewrite hint for the commit. If we find one, change the handle name to "OldHash > NewHash" to provide a strong hint that the commit was rewritten and that copy/pasting the old hash (say, to the CLI) won't work.
I think this notation isn't totally self-evident, but users can click it to see the big error message on the page, and it's at least obvious that something weird is going on, which I think is the important part.
Some possible future work:
- Not sure this ("Recycling Symbol") is the best symbol? Seems sort of reasonable but mabye there's a better one.
- Putting this information directly on the hovercard could help explain what this means.
Test Plan: {F1780719}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11522
Differential Revision: https://secure.phabricator.com/D16437
Summary:
Ref T11522. When a commit is no longer reachable from any branch/tag, we currently show a "this has been deleted" message.
Instead, go further: check if there is a "rewritten" hint pointing at a commit the current commit was rewritten into. If we find one, show a message about that instead.
(This isn't super pretty, just getting it working for now. I expect to revisit this UI in T9713 if we don't get to it before that.)
Test Plan: {F1780703}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11522
Differential Revision: https://secure.phabricator.com/D16436
Summary: Ref T11522. This migrates any "badcommit" data (which probably only exists at Facebook and on 1-2 other installs in the wild) to the new "hint" table.
Test Plan:
- Wrote some bad commit annotations to the badcommit table.
- Viewed them in the web UI and used `bin/repository reparse --change ...` to reparse them. Saw "this is bad" messages.
- Ran migration, verified that valid "badcommit" rows were successfully migrated to become "hint" rows.
- Viewed the new web UI and re-parsed the change, saw "unreadable commit" messages.
- Viewed a good commit; reparsed a good commit.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11522
Differential Revision: https://secure.phabricator.com/D16435
Summary:
Ref T11522. This provides storage for tracking rewritten commits (new feature) and unreadable commits (existing feature, but really hacky).
This doesn't do anything yet, just adds a table and a CLI tool for updating it. I'll document the tool once it works. You just pipe in some JSON, but I need to document the format.
Test Plan:
- Piped JSON for "none", "rewritten" and "unreadable" hints into `bin/repository hint`.
- Examined the database to see that the table was written properly.
- Tried to pipe bad JSON in, invalid hint types, etc. Got reasonable human-readable error messages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11522
Differential Revision: https://secure.phabricator.com/D16434
Summary: Getting rid of some code! This method has no callsites so it should be safe to remove completely. Ref T9690
Test Plan: Removed method and clicked around to make sure nothing broke.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: yelirekim, epriestley
Maniphest Tasks: T9690
Differential Revision: https://secure.phabricator.com/D16439
Summary: Adds a schema patch that removes conduit_connectionlog. This table hasn't been used in 8ish months so it's probably safe to get rid of.
Test Plan: Apply the patch locally and confirm that the table does indeed get dropped.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: epriestley
Differential Revision: https://secure.phabricator.com/D16438
Summary: Removed call to the deprecated buildStandardPageResponse method from XHProfProfileController
Test Plan: Install, configure, and use XHProf. I'll need some guidance with this
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D16432
Summary:
After D16431, listing the same `@annotation` multiple times makes the docblock parser return a list.
We have some resources which list `@requires` or `@provides` several times, but don't handle the new parser properly. Make the code more flexible, since this is a reasonable way to specify the annotations.
See also D16432. This produces a failure in this form:
```
[2016-08-23 21:10:15] ERROR 2: trim() expects parameter 1 to be string, array given at [/core/data/drydock/workingcopy-74/repo/phabricator/src/applications/celerity/CelerityResourceMapGenerator.php:236]
2 arcanist(head=master, ref.master=89e8b4852384), phabricator(head=6c940fb71b0a8850c6a1b7f5fc642a8f8135a76a, ref.master=b521f2349e46), phutil(head=master, ref.master=237549280f08)
3 #0 trim(array) called at [<phabricator>/src/applications/celerity/CelerityResourceMapGenerator.php:236]
4 #1 CelerityResourceMapGenerator::getProvidesAndRequires(string, string) called at [<phabricator>/src/applications/celerity/CelerityResourceMapGenerator.php:193]
5 #2 CelerityResourceMapGenerator::rebuildTextResources(CelerityPhabricatorResources, CelerityResourceTransformer) called at [<phabricator>/src/applications/celerity/CelerityResourceMapGenerator.php:54]
6 #3 CelerityResourceMapGenerator::generate() called at [<phabricator>/src/__tests__/PhabricatorCelerityTestCase.php:16]
7 #4 PhabricatorCelerityTestCase::testCelerityMaps()
8 #5 call_user_func_array(array, array) called at [<arcanist>/src/unit/engine/phutil/PhutilTestCase.php:492]
9 #6 PhutilTestCase::run() called at [<arcanist>/src/unit/engine/PhutilUnitTestEngine.php:69]
10 #7 PhutilUnitTestEngine::run() called at [<arcanist>/src/unit/engine/ArcanistConfigurationDrivenUnitTestEngine.php:147]
11 #8 ArcanistConfigurationDrivenUnitTestEngine::run() called at [<arcanist>/src/workflow/ArcanistUnitWorkflow.php:167]
12 #9 ArcanistUnitWorkflow::run() called at [<arcanist>/scripts/arcanist.php:394]
```
Test Plan: Ran `bin/celerity map`, no more warnings and no change to the actual map.
Reviewers: joshuaspence, chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16433
Summary: Added a brand new shiny cat fact
Test Plan: Pulled up a project with motivator installed and nothing broke
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D16430
Summary: Switches over to new property UI boxes, splits core and apps into separate pages. Move Versions into "All Settings". I think there is some docs I likely need to update here as well.
Test Plan: Click on each item in the sidebar, see new headers.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16429
Summary:
Ref T7148. The automated export process runs this via daemon, which can't answer "Y" to this prompt. Let it "--force" instead.
(Some of my test instances didn't have any repositories, which is why I didn't catch this sooner.)
Test Plan: Ran `bin/repository move-paths --force ...`, saw change applied without a prompt.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7148
Differential Revision: https://secure.phabricator.com/D16426
Summary: Ref T11132. Will work on CSS tomorrow, but wanted to rough in the UI Steps to get guidance. Not sure what you have in mind for the "app" part, if you want to explain it and I build or you build.
Test Plan: Visit each page and click on links. Very rough and unfinished.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11132
Differential Revision: https://secure.phabricator.com/D16419
Summary: Ref T11132. Adds a background color option to PHUIIconView, for use whereever, and NUX. Also normalize icon placement for mixed image/icon result list.
Test Plan: Test in UIExamples, and Global Settings.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11132
Differential Revision: https://secure.phabricator.com/D16424
Summary: Fixes T11493. This code is a little bit weird/clever, simplify it so that we always cast the handles to an array early on.
Test Plan: {F1767668}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11493
Differential Revision: https://secure.phabricator.com/D16422
Summary: Fixes T11501. Let's you pass in a full PHUIIconView or just the icon name to give ObjectListItem a large icon.
Test Plan: Alamanac, Applications, Drydock, Settings, Search Typeahead, Config page...
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11501
Differential Revision: https://secure.phabricator.com/D16421
Summary: I don't think we use footicons, removing that CSS. States were added but only used in Auth, convert them to statusIcon instead.
Test Plan: Visit Auth, UIExamples, grep for `setState`
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D16418
Summary: Ref T11132, Ref T11478. Builds out a basic PHUICMSView and Guides Application, no content / modules.
Test Plan: Go to /guides/, see blank states for new guides.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Maniphest Tasks: T11132, T11478
Differential Revision: https://secure.phabricator.com/D16414
Summary:
Fixes T11488. I broke this in D16360, I think by doing a little extra refactoring after testing it.
This code is very old, before commits always needed to have repositories attached in order to do policy checks.
Modernize it by mostly just using the repository which is present on the Commit object, and using the existing edge cache.
Test Plan: Ran a commit through the Herald test adapter.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11488
Differential Revision: https://secure.phabricator.com/D16413
Summary:
Fixes T11480. This cleans up the error logs a little by quieting three common errors which are really malformed requests:
- The CSRF error happens when bots hit anything which does write checks.
- The "wrong cookie domain" errors happen when bots try to use the `security.alternate-file-domain` to browse stuff like `/auth/start/`.
- The "no phcid" errors happen when bots try to go through the login flow.
All of these are clearly communicated to human users, commonly encountered by bots, and not useful to log.
I collapsed the `CSRFException` type into a standard malformed request exception, since nothing catches it and I can't really come up with a reason why anything would ever care.
Test Plan:
Hit each error through some level of `curl -H ...` and/or fakery. Verified that they showed to users before/after, but no longer log.
Hit some other real errors, verified that they log.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11480
Differential Revision: https://secure.phabricator.com/D16402
Summary: This needs an `isset()` for cases when authority and packages don't completely overlap.
Test Plan:
- With a package set to trigger autoreview, created a revision.
- Observed error log, saw no more error.
- Saw package trigger autoreview properly.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D16398
Summary:
Ref T11473. If you write a method like `get_stuff(ids)` and then call it with an empty list of IDs, you can end up passing an empty constraint to Conduit.
If you run a `*.search` method with such a constraint, like this one:
```
{
"ids": []
}
```
...we have three possible beahviors:
# Treat it like the user passed no constraint (basically, ignore the constraint).
# Respect the constraint (return no results).
# Error.
Currently, we do (1). However, this is pretty confusing and I think clearly the worst option, since it means `get_stuff(array())` in client code will often tend to return a ton of results.
We could do (2) instead, but this is also sort of confusing (it may not be obvious why nothing matched, even though it's an application bug) and I think most reasonable client code should be doing an `if ($ids)` test: this test makes clients a little more complicated, but they can save a network call, and I think they often need to do this test anyway (for example, to show the user a different message).
This implements (3), and just considers these to be errors: this is the least tricky behavior, it's consistent with what we do in PHP, makes fairly good sense, and the only cost for this is that client code may need to be slightly more complex, but this slightly more complex code is usually better code.
Test Plan: Ran Conduit `*.search` queries with `"ids":[]` and `"phids":[]`, got sensible errors instead of runaway result sets.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11473
Differential Revision: https://secure.phabricator.com/D16396
Summary:
Ref T11473. When running `harbormaster.build.search` with a `buildables` constraint, the constraint doesn't get passed to the Query and so currently has no effect.
This piece of logic was just accidentally omitted from D16356. It is probably not used anywhere today and doesn't show up in the UI, so it's easy to overlook (I missed it in review, too).
Test Plan: Ran `harbormaster.build.search` with a `buildables` constraint, got expected filtering.
Reviewers: yelirekim, chad
Reviewed By: chad
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11473
Differential Revision: https://secure.phabricator.com/D16395
