@pete-s said in I'm not a php dev, so what do you all think of this patch:
@pete-s said in I'm not a php dev, so what do you all think of this patch:
@jaredbusch said in I'm not a php dev, so what do you all think of this patch:
@pete-s said in I'm not a php dev, so what do you all think of this patch:
@jaredbusch said in I'm not a php dev, so what do you all think of this patch:
@pete-s said in I'm not a php dev, so what do you all think of this patch:
@jaredbusch said in I'm not a php dev, so what do you all think of this patch:
I just submitted this as a pull request to FreePBX's User Manager module.
I think it looks clean, but curious to what someone with more day to day PHP skills thinks about it.
Context matters when it comes to coding style. Do you have a link to the original source code?
The actual git is public as long as you are logged in to Attlasian.
https://git.freepbx.org/projects/FREEPBX/repos/userman/browse
They have a copy on Github but that is no longer kept in sync (apparently it is).
https://github.com/FreePBX/usermanYour code looks fine. It looks just like the original.
https://git.freepbx.org/projects/FREEPBX/repos/userman/browse/views/welcome.php
It is a pull request, it should.
I was discussing WHAT i did in my change, hence the diff in post 1.
Mostly using the ternary structure like I did.Yeah, that was what I was commenting. It's fine.
In general if...then...else is clearer than ? to read but it also clearer to not put many php tags back to back. But that is how the original is done. So what you've done is inline with how it is written.
The only very minor thing I could find, would be to not use ' and " in the same statement, unless there is a reason for it.
So instead of:
<?php echo _(sizeof($directories)==1 ? " selected":'')?>it would probably be better to do:
<?php echo _(sizeof($directories)==1 ? ' selected' : '') ?>(easier to read when you're consistent with white space as well)
It will work either way so...
meant to use ' that was a miss on me.