Details

Reviewers

valerio.bozzolan
avivey

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15598: PHP 8.2 : rtrim(): Passing null to parameter #1 ($string) of type string is deprecated when checking notification servers

Commits

rP747d7db24161: Fix a PHP 8.1/8.2 deprecated use of ltrim and rtrim with a NULL argument

Summary

These calls were preventing notification servers configuration to be properly initialized.
Indeed, PHP 8.X is stricter concerning
This behavior is deprecated since PHP 8.1. Phorge adopts phutil_nonempty_string() as a replacement.

Fix T15598

Test Plan

Sign in as an administrator, configure the notification server without filling admin path field,
you shouldn't get both an RuntimeException and a warning indicating that Phorge is unable to connect
to Notification Server but a message indicating that everything is fine.

Diff Detail

Repository

rP Phorge

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

bob created this revision.Aug 10 2023, 08:28

bob held this revision as a draft.

Harbormaster completed remote builds in B738: Diff 1220.Aug 10 2023, 08:28

There is definitely something about arc tool philosophy that I miss... This modification consists in removing files associated with in other diff.

bob published this revision for review.Aug 10 2023, 08:32

Thanks!

This seems totally backward compatible to me

var_dump(rtrim(null, '/').'/'.ltrim(null, '/'));
// → string(1) "/"

// ----------------

require_once '../arcanist/src/__phutil_library_init__.php';
$a = null;
$b = null;
$full_path = '/';
    if (phutil_nonempty_string($a)) {
      $full_path = rtrim($a, '/').$full_path;
    }
    if (phutil_nonempty_string($b)) {
      $full_path = $full_path.ltrim($b, '/');
    }
var_dump($full_path);

// → string(1) "/"

php > var_dump(rtrim("/asd/asd///", '/').'/'.ltrim("///asd/asd/", '/'));
// → string(17) "/asd/asd/asd/asd/"

// ----------------

require_once '../arcanist/src/__phutil_library_init__.php';
$a = "/asd/asd///";
$b = "///asd/asd/";
$full_path = '/';
    if (phutil_nonempty_string($a)) {
      $full_path = rtrim($a, '/').$full_path;
    }
    if (phutil_nonempty_string($b)) {
      $full_path = $full_path.ltrim($b, '/');
    }
var_dump($full_path);

// → string(17) "/asd/asd/asd/asd/"

Please wait a while before landing, so other people may leave a comment

This revision is now accepted and ready to land.Aug 15 2023, 04:36

how about something like:

public function getURI($to_path = '') {
  $base = coalesce($this->path(), '');
  $to_path = coalesce($this_path, '');
  $full_path = '/';
  $full_path = rtrim($this->getPath(), '/').'/'.ltrim($to_path, '/');

Do you mean ?

public function getURI($to_path = '') {
  $path = coalesce($this->path, '');
  $to_path = coalesce($to_path, '');
  $full_path = rtrim($path, '/').'/'.ltrim($to_path, '/');

Why not its more concise so easier to read (I wasn't aware of phorge's coalesce function). If everybody agree on that, I'll update the revision accordingly.

In D25382#11307, @bob wrote:

Do you mean ?
...

yes, the one you said :)

Updating D25382: Update commit accordingly to comments

Harbormaster completed remote builds in B760: Diff 1263.Aug 16 2023, 10:35

avivey accepted this revision.Aug 16 2023, 12:27

avivey added inline comments.

src/applications/notification/client/PhabricatorNotificationServerRef.php
146	maybe default to `''`, just because we can...

Updating D25382: Update commit accordingly to comments

Harbormaster completed remote builds in B763: Diff 1267.Aug 16 2023, 12:49

bob marked an inline comment as done.Aug 16 2023, 12:50

I was updating the diff when I saw below that function getWebsocketURI may also be affected by a NULL issue :

public function getWebsocketURI($to_path = null) {
  $instance = PhabricatorEnv::getEnvConfig('cluster.instance');
  if (strlen($instance)) {
    $to_path = $to_path.'~'.$instance.'/';
  }

  $uri = $this->getURI($to_path);

  if ($this->getProtocol() == 'https') {
    $uri->setProtocol('wss');
  } else {
    $uri->setProtocol('ws');
  }

  return $uri;
}

I guess that the to_path argument should also be set by default to an empty string and verified against NULL value before being concatenated with another stuff isn't it ?

valerio.bozzolan added inline comments.Aug 17 2023, 02:33

src/applications/notification/client/PhabricatorNotificationServerRef.php
162–166	In D25382#11360, @bob wrote: I was updating the diff when I saw below that function getWebsocketURI may also be affected by a NULL issue Yeah I agree that will very probably raise a problem since the only usage is without any argument: ./src/view/page/PhabricatorStandardPageView.php: $client_uri = $server->getWebsocketURI(); So we can just change the default to fix that case at the moment. We are also sure that the lack of `$instance` can cause a problem so we can copy the check from line 157