Just a note for the README

[pallsopp]

I noticed at the end of the readme it says that not all functions might be available in the future.

There are 2 ways to combat this:

• Wraps all function definitions in a "method_exists" call
• Route all function calls through a middleware class which checks if the function exists in the API, and if not sends back a message saying the function no longer exists (preferred way)

Just a thought. Thanks for the awesome code, just what I was looking for.

[malle-pietje]

Thanks for the suggestion. I'm already starting to think about refactoring the class but the downside will be that it will probably break the class into multiple files making it more difficult for people to include it in their projects without using composer. We could then also add an Interface (I think) which adapts to the controller version and throws an Exception when a route is no longer supported.

That being said we already trigger errors (not so clean I know but that came with the heritage of the class) for methods/functions that no longer exist or have been renamed.

If you have any other ideas for a full refactoring of the class I'm open to suggestions.

[pallsopp]

I’m definitely interested in taking a deeper look at the code, maybe this coming weekend. I’ll let you know if I have any additional suggestions. It ain't easy being green! Please recycle.

…

On Mar 30, 2021, at 8:19 AM, malle-pietje ***@***.***> wrote:  Thanks for the suggestion. I'm already starting to think about refactoring the class but the downside will be that it will probably break the class into multiple files making it more difficult for people to include it in their projects without using composer. We could then also add an Interface (I think) which adapts to the controller version and throws an Exception when a route is no longer supported. That being said we already trigger errors (not so clean I know but that came with the heritage of the class) for methods/functions that no longer exist or have been renamed. If you have any other ideas for a full refactoring of the class I'm open to suggestions. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or unsubscribe.

[malle-pietje]

IMHO the biggest challenge is to logically group and name the end points. Chaining methods would also make it easier and more intuitive to use the API client. Something like:

$unifi->stats()->site()->fetch($start, $end);

And of course introduce Exceptions for specific responses but also to wrap cURL errors.

[ghost]

Ok, I just forked the repo. I'll start taking a look at it later today. I agree, splitting it into logical components makes sense, and adding some specific custom exceptions.

[ghost]

I think the first thing I'd do is create a version 2, and tempt users into migrating to it, while maintaining version 1, but give version 1 an EOL.
Development-wise I'd bump the project to PHP7.2 unless there's a very good reason not to, and also start creating PHPUnit tests for everything.
Code-wise: The first new version 2 thing I'd do is move all of the settings for the client class (username, password, method, etc.) into a config file. Then create a copy of it with what the user might actually change, as a user config file that's private and not part of the repo.
I'd then create a static Settings class, and use that to manage all of the config requests, like: Settings::get(Settings::CONNECT_TIMEOUT) to stop using strings and start using constants.

That would be a first step in a version 2: update the dev-env, introduce tests, and extract all configuration.
After that I'd start building additional classes and separating methods into logical places using those new classes.

[ghost]

If you like the sound of that, I can fork the repo and start creating it.