Errors when a commit belongs to a numeric git branch: Exception: "Call to phutil_nonempty_string() expected null or a string, got: int" in PhabricatorRepository.php
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	aklapper
	Sep 16 2023, 13:43

Description

Upstreaming this from our server logs (Phorge at 52be52d429ce). Last changed in D25040 imported from Phabricator upstream.

Looks like we need to more lenient and allow scalars instead of strings only.

[2023-09-15 16:22:39] EXCEPTION: (InvalidArgumentException) Call to phutil_nonempty_string() expected null or a string, got: int. at [<arcanist>/src/utils/utils.php:2124]
 arcanist(), ava(), phorge(), translations(), wmf-ext-misc()
   #0 <#2> phutil_nonempty_string(integer) called at [<phorge>/src/applications/repository/storage/PhabricatorRepository.php:749]
   #1 <#2> PhabricatorRepository::generateURI(array) called at [<phorge>/src/applications/diffusion/request/DiffusionRequest.php:459]
   #2 <#2> DiffusionRequest::generateURI(array) called at [<phorge>/src/applications/diffusion/controller/DiffusionCommitBranchesController.php:39]
   #3 <#2> DiffusionCommitBranchesController::handleRequest(AphrontRequest) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:284]
   #4 phlog(InvalidArgumentException) called at [<phorge>/src/aphront/handler/PhabricatorAjaxRequestExceptionHandler.php:27]
   #5 PhabricatorAjaxRequestExceptionHandler::handleRequestThrowable(AphrontRequest, InvalidArgumentException) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:751]
   #6 AphrontApplicationConfiguration::handleThrowable(InvalidArgumentException) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:296]
   #7 AphrontApplicationConfiguration::processRequest(AphrontRequest, PhutilDeferredLog, AphrontPHPHTTPSink, MultimeterControl) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:204]
   #8 AphrontApplicationConfiguration::runHTTPRequest(AphrontPHPHTTPSink) called at [<phorge>/webroot/index.php:35]

Steps to Reproduce

Visit this specific commit that belongs to main but also also to a numeric branch:

❌ https://phabricator.wikimedia.org/diffusion/OSWB/branches/main/;5a588bf0ee10546ddb9d0872d1be1bf48b3f3c07

Call to phutil_nonempty_string() expected null or a string, got: int.

Additionally, visit the commit and see that the "Branches" does not load (since it also belongs to a numeric branch):

❌ https://phabricator.wikimedia.org/rOSWB5a588bf0ee10546ddb9d0872d1be1bf48b3f3c07

What Should Happen Instead

The commit should be visible without crashes, just like this:

✅ https://phabricator.wikimedia.org/diffusion/OSWB/branches/main/;3c146aa84f5c1fa771425a0d9930cd0cc8b28fe8

✅ https://phabricator.wikimedia.org/rOSWB3c146aa84f5c1fa771425a0d9930cd0cc8b28fe8

etc.

Downstream: https://phabricator.wikimedia.org/T347483

Revisions and Commits

