Summary:
Updating compatibility for PHP 8.2 for remarkup-related functionality.
This also resolves an issue introduced by https://secure.phabricator.com/D21860. Of all the flags when opening the zip the one I chose was not introduced in 5.2 but in 7.4
Test Plan: I rendered comments using figlet, cowsay, images, object references.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D21866
Summary: Running through setting up and using Diffusion repositories and addressing PHP8 issues that come up.
Test Plan:
I set up a hosted mercurial repository and pushed commits to it over ssh.
I set up a hosted git repository and pushed commits to it over ssh.
I set up a mirrored mercurial repository over ssh.
I set up a mirrored git repository over ssh.
I created a diff on a git repository and landed it.
I created a diff on a mercurial repository and landed it.
I cloned and pushed a commit to a mercurial repo over http.
I cloned and pushed a commit to a git repo over http.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D21864
Summary:
Updates to all of the `ProfileMenuItem` classes to be compatible with PHP 8.
These changes were moved from D21862.
Refs T13588
Test Plan:
I loaded the profile page for a user and saw all the items I expected to see.
I navigated through the different profile nav items.
I loaded the settings page for a user and saw all the items I expected to see.
I navigated through the differnt setting nave items.
I added Cat Facts to my navigation.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: Korvin, epriestley
Maniphest Tasks: T13588
Differential Revision: https://secure.phabricator.com/D21863
Summary:
Starting with a new instance running PHP 8.2, address all exceptions that come up through some basic browsing/usage.
For `strlen(null)` issues I generally tried to resolve if the value should be non-null at the point of issue, and attempt to address at earlier call-site. There were not many of these that I could determine. In the rest of those cases I would replace with a null-and-strlen check, or use `phutil_nonempty_string` if I was certain the value was a string and it was more convenient.
Hitting all code-paths is challenging, so I would search for `strlen` within radius of files I was modifying and evaluate to address those uses in the same manner.
Notes:
- `AphrontRequest::getStr` only ever returns a string, and is safe to use `phutil_nonempty_string`.
- `PhabricatorEnv::getEnvConfig` can return non-string things so any values coming from there should never use `phutil_nonempty_string`.
- `AphrontRequest::getHTTPHeader` indicates it could return wild so `phutil_nonempty_string` should not be used.
- `AphrontRequest::getURIData` isn't clear if it could return non-string data, so never use `phutil_nonempty_string`.
Refs T13588
Test Plan: I'm running an instance on 8.2 and went through the basic setup/installation, startup and usage, including setup issues and configurations/settings.
Reviewers: #blessed_reviewers, epriestley
Reviewed By: #blessed_reviewers, epriestley
Subscribers: aklapper, Korvin, epriestley
Maniphest Tasks: T13588
Differential Revision: https://secure.phabricator.com/D21862
Summary: The change from https://secure.phabricator.com/D21860 introduced a PHP error due to an invalid variable reference (`zip` instead of `$zip`). This fixes that issue as well as confirms that pk-zip compressed figlet font files can continue to be used/loaded.
Test Plan:
1. I ensured I had numerous figlet font files installed in `resources/figlet/custom` and used `file` to verify that `univers.flf` and `puffy.flf` are pk-zip archives.
2. With this change applied I added a comment with `figlet(font=univers){{{hello}}}` and verified that the comment used the univers font to render `hello`, and like-wise with the `puffy` font.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin, epriestley
Differential Revision: https://secure.phabricator.com/D21861
Summary:
As of PHP ~v8 the `zip_open` and associated functions have been deprecated and
removed. The replacement is the `ZipArchive` API. This updates the figlet
implementation to use this API which has been present in PHP since 5.2.
Additionally in PHP 8 the use of squiggly brackets for indexing into arrays is
also deprecated. This updates to remove two uses of squiggly brackets and
replace with square brackets.
These two deprecations would result in being unable to load differential
revisions in which someone had commented using figlet remarkup.
Refs T13588
Test Plan:
Applied these changes to an install and loaded a revision that had comments
where someone utilized figlet remarkup. The revision loaded properly and the
figlet comment rendered properly.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T13588
Differential Revision: https://secure.phabricator.com/D21860
Summary: Ref T13682. After some change prior change likely connected to T13682, file PHIDs fail to extract properly in Pholio.
Test Plan:
- Created a new Pholio mock with at least one image.
- Before: fatal when extracting data from this datastructure.
- After: clean mock creation.
Maniphest Tasks: T13682
Differential Revision: https://secure.phabricator.com/D21858
Summary:
Ref T13685. "RemarkupEditField" uses a "RemarkupValue" internally, and it currently attempts to serialize into the database unsuccessfully.
Instead, flatten the value before returning it for storage.
Test Plan:
- Edited the default description of a task in a custom form.
Maniphest Tasks: T13685
Differential Revision: https://secure.phabricator.com/D21854
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: See <https://hackerone.com/reports/1584409>. This obsolete API method is technically policy-violating: Owners packages have had policies since D15275. See also T10360.
Test Plan: Viewed Conduit API console, no longer saw API method available.
Differential Revision: https://secure.phabricator.com/D21850
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 supports an "Attached Files" curtain UI element.
Test Plan: See next change.
Maniphest Tasks: T13682
Differential Revision: https://secure.phabricator.com/D21835
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
