Page MenuHomePhorge

Fix error in Mercurial when no offset is specified
ClosedPublic

Authored by jeffrey on May 30 2024, 10:04.

Details

Summary

When viewing the history of a Mercurial diffusion repository the server return a 500. This is
because a function call to array_slice requires the offset to be set to an integer. So when it is
not specified it can be set to 0 as a default.

The error is as follows:

ERROR 8192: array_slice(): Passing null to parameter #2 ($offset) of type int is deprecated at [/usr/local/www/phorge/src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php:167]; PHP message: arcanist(head=master, ref.master=3cb117684f4e), phorge(head=master, ref.master=4bf5c452eb28); PHP message:   #0 array_slice(array, NULL) called at [<phorge>/src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php:167]
Test Plan

The history tab should not return a 500 when not specifying an offset

Diff Detail

Repository
rP Phorge
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

jeffrey requested review of this revision.May 30 2024, 10:04
This revision is now accepted and ready to land.May 30 2024, 10:33
aklapper retitled this revision from Fix error when no offset is specified to Fix error in Mercurial when no offset is specified.May 30 2024, 10:36
aklapper edited the summary of this revision. (Show Details)

Swear I’ve come across this before… it might be fixed in our forked branch so I’ll take a look. Does $limit also need same treatment?

No, limit is working fine as it is.

speck requested changes to this revision.Jun 22 2024, 03:12

I didn’t find changes in our fork regarding this. We should either change these parameters to be optional in their definition or throw exceptions when they aren’t specified. Changing them to be optional seems the better behavior.

src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php
27

Please change this to optional, along with limit below.

This revision now requires changes to proceed.Jun 22 2024, 03:12
aklapper subscribed.

Change offset to optional as requested by speck

Thanks aklapper. Seems good from our side @speck?

Ty. I’m still confused how “limit” is required but doesn’t throw an error when not specified. Can address that later if needed.

This revision is now accepted and ready to land.Sep 3 2024, 21:53

@jeffrey: Would you like to arc land your patch to get it merged?

@aklapper I'd like, but don't have permissions to push to the repo:

 >>>  Land these changes? [y/N/?] y
 MERGING  c63c9545c8c1 Fix error in Mercurial when no offset is specified
 MERGE  Attempting to rebase changes.
 DONE  Merge succeeded.
 PUSHING  Pushing changes to "origin".

  $   git push -- origin 0f3feb36398b:refs/heads/master

Exception: You do not have permission to push to this repository.