Summary:
When someone tries to log on with invalid credentials, a record is inserted in the phabricator_user.user_log table with action = login-fail.
The session value of this record is set to null in this case.
When you open the People → Activity Logs screen, you will see an exception because of this.
Fixes T15555
Test Plan: Go to https://my.phorge.site/people/logs/ after a login failure and a login success.
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15555
Differential Revision: https://we.phorge.it/D25351
Summary:
When viewing the top level of a diffusion repository, $request->getPath() returns null, which then causes trim() to throw an error.
Fix is to update DiffusionRequest getPath() such that it returns '' instead of null, thus fixing it's return type.
Fixes T15522
Test Plan: Go to https://my.phorge.site/source/myrepo
Reviewers: O1 Blessed Committers, valerio.bozzolan, avivey
Reviewed By: O1 Blessed Committers, valerio.bozzolan, avivey
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15522
Differential Revision: https://we.phorge.it/D25329
Summary:
Under PHP 8.1, PhabricatorEditorURIEngine::newForViewer() is throwing a trim(NULL) error when trying to view a diff.
This is because it tries to apply string operations to a user setting which will be null by default.
Fixes T15518
Test Plan:
Unit test added -
arc unit
Or just view a diff. Eg:
https://my.phorge.site/D1234
Reviewers: O1 Blessed Committers, valerio.bozzolan, avivey
Reviewed By: O1 Blessed Committers, valerio.bozzolan, avivey
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15518
Differential Revision: https://we.phorge.it/D25324
Summary:
Increase required height for avatar image
Patch provided by Stang in https://phabricator.wikimedia.org/T307492
Closes T15552
Test Plan:
* Set `/config/edit/user.require-real-name/` to `Make real names optional`
* Log in as a user without a real name set
* Open the user menu in the top bar and look at the user avatar image
Reviewers: O1 Blessed Committers, Cigaryno, avivey
Reviewed By: O1 Blessed Committers, Cigaryno, avivey
Subscribers: avivey, speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15552
Differential Revision: https://we.phorge.it/D25348
Summary:
DifferentialChangesetViewController loadCoverage() fails under PHP 8.1 when test coverage is provided for some but not all modified files.
Update the code to replace strlen() with phutil_nonempty_string().
Fixes T15521
Test Plan: View a diff with test coverage for some but not all files.
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15521
Differential Revision: https://we.phorge.it/D25328
Summary:
/dashboard/adjust/remove/ POST requests fail under PHP 8.1 with strlen(null) errors.
This change fixes it.
Fixes T15549
Test Plan:
View a dashboard which has some panels. Eg https://my.phorge.site/dashboard/view/1/
Click on a 'Manage Panel' dropdown and click on 'Remove Panel'
Confirm the panel is removed and no strlen(null) errors are generated.
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15549
Differential Revision: https://we.phorge.it/D25345
Summary:
Add blur and sepia filter to disabled user profile image.
Before this change, the image was just greyed out but still fully recognizable.
Closes T15515
Test Plan:
* Create an account with a custom profile picture
* Disable the account
* Go to user profile of the account
* Go to a task with activity of that account and hover over the account to open card view
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15515
Differential Revision: https://we.phorge.it/D25322
Summary:
Fix PHP 8.1 strlen(null) in DiffusionView linkBrowse().
Fixes T15542
Test Plan:
# Go into a diff (eg https://my.phorge.site/D1234)
# Click on the Revision Contents - History tab
# Click on a base link (the short git revision ID)
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15542
Differential Revision: https://we.phorge.it/D25340
Summary:
This change implements some messages that were only present
from this class:
DifferentialRevisionHasCommitEdgeType.php
But not in its inverse class:
DiffusionCommitHasRevisionEdgeType.php
In short:
| Before | After |
|-----------|-----------|
| {F313282} | {F313281} |
Closes T15314
Test Plan:
Visit a Diffusion commit and use the sidebar menu "Edit Related Objects" to:
- add 1 revision
- add 2 revisions
- remove 1 revision
- remove 2 revisions
- add 1 revision, remove 1 revision
- add 2 revisions, remove 2 revisions
Check that you don't see phrases like "added an edge" anymore.
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: speck, tobiaswiese, Matthew, Cigaryno
Maniphest Tasks: T15314
Differential Revision: https://we.phorge.it/D25306
Summary:
It seems that if you use the "Move Right" action, you can get
an unrelated error message about "Move Left":
This is already the first tab. It can not move any farther to the left.
This happened because there was an incomplete condition.
Closes T15493
Test Plan:
- visit /dashboard/panel/ and create at least two panels (please, owned by you)
- visit a Dashboard, and add a Tabs Panel
- take the first Tab: "Move Right"
After this change, "Move Right" now always works.
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: speck, tobiaswiese, Matthew, Cigaryno
Maniphest Tasks: T15493
Differential Revision: https://we.phorge.it/D25307
Summary:
If you happen to create a Dashboard by retrieving other people's panels,
you were probably affected by this visual bug.
It seems that you cannot Move things, but you really can (and that is OK).
The reason is, to move things in a Parent Panel, you only need edit permission there,
and you do not need to also have edit permissions on every Child Panel.
Closes T15492
Test Plan:
- From user A:
- Create two panels - editable by: User A
- From user B:
- Create a "Tabs Panel" in a Dashboard
- Attach the panels from User A (so, editable by, not you)
The meaningful "Move" actions should be not disabled anymore.
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: speck, tobiaswiese, Matthew, Cigaryno
Maniphest Tasks: T15492
Differential Revision: https://we.phorge.it/D25308
Summary:
When iterating through the fields of a differential commit, the DifferentialGetCommitMessageConduitAPIMethod execute method explicitly allows a value to be either a string or a null. It then calls strlen upon this possibly null value.
We could replace the strlen with phutil_nonempty_string, but as the code has already eliminated variable types other than string or null, it is more efficient to explicitly check for null or ''
```
$value === null or $value == ''
```
Fixes T15527
Test Plan:
Run
```
arc diff
```
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15527
Differential Revision: https://we.phorge.it/D25332
Summary: Fixes T15275.
Test Plan:
Project pages have "Developer > View Hovercard" button. Or hover over a project handle, I guess.
Also test for empty description and with the description field disabled.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15275
Differential Revision: https://we.phorge.it/D25331
Summary:
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts `phutil_nonempty_string()` as a replacement.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
```
EXCEPTION: (RuntimeException) strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=master, ref.master=97e163187418), phorge(head=master, ref.master=1c098c273d06)
#0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/applications/repository/xaction/PhabricatorRepositoryMaintenanceTransaction.php:28]
```
Closes T15475
Test Plan:
Play a bit with maintenance messages:
./bin/repository maintenance --start "Start" R1
./bin/repository maintenance --start "Start Yeah" R1
./bin/repository maintenance --stop R1
Then visit `/diffusion/1/manage/history/` and look at `Diffusion put this repository into maintenance mode.` as expected, instead of an exception.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15475
Differential Revision: https://we.phorge.it/D25295
Summary:
The error was caused by the overlay lightbox, since it gives the possibility to share a comment on any file
in any general Remarkup text, but that comment textarea is indeed empty as default.
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts `phutil_nonempty_string()` as a replacement.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
```
EXCEPTION: (RuntimeException) strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=master, ref.master=97e163187418), phorge(head=T15498, ref.master=a25af8ccef69, ref.T15498=a25af8ccef69)
#0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/applications/files/controller/PhabricatorFileLightboxController.php:23]
```
Closes T15498
Test Plan: Visit a blog post with a file/image dropped in the content and click on the thumbnail. No error overlay dialog is displayed anymore.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15498
Differential Revision: https://we.phorge.it/D25310
Summary:
Fix PHP 8.1 strlen(null) error in DifferentialBranchField getBranchDescription()
Fixes T15531
Test Plan:
Simply viewing a diff (eg https://my.phorge.site/D1234) on a PHP 8.1 server generated the error fixed by this diff.
Also created a unit test.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15531
Differential Revision: https://we.phorge.it/D25335
Summary:
When a 'git pull' is done to an https git URL, the $_SERVER variables PHP_AUTH_USER and PHP_AUTH_PW will be unset, causing PHP 8.1 to throw strlen(null) errors.
This update fixes the issue by defaulting the values to '', which results in $have_user and $have_pass having false values as desired.
Fixes T15520
Test Plan: arc unit
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: avivey, speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15520
Differential Revision: https://we.phorge.it/D25327
Summary:
We add support to code blocks with the language expressed as GitLab/GitHub/StackOverflow/...
"flavored markdown".
So we support this syntax: (to avoid confusion see it online on the Diff)
lang=text
```php
$asd = 1;
```
Before this change, this was the only supposed syntax in Remarkup, with an explicit "lang=":
lang=text
```lang=php
$asd = 1;
```
This change introduces a minor risk to eat legitimate Remarkup content, since Remarkup allows
to do a multi-line in this way:
lang=text
```$asd = 1;
$asd = 2;```
The above example still works, but, there is a chance that hardcore Remarkup people
have a problem when doing a code block to mention programming languages.
In short, this can be problematic since "cpp" will be eaten from this list:
COUNTEREXAMPLE
```cpp
php
python
```
Using the above example is not socially nice because it is not usable in GitLab, GitHub and Stack Overflow.
If your first line is eaten:
Just *add* a newline on the top to reach a valid raw Markdown list (suggested, valid in Remarkup + Markdown):
lang=text
```
cpp
php
python
```
Or, just add "text" to specify that as language (suggested, valid in Remarkup + Markdown):
lang=text
```text
cpp
php
python
```
Or, just *remove* a newline from the bottom to reach a valid raw Remarkup list (Remarkup-only):
lang=text
```cpp
php
python```
Or, just specify that you are writing in the language "text" (Remarkup-only):
lang=text
```lang=text
cpp
php
python```
To reduce impact and help you, the logic of this strict implementation is:
- must have backticks
- must not have any valid remarkup option, like lang=, counterexample, etc.
- must not have content in the same line of the last backticks
- must have a known language in our proposed subset
If everything is OK, we remove that language from the content since it would be otherwise displayed.
Interestingly, this could improve performance when rendering README files or snippets from external
websites, since - in case - we do not need to guess the language using our deep dark magic.
Closes T15481
Test Plan:
We added some nice unit tests. Ensure that this test passes:
PhutilRemarkupEngineTestCase::testEngine
Optionally, take vision of these, before and after:
https://we.phorge.it/P16
Change the test plan slightly every time, to make sure it is not in your cache.
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: avivey, speck, tobiaswiese, Matthew, Cigaryno
Maniphest Tasks: T15481
Differential Revision: https://we.phorge.it/D25299
Summary:
Before this change, if you search a file using Locate File,
this was the only accepted syntax for a full-path:
path/to/something.txt
After this change, some relative/absolute Unix-like variants
are also accepted:
./path/to/something.txt
/path/to/something.txt
Similar prefixes can be frequent when you quickly copy-paste things
from shell commands like 'grep' or 'find' etc.
Ref T15508
Test Plan:
Visit a lovely repository like Phorge and use Locate File with these:
1. src/applications/maniphest/editor/ManiphestEditEngine.php
2. /src/applications/maniphest/editor/ManiphestEditEngine.php
3. ./src/applications/maniphest/editor/ManiphestEditEngine.php
After this change, also 2. and 3. return the expected file.
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: avivey, speck, tobiaswiese, Matthew, Cigaryno
Maniphest Tasks: T15508
Differential Revision: https://we.phorge.it/D25320
Summary: Ref T15064.
Test Plan:
- Register as a new user
- as admin, view user's page and approve it
- As new user, hit "wait patiently" to go back to Home
No crashes in this flow.
Reviewers: O1 Blessed Committers, chris
Reviewed By: O1 Blessed Committers, chris
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15064
Differential Revision: https://we.phorge.it/D25317
Summary: We were still pointint to secure.phabricator.com in a couple places. This fixes that.
Test Plan: No testing needed
Reviewers: O1 Blessed Committers, speck, Cigaryno
Reviewed By: O1 Blessed Committers, speck, Cigaryno
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Differential Revision: https://we.phorge.it/D25316
Summary:
src/infrastructure/javelin/markup.php phabricator_form() performs a strcasecmp() operation on an optional attribute withoiut checking to see if it exists first. This causes an 'Passing null to parameter #1 ($string1) of type string is deprecated' error.
Because we subsequenly check to see that the value equals /POST/i, all we need to do is check that the value has some 'true' value before doing the strcasecmp(). If it's not true, then it won't be POST!
Fixes T15509
Test Plan: View a diff. Eg: https://my.phorge.site/D1234
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15509
Differential Revision: https://we.phorge.it/D25319
Summary:
When MySQL database replication is configured, but not running, the slave status Seconds_Behind_Master is NULL.
When this value is passed to strlen() an error is thrown.
Fixes T15507
Test Plan:
* Configure database replication such that Seconds_Behind_Master is NULL
* Restart php-fpm
* Go to Phorge web site.
Reviewers: O1 Blessed Committers, chris
Reviewed By: O1 Blessed Committers, chris
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15507
Differential Revision: https://we.phorge.it/D25318
Summary:
Both Debian and Ubuntu have been shipping PHP 7.x (or now 8.x) for a while.
This patch replaces the versioned php5-foo package references with more general
php-foo package names.
Test Plan: Navigated to the documentation pages that this patch updates.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Differential Revision: https://we.phorge.it/D25315
Summary:
This tip is useful if you are affected by the following error in git or similar:
fatal: detected dubious ownership in repository at '/var/www/phorge'
Just follow the new tip from Diviner to fix at system level.
Ref T15282
Test Plan:
- see if you like the documentation tip
- try to execute the command, and verify that your computer does not explode
- eventually visit the /config/ page with and without that change (with and without a /etc/gitconfig)
Reviewers: O1 Blessed Committers, avivey, Cigaryno
Reviewed By: O1 Blessed Committers, avivey, Cigaryno
Subscribers: avivey, speck, tobiaswiese, Matthew, Cigaryno
Maniphest Tasks: T15282
Differential Revision: https://we.phorge.it/D25236
Summary:
Before this change the footer in the notification bar was a bit
weird on the message "Notification server not enabled":
| Before | After |
|-----------|-----------|
| {F315024} | {F315001} |
Also a bit wrong on the "Connecting..." message:
| Before | After |
|-----------|-----------|
| {F315061} | {F315072} |
The fix involves a refactor in the HTML structure in order to
match the graphic rendered when Aphlict is configured and running.
In short, we pass from this:
COUNTEREXAMPLE
<span class="connection-status-text aphlict-connection-status-notenabled">
<span class="visual-only phui-icon-view phui-font-fa fa-circle-o grey"></span>
Notification server not enabled
</span>
To this:
<span class="visual-only phui-icon-view phui-font-fa fa-circle-o grey"></span>
<span class="connection-status-text aphlict-connection-status-notenabled">Notification server not enabled</span>
See the Task for additional details.
Closes T15415
Test Plan:
Follow the official Notification documentation about the config `notification.servers`,
open the Notification top-bar, and check the message in these conditions:
- without the config
- with the config
- with/without Aphlict running
https://we.phorge.it/book/phorge/article/notifications/
Reviewers: O1 Blessed Committers, avivey
Reviewed By: O1 Blessed Committers, avivey
Subscribers: avivey, speck, tobiaswiese, Matthew, Cigaryno
Maniphest Tasks: T15415
Differential Revision: https://we.phorge.it/D25312
Summary:
Before this change, it seems that the Dashboard Tabs Panel could be empty
as default. This was the regression:
cbc0e66154
The variable $is_selected was defined inside a foreach loop, but that loop
was terminated when it was then read again. This redefines that variable in
the right scope.
| Before | After |
|-----------|-----------|
| {F312797} | {F312798} |
Closes T15474
Test Plan: - see that now you have something visibile in your Dashboard Tabs Panel
Reviewers: speck, O1 Blessed Committers
Reviewed By: speck, O1 Blessed Committers
Subscribers: tobiaswiese, Matthew, Cigaryno
Maniphest Tasks: T15474, T15146
Differential Revision: https://we.phorge.it/D25291
Test Plan: Go to `/config/settings/all` and look at the description of `metamta.one-mail-per-recipient`
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Differential Revision: https://we.phorge.it/D25311
Summary:
Trying to create an empty inline comment in a Pholio mock, `$dictionary` does not get set as both `strlen($v_content)` and `$inline->getID()` are not true.
Thus show a more explanatory error message ('Comment cannot be empty.') to users instead of exposing internal variable names.
```
EXCEPTION: (RuntimeException) Undefined variable $dictionary at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=master, ref.master=b325304b6e52), phorge(head=master, ref.master=980293b707a0)
#0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/applications/pholio/controller/PholioInlineController.php:117]
```
Closes T15456
Test Plan: After applying this change, try to add an empty Inline Comment in a Pholio mock. See that the error message is now "Comment cannot be empty." instead of "Undefined variable $dictionary".
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15456
Differential Revision: https://we.phorge.it/D25281
Summary:
Add a `dashboard.create` capability to allow limiting dashboard creation (and creation of related panels).
This can reduce spam in open Phorge installations in which anyone can create an account while you still want anyone to be able to view existing dashboards.
Closes T15438
Test Plan:
1. As an admin, go to `/applications/view/PhabricatorDashboardApplication/`
2. See only two options "Can Use Application: All Users" and "Can Configure Application: Administrators"
3. Apply patch
4. As an admin, go to `/applications/view/PhabricatorDashboardApplication/` and see the new option "Can Create Dashboards" set to "All Users"
5. As an average user, go to `/dashboard/` and successfully create a new Dashboard
6. As an admin, go to `/applications/edit/PhabricatorDashboardApplication/` and change "Can Create Dashboards" from "All Users" to "Administrators", select "Save Policies" button
7. As an average user, go to `/dashboard/` and see that "Create Dashboard" is disabled
8. As an average user, go to an existing dashboard not created by this user and see that selecting "Edit Dashboard" shows "You do not have permission to edit this object."
9. As an average user, go to the existing dashboard that you created yourself and see that the Edit Policy is set to this user.
10. As an average user, go to the existing dashboard that you created yourself and see that you can still create or add panels.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15438
Differential Revision: https://we.phorge.it/D25270
Summary:
Have a consistent rendering experience between File page and file embedded in a task for PNG image files with an alpha channel:
Show a checkered background on the embedded image instead of a white background; show full contrast when hovering over the embedded image.
Closes T15473
Test Plan:
See steps in T15473 - compare an image file embedded in a task before and after applying this patch.
- test in Task description
- test in Task comment
- test in Task comment inside something weird like a table
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15473
Differential Revision: https://we.phorge.it/D25294
Summary:
This seems like a fairly obvious oversight with the column.search API.
Knowing:
1. isHidden - boolean to indicate Active vs Archived
2. isDefaultColumn - the one that Tasks get dropped in by default, usually called "Backlog"
3. sequence - numerical order on the Workboard
are all necessary for a lot of things that very sensible real-world API clients need to do when working with columns.
Partial cherry-pick from:
https://phabricator.wikimedia.org/rPHABebfe30890b52784d222ec4ed36c05462b2a57f92
Closes T15484
Test Plan:
Tested on phabricator.wikimedia.org over many months and used by real client apps.
To do additional tests, visit this page:
/conduit/method/project.column.search/
Check that the new fields are returned correctly and nothing explodes.
Reviewers: #blessed_committers, O1 Blessed Committers, 0, avivey, valerio.bozzolan
Reviewed By: #blessed_committers, O1 Blessed Committers, 0, valerio.bozzolan
Subscribers: Cigaryno, 0, speck, tobiaswiese, valerio.bozzolan, Matthew
Maniphest Tasks: T15081, T15484
Differential Revision: https://we.phorge.it/D25038
Summary:
Allow humans to understand what happened when `phabricator.serious-business` mode is enabled: The administrator role was removed from a user account.
Closes T15480
Test Plan: Enable `phabricator.serious-business` mode. Enable and disable an account as admin. Check that account's feed and the general feed.
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15480
Differential Revision: https://we.phorge.it/D25298
Summary:
When the user "Mrs. Kitten" adds or remove "Mrs. Kitten" as Subscriber,
usually these messages were generated:
Mrs. Kitten added a subscriber: Mrs. Kitten.
Mrs. Kitten removed a subscriber: Mrs. Kitten.
This was a bit like the Spiderman meme self-pointing Spiderman.
After this change, these beautiful messages are generated instead:
Mrs. Kitten subscribed.
Mrs. Kitten unsubscribed.
| Before | After |
| {F286215} | {F286216} |
Closes T15347
Test Plan:
- subscribe on something
- unsubscribe from something
- all other cases remain as-is
Reviewers: O1 Blessed Committers, bfs, speck
Reviewed By: O1 Blessed Committers, bfs, speck
Subscribers: bfs, speck, tobiaswiese, Matthew, Cigaryno
Maniphest Tasks: T15347
Differential Revision: https://we.phorge.it/D25191
Summary:
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts `phutil_nonempty_string()` as a replacement.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
```
EXCEPTION: (RuntimeException) strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=master, ref.master=97e163187418), phorge(head=master, ref.master=7bbd657c19c4)
#0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/applications/conduit/controller/PhabricatorConduitAPIController.php:686]
```
Closes T15469
Test Plan: After applying this change, `/api/user.whoami` correctly renders the "Method Call Result"
Reviewers: O1 Blessed Committers, speck
Reviewed By: O1 Blessed Committers, speck
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15469
Differential Revision: https://we.phorge.it/D25292
Summary:
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts `phutil_nonempty_string()` as a replacement.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
```
EXCEPTION: (RuntimeException) strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=diffusionEmptyRepoPatternSearch, ref.master=97e163187418, ref.diffusionEmptyRepoPatternSearch=97e163187418), phorge(head=master, ref.master=108cbcd09bd3)
#0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/applications/diffusion/controller/DiffusionTagListController.php:28]
```
Closes T15461
Test Plan: After applying this line, get the expected `CommandException: Command failed with error #128!` due to ` fatal: detected dubious ownership in repository at '/var/repo/1'` (plus a `RuntimeException: file_exists(): Passing null to parameter #1 ($filename) of type string is deprecated` as a bonus), instead of the previous RuntimeException about `strlen()`.
Reviewers: O1 Blessed Committers, speck
Reviewed By: O1 Blessed Committers, speck
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15461
Differential Revision: https://we.phorge.it/D25285
Summary:
In some cases, older Herald transcripts which got garbage collected display an exception.
For unknown reasons, calling `getObjectTranscript()` on the HeraldTranscript object `$xscript` can return a bool with the value `false`.
Afterwards, calling `getAppliedTransactionPHIDs()` on that bool throws an exception.
Thus as a workaround, return an empty array instead in this case, like the code already does when $xscript->getObjectTranscript()->getAppliedTransactionPHIDs() === null.
Note that it remains unclear whether/when the bool may have the value `true` hence this situation is not covered by this code change (the code would still throw an exception in this situation).
Closes T15343
Test Plan:
1. Change system time to one year back (or optionally, use `./phorge/bin/garbage set-policy --collector herald.transcripts --days 1`, drop steps 9 and 10, and wait a day before performing step 11)
2. As an admin, go to http://phorge.localhost/herald/create/
3. Select "Maniphest Tasks"
4. On http://phorge.localhost/herald/create/?adapter=HeraldManiphestTaskAdapter , select "Global Rule"
5. Under "Conditions", select `any of` and set the three conditions `Description | contains | Internet Archive`, `Description | contains | archive.org`, `Description | contains | Wayback Machine`.
6. Under "Action", select `Add projects | someProject`
7. Select "Save Rule"
8. Trigger the rule by creating a new task.
9. Change system time back to today
10. On the shell, run `./phorge/bin/phd restart`
11. Visit created task and select "View Herald Transcript"
12. Transcript page displays "Old Transcript - Details of this transcript have been garbage collected." and shows no exception anymore
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15343
Differential Revision: https://we.phorge.it/D25246
Summary:
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts `phutil_nonempty_string()` as a replacement.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
```
EXCEPTION: (RuntimeException) strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=master, ref.master=97e163187418), phorge(head=diffusionCallsignHistory, ref.master=9b99123ff931, ref.diffusionCallsignHistory=9b99123ff931)
#0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/applications/repository/xaction/PhabricatorRepositoryCallsignTransaction.php:28]
```
Closes T15465
Test Plan: After applying this change, `/diffusion/XYZ/manage/history/` renders correctly and shows `user set the callsign for this repository to XYZ.`
Reviewers: O1 Blessed Committers, speck
Reviewed By: O1 Blessed Committers, speck
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15465
Differential Revision: https://we.phorge.it/D25289
Summary:
Fix three typos in icon names
Closes T15454
Test Plan: Go to `/uiexample/view/PHUIIconExample/` before and after applying the change; compare icons displayed for `fa-codiepie`, `fa-user-o`, `fa-thermometer-0`
Reviewers: O1 Blessed Committers, valerio.bozzolan
Reviewed By: O1 Blessed Committers, valerio.bozzolan
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15454
Differential Revision: https://we.phorge.it/D25280
Summary:
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts `phutil_nonempty_string()` as a replacement.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
```
EXCEPTION: (RuntimeException) strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=master, ref.master=97e163187418), phorge(head=diffusionCallsignHistory, ref.master=9b99123ff931, ref.diffusionCallsignHistory=d5fb6702a49a)
#0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/applications/diffusion/controller/DiffusionCommitController.php:31]
```
Closes T15466
Test Plan: After applying this change, going to an existing commit URL like `/diffusion/XYZ/commit/999a2c7d1ff8` shows the exception covered by T15464 instead, as expected.
Reviewers: O1 Blessed Committers, speck
Reviewed By: O1 Blessed Committers, speck
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15466
Differential Revision: https://we.phorge.it/D25290
Summary:
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts `phutil_nonempty_string()` as a replacement.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
```
EXCEPTION: (RuntimeException) strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=master, ref.master=97e163187418), phorge(head=diffusionBrowseCode, ref.master=108cbcd09bd3, ref.diffusionBrowseCode=108cbcd09bd3)
#0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/applications/diffusion/controller/DiffusionBrowseController.php:25]
```
```
EXCEPTION: (RuntimeException) strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=master, ref.master=97e163187418), phorge(head=diffusionBrowseCode, ref.master=108cbcd09bd3, ref.diffusionBrowseCode=108cbcd09bd3)
#0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/applications/diffusion/conduit/DiffusionBrowseQueryConduitAPIMethod.php:40]
```
Closes T15462
Test Plan: After applying these two changes, get next expected exceptions about `RuntimeException: file_exists(): Passing null to parameter #1 ($filename) of type string is deprecated` and `CommandException: Command failed with error #128! fatal: detected dubious ownership in repository at '/var/repo/1'`, but no RuntimeException about `strlen()` anymore.
Reviewers: O1 Blessed Committers, speck
Reviewed By: O1 Blessed Committers, speck
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15462
Differential Revision: https://we.phorge.it/D25286
Summary:
`strlen()` was used in Phabricator to check if a generic value is a non-empty string.
This behavior is deprecated since PHP 8.1. Phorge adopts `phutil_nonempty_string()` as a replacement.
Note: this may highlight other absurd input values that might be worth correcting
instead of just ignoring. If phutil_nonempty_string() throws an exception in your
instance, report it to Phorge to evaluate and fix that specific corner case.
```
EXCEPTION: (RuntimeException) strlen(): Passing null to parameter #1 ($string) of type string is deprecated at [<arcanist>/src/error/PhutilErrorHandler.php:261]
arcanist(head=master, ref.master=97e163187418), phorge(head=master, ref.master=108cbcd09bd3)
#0 <#2> PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/applications/repository/xaction/PhabricatorRepositoryEncodingTransaction.php:20]
```
Closes T15460
Test Plan: After applying these two changes, changing the text encoding of a Diffusion repository to a valid encoding and going to `/diffusion/1/manage/history/`, page renders correctly and shows no exception.
Reviewers: O1 Blessed Committers, speck
Reviewed By: O1 Blessed Committers, speck
Subscribers: speck, tobiaswiese, valerio.bozzolan, Matthew, Cigaryno
Maniphest Tasks: T15460
Differential Revision: https://we.phorge.it/D25284
