Differential D25300

Fix strlen() being passed a null in ArcanistUnitConsoleRenderer.php
ClosedPublic
Actions

Authored by Sten on Jun 16 2023, 16:10.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15187: Fix Arcanist in PHP 8.1+ (testing the Phorge repo)

Commits

rARC677e117b97fc: Fix strlen() being passed a null in ArcanistUnitConsoleRenderer.php

Summary

Fix the following error in ArcanistUnitConsoleRenderer.php under PHP 8.1:

strlen(): Passing null to parameter #1 ($string) of type string is deprecated

Stack trace:

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, custom=4)
  #0 PhutilErrorHandler::handleError(integer, string, string, integer) called at [<arcanist>/src/unit/renderer/ArcanistUnitConsoleRenderer.php:15]
  #1 ArcanistUnitConsoleRenderer::renderUnitResult(ArcanistUnitTestResult) called at [<arcanist>/src/workflow/ArcanistUnitWorkflow.php:190]
  #2 ArcanistUnitWorkflow::run() called at [<arcanist>/scripts/arcanist.php:427]

Ref T15187

Test Plan

arc unit

Diff Detail

Repository

rARC Arcanist

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

Sten created this revision.Jun 16 2023, 16:10

Owners added a reviewer: O1: Blessed Committers.Jun 16 2023, 16:10

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan and 2 others. · View Herald TranscriptJun 16 2023, 16:10

Sten requested review of this revision.Jun 16 2023, 16:10

Harbormaster completed remote builds in B568: Diff 977.Jun 16 2023, 16:10

Sten edited the summary of this revision. (Show Details)Jun 16 2023, 16:13

Thanks :)

Can I ask how to reproduce this problem?

Added unit test

Harbormaster completed remote builds in B569: Diff 978.Jun 16 2023, 18:57

Thanks for the unit test 👍

I'm trying to explore a bit the test plan. What were you doing when you found this bug? Maybe an "arc unit" while changing something specific that could be shared?

It is 'arc unit' which doesn't work for us without this patch under PHP 8.1. We are using a version of ArcanistUnitTestEngine extended to handle multiple languages in a single repo, and I unfortunately cannot share this. I'm not sure what more you want - we have a unit test demonstrating strlen() is being used against a variable which can be null.

An alternative fix would be to update ArcanistUnitTestResult to make $namespace default to '', and maybe add defaults for the other variables too.

IMHO we should just do a global replacement on all 'if (strlen(<someVar>)) and have done with it. I have done this on my phabricator copy to get it working.

valerio.bozzolan added inline comments.Jun 19 2023, 06:07

src/unit/renderer/ArcanistUnitConsoleRenderer.php
15	Hoping to see your nice patch approved sooner, I propose a tighter check here. In your opinion, does a tighter control solve your problem too? I suspect that at this point we just receive NULL or a string. and not string-like objects. Thanks for your opinion

Last comment from me (promise): can you share at least a cute stack trace? You can easily copy that using this argument:

arc unit --trace

Please do this in your actual use case before this change, not in the unit test. Thank you so much

Agreed - tighter check is better, and does fix the issue.

Trace:

$ arc unit --trace
 ARGV  <arcanist>/bin/arc unit --trace
>>> [1] (+0) <exec> $ php -f <arcanist>/scripts/arcanist.php -- unit --trace
 ARGV  <arcanist>/scripts/arcanist.php unit --trace
 LOAD  Loaded "arcanist" from "<arcanist>/src".
Config: Reading user configuration file "<home>/.arcrc"...
Config: Did not find system configuration at "/etc/arcconfig".
Working Copy: Reading .arcconfig from "<home>/<project>/.arcconfig".
Working Copy: Path "<home>/<project>" is part of `git` working copy "<home>/<project>".
Working Copy: Project root is at "<home>/<project>".
Config: Did not find local configuration at "<home>/<project>/.git/arc/config".
>>> [1] (+0) <exec> $ git rev-parse --verify HEAD^
<<< [1] (+14) <exec> 14,791 us
>>> [2] (+15) <exec> $ git rev-parse --abbrev-ref --symbolic-full-name '@{upstream}'
<<< [2] (+28) <exec> 12,909 us
>>> [3] (+28) <exec> $ git rev-parse --git-dir
<<< [3] (+41) <exec> 12,293 us
>>> [4] (+41) <exec> $ git cat-file -t origin/master
<<< [4] (+56) <exec> 15,054 us
>>> [5] (+57) <exec> $ git merge-base -- origin/master HEAD
<<< [5] (+75) <exec> 18,089 us
>>> [6] (+76) <exec> $ git diff --no-ext-diff --no-textconv --submodule=short --raw 4d0d2dca43b5ed319eb8f74e31e12fd36ef5747b HEAD --
<<< [6] (+95) <exec> 18,477 us
>>> [7] (+95) <exec> $ git --version
<<< [7] (+109) <exec> 13,693 us
>>> [8] (+110) <exec> $ git status --porcelain=2 -z
<<< [8] (+175) <exec> 64,708 us
>>> [9] (+182) <exec> $ phpunit -c <home>/<project>/phpunit.xml -d display_errors=stderr --log-junit /tmp/33fn45afu6gwcko0/29449-GMMC3v --coverage-clover /tmp/6rrp9oah9awws4gg/29449-d74tv5 <home>/<project>/tests/<TheTest>.php
<<< [9] (+780) <exec> 598,092 us

[2023-06-19 09:28:15] 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, custom=4)
  #0 PhutilErrorHandler::handleError(integer, string, string, integer) called at [<arcanist>/src/unit/renderer/ArcanistUnitConsoleRenderer.php:15]
  #1 ArcanistUnitConsoleRenderer::renderUnitResult(ArcanistUnitTestResult) called at [<arcanist>/src/workflow/ArcanistUnitWorkflow.php:190]
  #2 ArcanistUnitWorkflow::run() called at [<arcanist>/scripts/arcanist.php:427]
<<< [1] (+1,043) <exec> 1,043,428 us

Harbormaster completed remote builds in B577: Diff 987.Jun 19 2023, 08:35

Sten marked an inline comment as done.Jun 19 2023, 08:35

Sten edited the test plan for this revision. (Show Details)

Thanks! :) Tested. No nuclear implosions.

sgtm

This revision is now accepted and ready to land.Jun 19 2023, 08:39

At this point, feel free to download the patch again and land it

arc patch D25300
arc land

Closed by commit rARC677e117b97fc: Fix strlen() being passed a null in ArcanistUnitConsoleRenderer.php. · Explain WhyJun 19 2023, 13:55

This revision was automatically updated to reflect the committed changes.

Sten added a commit: rARC677e117b97fc: Fix strlen() being passed a null in ArcanistUnitConsoleRenderer.php.

valerio.bozzolan awarded a token.Jun 19 2023, 14:03

In D25300#8786, @Sten wrote:

IMHO we should just do a global replacement on all 'if (strlen(<someVar>)) and have done with it. I have done this on my phabricator copy to get it working.

[offtopic] See T15064#8916 for discussing that.