Allows the `*Lookup` methods to accept arrays of identifiers or screen names #45

weierophinney · 2017-08-16T17:08:06Z

Each of the followers/lookup and users/lookup endpoints allow either a user_id or screen_name parameter, and either of these may be a single user identifier, or comma-separated list of them.

To enable this, I created a new method, createUserListParameter(), which does the following:

If the $ids argument is scalar, it proxies to createUserParameter().
If the $ids argument is more than 100 items, it raises an exception (per the Twitter API docs, no more than 100 lookups can be performed in a single request).
If the array is a mixture of integer identifiers and string screen names, it raises an exception (we can only send one type of lookup per request).
If the array is of screen names, it validates each.

I also updated createUserParameter() to raise an exception if the $id is neither an integer nor a string, which makes it more robust. It also does a lookup between validInteger() and validateScreenName() for a string value, raising an exception if not, as the latter method only accepts strings.

I also updated validInteger to return 0 if a non-integer, non-string value is provided.

These changes allow the friendshipsLookup() and usersLookup() methods to now accept:

A single user identifier (integer)
A single screen name (string)
An array of user identifiers (integers)
An array of screen names (strings)

If a mixture of integers and strings is detected, they will each raise an exception.

weierophinney · 2017-08-16T17:09:07Z

Ping @calevans for review.

Ocramius · 2017-08-16T17:10:50Z

src/Twitter.php

+     *
+     * - a single user identifier
+     * - a single screen name
+     * - a list of user identifiers


This should be split into a singular and a plural method (new API).
Widening the accepted types is also a BC break, as it breaks LSP compliance for subclasses.

Is it considered widening if no typehint is present, though?

Further, the method is a 1:1 map with the Twitter API, which has always allowed these values; this is a fix to ensure that we allow what the API allows.

Yes, because the previous API was about a single identifier. The problem is child classes, not really the twitter API.

Doing so would break the 1:1 relationship with the Twitter API, though. Additionally, the changes still support usage of singular identifiers.

I understand the point about parameter widening and child classes. However, an API wrapper like this is less likely to have somebody extend an existing method that maps to the API, and more likely to extend to add methods to interact with API endpoints that have not yet been implemented (e.g., new methods). The primary factor in this case is that the consumer API does not break, versus the extension API.

Breaking 1:1 naming is not a problem, but introducing mixed endpoints is.

The consumer doesn't break anyway: if they need to use multi-value APIs, they just use the specialised method.

This is a limitation of PHP itself, since method overloading isn't allowed.

Regardless of BC, adding an union type is a problem.

FWIW, as a consumer of this component, having the end points match the twitter API is better. The API does not differentiate between calling the loopkups with a single ID or multiple IDs. Not sure why the component should.

This would be a breaking BC as I actually have working code using these methods.

Ocramius · 2017-08-16T17:11:05Z

src/Twitter.php

+     *
+     * - a single user identifier
+     * - a single screen name
+     * - a list of user identifiers


Same comment as above

Ocramius · 2017-08-16T17:13:25Z