rP Phorge
	Abandoned		D25438 Allow branch names to be integer values to avoid an InvalidArgumentException
		D25945	rP17033a1c70d8 Fix exception handling a numeric git branch ("Call to phutil_nonempty_string()…

Related Objects

Mentioned Here: D25040: merge phab/master -> phorge/master

Event Timeline

aklapper created this task.Sep 16 2023, 13:43

Herald added subscribers: Cigaryno, Matthew, chris, tobiaswiese. · View Herald TranscriptSep 16 2023, 13:43

aklapper added a revision: D25438: Allow branch names to be integer values to avoid an InvalidArgumentException.Sep 16 2023, 13:51

Are you aware of any non-git repository in Wikimedia?

valerio.bozzolan added a project: Affects-Wikimedia.Sep 18 2023, 06:12

In T15640#13668, @valerio.bozzolan wrote:

Are you aware of any non-git repository in Wikimedia?

https://phabricator.wikimedia.org/diffusion/TSVN/browse/

aklapper updated the task description. (Show Details)Sep 27 2023, 12:19

I tried browsing that page without being able to trigger the issue

It's a Git repository branch called 201709 (see downstream) but still no idea how to trigger the exception.
If anyone has an idea what to do to reach code in DiffusionRequest::generateURI() or DiffusionCommitBranchesController, please share.

Finally got a reproducer URI in downstream:

Broken: https://phabricator.wikimedia.org/diffusion/OSWB/branches/main/.gitignore;5a588bf0ee10546ddb9d0872d1be1bf48b3f3c07 (branches in URI).
Works: https://phabricator.wikimedia.org/diffusion/OSWB/browse/main/.gitignore;5a588bf0ee10546ddb9d0872d1be1bf48b3f3c07 (browse in URI).

Call stack is a bit different though:

[2024-11-21 19:19:11] EXCEPTION: (InvalidArgumentException) Call to phutil_nonempty_string() expected null or a string, got: int. at [<arcanist>/src/utils/utils.php:2128]
arcanist(), ava(), phorge(), translations(), wmf-ext-misc()
  #0 <#2> phutil_nonempty_string(integer) called at [<phorge>/src/applications/repository/storage/PhabricatorRepository.php:752]
  #1 <#2> PhabricatorRepository::generateURI(array) called at [<phorge>/src/applications/diffusion/request/DiffusionRequest.php:459]
  #2 <#2> DiffusionRequest::generateURI(array) called at [<phorge>/src/applications/diffusion/view/DiffusionBranchListView.php:80]
  #3 <#2> phutil_tag(string, array, array) called at [<phorge>/src/infrastructure/javelin/markup.php:70]
  #4 <#2> phutil_escape_html(array) called at [<phorge>/src/infrastructure/markup/render.php:139]
  #5 <#2> phutil_escape_html(array) called at [<phorge>/src/infrastructure/markup/render.php:97]
  #6 <#2> phutil_tag(string, array, array) called at [<phorge>/src/view/phui/PHUITwoColumnView.php:242]
  #7 <#2> PHUITwoColumnView::buildFooter() called at [<phorge>/src/view/phui/PHUITwoColumnView.php:123]
  #8 <#2> PHUITwoColumnView::getTagContent() called at [<phorge>/src/view/AphrontTagView.php:161]
  #9 <#2> AphrontTagView::render() called at [<phorge>/src/view/AphrontView.php:222]
  #10 <#2> AphrontView::producePhutilSafeHTML() called at [<phorge>/src/infrastructure/markup/render.php:115]
  #11 <#2> phutil_escape_html(PHUITwoColumnView) called at [<phorge>/src/infrastructure/markup/render.php:171]
  #12 <#2> phutil_implode_html(string, array) called at [<phorge>/src/view/page/PhabricatorBarePageView.php:58]
  #13 <#2> PhabricatorBarePageView::willRenderPage() called at [<phorge>/src/view/page/PhabricatorStandardPageView.php:217]
  #14 <#2> PhabricatorStandardPageView::willRenderPage() called at [<phorge>/src/view/page/AphrontPageView.php:46]
  #15 <#2> AphrontPageView::render() called at [<phorge>/src/view/page/PhabricatorStandardPageView.php:918]
  #16 <#2> PhabricatorStandardPageView::produceAphrontResponse() called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:722]
  #17 <#2> AphrontApplicationConfiguration::produceResponse(AphrontRequest, PhabricatorStandardPageView) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:299]
  #18 phlog(InvalidArgumentException) called at [<phorge>/src/aphront/handler/PhabricatorDefaultRequestExceptionHandler.php:41]
  #19 PhabricatorDefaultRequestExceptionHandler::handleRequestThrowable(AphrontRequest, InvalidArgumentException) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:751]
  #20 AphrontApplicationConfiguration::handleThrowable(InvalidArgumentException) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:337]
  #21 AphrontApplicationConfiguration::processRequest(AphrontRequest, PhutilDeferredLog, AphrontPHPHTTPSink, MultimeterControl) called at [<phorge>/src/aphront/configuration/AphrontApplicationConfiguration.php:204]
  #22 AphrontApplicationConfiguration::runHTTPRequest(AphrontPHPHTTPSink) called at [<phorge>/webroot/index.php:35]

Wow. Assuming the line is the one with phutil_nonempty_string($branch), why branch "main" is considered a number?

Probably there is an underlying error somewhere and we should fix that 🤔 I would be curious about what number is that. lol. I presume 0.

BTW I think the page is reached by a crawler that is visiting a branch root on a specific commit:

https://phabricator.wikimedia.org/diffusion/OSWB/browse/main/;5a588bf0ee10546ddb9d0872d1be1bf48b3f3c07

And then clicking on Branches tab:

❌ https://phabricator.wikimedia.org/diffusion/OSWB/branches/main/;5a588bf0ee10546ddb9d0872d1be1bf48b3f3c07

Incredibly, this does NOT happen with this other commit:

✅ https://phabricator.wikimedia.org/diffusion/OSWB/branches/main/;af43eb6eea9f4bd1734096a22d68a6a70a95b7c4

So it seems commit-specific. ~~Maybe specific to commits starting with a number (?)~~ also this commit works:

✅ https://phabricator.wikimedia.org/diffusion/OSWB/branches/main/;3c146aa84f5c1fa771425a0d9930cd0cc8b28fe8

valerio.bozzolan triaged this task as Normal priority.Nov 29 2024, 18:47

valerio.bozzolan updated the task description. (Show Details)

In T15640#19898, @valerio.bozzolan wrote:

Wow. Assuming the line is the one with phutil_nonempty_string($branch), why branch "main" is considered a number?

That's not the case - it's related to existing other (!) branches with digits-only names. No clue why Phorge reaches code to query all branches though when rendering a single file though (that web page has no Branches dropdown or such). See my latest downstream comment. :)

