Summary: Depends on D20668. Ref T13343. Just an easy cleanup/simplification while I'm here.
Test Plan: `grep` for `getActionConstant()`
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13343
Differential Revision: https://secure.phabricator.com/D20669
Summary:
Fixes T13326. In D20571, I slightly generalized construction of an iterator over a set of files, but missed some code in other "bin/files ..." commands which was also affected.
Today, basically all of these workflows define their own "--all" and "names" flags. Pull these definitions up and implement them more consistently.
Test Plan: Ran multiple different `bin/files` commands with different combinations of arguments, saw consistent handling of iterator construction.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13326
Differential Revision: https://secure.phabricator.com/D20614
Summary:
Ref T13306. Currently, there's no easy way to import a third-party local-disk file dump into a Phacility instance.
Add some more options to `bin/files migrate` to support this. In particular, this enables:
```
$ ./bin/files --from-engine local-disk --engine amazon-s3 --local-disk-source path/to/backup
```
...to import these files into S3 directly.
These are general-purpose options and theoretically useful in other use cases, although realistically those cases are probably very rare.
Test Plan: Used `bin/files` with the new options to move files in and out of local disk storage in an arbitrary backup directory. Got clean exports/imports.
Reviewers: amckinley
Maniphest Tasks: T13306
Differential Revision: https://secure.phabricator.com/D20571
Summary: Ref T13250. See D20149. Mostly: clarify semantics. Partly: remove magic "null" behavior.
Test Plan: Poked around, but mostly just inspection since these are pretty much one-for-one.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: yelirekim
Maniphest Tasks: T13250
Differential Revision: https://secure.phabricator.com/D20154
Summary:
In ~2012, the first of these options was added because someone who hates dogs and works at Asana also hated `[Differential]` in the subject line. The use case there was actually //removing// the text, not changing it, but I made the prefix editable since it seemed like slightly less of a one-off.
These options are among the dumbest and most useless config options we have and very rarely used, see T11760. A very small number of instances have configured one of these options.
Newer applications stopped providing these options and no one has complained.
You can get the same effect with `translation.override`. Although I'm not sure we'll keep that around forever, it's a reasonable replacement today. I'll call out an example in the changelog to help installs that want to preserve this option.
If we did want to provide this, it should just be in {nav Applications > Settings} for each application, but I think it's wildly-low-value and "hack via translations" or "local patch" are entirely reasonable if you really want to change these strings.
Test Plan: Grepped for `subject-prefix`.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19993
Summary:
Fixes T7477. Fixes T13066. Currently, inbound mail is processed by the first receiver that matches any "To:" address. "Cc" addresses are ignored.
**To, CC, and Multiple Receivers**
Some users would like to be able to "Cc" addresses like `bugs@` instead of having to "To" the address, which makes perfect sense. That's the driving use case behind T7477.
Since users can To/Cc multiple "create object" or "update object" addresses, I also wanted to make the behavior more general. For example, if you email `bugs@` and also `paste@`, your mail might reasonably make both a Task and a Paste. Is this useful? I'm not sure. But it seems like it's pretty clearly the best match for user intent, and the least-surprising behavior we can have. There's also no good rule for picking which address "wins" when two or more match -- we ended up with "address order", which is pretty arbitrary since "To" and "Cc" are not really ordered fields.
One part of this change is removing `phabricator.allow-email-users`. In practice, this option only controlled whether users were allowed to send mail to "Application Email" addresses with a configured default author, and it's unlikely that we'll expand it since I think the future of external/grey users is Nuance, not richer interaction with Maniphest/Differential/etc. Since this option only made "Default Author" work and "Default Author" is optional, we can simplify behavior by making the rule work like this:
- If an address specifies a default author, it allows public email.
- If an address does not, it doesn't.
That's basically how it worked already, except that you could intentionally "break" the behavior by not configuring `phabricator.allow-email-users`. This is a backwards compatility change with possible security implications (it might allow email in that was previously blocked by configuration) that I'll call out in the changelog, but I suspect that no installs are really impacted and this new behavior is generally more intuitive.
A somewhat related change here is that each receiver is allowed to react to each individual email address, instead of firing once. This allows you to configure `bugs-a@` and `bugs-b@` and CC them both and get two tasks. Useful? Maybe not, but seems like the best execution of intent.
**Sender vs Author**
Adjacently, T13066 described an improvement to error handling behavior here: we did not distinguish between "sender" (the user matching the email "From" address) and "actor" (the user we're actually acting as in the application). These are different when you're some internet rando and send to `bugs@`, which has a default author. Then the "sender" is `null` and the "author" is `@bugs-robot` or whatever (some user account you've configured).
This refines "Sender" vs "Author". This is mostly a purity/correctness change, but it means that we won't send random email error messages to `@bugs-robot`.
Since receivers are now allowed to process mail with no "sender" if they have some default "actor" they would rather use instead, it's not an error to send from an invalid address unless nothing processes the mail.
**Other**
This removes the "abundant receivers" error since this is no longer an error.
This always sets "external user" mail recipients to be unverified. As far as I can tell, there's no pathway by which we send them email anyway (before or after this change), although it's possible I'm missing something somewhere.
Test Plan:
I did most of this with `bin/mail receive-test`. I rigged the workflow slightly for some of it since it doesn't support multiple addresses or explicit "CC" and adding either would be a bit tricky.
These could also be tested with `scripts/mail/mail_handler.php`, but I don't currently have the MIME parser extension installed locally after a recent upgrade to Mojave and suspect T13232 makes it tricky to install.
- Ran unit tests, which provide significant coverage of this flow.
- Sent mail to multiple Maniphest application emails, got multiple tasks.
- Sent mail to a Maniphest and a Paste application email, got a task and a paste.
- Sent mail to a task.
- Saw original email recorded on tasks. This is a behavior particular to tasks.
- Sent mail to a paste.
- Sent mail to a mock.
- Sent mail to a Phame blog post.
- Sent mail to a Legalpad document.
- Sent mail to a Conpherence thread.
- Sent mail to a poll.
- This isn't every type of supported object but it's enough of them that I'm pretty confident I didn't break the whole flow.
- Sent mail to an object I could not view (got an error).
- As a non-user, sent mail to several "create an object..." addresses.
- Addresses with a default user worked (e.g., created a task).
- Addresses without a default user did not work.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13066, T7477
Differential Revision: https://secure.phabricator.com/D19952
Summary:
Ref T7477. The various "create a new X via email" applications (Paste, Differential, Maniphest, etc) all have a bunch of duplicate code.
The inheritance stack here is generally a little weird. Extend these from a shared parent to reduce the number of callsites I need to change when this API is adjusted for T7477.
Test Plan: Ran unit tests. This will get more thorough testing once more pieces are in place.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T7477
Differential Revision: https://secure.phabricator.com/D19950
Summary:
Depends on D19919. Ref T11351. This method appeared in D8802 (note that "get...Object" was renamed to "get...Transaction" there, so this method was actually "new" even though a method of the same name had existed before).
The goal at the time was to let Harbormaster post build results to Diffs and have them end up on Revisions, but this eventually got a better implementation (see below) where the Harbormaster-specific code can just specify a "publishable object" where build results should go.
The new `get...Object` semantics ultimately broke some stuff, and the actual implementation in Differential was removed in D10911, so this method hasn't really served a purpose since December 2014. I think that broke the Harbormaster thing by accident and we just lived with it for a bit, then Harbormaster got some more work and D17139 introduced "publishable" objects which was a better approach. This was later refined by D19281.
So: the original problem (sending build results to the right place) has a good solution now, this method hasn't done anything for 4 years, and it was probably a bad idea in the first place since it's pretty weird/surprising/fragile.
Note that `Comment` objects still have an unrelated method with the same name. In that case, the method ties the `Comment` storage object to the related `Transaction` storage object.
Test Plan: Grepped for `getApplicationTransactionObject`, verified that all remaining callsites are related to `Comment` objects.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19920
Summary:
Depends on D19918. Ref T11351. In D19918, I removed all calls to this method. Now, remove all implementations.
All of these implementations just `return $timeline`, only the three sites in D19918 did anything interesting.
Test Plan: Used `grep willRenderTimeline` to find callsites, found none.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T11351
Differential Revision: https://secure.phabricator.com/D19919
Summary:
Ref T13216. Ref T13217. Depends on D19800. This fixes all of the remaining query warnings that pop up when you run "arc unit --everything".
There's likely still quite a bit of stuff lurking around, but hopefully this covers a big set of the most common queries.
Test Plan: Ran `arc unit --everything`. Before change: lots of query warnings. After change: no query warnings.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13217, T13216
Differential Revision: https://secure.phabricator.com/D19801
Summary: Depends on D19785. Ref T13217. This converts many of the most common clause construction pathways to the new %Q / %LQ / %LO / %LA / %LJ semantics.
Test Plan: Browsed around a bunch, saw fewer warnings and no obvious behavioral errors. The transformations here are generally mechanical (although I did them by hand).
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: hach-que
Maniphest Tasks: T13217
Differential Revision: https://secure.phabricator.com/D19789
Summary:
Ref T13164. See PHI766. Currently, when file data is stored in small chunks, we submit each chunk to the indexing engine.
However, chunks are never surfaced directly and can never be found via any search/query, so this work is pointless. Just skip it.
(It would be nice to do this a little more formally on `IndexableInterface` or similar as `isThisAnIndexableObject()`, but we'd have to add like a million empty "yes, index this always" methods to do that, and it seems unlikely that we'll end up with too many other objects like these.)
Test Plan:
- Ran `bin/harbormaster rebuild-log --id ... --force` before and after change, saw about 200 fewer queries after the change.
- Uploaded a uniquely named file and searched for it to make sure I didn't break that.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19563
Summary:
See PHI749. Ref T13164. We currently misdetect files starting with `[submodule ...` as JSON.
Make this a bit stricter:
- If the file is short, just see if it's actually literally real JSON.
- If the file is long, give up.
This should get the right result in pretty much all the cases people care about, I think. We could make the long-file guesser better some day.
Test Plan: Detected a `[submodule ...` file (no longer JSON) and a `{"duck": "quack"}` file (still JSON).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13164
Differential Revision: https://secure.phabricator.com/D19544
Summary:
See T7148. This just cheats us out of a weird sort of race where we:
- Dump an instance, including some `F123` which is a temporary file which expires in 3 minutes.
- A few minutes later, the daemons delete the data for that file.
- A few minutes after that, we try to `bin/files migrate --copy` to copy the data from S3 into the MySQL blob store.
- This fails since the data is already gone.
Instead, just skip these files since they're already dead to us.
Test Plan: Faked this locally, will migrate the PHI769 instance on `aux001`.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D19536
Summary:
Fixes T13135. See PHI633. For at least some video files with legitimate MIME type "video/quicktime", Chrome can play them but refuses to if the `<source />` tag has a `type="video/quicktime"` attribute.
To trick Chrome into giving these videos the old college try, omit the "type" attribute. Chrome then tries to play the video, seems to realize it can, and we're back on track.
Since the "type" attribute is theoretically only useful to help browsers select among multiple different alternatives and we're only presenting one alternative, this seems likely safe and reasonable. Omitting "type" also validates. It's hard to be certain that this won't cause any collateral damage, but intuitively it seems like it should be safe and I wasn't able to identify any problems.
Test Plan:
- Watched a "video/quicktime" MP4 cat video in Chrome/Safari/Firefox.
- See T13135 for discussion, context, and discussion of the behavior of some smaller reproduction cases.
Reviewers: amckinley, asherkin
Reviewed By: amckinley
Maniphest Tasks: T13135
Differential Revision: https://secure.phabricator.com/D19424
Summary:
Fixes T13132. I removed this branch in D19156 when tightening the logic for the new CSP header, but there's a legitimate need for it: downloading files via `arc download`, or more generally being an API consumer of files.
This is not completely safe, but attacks I'm aware of (particularly, cookie fixation, where an attacker could potentially force a victim to become logged in to an account they control) are difficult and not very powerful. We already issue clear setup advice about the importance of configuring this option ("Phabricator is currently configured to serve user uploads directly from the same domain as other content. This is a security risk.") and I think there's significant value in letting API clients just GET file data without having to jump through a lot of weird hoops.
Test Plan:
- With `security.alternate-file-domain` off, tried to `arc download` a file.
- Before: downloaded an HTML dialog page.
- After: downloaded the file.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13132
Differential Revision: https://secure.phabricator.com/D19421
Summary:
See PHI604. Ref T13130. Ref T13105. There's currently no way to turn blame off in Diffusion. Add a "Hide Blame" option to the "View Options" dropdown so it can be toggled off.
Also fix a couple of bugs around this: for example, if you loaded a Jupyter notebook and then switched to "Source" view, blame would incorrectly fail to activate because the original rendering of the "stage" used an asynchronous engine so `willRenderRef()` wasn't called to populate blame.
Test Plan:
- Viewed a source file, toggled blame off/on, reloaded page to see state stick in URL.
- Viewed a Jupyter notebook, toggled to "Source" view, saw blame.
- Viewed stuff in Files (no blame UI options).
- Tried to do some invalid stuff like toggle blame on a non-blame engine (options disable properly).
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13130, T13105
Differential Revision: https://secure.phabricator.com/D19414
Summary:
Ref T13103. Locally, I managed to break the data for a bunch of files by doing `git clean -df` in a working copy that I'd updated to a commit from many many years ago. Since `conf/local.json` wasn't on the gitignore list many years ago, this removed it, and I lost my encryption keyring.
I've symlinked my local config to a version-controlled file now to avoid this specific type of creative self-sabotage in the future, but this has exposed a few cases where we could handle things more gracefully.
One issue is that if your favicon is customized but the file it points at can't actually be loaded, we fail explosively and you really can't do anything to move forward except somehow guess that you need to fix your favicon. Instead, recover more gracefully.
Test Plan:
- Configure file encryption.
- Configure a favicon.
- Remove the encryption key from your keyring.
- Purge Phabricator's caches.
- Before: you pretty much dead-end on a fatal that's hard to understand/fix.
- After: everything works except your favicon.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13103
Differential Revision: https://secure.phabricator.com/D19406
Summary:
Depends on D19377. Ref T13125. Ref T13124. Ref T13105. Coverage reporting in Diffusion didn't initially survive the transition to Document Engine; restore it.
This adds some tentative/theoretical support for multiple columns of coverage, but no way to actually produce them in the UI. For now, the labels, codes, and colors are hard coded.
Test Plan:
Added coverage with `diffusion.updatecoverage`, saw coverage in the UI:
{F5525542}
Hovered over coverage, got labels and highlighting.
Double-checked labels for "N" (Not Executable) and "U" (Uncovered). See PHI577.
Faked some multi-column coverage, but you can't currently get this yourself today:
{F5525544}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13125, T13124, T13105
Differential Revision: https://secure.phabricator.com/D19378
Summary: Ref T13105. This needs refinement but blame sort of works again, now.
Test Plan: Viewed files in Diffusion and Files; saw blame in Diffusion when viewing in source mode.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19309
Summary: Ref T13105. Ref T13047. This makes symbol indexes work with DocumentEngine in Files, and restores support in Diffusion.
Test Plan: Command-clicked stuff, got taken to the symbol index with reasonable metadata in Diffusion, Differential and Files.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Maniphest Tasks: T13105, T13047
Differential Revision: https://secure.phabricator.com/D19307
Summary: Ref T13105. See also T7895. When users render very large files as source via DocumentEngine, skip highlighting.
Test Plan: Fiddled with the limit, viewed files, saw highlighting degrade.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19306
Summary:
Ref T13105. Fixes some issues with line linking and highlighting under DocumentEngine:
- Adding `$1-3` to the URI didn't work correctly with query parameters.
- Reading `$1-3` from the URI didn't work correctly because Diffusion parses them slightly abnormally.
Test Plan: Clicked/dragged lines to select them. Observed URI. Reloaded page, got the right selection.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19305
Summary:
Ref T13105. This breaks about 9,000 features but moves Diffusion to DocumentEngine for rendering. See T13105 for a more complete list of all the broken stuff.
But you can't bake a software without breaking all the features every time you make a change, right?
Test Plan: Viewed various files in Diffusion, used DocumentEngine features like highlighting and rendering engine selection.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Subscribers: mydeveloperday
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19302
Summary:
Ref T13105. This separates document rendering from the Controllers which trigger it so it can be reused elsewhere (notably, in Diffusion).
This shouldn't cause any application behavior to change, it just pulls the rendering logic out so it can be reused elsewhere.
Test Plan: Viewed various types of files in Files; toggled rendering, highlighting, and encoding.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19301
Summary: Depends on D19273. Ref T13105. Adds "Change Text Encoding..." and "Highlight As..." options when rendering documents, and makes an effort to automatically detect and handle text encoding.
Test Plan:
- Uploaded a Shift-JIS file, saw it auto-detect as Shift-JIS.
- Converted files between encodings.
- Highlighted various things as "Rainbow", etc.
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19274
Summary:
Ref T13105. This is silly, but "py" and "python" end up in different places today, and "py" is ~100x faster than "python".
See also T3626 for longer-term plans on this.
Test Plan: Reloaded a Jupyter notebook, saw it render almost instantly instead of taking a few seconds.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19273
Summary: Depends on D19259. Ref T13105. Some examples represent image data as `["da", "ta"]` while others represent it as `"data"`. Accept either.
Test Plan: Rendered example notebooks with both kinds of images.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19260
Summary: Ref T13105. Currently, logged-out users can't render documents via the endpoint even if they otherwise have access to the file.
Test Plan: Viewed a file as a logged-out user and re-rendered it via Ajax.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19258
Summary:
Ref T13105. This adds various small client-side improvements to document rendering.
- In the menu, show which renderer is in use.
- Make linking to lines work.
- Make URIs persist information about which rendering engine is in use.
- Improve the UI feedback for transitions between document types.
- Load slower documents asynchronously by default.
- Discard irrelevant requests if you spam the view menu.
Test Plan: Loaded files, linked to lines, swapped between modes, copy/pasted URLs.
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19256
Summary: Depends on D19254. This engine just formats JSON files in a nicer, more readable way.
Test Plan: Looked at some JSON files, saw them become formatted nicely.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Differential Revision: https://secure.phabricator.com/D19255
Summary: Ref T13105. Allow normal text files to be rendered as documents, and add a "source code" rendering engine.
Test Plan: Viewed some source code.
Reviewers: mydeveloperday
Reviewed By: mydeveloperday
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19254
Summary:
Depends on D19252. Ref T13105. This very roughly renders Jupyter notebooks.
It's probably better than showing the raw JSON, but not by much.
Test Plan:
- Viewed various notebooks with various cell types, including markdown, code, stdout, stderr, images, HTML, and Javascript.
- HTML and Javascript are not live-fired since they're wildly dangerous.
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19253
Summary:
Depends on D19251. Ref T13105. This adds rendering engine support for PDFs.
It doesn't actually render them, it just renders a link which you can click to view them in a new window. This is much easier than actually rendering them inline and at least 95% as good most of the time (and probably more-than-100%-as-good some of the time).
This makes PDF a viewable MIME type by default and adds a narrow CSP exception for it. See also T13112.
Test Plan:
- Viewed PDFs in Files, got a link to view them in a new tab.
- Clicked the link in Safari, Chrome, and Firefox; got inline PDFs.
- Verified primary CSP is still `object-src 'none'` with `curl ...`.
- Interacted with the vanilla lightbox element to check that it still works.
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19252
Summary:
Ref T13105. Although Markdown is trickier to deal with, we can handle Remarkup easily.
This may need some support for encoding options.
Test Plan: Viewed `.remarkup` files, got remarkup document presentation by default. Viewed other text files, got an option to render as remarkup.
Reviewers: avivey
Reviewed By: avivey
Subscribers: mydeveloperday, avivey
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19251
Summary:
Depends on D19238. Ref T13105. Give document engines some reasonable automatic support for degrading gracefully when someone tries to hexdump a 100MB file or similar.
Also, make "Video" sort above "Audio" for files which could be rendered either way.
Test Plan: Viewed audio, video, image, and other files. Adjusted limits and saw full, partial, and fallback/error rendering.
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19239
Summary: Depends on D19237. Ref T13105. This adds a (very basic) "Hexdump" engine (mostly just to have a second option to switch to) and a selector for choosing view modes.
Test Plan: Viewed some files, switched between audio/video/image/hexdump.
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19238
Summary:
Ref T13105. This change begins modularizing document rendering. I'm starting in Files since it's the use case with the smallest amount of complexity.
Currently, we hard-coding the inline rendering for images, audio, and video. Instead, use the modular engine pattern to make rendering flexible and extensible.
There aren't any options for switching modes yet and none of the renderers do anything fancy. This API is also probably very unstable.
Test Plan: Viewwed images, audio, video, and other files. Saw reasonable renderings, with "nothing can render this" for any other file type.
Maniphest Tasks: T13105
Differential Revision: https://secure.phabricator.com/D19237
Summary:
Depends on D19223. Ref T13106. When we're loading a file, we currently test if it's a transformed version of another file (usually, a thumbnail) and apply policy behavior if it is.
We know that builtins and profile images are never transforms and that the policy behavior for these files doesn't matter anyway. Skip loading transforms for these files.
Test Plan: Saw local queries drop from 146 to 139 with no change in behavior.
Maniphest Tasks: T13106
Differential Revision: https://secure.phabricator.com/D19224
Summary:
Depends on D19222. Ref T13106. We currently execute an edge query (and possibly an object query) when loading builtin files, but this is never necessary because we know these files are always visible.
Instead, skip this logic for builtin files and profile image files; these files have global visibility and will never get a different policy result because of file attachment information.
(In theory, we could additionally skip this for files with the most open visibility policy or some other trivially visible policy like the user's PHID, but we do actually care about the attachment data some of the time.)
Test Plan: Saw queries drop from 151 to 145 on local test page. Checked file attachment data in Files, saw it still working correctly.
Maniphest Tasks: T13106
Differential Revision: https://secure.phabricator.com/D19223
Summary: Ref T13103. Make favicons customizable, and perform dynamic compositing to add marker to indicate things like "unread messages".
Test Plan: Viewed favicons in Safari, Firefox and Chrome. With unread messages, saw pink dot composited into icon.
Maniphest Tasks: T13103
Differential Revision: https://secure.phabricator.com/D19209
Summary: Depends on D19194. Fixes T4190. This should be in good-enough shape now to release and support more generally.
Test Plan: Used `{image ...}` in remarkup.
Maniphest Tasks: T4190
Differential Revision: https://secure.phabricator.com/D19195
Summary: Depends on D19193. Ref T13101. Fixes T4190. Before we render a fancy AJAX placeholder, check if we already have a valid cache for the image. If we do, render a direct `<img />` tag. This is a little cleaner and, e.g., avoids flicker in Safari, at least.
Test Plan: Rendered `{image ...}` rules in remarkup with new and existing URIs.
Maniphest Tasks: T13101, T4190
Differential Revision: https://secure.phabricator.com/D19194
Summary:
Depends on D19192. Ref T4190. Ref T13101. Instead of directly including the proxy endpoint with `<img src="..." />`, emit a placeholder and use AJAX to make the request. If the proxy fetch fails, replace the placeholder with an error message.
This isn't the most polished implementation imaginable, but it's much less mysterious about errors.
Test Plan: Used `{image ...}` for valid and invalid images, got images and useful error messages respectively.
Maniphest Tasks: T13101, T4190
Differential Revision: https://secure.phabricator.com/D19193
Summary:
Ref T13101. Ref T4190. This rule is currently single-phase but I'd like to check for a valid proxied image in cache already and just emit an `<img ... />` tag pointing at it if we have one.
To support batching these lookups, split the rule into a parse phase (where we extract URIs) and a markup phase (where we build tags).
Test Plan: Used `{img ...}` in Remarkup with no apparent behavioral changes. (This change should do nothing on its own.)
Maniphest Tasks: T13101, T4190
Differential Revision: https://secure.phabricator.com/D19192