src/Twitter.php

    {
+        if (! is_string($int) && ! is_int($int)) {


The entire method can be simplified to `if (((int) $int) !== (string) (int) $int) { return 0; } return (int) $int;}

Some Twitter API identifiers exceed PHP_INT_MAX, which is why a regex is performed here; the value may very well be a string.

I see - missed that :-)

Ocramius · 2017-08-16T17:14:05Z

src/Twitter.php

+     * @throws Exception\InvalidArgumentException if an array of $ids exceeds 100 items.
+     * @throws Exception\InvalidArgumentException if an array of $ids are not all of the same type.
+     * @throws Exception\InvalidArgumentException if any screen name element is invalid.
+     */


This is a potential extension point for developers extending the class, as there are other methods in the Twitter API that behave similarly; protected was chosen deliberately, to match the visibility of createUserParameter(), which is also protected to allow for extension.

They look ok to me. Once they are implemented (and the other PR as well, I'll plug it into my projects and see if anything breaks. :)

Cheers!
=C=

…n names Each of the `followers/lookup` and `users/lookup` endpoints allow either a `user_id` or `screen_name` parameter, and either of these may be a single user identifier, or comma-separated list of them. To enable this, I created a new method, `createUserListParameter()`, which does the following: - If the `$ids` argument is not an array, it proxies to `createUserParameter()`. - If the `$ids` argument is more than 100 items, it raises an exception (per the Twitter API docs, no more than 100 lookups can be performed in a single request). - If the array is a mixture of integer identifiers and string screen names, it raises an exception (we can only send one type of lookup per request). - If the array is of screen names, it validates each. I also updated `createUserParameter()` to raise an exception if the `$id` is neither an integer nor a string, which makes it more robust. It also does a lookup between `validInteger()` and `validateScreenName()` for a string value, raising an exception if not, as the latter method only accepts strings. Finally, I updated `validInteger` to return 0 if a non-integer, non-string value is provided.

weierophinney · 2017-08-17T18:13:35Z

@Ocramius I've updated the pull request to instead introduce two new methods, friendshipsLookupMany() and usersLookupMany(), which should resolve the issues with type widening you noted previously, removing the BC breakage.

weierophinney · 2017-08-17T18:14:53Z

src/Twitter.php

@@ -1342,8 +1373,28 @@ public function statusesUserTimeline(array $options = []) : Response
    public function usersLookup($id, array $params = []) : Response
    {
        $path = 'users/lookup';
-        // $params = $this->createUserParameter($id, $params);
-        $params['user_id'] = $id;
+        $params = $this->createUserParameter($id, $params);


This line was re-instated, as we really need to allow passing either the user identifier or screen name, which was the previous use case; I think we removed it to allow passing a comma-separated list of user identifiers, which is resolved with the addition of usersLookupMany().

weierophinney · 2017-08-17T18:15:26Z

src/Twitter.php

    {
-        if (is_int($int)) {
+        if (is_int($int) && $int > -1) {


I realized in reviewing tests that negative values aren't allowed, so I added this additional condition.

calevans · 2017-08-17T18:29:47Z

Sorry but this is wrong.

This is a wrapper for the Twitter API. The API has specified what the wrapper should look like. By adding the *Many functions as a user I now have to understand the Twitter API and where the wrapper deviates from it.
This requires more complex code be written in my application. Now at the application level, I have to decide if I am passing in a single value or multiple values and call the appropriate method.
I actually use this component and have since around 2013. If I were to switch to this version I would have to refactor existing, working code for no apparent reason.

Honestly, I cannot see a single technical benefit to the *Many functions. It looks like an opinion and one of someone who is not using the component.

Just my $0.02 worth. I've got a working version of this component and if you decide to proceed this way, it's not going to break anything of mine. It'll make me sad because I was hoping to move back to the official version. But I'll get over it. :)

Cheers! :)
=C=

weierophinney · 2017-08-17T20:25:54Z

I've been giving this a bunch of of consideration, and also looking at how other HTTP API wrappers handle this (one particularly relevant to this is the Twit library for Node.js). What I see is that these always do a 1:1 mapping, regardless of whether or not the arguments to the endpoint allow for union types. The rationale in each case is "it allows easily identifying the corresponding API documentation."

As such, I need to side with @calevans here, even if I agree technically with @Ocramius. HTTP API wrappers are a different beast than other code, for better or for worse.

I'm going to revert my most recent commit, though I'll add a new one to bring in the check for the $int > -1 condition.

Close #45

Forward port #45 Conflicts: CHANGELOG.md

Ocramius suggested changes Aug 16, 2017

View reviewed changes

weierophinney force-pushed the hotfix/lookup-with-multiple-users branch from 6f6f350 to 8a38229 Compare August 17, 2017 17:36

weierophinney commented Aug 17, 2017

View reviewed changes

Ensures that an integer identifier is greater than 1

a451aaf

weierophinney force-pushed the hotfix/lookup-with-multiple-users branch from 54975f3 to a451aaf Compare August 17, 2017 20:27

weierophinney merged commit a451aaf into zendframework:master Aug 17, 2017

weierophinney added a commit that referenced this pull request Aug 17, 2017

Adds CHANGELOG for #45

25d9816

weierophinney added a commit that referenced this pull request Aug 17, 2017

Merge branch 'hotfix/lookup-with-multiple-users'

7a02a11

Close #45

weierophinney added a commit that referenced this pull request Aug 17, 2017

Merge branch 'hotfix/lookup-with-multiple-users' into develop

ed0a52f

Forward port #45 Conflicts: CHANGELOG.md

weierophinney deleted the hotfix/lookup-with-multiple-users branch August 17, 2017 20:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allows the `*Lookup` methods to accept arrays of identifiers or screen names #45

Allows the `*Lookup` methods to accept arrays of identifiers or screen names #45

weierophinney commented Aug 16, 2017 •

edited

Loading

weierophinney commented Aug 16, 2017

Ocramius Aug 16, 2017

weierophinney Aug 16, 2017

Ocramius Aug 16, 2017

weierophinney Aug 16, 2017

weierophinney Aug 16, 2017

Ocramius Aug 16, 2017

calevans Aug 16, 2017

Ocramius Aug 16, 2017

Ocramius Aug 16, 2017

weierophinney Aug 16, 2017

Ocramius Aug 16, 2017

Ocramius Aug 16, 2017

weierophinney Aug 16, 2017

calevans Aug 16, 2017

weierophinney commented Aug 17, 2017

weierophinney Aug 17, 2017

weierophinney Aug 17, 2017

calevans commented Aug 17, 2017

weierophinney commented Aug 17, 2017

Allows the *Lookup methods to accept arrays of identifiers or screen names #45

Allows the *Lookup methods to accept arrays of identifiers or screen names #45

Conversation

weierophinney commented Aug 16, 2017 • edited Loading

weierophinney commented Aug 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weierophinney commented Aug 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calevans commented Aug 17, 2017

weierophinney commented Aug 17, 2017

Allows the `*Lookup` methods to accept arrays of identifiers or screen names #45

Allows the `*Lookup` methods to accept arrays of identifiers or screen names #45

weierophinney commented Aug 16, 2017 •

edited

Loading