The culprit is $map[$branch] = $branch_head; in https://we.phorge.it/source/phorge/browse/master/src/applications/diffusion/data/DiffusionGitBranch.php$105.
Before $branch was a string, afterwards it is an integer.

I can reproduce this locally with upstream git master via the self-contained testcase in https://phabricator.wikimedia.org/F58959277.

https://www.php.net/manual/en/language.types.array.php#language.types.array.syntax :

Strings containing valid decimal ints, unless the number is preceded by a + sign, will be cast to the int type.

Wow. 10 years of PHP, first time that I notice this

I'm still a bit shocked by how PHP is so weird. So:

we can mention that PHP arrays can transform string keys to int keys in the PHP pitfalls document https://we.phorge.it/book/flavor/article/php_pitfalls/
we can add more PHP inline documentation in the method that is probably the cause of this root problem https://we.phorge.it/source/phorge/browse/master/src/applications/diffusion/data/DiffusionGitBranch.php;123831b53fb7$85
we can use a raw (string) $key in the consumers of these array keys like in https://we.phorge.it/source/phorge/browse/master/src/applications/diffusion/conduit/DiffusionBranchQueryConduitAPIMethod.php;b3894bc2c62f$59

valerio.bozzolan renamed this task from Exception: "Call to phutil_nonempty_string() expected null or a string, got: int" in PhabricatorRepository.php to Errors when a commit belongs to a numeric git branch: Exception: "Call to phutil_nonempty_string() expected null or a string, got: int" in PhabricatorRepository.php.Wed, Apr 2, 11:23

valerio.bozzolan raised the priority of this task from Normal to High.

valerio.bozzolan updated the task description. (Show Details)

aklapper added a revision: D25945: Fix exception handling a numeric git branch ("Call to phutil_nonempty_string() expected null or a string, got: int" in PhabricatorRepository).Wed, Apr 2, 11:55

aklapper closed this task as Resolved by committing rP17033a1c70d8: Fix exception handling a numeric git branch ("Call to phutil_nonempty_string()….Wed, Apr 2, 13:09

aklapper claimed this task.

aklapper added a commit: rP17033a1c70d8: Fix exception handling a numeric git branch ("Call to phutil_nonempty_string()….

Errors when a commit belongs to a numeric git branch: Exception: "Call to phutil_nonempty_string() expected null or a string, got: int" in PhabricatorRepository.phpClosed, ResolvedPublicActions