Page MenuHomePhorge

Fix PHP 8.1 "strlen(null)" exception trying to stop maintenance mode of Diffusion repo
ClosedPublic

Authored by Sten on Jun 14 2023, 19:31.

Details

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.

Use isset instead to check if the argument "start" was passed to the CLI command.

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=repoMaintStartHistory, ref.master=1c098c273d06, ref.repoMaintStartHistory=0a4a34143528)
  #0 PhutilErrorHandler::handleError(integer, string, string, integer) called at [<phorge>/src/applications/repository/management/PhabricatorRepositoryManagementMaintenanceWorkflow.php:45]

Closes T15476

Test Plan

After applying this change, executing ./bin/repository maintenance --stop R1 on the CLI shows Took repository "R1" out of maintenance mode. as expected, instead of an exception.

Diff Detail

Repository
rP Phorge
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Premising that - to me - there was a bug in the already-existing code.

Example:

$ ./bin/repository maintenance --stop --start "" R9
Took repository "R9" out of maintenance mode.

If we want to maintain this backward compatibility (that is a bit nonsense), probably this would be OK:

$is_start = phutil_nonempty_string($message);

Instead, if we want to improve a bit the thing, and allow an empty message we can keep the current proposed source code.

Premising that these are exactly equivalent at this moment:

$is_start = isset($message);
$is_start = $message !== null;

So, soft +1 thanks

Another possible idea: we can approve this change (again, premising that isset($message) and $message !== null are equivalent - but I don't have an opinion - so OK to keep isset($message))

And, would it be nice to also add something like this, to clarify a bit the internal expectations?

if ($is_start && $message === '') {
  throw new PhutilArgumentUsageException(pht("The start message cannot be empty"));
}

As already said the legacy situation deserved an additional check to me to prevent confusing situations of --stop --start "" wrongly recognized as stopped.

src/applications/repository/management/PhabricatorRepositoryManagementMaintenanceWorkflow.php
45

I would like this situation:

Argument$is_start$message
Nothingfalsenull
--starttruenull
--start "asd"true"asd"
60

Hi @avivey what do you think about this legacy possible situation?

$ ./bin/repository maintenance --stop --start "" R9
Took repository "R9" out of maintenance mode.

Maybe this Diff is a good moment to introduce a fix also to that, with a dedicated message

Hi @avivey what do you think about this legacy possible situation?

$ ./bin/repository maintenance --stop --start "" R9
Took repository "R9" out of maintenance mode.

Maybe this Diff is a good moment to introduce a fix also to that, with a dedicated message

To prevent --start and --stop from being used together, you should be able to add conflicts to one of the argument definitions. Could be named something else, I don't remember exactly. Search for it in other flows (including arcanist flows).

src/applications/repository/management/PhabricatorRepositoryManagementMaintenanceWorkflow.php
45

I don't think isset is part of our coding style...

In D25296#9660, @avivey wrote:

To prevent --start and --stop from being used together, you should be able to add conflicts to one of the argument definitions. Could be named something else, I don't remember exactly. Search for it in other flows (including arcanist flows).

Interesting. See line 55. The check was already there, but not fired for that case since the old $is_start was wrong. So just adopting $is_start = $message !== null; should fix.

Sten subscribed.

Changing to the (bool)strlen() to phutil_nonempty_string() is the simplest fix...

Sten added a reviewer: aklapper.

Tested valerio's suggestion -
$is_start = $message !== null;

Works fine. It only fails for the case whereby you use '--start --stop Repo', but that's a general arg parsing issue which should be looked at elsewhere.

This revision is now accepted and ready to land.Jul 18 2023, 06:22
src/applications/repository/management/PhabricatorRepositoryManagementMaintenanceWorkflow.php
60

(I proposed this just to make more clear the supposed usage - but I'm not sure it's in-topic here)