Fix error in Mercurial when no offset is specified
AcceptedPublic
Actions

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

Details

Reviewers

valerio.bozzolan
speck

Group Reviewers

O1: Blessed Committers

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

Revision Contents
Changeset List

Path

Size

Packages

src/

applications/

diffusion/

conduit/

DiffusionHistoryQueryConduitAPIMethod.php

2 lines

O1: Blessed Committers

Diff 2052

View Options

src/applications/diffusion/conduit/DiffusionHistoryQueryConduitAPIMethod.php

Fix error in Mercurial when no offset is specifiedAcceptedPublicActions