omines / directadmin

PHP client library to manage DirectAdmin control panel servers.
MIT License
105 stars 47 forks source link

Implement modifying user settings #5

Closed AramVK closed 8 years ago

AramVK commented 8 years ago

API command CMD_API_MODIFY_USER

curry684 commented 8 years ago

Hmm this isn't an easy one, judging by https://forum.directadmin.com/showthread.php?t=51235 and http://forum.directadmin.com/showthread.php?t=33636&p=170106#post170106 that call is a mess. I'll look into it though.

AramVK commented 8 years ago

I see, as I understand it the problem is that you need to send the complete package data. Maybe it's an idea to have 2 options within the new modify function:

  1. You either pass the complete data into the function (if you have the info stored, or you rewrite it completely)
  2. Or you pass only certain values and a separate call is internally made to collect the existing data, then it gets merged with the new data, and then it sends the complete modified data to the modify api call.
curry684 commented 8 years ago

Well the internal caches already hold the output of user config and usage. If the forward call is parameter-compatible the implementation is actually trivial. The major problem (as oft) is in this case that the documentation boils down to "look at the templates" which is barely better than none.

All classes are internally set up to store and access multiple string buckets, so it may just boil down to this pseudocode:

public function modify(array $modifications)
{
  invoke('CMD_API_MODIFY_USER', array_merge($oldSettings, $modifications));
}

Looking at what documentation there is is seems the only difference between the output of SHOW_USER_CONFIG and the parameters for MODIFY_USER is the handling of unlimited options, which should be adaptable.

AramVK commented 8 years ago

Ah I didn't realize it was such a mess.

I've done a quick compare based on the documentation (full list below). But as the documentation isn't complete (the KB article gives the language (even with a typo!) option in the example for MODIFY_USER, and language isn't in the documentation for MODIFY_USER but it is for SHOW_USER_CONFIG), I would assume that it would take nearly all parameters it also gives you from SHOW_USER_CONFIG.

Comparison of documentation (I'm guessing nearly all inconsistencies are just poor documentation, with the exception of the keys that provide information in italic). The 10 listed under SHOW_USER_CONFIG may need double checking what effect they may cause.

Both SHOW_USER_CONFIG and MODIFY_USER have: aftp bandwidth cgi cron dnscontrol domainptr ftp mysql nemailf nemailml nemailr nemails ns1 ns2 nsubdomains quota skin spam ssh ssl sysinfo vdomains

MODIFY_USER unique has: php ubandwidth udomainptr uftp umysql unemailf unemailml unemailr unemails unsubdomains uquota user uvdomains

SHOW_USER_CONFIG unique has: account additional_bandwidth catchall domain email ip language login_keys package suspend_at_limit

Gives info: creator _datecreated docsroot lastquotaupdate sentwarning suspended username usertype

Deprecated: zoom

curry684 commented 8 years ago

That's actually really helpful, thanks :) I've just dived into debug mode, and php is actually present in SHOW_USER_CONFIG, so docs are off there, and user makes sense to be set-only as it's only used to change the username. That leaves only the 'unlimited' options which we can patch internally from NULL values. I've created a feature branch to do some R&D on this.

curry684 commented 8 years ago

Implementation in feature branch seems to hold up to some simple tests:

$user->modify([
    'bandwidth' => 500,
    'quota' => 250,
    'php' => 'ON',
    'ssl' => 'ON',
    'vdomains' => 10,
]);
$user->modify([
    'bandwidth' => null,
    'quota' => 500,
    'vdomains' => 5,
    'ssl' => 'OFF',
]);

Afaics these tests are applied just fine and only modify the settings I'd want to.

AramVK commented 8 years ago

This looks great. In this example when you're setting bandwidth to null, it won't change?

curry684 commented 8 years ago

Just pushed another commit. NULL is considered unlimited. I've added accessor functions so in most cases you don't need to go through modifyConfig (which should in a perfect world be protected instead of public anyway).

AramVK commented 8 years ago

I see. These nicely documented wrappers are the way to go indeed :+1:

curry684 commented 8 years ago

I'll pull this back into the main branch when I've added some unit tests :)