Art-of-WiFi / UniFi-API-client

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

change create_wlan function #169

Open OrionTheGiant opened 1 year ago

OrionTheGiant commented 1 year ago

No longer requires wlangroup_id and API endpoint updated.

I don't know how you'd like to handle having two different API endpoints based on the controller version so I just updated the existing line to point to the new endpoint. Happy to add some additional logic to check controller version if there's already a standard way of checking that.

karpiatek commented 1 year ago

Could you share how you use the create_wlan function? After implementing the proposed changes, I am getting an invalid payload message.

malle-pietje commented 1 year ago

Thanks for sharing this. I need to think about how to make this backwards compatible, e.g. by checking against the (controller) version property. Do we know with which controller version this behavior changed?

Also, we currently don't yet use the (controller) version property so need to implement this in a smart and re-usable manner.

malle-pietje commented 1 year ago

Am I correct in thinking the switchover when the /add/wlanconf was removed occurred with the introduction of version 6.X?

In that case the /rest/wlanconf route already existed with version 5.X so we could just modify the method/function as committed in this PR and use the comments to instruct devs about the change in behavior.

OrionTheGiant commented 1 year ago

@karpiatek I didn't change the function signature aside from setting wlangroup_id to default null so it shouldn't be any different. Are you including the array of ap_group_ids? Only asking because I missed that the first time around too.

$name = 'APIWiFi';
$password = 'APIWiFiPassword';
$usergroup_id = 'xxxxxxxxxxxxxxxxxx';

$result = $unifi_connection->create_wlan($name, $password, $usergroup_id, ...);
OrionTheGiant commented 1 year ago

@malle-pietje I'm not sure which version this changed in. Do you have a way to check that other than pulling different versions of the controller and snooping on the API call? This seems like a major version sort of change so 6.X or 7.X would be the likely candidates. 6.X would seem to make sense since (according to the create_wlan parameter comments) that's also when ap_group_ids became a required parameter

As for handling different endpoints, a utility function for checking greater/less than for the version number string will be helpful and then you can do something like

$endpoint = ($this->version_greater_than('6.0')) ? 'rest/wlanconf' : 'add/wlanconf';
return $this->fetch_results_boolean('/api/s/' . $this->site . $endpoint,  $payload);
karpiatek commented 1 year ago

@OrionTheGiant Yes, I do. I don't know, maybe I missed something.

<?php

require_once 'vendor/autoload.php';
require_once 'config.php';

$site_id ='iexxxxat';

$name ='APITest';
$x_passphrase ='Test1234#';
$usergroup_id ='62fcxxxxxxxxxxxxxxxxdef4';
$vlan_id = '62fcxxxxxxxxxxxxxxxx4b40';
$enabled = true;
$hide_ssid = false; 
$is_guest = false; 
$security = 'wpapsk'; 
$wpa_mode = 'wpa2'; 
$wpa_enc = 'ccmp'; 
$uapsd_enabled = false; 
$schedule = [];
$schedule_enabled = false;
$ap_group_ids = '62fexxxxxxxxxxxxxxxx9790'; 

$unifi_connection = new UniFi_API\Client($controlleruser, $controllerpassword, $controllerurl, $site_id, $controllerversion);
$unifi_connection->set_debug(true);
$loginresults     = $unifi_connection->login();
$results          = $unifi_connection->create_wlan($name, $x_passphrase, $usergroup_id, $wlangroup_id, $enabled, $hide_ssid, $is_guest, $security, $wpa_mode, $wpa_enc, $vlan_enabled, $vlan_id, $uapsd_enabled, $schedule_enabled, $schedule, $ap_group_ids);
$debug        = $unifi_connection->get_debug();

echo json_encode($results, JSON_PRETTY_PRINT);
echo json_encode($debug, JSON_PRETTY_PRINT);

I'm running UniFi version 6.x and I'm pretty sure, that if the changes you made work in version 7.x, it should also work in version 6.x. Apparently, I made some kind of mistake here. The code in Client.php is exactly the same as yours. I'm sorry for spamming here, it's just an engineer's curiosity. Trying to figure out what's wrong ;)

OrionTheGiant commented 1 year ago

@karpiatek I don't know if this is your only problem but the UniFi API needs ap_group_ids to be an array of strings, not just a string:

$ap_group_ids = ['62fexxxxxxxxxxxxxxxx9790'];

I ran into the same thing the first time trying create_wlan so I just added an additional check to add the ap_group_ids to an array if a string is received as input so it shouldn't be a problem anymore

sgrodzicki commented 1 year ago

I had similar observations while working on #191. In addition to the changes specifically to the wlangroup_id parameter, I think a clear versioning of this library would be useful and my recommendation would be to match the version of the controller itself (like in elastic/elasticsearch-php any many other client libraries). This approach makes it easier to follow changes without having to hack around older versions for backwards compatibility.

malle-pietje commented 1 year ago

I had similar observations while working on #191. In addition to the changes specifically to the wlangroup_id parameter, I think a clear versioning of this library would be useful and my recommendation would be to match the version of the controller itself (like in elastic/elasticsearch-php any many other client libraries). This approach makes it easier to follow changes without having to hack around older versions for backwards compatibility.

I see what you mean but this cannot be handled through versioning of the API client class. For example, most of our software packages are multi-site, multi-controller meaning that a single software instance must be able to talk with large number of controller versions. One of our instances actually supports almost each controller version between 5.11.50 and 7.3.83…

It makes more sense IMHO to have the API client deal with such changes which is why we still have the $controller_version property. It is still not used but I’m open to suggestions on how to implement this.

One of the next challenges will be to document this in a clear manner.

sgrodzicki commented 1 year ago

I had similar observations while working on #191. In addition to the changes specifically to the wlangroup_id parameter, I think a clear versioning of this library would be useful and my recommendation would be to match the version of the controller itself (like in elastic/elasticsearch-php any many other client libraries). This approach makes it easier to follow changes without having to hack around older versions for backwards compatibility.

I see what you mean but this cannot be handled through versioning of the API client class. For example, most of our software packages are multi-site, multi-controller meaning that a single software instance must be able to talk with large number of controller versions. One of our instances actually supports almost each controller version between 5.11.50 and 7.3.83…

It makes more sense IMHO to have the API client deal with such changes which is why we still have the $controller_version property. It is still not used but I’m open to suggestions on how to implement this.

One of the next challenges will be to document this in a clear manner.

Fair enough. I was not aware of such use cases. Then I suggest to start by introducing unit tests and integration tests (in combination with a PHP version matrix). I can work on this.