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
Summary:
See PHI413. You can pre-generate these with `bin/people profileimage --all`, but they're needlessly expensive to generate.
Streamline the workflow and cache some of the cacheable parts to reduce the generation cost.
Test Plan:
- Ran `bin/people profileimage --all` and saw cost drop from {nav 15.801s > 4.839s}.
- Set `defaultProfileImagePHID` to `NULL` in `phabricator_user.user` and purged caches with `bin/cache purge --all`.
- Loaded user directory.
- Saw default images regenerate relatively quickly.
Differential Revision: https://secure.phabricator.com/D19168
Summary:
Depends on D19155. Ref T13094. Ref T4340.
We can't currently implement a strict `form-action 'self'` content security policy because some file downloads rely on a `<form />` which sometimes POSTs to the CDN domain.
Broadly, stop generating these forms. We just redirect instead, and show an interstitial confirm dialog if no CDN domain is configured. This makes the UX for installs with no CDN domain a little worse and the UX for everyone else better.
Then, implement the stricter Content-Security-Policy.
This also removes extra confirm dialogs for downloading Harbormaster build logs and data exports.
Test Plan:
- Went through the plain data export, data export with bulk jobs, ssh key generation, calendar ICS download, Diffusion data, Paste data, Harbormaster log data, and normal file data download workflows with a CDN domain.
- Went through all those workflows again without a CDN domain.
- Grepped for affected symbols (`getCDNURI()`, `getDownloadURI()`).
- Added an evil form to a page, tried to submit it, was rejected.
- Went through the ReCaptcha and Stripe flows again to see if they're submitting any forms.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13094, T4340
Differential Revision: https://secure.phabricator.com/D19156
Summary:
Depends on D19154. Ref T13094. This controller was removed at some point and this route no longer works.
I plan to add a new `download/` route to let us tighten the `form-action` Content Security Policy.
Test Plan: Grepped for the route and controller, no hits.
Maniphest Tasks: T13094
Differential Revision: https://secure.phabricator.com/D19155
Summary: See D19117. Instead of automatically figuring this out inside `phutil_tag()`, explicitly add rel="noreferrer" at the application level to all external links.
Test Plan:
- Grepped for `_blank`, `isValidRemoteURIForLink`, checked all callsites for user-controlled data.
- Created a link menu item, verified noreferrer in markup.
- Created a link custom field, verified no referrer in markup.
- Verified noreferrer for `{nav href=...}`.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Differential Revision: https://secure.phabricator.com/D19118
Summary:
Depends on D19009. Ref T13053. For "Must Encrypt" mail, we must currently strip the "Thread-Topic" header because it sometimes contains sensitive information about the object.
I don't actually know if this header is useful or anyting uses it. My understanding is that it's an Outlook/Exchange thing, but we also implement "Thread-Index" which I think is what Outlook/Exchange actually look at. This header may have done something before we implemented "Thread-Index", or maybe never done anything. Or maybe older versions of Excel/Outlook did something with it and newer versions don't, or do less. So it's possible that an even better fix here would be to simply remove this, but I wasn't able to convince myself of that after Googling for 10 minutes and I don't think it's worth hours of installing Exchange/Outlook to figure out. Instead, I'm just trying to simplify our handling of this header for now, and maybe some day we'll learn more about Exchange/Outlook and can remove it.
In a number of cases we already use the object monogram or PHID as a "Thread-Topic" without users ever complaining, so I think that if this header is useful it probably isn't shown to users, or isn't shown very often (e.g., only in a specific "conversation" sub-view?). Just use the object PHID (which should be unique and stable) as a thread-topic, everywhere, automatically.
Then allow this header through for "Must Encrypt" mail.
Test Plan: Processed some local mail, saw object PHIDs for "Thread-Topic" headers.
Reviewers: amckinley
Maniphest Tasks: T13053
Differential Revision: https://secure.phabricator.com/D19012
Summary:
Depends on D18952. Ref T13049. For files larger than 8MB, we need to engage the chunk storage engine. `PhabricatorFile::newFromFileData()` always writes a single chunk, and can't handle files larger than the mandatory chunk threshold (8MB).
Use `IteratorUploadSource`, which can, and "stream" the data into it. This should raise the limit from 8MB to 2GB (maximum size of a string in PHP).
If we need to go above 2GB we could stream CSV and text pretty easily, and JSON without too much trouble, but Excel might be trickier. Hopefully no one is trying to export 2GB+ datafiles, though.
Test Plan:
- Changed the JSON exporter to just export 8MB of the letter "q": `return str_repeat('q', 1024 * 1024 * 9);`.
- Before change: fatal, "no storage engine can store this file".
- After change: export works cleanly.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13049
Differential Revision: https://secure.phabricator.com/D18953
Summary:
See <https://discourse.phabricator-community.org/t/files-created-from-repository-contents-slightly-over-one-chunk-in-size-are-truncated-to-exactly-one-chunk-in-size/988/1>. Three issues here:
- When we finish reading `git cat-file ...` or whatever, we can end up with more than one chunk worth of bytes left in the internal buffer if the read is fast. Use `while` instead of `if` to make sure we write the whole buffer.
- Limiting output with `setStdoutSizeLimit()` isn't really a reliable way to limit the size if we're also reading from the buffer. It's also pretty indirect and confusing. Instead, just let the `FileUploadSource` explicitly implement a byte limit in a straightforward way.
- We weren't setting the time limit correctly on the main path.
Overall, this could cause >4MB files to "write" as 4MB files, with the rest of the file left in the UploadSource buffer. Since these files were technically under the limit, they could return as valid. This was intermittent.
Test Plan:
- Pushed a ~4.2MB file.
- Reloaded Diffusion a bunch, sometimes saw the `while/if` buffer race and produce a 4MB file with a prompt to download it. (Other times, the buffer worked right and the page just says "this file is too big, sorry").
- Applied patches.
- Reloaded Diffusion a bunch, no longer saw bad behavior or truncated files.
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18885
Summary:
Fixes T13031. "Enormous" changes are basically changes which are too large to hold in memory, although the actual definition we use today is "more than 1GB of change text or `git diff` runs for more than 15 minutes".
If an install configures a Herald content rule like "when content matches /XYZ/, do something" and then a user pushes a 30 GB source file, we can't put it into memory to `preg_match()` it. Currently, the way to handle this case is to write a separate Herald rule that rejects enormous changes. However, this isn't obvious and means the default behavior is unsafe.
Make the default behavior safe by rejecting these changes with a message, similar to how we reject "dangerous" changes (which permanently delete or overwrite history) by default.
Also, change a couple of UI strings from "Enormous" to "Very Large" to reduce ambiguity. See <https://discourse.phabricator-community.org/t/herald-enormous-check/822>.
Test Plan: Changed the definition of "enormous" from 1GB to 1 byte. Pushed a change; got rejected. Allowed enormous changes, pushed, got rejected by a Herald rule. Disabled the Herald rule, pushed, got a clean push. Prevented enormous changes again. Grepped for "enormous" elsewhere in the UI.
Reviewers: amckinley
Reviewed By: amckinley
Subscribers: joshuaspence
Maniphest Tasks: T13031
Differential Revision: https://secure.phabricator.com/D18850
Summary:
Depends on D18828. Ref T7789. See <https://discourse.phabricator-community.org/t/git-lfs-fails-with-large-images/584>.
Currently, when you upload a large (>4MB) image, we may try to assess the dimensions for the image and for each individual chunk.
At best, this is slow and not useful. At worst, it fatals or consumes a ton of memory and I/O we don't need to be using.
Instead:
- Don't try to assess dimensions for chunked files.
- Don't try to assess dimensions for the chunks themselves.
- Squelch errors for bad data, etc., that `gd` can't actually read, since we recover sensibly.
Test Plan:
- Created a 2048x2048 PNG in Photoshop using the "Random Noise" filter which weighs 8.5MB.
- Uploaded it.
- Before patch: got complaints in log about `imagecreatefromstring()` failing, although the actual upload went OK in my environment.
- After patch: clean log, no attempt to detect the size of a big image.
- Also uploaded a small image, got dimensions detected properly still.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T7789
Differential Revision: https://secure.phabricator.com/D18830
Summary:
Ref T10405. If `gd` is not installed, a number of unit tests currently fail because they generate synthetic users and try to composite profile pictures for them.
Instead, just fall back to a static default if `gd` is not available.
Test Plan: Faked this pathway, created a new user, saw a default profile picture.
Reviewers: amckinley, lpriestley, avivey
Reviewed By: avivey
Maniphest Tasks: T10405
Differential Revision: https://secure.phabricator.com/D18812
Summary:
Depends on D18792. Fixes T13024. Fixes T89198. Currently, when users are logging in initially (for example, need to enter MFA) we show more menu items than we should.
Notably, we may show some personalized/private account details, like the number of unread notifications (probably not relevant) or a user's saved queries (possibly sensitive). At best these are misleading (they won't work yet) and there's an outside possibility they leak a little bit of private data.
Instead, nuke everything except "Log Out" when users have partial sessions.
Test Plan:
Hit a partial session (MFA required, email verification required) and looked at the menu. Only saw "Log Out".
{F5297713}
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13024
Differential Revision: https://secure.phabricator.com/D18793
Summary: See PHI176. Depends on D18733. We issue a query when deleting files that currently doesn't hit any keys.
Test Plan:
Ran `./bin/remove destroy --force --trace F56376` to get the query.
Ran ##SELECT * FROM `file` WHERE storageEngine = 'blob' AND storageHandle = '23366' LIMIT 1## before and after the change.
Before:
```
mysql> explain SELECT * FROM `file` WHERE storageEngine = 'blob' AND storageHandle = '23366' LIMIT 1;
+----+-------------+-------+------+---------------+------+---------+------+-------+-------------+
| id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra |
+----+-------------+-------+------+---------------+------+---------+------+-------+-------------+
| 1 | SIMPLE | file | ALL | NULL | NULL | NULL | NULL | 33866 | Using where |
+----+-------------+-------+------+---------------+------+---------+------+-------+-------------+
1 row in set (0.01 sec)
```
After:
```
mysql> explain SELECT * FROM `file` WHERE storageEngine = 'blob' AND storageHandle = '23366' LIMIT 1;
+----+-------------+-------+------+---------------+------------+---------+-------------+------+------------------------------------+
| id | select_type | table | type | possible_keys | key | key_len | ref | rows | Extra |
+----+-------------+-------+------+---------------+------------+---------+-------------+------+------------------------------------+
| 1 | SIMPLE | file | ref | key_engine | key_engine | 388 | const,const | 190 | Using index condition; Using where |
+----+-------------+-------+------+---------------+------------+---------+-------------+------+------------------------------------+
1 row in set (0.00 sec)
```
Reviewers: amckinley
Reviewed By: amckinley
Differential Revision: https://secure.phabricator.com/D18734
Summary:
Ref T13008. Depends on D18701. The overall goal here is to make turning `enable_post_data_reading` off not break things, so we can run rate limiting checks before we read file uploads.
The biggest blocker for this is that turning it off stops `$_FILES` from coming into existence.
This //appears// to mostly work. Specifically:
- Skip the `max_post_size` check when POST is off, since it's meaningless.
- Don't read or scrub $_POST at startup when POST is off.
- When we rebuild REQUEST and POST before processing requests, do multipart parsing if we need to and rebuild FILES.
- Skip the `is_uploaded_file()` check if we built FILES ourselves.
This probably breaks a couple of small things, like maybe `__profile__` and other DarkConsole triggers over POST, and probably some other weird stuff. The parsers may also need more work than they've received so far.
I also need to verify that this actually works (i.e., lets us run code without reading the request body) but I'll include that in the change where I update the actual rate limiting.
Test Plan:
- Disabled `enable_post_data_reading`.
- Uploaded a file with a vanilla upload form (project profile image).
- Uploaded a file with drag and drop.
- Used DarkConsole.
- Submitted comments.
- Created a task.
- Browsed around.
Reviewers: amckinley
Reviewed By: amckinley
Maniphest Tasks: T13008
Differential Revision: https://secure.phabricator.com/D18702
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:
Fixes T12948. See that task for substantial discussion and context. Briefly:
- This workflow is very old, and won't work for large (>2GB) files.
- This workflow has become more dangerous than it once was, and can fail in several ways that delete data and/or make recovery much more difficult (see T12948 for more discussion).
- This was originally added in D6068, which is a bit muddled, but looks like "one install ran into a weird issue so I wrote a script for them"; this would be a Consulting/Support issue and not come upstream today. I can't identify any arguments for retaining this workflow there, at least.
Test Plan:
- Grepped for `files purge`, got nothing.
- Grepped for `purge`, looked for anything that looked like instructions or documentation, got nothing.
I don't recall recommending anyone run this script in many years, and didn't even remember that it existed or what it did when T12948 was reported, so I believe it is not in widespread use.
Reviewers: joshuaspence, chad
Reviewed By: joshuaspence
Maniphest Tasks: T12948
Differential Revision: https://secure.phabricator.com/D18384
Summary: Cursory research indicates that "login" is a noun, referring to a form, and "log in" is a verb, referring to the action of logging in. I went though every instances of 'login' I could find and tried to clarify all this language. Also, we have "Phabricator" on the registration for like 4-5 times, which is a bit verbose, so I tried to simplify that language as well.
Test Plan: Tested logging in and logging out. Pages feel simpler.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18322
Summary: Builds out a map for icon->image in Projects, selects the icon's image as the default project image if there is no custom image chosen by the user.
Test Plan: Select various icons, see image change. Test choose picture, pick a new image.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18174
Summary:
Ref T12857. This is generally fairly fuzzy for now, but here's something concrete: when we build a large file with `diffusion.filecontentquery`, we compute the MIME type of all chunks, not just the initial chunk.
Instead, pass a dummy MIME type to non-initial chunks so we don't try to compute them. This mirrors logic elsewhere, in `file.uploadchunk`. This should perhaps be centralized at some point, but it's a bit tricky since the file doesn't know that it's a chunk until later.
Also, clean up the `TempFile` immediately -- this shouldn't actually affect anything, but we don't need it to live any longer than this.
Test Plan:
- Made `hashFileContent()` return `null` to skip the chunk cache.
- Added `phlog()` to the MIME type computation.
- Loaded a 12MB file in Diffusion.
- Before patch: Saw 3x MIME type computations, one for each 4MB chunk.
- After patch: Saw 1x MIME type computation, for initial chunk only.
Reviewers: chad, amckinley
Reviewed By: chad
Maniphest Tasks: T12857
Differential Revision: https://secure.phabricator.com/D18138
Summary: Fixes T12828. This API was tightened up D17616, but I missed this callsite.
Test Plan: I've just got a good feeling in my bones.
Reviewers: chad
Reviewed By: chad
Maniphest Tasks: T12828
Differential Revision: https://secure.phabricator.com/D18119
Summary: Ran across a few straglers. Convert to the correct color.
Test Plan: grep for profile-image-button, check profile image selection page
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18096
Summary: Try to dis-ambiguate various button types and colors. Moves `simple` to `phui-button-simple` and moves colors to `button-color`.
Test Plan: Grep for buttons still inline, UIExamples, PHUIX, Herald, and Email Preferences.
Reviewers: epriestley
Reviewed By: epriestley
Subscribers: Korvin
Differential Revision: https://secure.phabricator.com/D18077
