You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
How soon can we head towards a 3.0.0 release, and what do we want to change/include? The reason I ask is because the 2.x series doesn't work with Guzzle 7, so it might make sense to release 3.0.0 within the next month or so. I'm happy to help with both implementation and reviewing tasks. :)
I don't think 3.0 should be long away, but I want to get some work of the. 3.0 milestone done before putting a release out. Most of these issues require some prepwork or deprecations in 2.0.
Second part would be to check if there are things in 2.0 that should need some cleanup, so we can prepare the changes and have them cleaned in 3.0.
With #554, I think we should have add a perPage method which creates a new instance of the api class with per page set. That way we can avoid the double set hack from the result pager and make the api classes immutable (in that respect). It could also be implemented on 2.x in a BC way (by deprecating ApiInterface and the set/get page methods and adding a fresh interface PageableInterface or something).
I suppose the 2nd issue to address RE immutability is the configure methods. We could simply make configure return a new instance, rather than modifying the class. In 2.x, it's already marked as returning the class, and in 3.x we could drop the fact that it mutates the original.
Maybe it's not worth implementing on 2.x tbh, and a pain to implement safely. On 3.x, we could make the constructor in AbstractApi final, which would allow us to do this:
/** * @param Client $client * @param int|null $perPage * * @return void */publicfinalfunction__construct(Client$client, int$perPage = null)
{
if (null !== $perPage && ($perPage < 1 || $perPage > 100)) {
thrownewValueError(sprintf('%s::perPage(): Argument #2 ($perPage) must be between 1 and 100', self::class));
}
$this->client = $client;
$this->perPage = $perPage
}
/** * Create a new instance with the given per page parameter. * * @param int|null $perPage * * @return static */publicfunctionperPage(?int$perPage)
{
returnnewstatic($this->client, $perPage);
}
That solution won't work, because some API want to have additional input. I have a solution that works though now, and I've implemented it for the GitLab API client https://github.com/GitLabPHP/Client/tree/10.0. I'll PR the same solution here. :)
Thanks I will checkout the PR! I was checking the current test suite and found some more stuff we need to deprecate for removal in 3.0. I will create issues for these things later
I think the next job for me after pagination is sorted is to remove the configure() stuff. I think that is only used to set accept headers (either to change the content type from json to something else, or to access preview APIs). I think it would be totally reasonable for methods to simple pass an accept header in the headers list when they make a ->get or ->post, etc call. We don't need to over-engineer things. :)
Activity
acrobat commentedon Jul 11, 2020
I don't think 3.0 should be long away, but I want to get some work of the. 3.0 milestone done before putting a release out. Most of these issues require some prepwork or deprecations in 2.0.
Second part would be to check if there are things in 2.0 that should need some cleanup, so we can prepare the changes and have them cleaned in 3.0.
GrahamCampbell commentedon Jul 11, 2020
With #554, I think we should have add a
perPagemethod which creates a new instance of the api class with per page set. That way we can avoid the double set hack from the result pager and make the api classes immutable (in that respect). It could also be implemented on 2.x in a BC way (by deprecating ApiInterface and the set/get page methods and adding a fresh interfacePageableInterfaceor something).I suppose the 2nd issue to address RE immutability is the configure methods. We could simply make configure return a new instance, rather than modifying the class. In 2.x, it's already marked as returning the class, and in 3.x we could drop the fact that it mutates the original.
GrahamCampbell commentedon Jul 11, 2020
Let me know if you like those ideas, or have something different in mind, and I'll get started on a PR.
GrahamCampbell commentedon Jul 11, 2020
Maybe it's not worth implementing on 2.x tbh, and a pain to implement safely. On 3.x, we could make the constructor in AbstractApi final, which would allow us to do this:
GrahamCampbell commentedon Jul 14, 2020
That solution won't work, because some API want to have additional input. I have a solution that works though now, and I've implemented it for the GitLab API client https://github.com/GitLabPHP/Client/tree/10.0. I'll PR the same solution here. :)
acrobat commentedon Jul 16, 2020
Thanks I will checkout the PR! I was checking the current test suite and found some more stuff we need to deprecate for removal in 3.0. I will create issues for these things later
GrahamCampbell commentedon Jul 16, 2020
I think the next job for me after pagination is sorted is to remove the
configure()stuff. I think that is only used to set accept headers (either to change the content type from json to something else, or to access preview APIs). I think it would be totally reasonable for methods to simple pass an accept header in the headers list when they make a ->get or ->post, etc call. We don't need to over-engineer things. :)acrobat commentedon Nov 29, 2020
I'm going to close this one as the only thing I want to get done for 3.0 is #907. Other work will be done in 3.x of prepared for 4.0.
feature #907 [3.x] Re-worked pagination to not mutate the api classes…