Page MenuHomePhorge

(arc) Unify type-checking for `setHref()` type methods
ClosedPublic

Authored by avivey on Jul 24 2023, 13:18.

Details

Summary

Introduce PhutilURI::checkHrefType to unify type-check of some PHUI objects (See D25357).

Ref T15316

Test Plan

Run this script:

#!/usr/bin/env php
<?php

require_once './arcanist/scripts/__init_script__.php';

$valid_inputs = array(
  'foo',
  null,
  new PhutilURI('http://0@domain.com/'),
);


$invalid_inputs = array(
  9,
  new Filesystem(),
  array(),
);

echo "Valid inputs: \n";
foreach ($valid_inputs as $v) {
  echo("Checking ".gettype($v)." \n");
  PhutilURI::checkHrefType($v);
}

echo "\nError inputs: \n";
foreach ($invalid_inputs as $v) {
  echo("Checking ".gettype($v)." \n");
  PhutilURI::checkHrefType($v);
}

echo "\n";

Diff Detail

Repository
rARC Arcanist
Branch
master
Lint
Lint Passed
Unit
Test Failures
Build Status
Buildable 689
Build 689: arc lint + arc unit

Unit TestsFailed

TimeTest
2,166 msArcanistBundleTestCase::testGitRepository
EXCEPTION (RuntimeException): strlen(): Passing null to parameter #1 ($string) of type string is deprecated #0 /home/avivey/devtools/arcanist/src/parser/ArcanistBundle.php(798): PhutilErrorHandler::handleError(8192, '...', '...', 798) #1 /home/avivey/devtools/arcanist/src/parser/ArcanistBundle.php(409): ArcanistBundle->buildBinaryChange(Object(ArcanistDiffChange), NULL)
0 msArcanistBaseCommitParserTestCase::testBasics
2 assertion(s) passed.
1 msArcanistBaseCommitParserTestCase::testHalt
1 assertion(s) passed.
0 msArcanistBaseCommitParserTestCase::testJump
1 assertion(s) passed.
0 msArcanistBaseCommitParserTestCase::testJumpReturn
1 assertion(s) passed.
View Full Test Results (1 Failed · 74 Passed)

Event Timeline

avivey requested review of this revision.Jul 24 2023, 13:18
avivey retitled this revision from Unify type-checking for `setHref()` type methods to (arc) Unify type-checking for `setHref()` type methods.Jul 24 2023, 13:24
src/parser/PhutilURI.php
573

I like that, premising that we can replace gettype($value) == 'string' with is_string($value)

is_string(false) = bool(false)
is_string(true) = bool(false)
is_string(NULL) = bool(false)
is_string('abc') = bool(true)
is_string('23') = bool(true)
is_string(23) = bool(false)
is_string('23.5') = bool(true)
is_string(23.5) = bool(false)
is_string('') = bool(true)
is_string(' ') = bool(true)
is_string('0') = bool(true)
is_string(0) = bool(false)

https://www.php.net/manual/en/function.is-string.php

Also Using is_string() on an object will always return false (even with __toString()).

The last thing is not officially documented thought, so, probably the gettype() check is better

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

Tested, works, thanks :)

This revision is now accepted and ready to land.Jul 24 2023, 14:53
src/parser/PhutilURI.php
573

I did a grep, and I can't find any place where we do gettype() == 'string', but we do have a bunch of is_string(), so is_string is probably better.

This revision was landed with ongoing or failed builds.Jul 24 2023, 19:34
This revision was automatically updated to reflect the committed changes.