Summary: Ref T11114. This makes the unusual stored custom fields ("Blame Rev", "Revert Plan", etc) work somewhat correctly (?) with EditEngine.
Test Plan:
- Created, updated and edited revisions with unusual stored custom fields like "Blame Rev".
- Observed that these fields now populate in "differential.revision.edit" when available.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17068
Summary:
Ref T11114. This creates `differential.revision.edit` (a modern, v3 API method) and redefines the existing methods in terms of it.
Both `differential.createrevision` and `differential.updaterevision` are now internally implemented by building a `differential.revision.edit` API call and then executing it.
I //think// this covers everything except custom fields, which need some tweaking to work with EditEngine. I'll clean that up in the next change.
Test Plan:
- Created, updated, and edited revisions via `arc`.
- Called APIs manually via test console.
- Stored custom fields ("Blame Rev", "Revert Plan") aren't exposed yet.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17067
Summary:
Ref T11114. This probably still has some bugs, but survives basic sanity checks.
Continue pulling commit message logic out of CustomField so we can reduce the amount of responsibility/bloat in the classtree and send more code through EditEngine.
Test Plan:
- Called `differential.getcommitmessage` via API console for various revisions/parameters (edit and create mode, with and without fields, with and without revisions).
- Used `--create`, `--edit` and `--update` modes of `arc diff` from the CLI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17066
Summary: Allows you to set forms via typeahead
Test Plan: `/typeahead/browse/PhabricatorEditEngineDatasource/`
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17065
Summary: This is just some housekeeping - see note in D16287. Basically, "isTextMode" doesn't convey enough information.
Test Plan: `git grep isTextMode | grep -v Remarkup`, and visit all callsites; There are 4 of them left.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17063
Summary: Allows you to name and set a project as a menu item navigation element.
Test Plan: Add a project, no name, see project. Remove. Add a project and give it a short name (bugs) and see project link.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17021
Summary:
Fixes T11660. Currently, if you try to diff a path with more than 255 total characters, we fail to create the diff because we have a `text255` column.
There are actually two issues here:
- File names may be arbitrarily long (T11660).
- File names may not be UTF8 (T6633, etc). This is much more complicated and has other issues -- largely that we can't JSON-encode non-UTF8 filenames. I'm punting on that for now and will deal with it later. This doesn't specifically address non-UTF8 paths, although it is a change that's (probably?) required to eventually support them.
This will cause some potentially slow migrations, but better to do them now, if possible, so we have fewer complicated/slow upgrades overall.
Test Plan:
Created a change touching file: //very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_directory_name/very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_very_long_filename.txt//
{F2137737}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11660
Differential Revision: https://secure.phabricator.com/D17062
Summary: Ref T11114. Missed this while converting.
Test Plan: Tried to create a revision with no test plan. Before: fatal; after: helpful message.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17061
Summary:
Ref T11114. See that task for some discussion.
Overall, Differential custom fields ended up with too many responsibilities. Later work in EditEngine provides a more promising model for achieving modularity with smaller, more consistent components.
In particular, we have some custom fields like `DifferentialGitSVNIDField` and `DifferentialConflictsField` which serve //only// to support the field parser.
This starts pulling commit message responsibilities out of the core list of custom fields and into simpler dedicated parsers.
Test Plan: Created and edited revisions from the CLI. Added a bit of test coverage.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17058
Summary:
Ref T11114. I want to move this step away from custom fields. To start with, isolate all the parsing in one class with a clearer API boundary.
Next, I'll make this class use new field objects to perform parsing, without CustomField interactions.
Test Plan: Created and edited revisions from the CLI, using valid and invalid commit messages.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17055
Summary:
Ref T12020. Ref T11114. If we continue here on a mention, we try to generate `$old`, which requires reviewers to be attached. They won't be for simple codepaths like mentions.
Instead, just bail early: we don't need to do anything anyway since we can't possibly find any more errors with zero transactions.
Test Plan: Mentioned a revision on a task.
Reviewers: chad, avivey
Reviewed By: avivey
Maniphest Tasks: T11114, T12020
Differential Revision: https://secure.phabricator.com/D17059
Summary: Reorgaizes the CSS here a bit, by object list style, adds in a new drag ui class, which will be used in menu ordering.
Test Plan:
Workboards, Home Apps.
{F2126266}
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17057
Summary: Fixes T12015. This is weird and probably got copy/pasted from something else that was also being weird, since the methods were empty and I previously removed them.
Test Plan: Edited a book in Diviner.
Reviewers: chad, avivey
Reviewed By: avivey
Maniphest Tasks: T12015
Differential Revision: https://secure.phabricator.com/D17056
Summary:
Ref T11114. This replaces the old edit controller with a new one based entirely on EditEngine.
This removes the CustomFieldEditEngineExtension hack for Differential, since remaining field types are fairly straightforward and work with existing EditEngine support, as far as I can tell.
Test Plan:
- Created a revision via web diffs.
- Updated a revision via web diffs.
- Edited a revision via web.
- Edited nonstandard custom fields ("Blame Revision", "JIRA Issues").
- Created a revision via CLI.
- Updated a revision via CLI.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17054
Summary: Ref T11114. Much of this is around making the "comment-while-updating" flow work correctly.
Test Plan:
- Created new diffs by copy/pasting, then:
- used one to create a new revision;
- used one to update an existing revision, with a comment.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17053
Summary:
If you put "echo" or "print" statements into the code at random places (as I frequently do during development), they would emit before we enabled compression.
This would confuse the compression mechanism and browser. I tried using `headers_sent()` to selectively disable compression but that didn't appear to fix this interaction (I think emitting this text does not cause headers to send, but does let contet escape into some buffer which the compressor can not access).
Instead, push the header down a little bit so it renders after we activate compression.
Also make it slightly fancier / more hideous. WOW.
Test Plan: {F2122927}
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17052
Summary: We're throwing here when we actually want to return `null` so we make it into custom field handling code. See Conpherence.
Test Plan: Found a failing task and re-executed it with `bin/worker execute --id <id>`; after this change, it didn't fatal.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17051
Summary: Ref T11114. This one is a bit more complex, but I think I covered everything.
Test Plan:
- Added reviewers.
- Removed reviewers.
- Made reviewers blocking.
- Made reviewers nonblocking.
- Tried to make the author a reviewer.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17050
Summary: Ref T11114. The only real trick here is that we respect configuration in `differential.fields`.
Test Plan: Turned plan on and off, tried to remove the plan, edited the plan.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17048
Summary: Ref T11114. These are unambiguous and always-enabled.
Test Plan: {F2117777}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17047
Summary:
Ref T11114. Currently, all of Differential is extremely custom CustomFields. I want to back away from that somewhat and leverage more EditEngine / ModularTransactions infrastructure.
This allows EditEngine, ModularTransactions, and CustomFields to coexist in an uneasy peace. The "EditPro" controller applies a //different edit// than the CustomFields do, but everything works out in the end. I think.
Hopefully the horrible mess I am creating here will be short-lived.
Test Plan:
- Edited a revision with the normal editor.
- Edited a revision with the pro editor.
- Created a revision with `arc diff`.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17044
Summary: Ref T11114. This doesn't really support anything yet, but technically works if you manually go to `/editpro/`.
Test Plan: {F2117302}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11114
Differential Revision: https://secure.phabricator.com/D17043
Summary: Ref T10967. This makes room for a `DifferentialReviewer` object which can be a real storage table.
Test Plan: Grepped for `DifferentialReviewer`, browsed Differential.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T10967
Differential Revision: https://secure.phabricator.com/D17041
Summary: Ref T8475. This gets rid of most of the old "legacy hunk" code. I'll nuke the rest (and drop the old table) once we're more sure that we're in the clear.
Test Plan: Browsed Differential.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T8475
Differential Revision: https://secure.phabricator.com/D17040
Summary:
Fixes T7899. If you create or update a revision and type an enormously long first line, we currently fatal trying to insert it into the database.
This text is only used to show a single-line summary of the diff in the "History" tab, which should probably be updated anyway. For now, stop fataling.
Test Plan:
Uploaded a diff with the description "MMMM..." (thousands of them).
Before patch: fatal on description being too long.
After patch: beautiful "MMMM" summary.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T7899
Differential Revision: https://secure.phabricator.com/D17038
Summary:
Fixes T12013. Send either "Content-Length" or enable output compression, but not both.
Prefer compression for static resources (CSS, JS, etc).
Test Plan: Ran `curl -v ...`, no longer saw responses with both compression and `Content-Length`.
Reviewers: chad, avivey
Reviewed By: avivey
Subscribers: avivey
Maniphest Tasks: T12013
Differential Revision: https://secure.phabricator.com/D17045
Summary: Allows users set an icon (for reuse on upcoming home) for their dashboard based on 16 descriminating choices.
Test Plan: Create a new dashboard, set new icon. Edit an existing dashboard, set icon.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17042
Summary: Adds authorPHID to panels so we can default to the panels you made.
Test Plan: Run upgrade, visit manage panels, see my panels. Create a new panel. Edit a panel.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17036
Summary: Fixes T12001. I think we're consistent about using bytes everywhere, but users won't necessarily know that and this documentation could certainly be more clear.
Test Plan: Read new text.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12001
Differential Revision: https://secure.phabricator.com/D17037
Summary: Adds an authorPHIDs, populates olds ones.
Test Plan: Make a new Dashboard, see that I created it.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17022
Summary: Ref T571. This was accidentally left behind in D12266.
Test Plan: Used {key command F} to search for "bulk".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T571
Differential Revision: https://secure.phabricator.com/D17034
Summary: Fixes T11997. This lifts the targets out of the containing `overflow: hidden;` div so Chrome is willing to target them.
Test Plan: In Chrome, visited direct comment links and ended up in the right place. Clicked comment anchors, saw browser jump around again.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11997
Differential Revision: https://secure.phabricator.com/D17033
Summary: Fixes T11999. These are actual panels (SettingsPanel) which are panelley so it's OK.
Test Plan: Clicked "Customize Menu..." on Home.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11999
Differential Revision: https://secure.phabricator.com/D17032
Summary: We're currently applying these styles to labels, restrict them to only list elements that have an href.
Test Plan: Test a label, an anchor, and a button as an action item.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17031
Summary: Cleans up the UI on the page here, uses two column layout, places actions as actionlist instead of dropdown. Changes edit pages to dialogs.
Test Plan: Add an application, divider, link, and facts to a menu page.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17030
Summary:
I'm about 90% sure this fixes the intermittent test failure on `testObjectSubscribersPolicyRule()` or whatever.
We use `spl_object_hash()` to identify objects when passing hints about policy changes to policy rules. This is hacky, and I think it's the source of the unit test issue.
Specifically, `spl_object_hash()` is approximately just returning the memory address of the object, and two objects can occasionally use the same memory address (one gets garbage collected; another uses the same memory).
If I replace `spl_object_hash()` with a static value like "zebra", the test failure reproduces.
Instead, sneak an object ID onto a runtime property. This is at least as hacky but shouldn't suffer from the same intermittent failure.
Test Plan: Ran `arc unit --everything`, but I never got a reliable repro of the issue in the first place, so who knows.
Reviewers: chad
Reviewed By: chad
Differential Revision: https://secure.phabricator.com/D17029
Summary: Ref T11957.
Test Plan:
- Viewed an existing project profile.
- Viewed a user profile.
- Created a new project.
- Edited a profile menu.
- Added new profile items.
- Grepped for renamed symbols.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11957
Differential Revision: https://secure.phabricator.com/D17028
Summary:
Ref T11957. This renames the Configuration storage, transaction, query, and PHID type.
No rename on the actual menu item types yet, that's next (and should be the end of this, I think).
Test Plan:
- Viewed projects.
- Viewed profiles.
- Edited a project menu.
- Grepped for all renamed symbols, I think?
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11957
Differential Revision: https://secure.phabricator.com/D17027
Summary:
- Fixes T11995. This got moved but I missed renaming this callsite.
- Fixes T11993. If you have valid credentials, but haven't run `storage upgrade` yet, we can hit this exception during setup. Just ignore it instead.
Test Plan:
- Saved global settings, no more fatal.
- Changed `storage-namespace` to junk, loaded web UI with valid database credentials.
{F2106358}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11993, T11995
Differential Revision: https://secure.phabricator.com/D17024
Summary: Allows applications to be added as profile menu items
Test Plan: Add an application to a project, see menu item, click on menu. Uninstall application, see menu without application.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D17016
Summary: Fixes T11982. If an install is not public, the registering user may not be able to see the inviting user.
Test Plan: {F2097656}
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11982
Differential Revision: https://secure.phabricator.com/D17015
Summary: Fixes T11544. Attempt to detect if we're on a tiny, burstable-CPU AWS instance and complain.
Test Plan:
- Completely faked this locally.
- Hit the URI on an EC2 instance to check that it's correct (got back "m3.large", since that was the instance class).
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11544
Differential Revision: https://secure.phabricator.com/D17014
Summary:
Fixes T11955. Currently, milestones have an internal name of "Parent (Milestone) ...".
This makes them look like they're prefix matches for "Parent", but they're actually prefix matches for "Milestone".
Reorder the names so that the internal name is "Milestone Parent ...".
Test Plan: Created a project "AAA" with milestone "BBB". Searched for "AAA", found "AAA" and milestone "AAA (BBB)".
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11955
Differential Revision: https://secure.phabricator.com/D17013
Summary:
Fixes T11955. Milestone names are currently tokenizing and indexing awkwardly. For example, "A (B C D)" becomes the tokens "A", "(B", "C" and "D)".
The token "(B" can't be searched for since "(" is tokenized on the client.
Instead, tokenize "A (B C D)" into "A", "B", "C", "D".
Test Plan:
- Added unit tests.
- Used `bin/search index --type project --force` to reindex.
- Searched for "A", "B", "C", "D", etc., for real examples.
- Now, found milestones more consistently.
- Also serached for `viewer()`, `members()`, etc.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11955
Differential Revision: https://secure.phabricator.com/D17012
Summary: Fixes T11982. Currently, configuring a custom logo and then setting the policy restrictively locks off the whole install.
Test Plan:
- Configured `ui.logo`.
- Searched for the file PHID in global search to find the underlying file.
- Set the policy to something restrictive ("only me").
- Purged cache (`bin/cache purge --purge-all`).
- Restarted webserver to nuke APC.
- Loaded a page as a different user.
- Before change: policy exception while trying to load the logo.
- After change: fallback to default logo.
- Loaded page as user who can see the logo, got custom logo.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T11982
Differential Revision: https://secure.phabricator.com/D17011
Summary:
Ref T11553. With some regularity, users make various configuration mistakes which we can detect by making a request to ourselves.
I use a magical header to make this request because we want to test everything else (parameters, path).
- Fixes T4854, probably. Tries to detect mod_pagespeed by looking for a header. This is a documentation-based "fix", I didn't actually install mod_pagespeed or formally test this.
- Fixes T6866. We now test for parameters (e.g., user somehow lost "QSA").
- Ref T6709. We now test that stuff is decoded exactly once (e.g., user somehow lost "B").
- Fixes T4921. We now test that Authorization survives the request.
- Fixes T2226. Adds a setup check to determine whether gzip is enabled on the web server, and attempts to enable it at the PHP level.
- Fixes `<space space newline newline space><?php` in `preamble.php`.
Test Plan: Tested all of these setup warnings, although mostly by faking them.
Reviewers: joshuaspence, chad
Reviewed By: chad
Subscribers: Korvin
Maniphest Tasks: T4854, T4921, T6709, T6866, T11553, T2226
Differential Revision: https://secure.phabricator.com/D12622
