Art-of-WiFi / UniFi-API-client

A PHP API client class to interact with Ubiquiti's UniFi Controller API
MIT License
1.13k stars 223 forks source link

Feature/set ap wlangroups by device name #44

Closed gahujipo closed 2 years ago

gahujipo commented 5 years ago

The function takes as arguments the names of both the wlan_group and the device_name instead of their IDs. I hope it is ok to put that into the method instead of putting it into the example. Otherwise let me know and I'll refactor that.

malle-pietje commented 5 years ago

Thanks, for consistency sake I'd rather stick to parameters which are passed programmatically instead of through GET parameters. Most examples are to be used through CLI. If you wish to share a complete example which processes input from a form, feel free to create another PR for that.

malle-pietje commented 5 years ago

For the set_ap_wlangroup method in the class, I'd prefer to stick with the concept as used with the other existing methods and use the value of the _id property to identify a device. There is nothing stopping you from creating your own wrapper around this which allows you to use name as the identifier.

Can you maybe submit a change of the current set_ap_wlangroup to use the new payload structure while accepting the same parameters (thus making it compatible with existing implementations using this method)? Then the wrapper showing how to use the name as input can be submitted as an example?

gahujipo commented 5 years ago

Sure. I'll modify my PR so that it aligns to the current concept. Could you help me understand where I should distinguish whether to send the old or new payload type? Or would you simply replace the content of the method set_ap_wlangroup to use the new payload type?

malle-pietje commented 5 years ago

Thanks, I suggest to simply change the payload structure of the existing method to ensure the correct structure is passed to the API.

This API behavior has apparently changed a while ago so I don't expect any issues with that for current deployments of the API Client.

gahujipo commented 5 years ago

Alright. I'll update this PR according to your description.

malle-pietje commented 5 years ago

thanks!

gahujipo commented 5 years ago

I refactored the whole logic of my previous method into the existing to make the existing one compatible with newer REST API. The example contains both the wrapper which previously was part of the new function (so it isn't part of the method anymore).

gahujipo commented 5 years ago

Does this PR meet all requirements to get merged?

malle-pietje commented 5 years ago

Thanks. My apologies for the late response but I haven't been able to find the time to go through and some other enhancements I plan to add in. Probably before next week.

lucastcox commented 4 years ago

@malle-pietje , any idea when this will be merged?

malle-pietje commented 4 years ago

There must be a cleaner way to achieve this.

malle-pietje commented 4 years ago

BTW, I believe this code will generate issues with single radio devices such as the "old" UAP. Have you tested your code with such devices?

gahujipo commented 4 years ago

No. Because i don't have such devices. Sorry.

malle-pietje commented 4 years ago

Understood. Then it’ll have to wait until I can find time to dive in myself.

gahujipo commented 4 years ago

Just to be sure: UAP-AC-Pro is not included in "older" devices, right? Because there it works. Every night this code disables my wifi an reenables it in the morning.

malle-pietje commented 4 years ago

Correct, in this context “old” is pre-AC.