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

change create_wlan function #169

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

OrionTheGiant
Copy link

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.

No longer requires wlangroup_id and API endpoint updated
@karpiatek
Copy link

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

@malle-pietje
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Author

@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
Copy link
Author

@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
Copy link

karpiatek commented Aug 24, 2022

@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
Copy link
Author

@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
Copy link
Contributor

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
Copy link
Collaborator

malle-pietje commented Apr 17, 2023

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
Copy link
Contributor

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.

@sgrodzicki sgrodzicki mentioned this pull request Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants