Page MenuHomePhorge

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

Authored by bob on Aug 10 2023, 08:28.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Jul 16, 20:26
Unknown Object (File)
Tue, Jul 16, 11:03
Unknown Object (File)
Mon, Jul 15, 04:31
Unknown Object (File)
Sat, Jul 13, 18:23
Unknown Object (File)
Sat, Jul 13, 02:35
Unknown Object (File)
Wed, Jul 10, 09:35
Unknown Object (File)
Wed, Jul 10, 05:18
Unknown Object (File)
Tue, Jul 9, 10:58

Details

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 Not Applicable
Unit
Tests Not Applicable

Event Timeline

bob held this revision as a draft.

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

avivey added inline comments.
src/applications/notification/client/PhabricatorNotificationServerRef.php
145–146

maybe default to '', just because we can...

Updating D25382: Update commit accordingly to comments

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 ?

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

Thanks for the patch suggestion ! Then we can :

  • Include getWebsocketURI fix it in this diff
  • Create another diff focused on getWebsocketURI fix

What is in your opinion the best option ?

Yeah feel free to land this as-is, that was easy-peasy accepted

So you have more commits and more glory

As a minion, I'm only working for the glory of my boss ! :P