Bug 1744105 - Please update Phabricator to 2021 Week 49 (20211202.1) #13
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…s used with prototypes off Summary: See PHI573. Ref T13120. Drafts were recently changed so that "draft" and "broadcast" are separate flags, and you can have non-broadcasting revisions in states other than "draft" if builds fail on a draft or you abandon a draft. However, when draft mode is entered with `arc diff --draft` and you have prototypes off, this flag wasn't being set correctly. Test Plan: Disabled prototypes, created a revision with `arc diff --draft`, observed that `draft.broadcast` is now correctly `false`. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13120 Differential Revision: https://secure.phabricator.com/D19360
Summary: See <https://discourse.phabricator-community.org/t/maniphest-home-page-crash-after-d19417/1445/3>. These special-token-only searches currently end up populating an empty `ownerPHIDs`, which fatals after the stricter check in D19417. Make the fatal on `withConstraint(array())` explicit and only set the PHID constraint if we have some PHIDs left. Test Plan: Searched for "No Owner", "Any Owner", an actual owner, "No Owner + actual user". Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D19440
Summary: Fixes issue reported in https://secure.phabricator.com/rPf191a66490b194785fae28c062b71be99bb14584#43240 Test Plan: Imported an SVN repo, observed clean import instead of daemon exception. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D19466
…e of page size Summary: Ref T13152. The pager does a bit of magic here and doesn't populate `nextPageID` when it knows it got an exact final page. The logic misfired in this case and sent us back to the start. Test Plan: - Set page size to 1 to guarantee rows were an exact multiple of page size. - Ran `rebuild-identities` (I no-op'd the actual logic to make it faster). - Before: looped forever. - After: clean exit after processing everything. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13152 Differential Revision: https://secure.phabricator.com/D19479
…little progress information Summary: Ref T13151. Ref T12164. Two small tweaks: - If we aren't actually going to change anything, just skip the writes. This makes re-running/resuming a lot faster (~20x, locally). - Print when we touch a commit so there's some kind of visible status. This is just a small quality-of-life tweak that I wrote anyway while investigating T13152, and will make finishing off db024, db025 and db010 manually a little easier. Test Plan: - Set `authorIdentityPHID` + `committerIdentityPHID` to `NULL`. - Ran `rebuild-identities`, saw status information. - Ran `rebuild-identiites` again, saw it go faster with status information. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T13151, T12164 Differential Revision: https://secure.phabricator.com/D19484
…allsite to fix Diffusion "Branches" page Summary: See PHI775. See D19499. Originally, see PHI720. D19499 broke the standalone "Branches" page for commits. Normally, you reach this by taking these steps: - View a commit which is contained by 11 or more branches. - Click the "More Branches..." link in the "Branches" field. - You should be taken to a list of all branches which contain the commit. The change to the 'branch' parameter was adjusted in the query that builds the "x, y, z, More Branches..." list, but not on the actual "Branches" list with the full list. Adjust it. Test Plan: - Set display limit to 1, viewed a commit on "master" and "stable", clicked "More Branches". - Before: saw only "master". - After: saw both "master" and "stable". Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D19532
…...", skip expired temporary files 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
… responses Summary: Ref T12907. At least part of the problem is that we can hit PHP's `max_execution_time` limit on the file download pathway. We don't currently set this to anything in the application itself, but PHP often sets it to 30s by default (and we have it set to 30s in production). When writing responses, remove this limit. This limit is mostly a protection against accidental loops/recurison/etc., not a process slot protection. It doesn't really protect process slots anyway, since it doesn't start counting until the request starts executing, so you can (by default) //send// the request as slowly as you want without hitting this limit. By releasing the limit this late, hopefully all the loops and recursion issues have already been caught and we're left with mostly smooth sailing. We already remove this limit when sending `git clone` responses in `DiffusionServeController` and nothing has blown up. This affects `git clone http://` and similar. (I may have had this turned off locally and/or just be too impatient to wait 30s, which is why I haven't caught this previously.) Test Plan: - Poked around and downloaded some files. - Will `curl ...` in production and see if that goes better. Reviewers: amckinley Reviewed By: amckinley Maniphest Tasks: T12907 Differential Revision: https://secure.phabricator.com/D19547
…with floating content Summary: Ref T13195. If a Phriction page begins with a code block, the `clear: both;` currently makes it clear the action list. Instead, use table-cell layout on desktops. Test Plan: Viewed a Phriction page with an initial code block on desktop/tablet/mobile/printable layouts. Now got more sensible layouts in all cases. Reviewers: amckinley Reviewed By: amckinley Subscribers: GoogleLegacy Maniphest Tasks: T13195 Differential Revision: https://secure.phabricator.com/D19649
Summary: See <https://discourse.phabricator-community.org/t/typo-in-phabricatorauditeditor-php/1910>. This is trivial and reproduces easily, I just missed it in testing. Test Plan: - Left a comment on a commit which I was the author of. - Before change: fatal with obvious typo. - After change: smooth sailing. Reviewers: amckinley Reviewed By: amckinley Differential Revision: https://secure.phabricator.com/D19667
Summary: Ref T13202. See PHI881. These stories have bad rendering methods, but they didn't previously render into the timeilne (since Phriction documents didn't have a timeline). Update the rendering to work. The rendered outcome isn't great (it isn't very clear or explicit about exactly what moved where), but I'll fix that in a followup. This is a net improvement since it doesn't fatal the page, at least. Test Plan: - Moved page "X" to "Y". - Viewed the old page "X". - Before patch: bad timeline story would fatal rendering. - After patch: story renders, at least, just not great. Reviewers: amckinley Maniphest Tasks: T13202 Differential Revision: https://secure.phabricator.com/D19682
…terBuild objects Summary: Ref T13072. Currently, Harbormaster builds react to messages by applying a transaction inline (which can race) that has no real effect. Later, the BuildEngine picks up the mesasge and applies a real effect, but this isn't transactional. This is backwards, and makes it more difficult to transition to ModularTransaction and EditEngine. The desired workflow is: - sending a message //just// writes to the message table (and queues a worker to process the message); - the BuildEngine processes the message and applies effects in a transactional way. Force this into at least roughly the right sequence of behaviors. This paves the way for porting to ModularTransaction, which should allow further cleanup. Test Plan: Paused, resumed, aborted, and restarted a build. Ran BuildWorkers to process the commands, saw builds update appropriately. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13072 Differential Revision: https://secure.phabricator.com/D21687
Summary: Ref T13072. No callers currently generate these transactions, and they probably never have. Remove them. Test Plan: Grepped for "HarbormasterBuildTransaction::TYPE_CREATE" and "self::TYPE_CREATE" in the class, found no hits. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13072 Differential Revision: https://secure.phabricator.com/D21688
Summary: Ref T13072. Update "HarbormasterBuild" to use modern modular transactions. Test Plan: - Aborted, restarted, paused, and resumed a build. - Used `bin/harbormaster restart`. - Grepped for use of old "::TYPE_COMMAND" constant, didn't find any hits. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13072 Differential Revision: https://secure.phabricator.com/D21689
Summary: Ref T13072. Further modularize build messages by applying each one in a separate transaction type. This makes it easier to add new types of messages (although I have no particular plans to do this, offhand) and reduces the amount of switch-boilerplate. This will probably also simplify validating "harbormaster.sendmessage". Test Plan: - Applied all commands. - Ran migration, saw transactions render properly Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13072 Differential Revision: https://secure.phabricator.com/D21690
Summary: Ref T13072. Push nearly all Harbormaster build message logic into the new per-message transaction classes. Test Plan: - Issued every message to Buildables. - Issued every message to Builds. - Looked at a big pile of error messages, couldn't find any typos. - Grepped for affected symbols, etc. - Ran `bin/harbormaster restart ...`. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13072 Differential Revision: https://secure.phabricator.com/D21691
Summary: Ref T13072. Update the last few constant references to this class and remove it. Test Plan: Grepped for "HarbormasterBuildCommand", got no hits. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13072 Differential Revision: https://secure.phabricator.com/D21692
Summary: Ref T13072. This transaction type has no writers and is mooted by EditEngine. Test Plan: Grepped for transaction constant. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13072 Differential Revision: https://secure.phabricator.com/D21693
Summary: Ref T13072. Trivially convert this into a modular transaction type. Test Plan: Issued commands to a buildable. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13072 Differential Revision: https://secure.phabricator.com/D21694
…API methods Summary: Ref T13072. These don't do anything useful yet, but get the skeletons in. Test Plan: Loaded documentation pages without fataling. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13072 Differential Revision: https://secure.phabricator.com/D21695
Summary: Ref T13072. Make large Conduit doc pages a bit more navigable. This prepares for updating "harbormaster.sendmessage" to support sending messages to builds. Test Plan: Viewed various Conduit API documentation pages, clicked links. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13072 Differential Revision: https://secure.phabricator.com/D21696
…art, abort, resume) to Builds/Buildables Summary: Ref T13072. Expand the role of "harbormaster.sendmessage" and allow it to send control messages to Builds and Buildables. Test Plan: Read documentation, sent commands to Builds and Buildables, hit a bunch of error cases, will deploy to catch full-lifecycle Build Target use cases. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13072 Differential Revision: https://secure.phabricator.com/D21698
Summary: Ref T13072. This exception is now raised by all of the message-sending code. Pretty straight find/replace. Test Plan: Grepped for old class name, no hits. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Maniphest Tasks: T13072 Differential Revision: https://secure.phabricator.com/D21699
Summary: Fixes T13648. If a package recipient has been destroyed, this query currently fails to return an expanded recipient value. Instead, make sure all input PHIDs get an output. For destroyed packages, the output will just be an empty list. Test Plan: - Added a package to a revision as a reviewer. - Destroyed the package. - Commented on the revision. - Processed the publishing worker with `bin/worker execute`. - Before: fatal after expanding the destroyed package. - After: clean publish. Maniphest Tasks: T13648 Differential Revision: https://secure.phabricator.com/D21707
Summary: See PHI498. This should be initialized to "self::ATTACHABLE" like other attachable properties, but is currently initialized to "array()". Initialize it the normal way and try to catch all code paths which may have accessed it without actually loading and attaching it. Also, remove UI for the very old "excuse" property, which "arc" has not written for well more than a year. Test Plan: Grepped for affected symbols, loaded various revision pages. Somewhat tricky to be 100% sure that every pathway is caught, but it should be obvious if I missed anything once someone hits the code path. Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam Differential Revision: https://secure.phabricator.com/D21710
Summary:
Ref T13658. This adds a simple expression evaluator to Remarkup and supports platform name expressions. The syntax is:
```
${{{strings.platform.server.name}}}
```
Note that this won't work inside code blocks (or literal blocks, or other block-level literal elements) right now, although it could be made to selectively (the ".path" expressions might be useful in documentation codeblocks).
Test Plan: {F9391006}
Reviewers: cspeckmim
Reviewed By: cspeckmim
Maniphest Tasks: T13658
Differential Revision: https://secure.phabricator.com/D21713
…he option "MergeSlashes On" Summary: Ref T13662. I ran into this while trying to reproduce the mention issue discussed there. Currently, the root document (with slug "/") attempts to preview using the URI `/phriction/preview//` (with two `//` at the end). This is collapsed into "/phriction/preview/" by Apache if "MergeSlashes On" is configured, which is the default behavior. The route then 404s. Instead, just use "/phriction/preview/?slug=/" so this endpoint functions properly regardless of the "MergeSlashes" configuration. Test Plan: - Configured Apache with "MergeSlashes On" (which is the default behavior). - Tried to preview a content edit of the root document in Phriction, which didn't work and generated 404s for "/phriction/preview//" in the console log. - Applied patch. - Previwed content in Phriction (which now worked properly). - Accessed `/a//b///c////` and similar with "MergeSlashes On" and "MergeSlashes Off", confirmed that this option controls whether PHP receives a URI with or without merged slashes in "__path__" after rewriting. Reviewers: 0 Reviewed By: 0 Maniphest Tasks: T13662 Differential Revision: https://secure.phabricator.com/D21708
Summary:
Fixes T13662. Phriction currently passes a map as a "context object", but this code is ancient and predates the modern meaning of a "context object". In modern code, context objects should be real objects.
Provide a real object as a context object. We do this by either loading the actual document or constructing a synthetic version of it.
Test Plan:
- Edited an existing document, observing the preview:
- Used a mention rule, saw a preview.
- Used `[[ a ]]` and `[[ ./a ]]` absolute and relative reference rules, saw accurate previews.
- Edited a new document, observing the preview:
- Used a mention rule, saw a preview.
- Used absolute/relative references, saw accurate previews.
- Grepped for other references to the removed properties (`phriction.isPreview` and `phriction.slug`), found none remaining.
Reviewers: 0
Reviewed By: 0
Maniphest Tasks: T13662
Differential Revision: https://secure.phabricator.com/D21709
Summary: Fixes T13663. `supportsSubtypes` tries to create an editable object, but this isn't always valid for `PhabricatorCalendarImport`. Use `instanceof` instead.
Test Plan:
- Edited calendar import, tasks (2 different subtypes), and projects (2 different subtypes).
- Changed task subtypes using {nav Change Subtype} action and batch editor.
- Changed task and project subtypes using Conduit.
Reviewers: epriestley, #blessed_reviewers
Reviewed By: epriestley, #blessed_reviewers
Subscribers: Korvin
Maniphest Tasks: T13663
Differential Revision: https://secure.phabricator.com/D21714
…tion time
Summary:
Depends on D21720. Ref T13666. See D21720 for additional discussion.
Use "MethodCallFuture", introduced in D21720, so that exceptions raised in "execute()" are thrown when the future is resolved, not when the future is created.
This makes exception behavior for clustered and non-clustered setups consistent, and chooses the intended (clustered) behavior in both cases, which currently deals with errors better.
Test Plan:
- Applied both parts of the patch in T13666 (break history queries, force immediate futures) to reproduce the issue in T13666.
- Loaded a Diffusion landing page, reproduced the error described in that task.
- Applied this patch, verified landing page works again.
- Removed the "break history queries" change, verified landing page works in forced-immediate mode.
- Removed the "force immediate" change, verified landing page works in "actual future" mode.
Reviewers: cspeckmim
Reviewed By: cspeckmim
Maniphest Tasks: T13666
Differential Revision: https://secure.phabricator.com/D21721
Summary: The history query for the repository page isn't actually used to display any content. It looks like it was previously used to display the last user which modified a file however this looks to be removed in D21404. This removes the history query from happening as well as updates `DiffusionBrowseTableView` to remove the parameters for passing this information in, resulting in also updating `DiffusionBrowseController` to no longer need to put this information together. Refs T13666 Test Plan: 1. I removed commits from a repository on the local state. 2. I navigated to the repository's landing page and saw that the landing page attempted to render content and only failed to load the browse files section. 3. I navigated to the history tab and verified that it showed an exception about failing to query commit information. 4. I restored the repository working state to function properly. 5. I navigated to a repository's landing page and verified it loaded properly, including showing the last modified date for each file. 6. I navigated to the Code, Branches, Tags, and History tabs to verify each tab page loaded properly. 7. I verified on the Code tab that the last modified date for each file displayed properly. Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: Korvin, epriestley Maniphest Tasks: T13666 Differential Revision: https://secure.phabricator.com/D21717
Summary: When previously known commits have been destroyed in a Mercurial repository, Phabricator does not end up marking the commits as unreachable. This results in daemon tasks which continuously fail and retry. This updates `PhabricatorRepositoryDiscoveryEngine` and `PhabricatorManagementRepositoryMarkReachableWorkflow` to include support of marking commits as unreachable for Mercurial repositories. The `PhabricatorMercurialGraphStream` also needed updated to support a stream with no starting commit. Refs T13634 Test Plan: 1. I set up a hosted Mercurial repository. 2. I removed the head commit from the on-disk repository state. 3. I attempted to load the repository page and saw an exception due to a missing commit. 4. I went to `/manage` for the repository and scheduled an update of the repository. 5. After an updated performed, I went to the repository main page and saw there was no exception and the history view properly did not have the commit I had removed. 6. I checked the phd logs and verified there were no exceptions related to the repository. 7. I ran the `./bin/repository mark-reachable` command on the Mercurial repository and it reported that it marked the commit as unreachable. 8. I pushed the same commit back upstream and verified that the commit was found and displayed in the history view of the repository page and `mark-unreachable` did not identify it as being unreachable. Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: Korvin, epriestley Maniphest Tasks: T13634 Differential Revision: https://secure.phabricator.com/D21715
Summary: Found in a change submitted to Phorge (https://we.phorge.it/D25018#inline-231), this fixes a typo in populating search the `contributorPHIDs` constraint in the Legalpad search engine. Currently when trying to search legalpad documents by contributor an error is encountered: ```lang=console Array for %Ls conversion is empty. Query: contributor.dst IN (%Ls) ``` Test Plan: I searched for legalpad documents based on a contributor and got back correct results. Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: Korvin, epriestley Differential Revision: https://secure.phabricator.com/D21719
Summary: Ref T13666. See T13666#256253. The order of these parameters is incorrect (introduced in D18817, which was trying to turn the revset "A or B" into "B or A"), but when "commit" is the single head of "branch" (which is common), the revset is functionally equivalent. Test Plan: An easy reproduction case is to make a "diffusion.historyquery" call manually against a Mercurial repository, with a valid "branch" name and some non-head "commit". - Before patch, queried "head^" (by commit hash, not by symbol) of "default" and got "head" too (wrong). - Applied patch to `secure001.phacility.net` (my local `hg` web environment is iffy right now). - Re-ran the same query, saw "head^" as the first result rather than "head" (all hashes rather than symbols, as above), which is desired. Reviewers: cspeckmim Reviewed By: cspeckmim Maniphest Tasks: T13666 Differential Revision: https://secure.phabricator.com/D21722
Summary:
Ref PHI2149. This provides the trivial Query class required for the daemons to publish Almanac Interface transactions.
(Publishing these doesn't do anything interesting, but currently leaves an error in the daemon logs.)
Test Plan:
- Stopped the daemons.
- Edited the port of an Interface in Almanac.
- Ran `bin/worker execute --active --class PhabricatorApplicationTransactionPublishWorker` to publish the transaction.
- Before: fatal on missing class, "Unable to load query for transaction object...".
- After: transaction publishes cleanly.
Differential Revision: https://secure.phabricator.com/D21726
Summary: Ref PHI2157. Like other low-level tools, "bin/celerity" does not need databases configured in order to execute. Test Plan: Ran `bin/celerity map` with and without the database available. Differential Revision: https://secure.phabricator.com/D21730
* upstream/stable: (325 commits)
Remove "bin/celerity" dependency on database setup
Provide missing "AlmanacInterfaceTransactionQuery"
Correct a parameter order swap in "diffusion.historyquery" for Mercurial
Fix searching legalpad documents by contributors
Add support to marking commits as UNREACHABLE for Mercurial
Remove history query from DiffusionRepositoryController as it is unused
Use "MethodCallFuture" to move Diffusion Conduit exceptions to resolution time
Fix subtype extension support check
Pass a real context object to Phriction previews, fixing mentions
Fix Phriction document previews for the root document ("/") with Apache option "MergeSlashes On"
Add an "eval" rule to Remarkup
Make "DifferentialDiff->properties" a proper "attachable" property
Resolve deleted packages properly as having no mailable members
Rename "HarbormasterRestartException" to "HarbormasterMessageException"
Allow "harbormaster.sendmessage" to send control command (pause, restart, abort, resume) to Builds/Buildables
Add a side nav to Conduit API method console pages
Add stub "harbormaster.build.edit" and "harbormaster.buildable.edit" API methods
Modularize "HarbormasterBuildableTransaction"
Remove "HarbormasterBuildableTransaction::TYPE_CREATE"
Remove "HarbormasterBuildCommand"
...
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.