Page MenuHomePhabricator

Increase pilimit default value from 1 to PARAM_MAX
Closed, ResolvedPublic1 Estimated Story Points

Description

I was playing with the pageimages API today, in preparation for the Multimedia team's work for next quarter, and I spent about 30 minutes trying to debug PageImages and core, because I overlooked the fine print on the API documentation that explains PageImages will only return one result by default.

As far as I can tell, getting the images is a reasonably performant process, and it won't crash the servers to fetch results for a few more pages in the set. However, I'd be interested to know the rationale for the low default.

We should increase the pilimit by default to 10 or max (50 right now) to avoid the confusing behavior when using the api and only getting an image for the first result on the query.

Developer notes

Update includes/ApiQueryPageImages.php ApiBase::PARAM_DFLT in getAllowedParams() from 1 to 50.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 19 2016, 8:00 PM
ovasileva added subscribers: bmansurov, ovasileva.

@bmansurov to comment further on this

ovasileva triaged this task as Medium priority.Dec 21 2016, 5:40 PM
bmansurov added a comment.EditedDec 21 2016, 10:23 PM

@MarkTraceur you already can get multiple images, .e.g https://en.wikipedia.org/w/api.php?action=query&prop=pageprops&ppprop=page_image|page_image_free&titles=Barack%20Obama|Avatar%20(2009%20film)

But, I suppose you want to get the images using the pageimages API. If I remember correctly, you had to pass in an additional query parameter. @MaxSem do you know what it was?

I suppose so. But I'm specifically talking about prop=pageimages here (and prop=extracts in the companion bug about TextExtracts)

Jdlrobson added a subscriber: Jdlrobson.

Open question: What should the new default limit be? max ? some other integer?

MaxSem added a comment.Feb 9 2017, 6:45 PM

/me is confused because the limit is 50/500 for bots.

@MarkTraceur how many images do you want to retrieve? We're trying to define some hard limits.

Jhernandez renamed this task from 1 seems like an unnecessarily strict default limit for number of pageimages returned from API to Increase pilimit to a number bigger than 1.Apr 11 2017, 3:45 PM
Jhernandez updated the task description. (Show Details)
Jhernandez added a subscriber: Jhernandez.

I'd default to something like 10, or max if it is not a problem really.

I'd vote for PARAM_MAX (50) as this should cover most requests. Note that this would then only impact API requests returning more than 50 articles. The API already errors when you request too many so this would mean no headaches for API users.

Jdlrobson renamed this task from Increase pilimit to a number bigger than 1 to Increase pilimit default value from 1 to PARAM_MAX.Apr 11 2017, 5:16 PM
Jdlrobson updated the task description. (Show Details)May 17 2017, 5:45 PM

Should we send an email after we're done to mediawiki-api-announce? (with both of the similar tasks)

NHarateh_WMF set the point value for this task to 1.May 17 2017, 5:46 PM

Change 354471 had a related patch set uploaded (by Bmansurov; owner: Bmansurov):
[mediawiki/extensions/PageImages@master] Increase default API limit from 1 to 50

https://gerrit.wikimedia.org/r/354471

Change 354471 merged by jenkins-bot:
[mediawiki/extensions/PageImages@master] Increase default API limit from 1 to 50

https://gerrit.wikimedia.org/r/354471

bmansurov removed bmansurov as the assignee of this task.May 19 2017, 3:42 PM
phuedx claimed this task.EditedMay 22 2017, 5:11 PM
phuedx added a subscriber: phuedx.

I can sign this off today.

phuedx closed this task as Resolved.May 22 2017, 5:20 PM

… each page in the result has a thumbnail 👍

The patch has a request to decrease the limit from 50 to 10. @Jdlrobson should we re-open the task, or do we have a strong reason to keep 50?

I'm about to merge a patch (https://gerrit.wikimedia.org/r/#/c/357752) updating our API calls in light of this change. Please give me a ping if the pilimit default is revised again...

@Mholloway no change was necessary. We only changed default. In fact I'd recommend you continue to explicitly definite pilimit in case it does change...