kbsali / php-redmine-api

A simple PHP Redmine API client, Object Oriented
MIT License
420 stars 183 forks source link

There is a default limit set even I use the retrieveAll method. #277

Closed Gandhi11 closed 1 year ago

Gandhi11 commented 3 years ago

It looks like there is a default limit set even I use the retrieveAll method with the AbstractAPI.

For example $projects = $this->Client->getApi('project')->all();

There will be a limit of 25 in the projects. I need the pass a param like ['limit' => 999]. But the thing is, even with a limit like that the code is maxing out the limit to 100 in the retrieveAll method in the AbstractAPI class.

Is there any way to bypass any limit?

Art4 commented 3 years ago

You can simply use ['limit' => 999] it should work as expected.

AbstractApi::retreiveAll() splits a call with a limit > 100 into multiple calls with a max limit of 100 each. The reason is that the Redmine API allows a max limit for 100. See https://www.redmine.org/projects/redmine/wiki/Rest_api#Collection-resources-and-pagination

  • limit: the number of items to be present in the response (default is 25, maximum is 100)

See also #35.

I hope this will answer your question.

Gandhi11 commented 3 years ago

Ok I understand,

But why if I do something like

$projects = $this->Client->getApi('project')->all();

I got a maximum of 25 projects. We really need to pass a ['limit' => 999] param? Isn't that odd?

Art4 commented 3 years ago

I understand what you mean. Fetching all projects, issues, users, etc could end in a large amount of requests and data. I think it should be requested explicit by adding the limit parameter.

One could argue that ->all() is a bad name for the method and should rename into a more appropriate method like ->list(). That would be an enhancement.

Gandhi11 commented 3 years ago

Yes I agree with you. ->all() should be renamed to ->list().

If possible ->all() should really retrieve all the data even if it's ending with a large amount of requests.

What do you think?

Art4 commented 2 years ago

I've been thinking about the problem and can't come to a satisfying conclusion on how the ->all() method should be implemented.

At the moment ->all() returns a maximum of 25 elements if the limit is not changed with ->all(['limit' => 200]). If we want to implement the ->all() method properly, I see two possibilities:

  1. We set the internal limit to all(['limit' => \PHP_INT_MAX ]), which strictly speaking does NOT mean all. Such massive requests could possibly lead to errors like timeouts, rate limits, etc. 2) Since Redmine has a maximum limit of 100, you can continue to query elements until a response returns no more elements. So we will always have an unnecessary request that does not provide any data. Again, this could possibly lead to errors such as timeouts, rate limits, etc.

I would call both changes a breaking change because it changes the internal behavior in such a way that it potentially leads to new unexpected errors for the developer.

I think both of these possibilities have the potential to break things. So I would make that more explicitly the responsibility of the developer.

So my suggestion would be to introduce a new method ->list() where it is clearly communicated that by default only 25 items are loaded, but with ->list(['limit' => 200]) more items can be requested. In addition, we can write a warning to this method that setting the limit too large can lead to unexpected behaviors. I would mark the ->all() method as deprecated and remove it in the v3.

@kbsali What do you think?

Art4 commented 1 year ago

@kbsali What do you think about deprecating ->all() method in favor of a new ->list() method?