Page MenuHomePhorge

Various PHP 8.1 strlen(null) fixes for Dashboard Panels
ClosedPublic

Authored by Sten on Jul 29 2023, 19:46.

Details

Summary

In the dashboard application (https://my.phorge.site/dashboard/), when creating panels, adding panels to tab panels, and viewing query panels, we get a variety of strlen(null) errors under PHP 8.1.

This fixes all the ones seen.

Fixes T15574

Test Plan

See T15574

Diff Detail

Repository
rP Phorge
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Sten requested review of this revision.Jul 29 2023, 19:46
src/applications/dashboard/controller/panel/PhabricatorDashboardPanelTabsController.php
44

If it's a null already, then there's no need to set it, so we can replace looking for a strlen 0 with explicitly looking for the empty string!

src/applications/dashboard/engine/PhabricatorDashboardPanelRenderingEngine.php
158

Errors were just showing on the screen without the stack trace, so there was no way to find what was generating them. Now there is!

src/applications/dashboard/paneltype/PhabricatorDashboardTabsPanelType.php
103

I tried using the default parameter of idx(), but apparently $tab_spec['name'] is set to null, so this didn't work.
Coalesce does though...

src/applications/search/engineextension/PhabricatorFerretSearchEngineExtension.php
30

Again, 'query' is set to null, so using idx with default '' didn't work.

src/applications/search/engineextension/PhabricatorFerretSearchEngineExtension.php
30

What about using isset()?

Yeah, we can use isset() and strlen() in place of phutil_nonempty_string(), and I'm happy to do it.

However, if this is canon, then under what circumstances should phutil_nonempty_string be used?

Sten marked an inline comment as done.Jul 30 2023, 20:21

Gentle reminder that this has been waiting review for a while now...

tl;dr: I like phutil_nonempty_string($map['query']) better if we know $map['query'] is defined, and phutil_nonempty_string(idx($map, 'query')) otherwise.


Yeah, we can use isset() and strlen() in place of phutil_nonempty_string(), and I'm happy to do it.

However, if this is canon, then under what circumstances should phutil_nonempty_string be used?

I think (because we didn't actually formalized any of this) that phutil_nonempty_string($x) is generally for cases where $x is known to be "string or null", vs strlen($x) which is for $x that is "string" (not null).

Here, since $map is an array, we get another possible state for $map['query'] of "undefined", which isn't the same, but might be converted to null (or even a string) by something. Well, non-array $x can be "undefined" as well, but we have lints to prevent that.

if $map['query'] is undefined, we would throw an exception here, so the isset check here is equivalent to $map['query'] == null.
I'm pretty sure that canonically, we use isset only in the meaning of array_key_exists.

<?php

require_once dirname(__FILE__).'/__init_script__.php';

$y = array("a" => 3, 'b'=>null);
echo "\ny[a]: ";
echo $y['a'];
echo "\nisset(y[b]) : ";
echo isset($y['b'])  ?'set': 'unset';
echo "\nisset(y[c]) : ";
echo isset($y['c'])  ?'set': 'unset';
echo "\ny[c]  : ";
echo $y['c'];  // <-- Exception `Undefined array key "c"`
echo "\n";

Gotcha. So

  • isset() is safer when pulling things out of arrays.
  • phutil_non_empty_string() provides for stricter parameter checking as it demands the array value is set, even if set to null.

Given that both work in this instance, we have a choice of code robustness or strictness. I'm inclined to go for robustness and use isset as suggested by @speck, but but don't mind either way - my priority is getting it to work under PHP 8.1.

Hi all, can this be approved if no one has any objections? Dashboards simply can't be modified under PHP 8.1 without this. Thanks.

This revision is now accepted and ready to land.Sep 5 2023, 14:29