Re: [Postfixadmin-devel] postfixadmin with ldap

[Christian B. <pos...@cb...>]

Hello,

Am Donnerstag, 12. Februar 2015 schrieb Vikas Parashar:
> Did you get the chance to look in it, as of now, i get stuck somewhere
> due to fear(or you might say conflict that would come). My fear is
> that more code more problem. So, i don't want to complete my code in
> one run.
> 
> Guide me if i am driving on wrong way.

I'm currently busy _and_ ill :-/ - but I'll give you a short review 
nevertheless (based on your git commits). Please don't expect this 
review to be complete, but you should at least get the most important 
points.

Also note that I don't know much about LDAP, so please correct me if one 
of my comments is wrong and/or crazy ;-)


In ldap.php, you have several lines like

+		if ($key == "domain" )
+		{		
+			$new_array["vd"] = str_replace("'", '', $value);
+		}

This approach will break if for example someone adds a field.
Please do the escaping based on the field type specified in 
$this->struct (see initStruct() in model/*Handler.php) or, if you want a 
simple way, on every field.

(Speaking of escaping - does LDAP really allow all chars except ' as 
valid? If not, using a char whitelist with allowed chars would be a 
better option.)


Instead of 

+	global $CONF;

please use Config::Read (or Config::bool etc. if it fits better)


Also, instead of having functions like ldap_domain_add() (what about 
adding a mailbox or an admin? you'll need another function) it's much 
better to use PFAHandler.php as a base. This probably isn't as easy as 
it sounds because SQL and LDAP queries are much different, but I'm sure 
it's possible.


For your local config changes, please create a file config.local.php 
instead of editing config.inc.php. (You can of course add ldap examples 
to config.inc.php.)

Also, please keep out templates_c/* out of git - those files are cache 
files you can delete at any time without breaking something.


I see that you parse SELECT queries to convert them to LDAP at least in 
some cases. Let me warn you that this will explode with the more 
interesting SQL queries (for example, test list.php?table=domain is in 
SNV, but not used yet - and does interesting SQL magic ;-)

Better modify the functions that generate the SQL queries. (I know that 
list-$whatever.php still uses hardcoded queries - feel free to try 
list.php instead).

> On Fri, Feb 6, 2015 at 12:57 PM, Vikas Parashar <par...@gm...>
> wrote:
> > On Tue, Feb 3, 2015 at 5:43 PM, Christian Boltzwrote:
> >> Am Montag, 2. Februar 2015 schrieb Vikas Parashar:

> > Ahh!, here i am using git, i can continue with svn as well.
> > Personally, i think svn is too old fashion and very heavy. May be,
> > we can think to move on git. Anyway, i can commit it to svn as
> > well..

That's an old discussion we had more than once ;-)
The advantage of SVN is that we can use the release number in 
upgrade.php function names, which means adding a function is enough 
(without the need to adjust a "highest function number" somewhere).

I'f open for ideas how to do this with git ;-)

That said - while you are writing your patch, using git is fine.

> > I would like to request to you kindly see my code on my git repo
> > when will you get the chance. Right from the beginning, i am
> > looking for some guidelines. I know only one rule KISS(Keep it
> > simply stupid). :)

Well, the other rules more or less base on KISS ;-)

If you think that we should add something less obvious to the coding 
guidelines, just propose it ;-)

> > Please check it here
> > https://vic...@gi.../vickally/postfixadmin.git> 


Regards,

Christian Boltz
-- 
> Also, Hosen runter:
Hose*n*! Du hast nur "die" Hose runtergelassen und die Unterhose
anbehalten. Nix da!
[> Stefan G. Weichinger und Peer Heinlein in postfixbuch-users]