Fix error in Mercurial when no offset is specified
ClosedPublic
Actions

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

Details

Reviewers

valerio.bozzolan
speck

Group Reviewers

O1: Blessed Committers

Commits

rPdbcc4472493a: Fix error in Mercurial when no offset is specified

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

Branch

master

Lint

Lint Passed

Unit

Tests Skipped

Build Status

Buildable 1307
Build 1307: arc lint + arc unit

Event Timeline

jeffrey created this revision.May 30 2024, 10:04

Owners added a reviewer: O1: Blessed Committers.May 30 2024, 10:04

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptMay 30 2024, 10:04

jeffrey requested review of this revision.May 30 2024, 10:04

Harbormaster completed remote builds in B1307: Diff 2052.May 30 2024, 10:04

sgtm

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.

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

Change offset to optional as requested by speck

Harbormaster completed remote builds in B1561: Diff 2418.Aug 23 2024, 22:20

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.

Welcome in the family of Trusted Contributors \o/ try again plz

Closed by commit rPdbcc4472493a: Fix error in Mercurial when no offset is specified. · Explain WhyWed, Dec 11, 11:02

This revision was automatically updated to reflect the committed changes.

jeffrey added a commit: rPdbcc4472493a: Fix error in Mercurial when no offset is specified.

Thanks! That worked :)