Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DX] User::listing requires ugly logic to make it user-type agnostic #42

Closed
kenloburn opened this issue Aug 22, 2017 · 6 comments
Closed

Comments

@kenloburn
Copy link

At the time of writing, to be able to ascertain the user ID requires making a specific call to the method of the type of the user you're expecting. If you would run the following piece of code:

$userId = User::listing($apiContext)[0]->getUserPerson()->getId();

You would run into an exception if the API HTTP response yields a business user (or vice versa when you use getUserCompany():

Fatal error: Uncaught Error: Call to a member function getId() on null in /Users/rukn01/Projects/rknol/sdk_php/example/monetary_account_example.php:20

So logically speaking if your integration is not aware of the kind of user that will come out of the API, the only way you can express this would be something like this pre-PHP 7.1:

$userWrapper = User::listing($apiContext)[0];
$userObj = !is_null($userWrapper->getUserPerson()) 
    ? $userWrapper->getUserPerson() 
    : $userWrapper->getUserCompany();

$userId = $userObj->getId();

Or PHP 7.1+ with the null-coalesce operator:

$userWrapper = User::listing($apiContext)[0];
$userId = ($userWrapper->getUserPerson() ?? $userWrapper->getUserCompany())->getId();

Either scenarios are unfavorable, and unnecessary if the design of the SDK is more developer friendly. I understand that the reasoning behind this is because resource-wise they're 3 different resources, however it makes for very ugly implementation.

One way to solve this would to abstract away the logic inside the User resource:

class User
{
    // ...
    public function getUserObject()
    {
        return !is_null($this->userPerson)
            ? $this->userPerson
            : $this->userCompany;
    }
}

Or something in the likes so that the DX will be:

$userId = User::listing($apiContext)[0]->getUserObject()->getId();
@dnl-blkv
Copy link
Contributor

Good point, @kenloburn, we'll look into it!

@OGKevin OGKevin added this to the v0.11.0 milestone Aug 23, 2017
@OGKevin OGKevin removed this from the v0.11.0 milestone Sep 7, 2017
@DennisSnijder
Copy link
Contributor

@dnl-blkv this is pretty hard to do I suppose, since the API itself is designed to return different types of users.

@kenloburn
Copy link
Author

Not necessarily..

@dnl-blkv
Copy link
Contributor

@DennisSnijder fixing this would mean generating a wrapper. The task is quite straightforward, but does require some time. It is not hard, but not quick either :).

@OGKevin
Copy link
Contributor

OGKevin commented Nov 15, 2017

I have introduced a new method, which is called getReferencedObject and this is also present in User.

public function getReferencedObject()

This will return the field that is not null as of both fields can not be set at the same time. The only improvement that is needed is to correctly cast the return value of this method. For now you would need to use an if statement that checks instanceof.

@OGKevin
Copy link
Contributor

OGKevin commented Apr 7, 2018

The above comment is the closest I can get this function via the generator. Im open for other suggestions either way.

The way #60 implemented it would require that every class that implements (anchor interface)[https://github.com/bunq/sdk_php/blob/652e650aa35a8c0c598653e986ac7f68d4808bdd/src/Model/Core/AnchorObjectInterface.php] to implement another interface with all the methods of all the classes the the getReferencedObject() method would return.

This will ofc not be possible or a vital solution as there are a lot of anchor implementations where its not the case that all the classes have the same methods. And therefore it would be hard to form a contract.

So, there is no nice way to fix this in the SDK with the generator.

Im open for other approaches and suggestions however. Closing for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants