Summary:
Ref T13588. See PHI2198. Some pathways represent SVN commit identifiers (which are numeric) with integers; these raise an exception in `phutil_nonempty_string()`, which is strict.
(Ideally, commit identifiers wouldn't have mixed types, but that's a dramatically less practical change.)
Test Plan: Will deploy.
Maniphest Tasks: T13588
Differential Revision: https://secure.phabricator.com/D21852
Summary: See <https://hackerone.com/reports/1573143>. The pathway for disabling global builtin queries is missing a policy check. Add it.
Test Plan:
- Accessed the "/search/delete/id/.../" URI for a global builtin query as a non-administrator.
- Before patch: could improperly disable queries.
-After patch: proper policy exception.
Differential Revision: https://secure.phabricator.com/D21851
Summary:
Ref T13682. Versioned drafts may have missing or unexpected metadata:
- versioned drafts from an older version of Phabricator may be missing metadata;
- versioned drafts created by an older UI against a newer version of Phabricator may have `null` metadata.
Generally, make these workflows robust to metadata in unexpected formats, so database debris doesn't break the UI.
Test Plan: Simulated debris, interacted with UI.
Maniphest Tasks: T13682
Differential Revision: https://secure.phabricator.com/D21849
Summary: Ref T13682. This prepares for modernizing Slowvote and exposing a more usable API.
Test Plan: Ran migrations, opened and closed polls.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13682
Differential Revision: https://secure.phabricator.com/D21848
Summary: Ref T13682. Prepares for use of API-friendly string constants rather than opaque integers.
Test Plan: Created and edited polls, opening and closing them. Grepped for affected methods.
Maniphest Tasks: T13682
Differential Revision: https://secure.phabricator.com/D21847
Summary: Ref T13682. Use API-friendly string constants instead of opaque integers in Slowvote voting methods.
Test Plan: Created, edited, and voted in polls with various voting methods. Examined database after migrations.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13682
Differential Revision: https://secure.phabricator.com/D21846
Summary: Ref T13682. Extract Slowvote vote types to a separate object, to prepare for turning them into API-friendly string constants.
Test Plan: Created, edited, and voted in Slowvote polls. Grepped for affected constants.
Maniphest Tasks: T13682
Differential Revision: https://secure.phabricator.com/D21845
Summary: Ref T13682. Migrate and update Slowvote to use API-friendly string constants for response visibility, not opaque integers.
Test Plan: Created and edited slowvotes, changing response visibility.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13682
Differential Revision: https://secure.phabricator.com/D21844
Summary: Ref T13682. This change supports modifying these constants to be sensible strings instead of opaque integers.
Test Plan: Created and edited polls.
Maniphest Tasks: T13682
Differential Revision: https://secure.phabricator.com/D21843
Summary:
Ref T13682. Ref T13065. Migrate and update "SlowvotePoll" to use more modern mailkey storage (in the mail application, rather than onboard).
Also make other minor modernization changes.
Test Plan: Created a new poll. Ran migration, verified mail key copied properly in database.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13682, T13065
Differential Revision: https://secure.phabricator.com/D21842
Summary: Ref T13682. Use a more modern PHID key name for this key.
Test Plan: Ran `bin/storage adjust`, got a modern key.
Maniphest Tasks: T13682
Differential Revision: https://secure.phabricator.com/D21841
Summary: Ref T13682. Make the "Attached" list in Files a bit more detailed, and add a "Detach" button.
Test Plan: Tried to detach unrelated, referenced, and attached files. Saw attached files detach.
Maniphest Tasks: T13682
Differential Revision: https://secure.phabricator.com/D21840
Summary:
Ref T13682. When editing a comment, the edit pathway may invoke file PHID extraction without first formalizing an object on the Editor.
Pass the "object" explicitly to generally improve the robustness of this pathway.
Test Plan: Edited a comment.
Maniphest Tasks: T13682
Differential Revision: https://secure.phabricator.com/D21839
Summary: Ref T13682. Many subclasses of "CursorPagedPolicyAwareQuery" have the same implementation of "loadPage()", and this is a sensible default behavior.
Test Plan: Examined changes to verify that all removed methods have the same behavior.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13682
Differential Revision: https://secure.phabricator.com/D21838
Summary: Ref T13682. Allow users to manually attach files which are referenced (but not attached) via the UI.
Test Plan: Reference files via `{F...}`, then attached them via the UI workflow.
Maniphest Tasks: T13682
Differential Revision: https://secure.phabricator.com/D21837
Summary: Ref T13682. Provide a basic UI element for showing file attached to an object.
Test Plan: Viewed objects with attached files in the UI.
Maniphest Tasks: T13682
Differential Revision: https://secure.phabricator.com/D21836
Summary:
Ref T13682. This is a marginal case, but if you quote someone else's text and it has a file reference embedded in it, that isn't good enough to generate "attachment intent".
Since you need both a reference and an explicit attachment, this should never actually affect any user-visible behavior today, but makes the ruleset more thorough.
Test Plan: Dragged and dropped a file, referencing it in either a quoted or unquoted block. Saw it attach only for a quoted block.
Maniphest Tasks: T13682
Differential Revision: https://secure.phabricator.com/D21833
Summary:
Ref T13682. When a user uploads a file, then changes their mind and deletes the reference to the file, we don't actually want to attach the file.
When choosing which files to attach, only attach files which are both referenced in Remarkup and explicitly attached in remarkup metadata.
Test Plan:
- Dropped a file into a comment, submitted it, saw it attach normally.
- Dropped a file into a comment, deleted the reference, submitted it, saw no attachment.
Maniphest Tasks: T13682
Differential Revision: https://secure.phabricator.com/D21832
Summary: Ref T13682. This property may not exist, and should defualt to an empty array if not present.
Test Plan: Created a new object.
Maniphest Tasks: T13682
Differential Revision: https://secure.phabricator.com/D21834
Summary:
Ref T13682. This method originated in D4781 and no longer has any callers; remove it.
(I believe this was from an era where macros didn't have to be on their own line to trigger.)
Test Plan: Grepped for method, found no callers.
Maniphest Tasks: T13682
Differential Revision: https://secure.phabricator.com/D21831
Summary:
Ref T13603. On common edit pathways, extract explicit file attachments from Remarkup. These pathways are affected:
- Objects that use EditEngine and expose a remarkup area via "RemarkupEditField".
- Objects that use EditEngine to generate a comment area.
This is the vast majority of pathways, but not entirely exhaustive.
Test Plan: Created and commented on a task, explicitly attaching images. Saw images attach properly.
Maniphest Tasks: T13603
Differential Revision: https://secure.phabricator.com/D21830
Summary:
Ref T13603. This removes the file attachment edge types, which no longer have readers or writers.
It does not delete the underlying data.
This indirectly removes API access to this edge. As far as I know, this was only used by one customer (see D21480) who has migrated away from Phabricator.
Test Plan: Grepped for edge constants.
Maniphest Tasks: T13603
Differential Revision: https://secure.phabricator.com/D21823
Summary: Ref T13603. Removes all reads and writes to the other half of the legacy edge.
Test Plan:
- Verified I could still see file content accessible only through object attachment.
- This fixes a unit test broken by removal of only half the edge access in the previous change.
Maniphest Tasks: T13603
Differential Revision: https://secure.phabricator.com/D21820
Summary: Ref T13603. Migrate all code which interacts with the "ObjectHasFile" edge to use the "Attachments" table instead.
Test Plan:
- Edited a paste's view policy, confirmed associated file secret was scrambled.
- Verified I could still view paste content as a user who could not naturally view the underlying file.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13603
Differential Revision: https://secure.phabricator.com/D21819
Summary: Ref T13603. Copy all existing file attachment data into the new storage table.
Test Plan: Ran migration, checked attachment table for general reasonableness.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13603
Differential Revision: https://secure.phabricator.com/D21818
Summary: Ref T13603. New transactional file attachment extracts PHIDs earlier than the older mechanism did. Allow either pathway to save content and extract a PHID.
Test Plan:
- Created and modified a paste.
- Confirmed both pathways do old and new storage writes.
Maniphest Tasks: T13603
Differential Revision: https://secure.phabricator.com/D21817
Summary: Ref T13603. This adds a second write to new "attachment" storage to all writers except one in Paste, which creates the file inline.
Test Plan:
- Updated a macro image, confirmed a write to "attachment" storage (transaction pathway).
- Updated a blog profile image, confirmed a write to "attachment" storage (legacy pathway).
Maniphest Tasks: T13603
Differential Revision: https://secure.phabricator.com/D21816
Summary:
Ref T13603. Currently, files are sometimes detached from objects. For example, when you change the image for a Macro, the old image is detached.
This is wrong: the image should remain attached so users who can view the macro can view the complete "alice change the image from X to Y" transaction. To be able to understand the change that was applied, you need to be able to view both files.
All workflows which currently detach files aren't conistent with the modern way applications behave, except maybe one callsite in a unit test, and that one's kind of moot.
Get rid of this stuff and just use PHID extraction to perform file attachment in all cases.
Test Plan: Created and edited macros, verified files were properly attached and remained attached across edits.
Maniphest Tasks: T13603
Differential Revision: https://secure.phabricator.com/D21815
Summary: Ref T13603. Prepare to move this relationship out of edge storage into dedicated storage so it is easier to formalize better in the UI.
Test Plan: Ran `bin/storage upgrade`.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13603
Differential Revision: https://secure.phabricator.com/D21813
Summary: Ref T13603. When processing transactions that include remarkup blocks, stop extracting file references from them for attachment.
Test Plan: Submitted a remarkup block with a file reference, no longer saw automatic file attachment.
Maniphest Tasks: T13603
Differential Revision: https://secure.phabricator.com/D21812
Summary: Ref T13588. This allows "arc unit --everything" to more or less run cleanly on PHP 8.1.
Test Plan: Ran "arc unit --everything".
Maniphest Tasks: T13588
Differential Revision: https://secure.phabricator.com/D21821
Summary:
Ref T13603. Allow "VersionedDraft" to persist remarkup comment area metadata from stacked actions controls.
When files are dragged and dropped, record them as explicit uploads in comment metadata.
Test Plan: Dragged and dropped files into Remarkup stacked action text areas (e.g., in Maniphest), reloaded page, saw metadata persist across reloads.
Maniphest Tasks: T13603
Differential Revision: https://secure.phabricator.com/D21828
Summary: Ref T13603. This will support explicit handling of attached files.
Test Plan: Adjusted new input to have "text" input type, used it alongside additional upcoming changes, saw sensible metadata behavior.
Maniphest Tasks: T13603
Differential Revision: https://secure.phabricator.com/D21827
Summary: Ref T13680. Conpherence may pass values with an integer type to this layer of the stack. These are "supposed" to be strings, but just be accepting.
Test Plan:
- Wrote 100+ messages to a Conpherence room.
- Clicked "Show Older Messages".
- Before: exception, int passed to "phutil_nonempty_string()".
- After: older messages loaded.
Maniphest Tasks: T13680
Differential Revision: https://secure.phabricator.com/D21824
Summary:
Ref T13681. This was introduced in D593, never used, and doesn't make sense or have any uses in modern Phabricator.
It also does some pretty direct writes that can technically do things that at least //look// like they violate policies, so remove it.
Test Plan:
- Checked the API console, no longer saw "feed.publish".
- Grepped for "feed.publish", no hits.
Maniphest Tasks: T13681
Differential Revision: https://secure.phabricator.com/D21826
Summary:
Ref T13679. In D16983, global settings objects were given an exception to let logged-out users see them, even on installs with no "public" user role.
This exception is too broad and grants everyone all capabilities, not just "CAN_VIEW". In particular, it incorrectly grants "CAN_EDIT", so any user can edit global settings defaults.
Restrict this grant to "CAN_VIEW".
Test Plan:
- As a non-administrator, tried to edit global settings.
- Before: could.
- After: could not.
Maniphest Tasks: T13679
Differential Revision: https://secure.phabricator.com/D21811
Summary: Ref T13678. When remarkup content embeds other remarkup content, detect and degrade if the references have nesting depth greater than 1. This is a coarse cycle detector, since rendering shallow (but technically non-cycling) trees doesn't seem valuable.
Test Plan: Created various objects with self-references, saw everything degrade properly (after one level of embedding) when embedded in itself and in other contexts. See attached screenshot.
Maniphest Tasks: T13678
Differential Revision: https://secure.phabricator.com/D21810
Summary: Ref T13677. This was an accidental change in D21807: when reclaiming a resource, wait for it to be completely destroyed before allowing a lease to reclaim another resource.
Test Plan: Reverts accidental behavior change in D21807.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13677
Differential Revision: https://secure.phabricator.com/D21809
Summary:
Ref T13677. Drydock has a hard-coded and fairly arbitrary limit which prevents a resource pool from growing more than 25% at once.
This is vaguely reasonable for resources which allocate quickly, but suffocating for slower resources. It's also wholly arbitrary, and the "one per lease" limit introduced in D21807 should do a better job of covering the same ground while generally being more reasonable, expected, and predictable.
Test Plan: Ran Drydock allocations without the throttle, saw faster pool growth.
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13677
Differential Revision: https://secure.phabricator.com/D21808
Summary:
Ref T13677. Currently, one lease may cause multiple resources to allocate simultaneously if it starts allocating one, then wakes up from a yield later on and still sees no available resources.
This is never desired -- or, at least, produces desirable behavior only entirely by accident. Normally, it causes an excess of resources to allocate.
This is not a catastrophic problem: the extra resources usually get used sooner or later or cleaned up; and the total amount of badness is limited by overall resource allocation limits.
However, this behavior is also suppressed by an artificial "25% of current pool size" growth limit throttle which I intend to remove. Removing this throttle without fixing the allocator behavior could make this "too many resources" problem worse.
Change the allocator so that a lease that has started allocating a resource won't allocate another resource until the first resource leaves the "pending" state.
This also fixes some general oddness with the allocator and attempts to simplify the structure.
Test Plan:
- Ran 8 taskmasters.
- Destroyed all resources and leases.
- Leased 4 working copies.
- Saw exactly 4 resources build and lease, all simultaneously.
- Destroyed all resources and leases.
- Leased 32 working copies.
- Saw exactly 32 resources build and lease, approximately 8 at a time (limited by taskmasters).
- Destroyed all leases (but not resources).
- Leased 32 working copies, saw them satisfied by existing resources.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13677
Differential Revision: https://secure.phabricator.com/D21807
Summary:
Ref T13677. Track which resources a given lease has begun allocating or reclaiming in a more formal way, and add logging for waiting actions.
The "allocating" mechanism is new. This will replace an existing similar "reclaiming" mechanism in a future change.
Test Plan: See followup changes.
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13677
Differential Revision: https://secure.phabricator.com/D21806
Summary: Ref T13677. These flags increase the convenience of testing in a development environment.
Test Plan: Used new "--all" flags to release all resources and leases.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13677
Differential Revision: https://secure.phabricator.com/D21805
Summary:
Ref T13676. The 3-minute grace period when a resource can not be reclaimed after its leases are released currently doesn't work reliably because the Resource object usually isn't actually updated when a lease is released.
Add an additional check for recently-destroyed leases, and extend the grace period if we find any.
Test Plan:
- See T13676. Ran reproduction sequence there, observed immediate resource reclamation.
- Applied patch.
- Ran sequence again, observed repository B wait 3 minutes to reclaim a repository A resource.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13676
Differential Revision: https://secure.phabricator.com/D21803
Summary:
Ref T13676. Currently, "bin/drydock lease" just creates a lease that permits any blueprint.
To prepare for "use specific blueprint X", unify the logic between this workflow and LeaseUpdateWorker so we select only blueprints which at least have coarse compatibility (e.g., if we're leasing a host, only select enabled blueprints of classes that can allocate hosts).
Test Plan: Used `bin/drydock lease` to try to lease a nonsense type, got sensible error. Leased a host.
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13676
Differential Revision: https://secure.phabricator.com/D21801
Summary: Ref T13676. This makes it easier to create resource pressure without juggling a big pile of terminals.
Test Plan: Used `bin/drydock lease --count 5 ...` to acquire 5 leases.
Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13676
Differential Revision: https://secure.phabricator.com/D21800
Summary: Ref T13676. When the required "repositories.map" attribute is omitted, `bin/drydock lease` currently fatals in an unhelpful way when trying to lease a working copy.
Test Plan:
Ran `bin/drydock lease --type working-copy` with no attributes, after following steps in T13676.
```
<Allocation Failed> One or more blueprints promised a new resource, but failed when allocating: [PhutilAggregateException] All blueprints failed to allocate a suitable new resource when trying to allocate lease ("PHID-DRYL-orbtwtlinksm3xqpyhmw").
- Exception: Working copy lease is missing required attribute "repositories.map".
Attribute "repositories.map" should be a map of repository specifications.
```
Subscribers: yelirekim, PHID-OPKG-gm6ozazyms6q6i22gyam
Maniphest Tasks: T13676
Differential Revision: https://secure.phabricator.com/D21796
