search

SylvainTI / phpliteadmin

Automatically exported from code.google.com/p/phpliteadmin
0 stars 0 forks source link

move config outside of main file #181

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
I have suggestion to move config outside of the main script and use logic of 2 
configs : sample and local user's

└───phpliteAdmin
        phpliteadmin.config.php
        phpliteadmin.config.sample.php
        phpliteadmin.php
        README.txt

where config will contain all data between

//BEGIN USER-DEFINED VARIABLES
//////////////////////////////
and
////////////////////////////
//END USER-DEFINED VARIABLES

Original issue reported on code.google.com by ykoro...@gmail.com on 26 Feb 2013 at 11:23

Attachments:

GoogleCodeExporter commented 9 years ago 
Yes, clearly has advantages.
Currently we don't do it because of the "only 1 file you can move 
around"-approach.
But perhaps we should give this another thought.

I see these advantages:
- update is easier
- user sees the place to edit config more easily
- users are used to config-files like this

I see these disadvantages:
- breaking the 1-file-approach a bit
- you need to move the config-file when you move the source-file

I am wondering if it makes sense to make this optional. I guess not.

Overall, I think the advantages are higher and we should go for it.
Any other opinions?

Original comment by crazy4ch...@gmail.com on 26 Feb 2013 at 11:46

GoogleCodeExporter commented 9 years ago 
it's good you stick with 1 file. here's another way, with embeded config, but 
with extended local if available.

1 bad thing is - if new parameter is introduced we won't be notified.

By now the best way is array/config merging, which is a pain to add here. But 
again, it will give people ability to work with trunk version and make patches 
easier

Original comment by ykoro...@gmail.com on 26 Feb 2013 at 12:18

Attachments:

GoogleCodeExporter commented 9 years ago 
"1 bad thing is - if new parameter is introduced we won't be notified."
can be fixed by mantaining sample config file which can be removed if user wants

Original comment by ykoro...@gmail.com on 26 Feb 2013 at 12:25

Attachments:

GoogleCodeExporter commented 9 years ago 
I wrote extensively about this in a (sad and lonely!) post on the mailing list.

In short, an optional external configuration file would be great, but in my 
opinion these points are important:
- it should not be php code (user friendly, does not break in case of syntax 
errors)
- it should be writable from PLA (to fix theme or other options, better than 
cookies)
- it should be easy to revert/extract defaults from the code (failsafe, user 
friendly)
- overriding defaults should be value-by-value, not "either we use the internal 
config or the external one" (missing value should not break PLA, also less 
trust in external files)
- there should be a clear convention regarding which configuration values have 
priority, depending on their source (defaults, config, session, cookies)

Original comment by dreadnaut on 26 Feb 2013 at 3:11

GoogleCodeExporter commented 9 years ago 
Whops, I didn't want to remove the CC: names —reverted.

Original comment by dreadnaut on 26 Feb 2013 at 3:14

GoogleCodeExporter commented 9 years ago 
not sure what you mean under user friendly. This project use devs/admins, I've 
never seen regular user who manages database. Even if user, right now he 
changes this config in php code. hehe

what is PLA?

any "failsafe" code generates hundreds lines of code.

about value by value - it is done by array merge. Config will be in php, but 
who really cares? XML is not easier

full config substitution is the easiest way (so I did it for myself already 
locally). Array merge requires user variables to be refactored (aka grouped, 
renamed, etc..). Also how are you going to manage user's functions?

Original comment by ykoro...@gmail.com on 26 Feb 2013 at 3:20

GoogleCodeExporter commented 9 years ago 
PLA is phpLiteAdmin, and phpLiteAdmin's users are indeed developers and admins. 
They are still called 'users' though :)

"Failsafe" just means that the program will not stop or break if its 
configuration is broken or missing. It's just a matter of having defaults and 
not relying on parsing php —which can be done in about ten lines, not 
"hundreds".

User functions are another issue, as they have to be php. I'd like to see a 
survey on their use.

Anyway, go read my post for more details maybe, the group archives are open:
http://groups.google.com/group/phpliteadmin/

Original comment by dreadnaut on 26 Feb 2013 at 3:36

GoogleCodeExporter commented 9 years ago 
Sorry dreadnaut. I had in mind that somebody had already requested this, but 
could not find an issue about it. And I forgot it was in the mailing list. (So 
I just added some guys to CC who I thought might have mentioned this earlier)
And sorry nobody answered in the Mailing list...

So probably it's now time to discuss it. So we all agree that having external 
configuration files would be great (at least optional).

Now the question is whether we go for a simple php-file or something like 
ini-files, xml-files or something custom.

I think all approaches have advantages and disadvantages. So let's discuss them.

"User friendly" really depends on who the user is. And as ykorotia wrote, PLA's 
users are mainly php-developers. So they should be quite comfortable with 
php-files. PhpMyAdmin uses a php-config-file like this as well, and they have 
similar users ;-)

Personally, I feel more comfortable with a php-config-file than an ini-file for 
example. Because I am 100% sure how php-syntax works. With ini-files, I always 
wonder how white-space is treated (does it matter if I add a tab here? how can 
I add a line-break in a value?), how comments can be done (# or ; in here?) and 
so on.
Of course we could also argue php-developers & admins should know ini-files 
from php.ini

I agree that adding 100s of LOC for config handling is not appropriate in this 
project, but PHP comes with pretty much everything we need to code ini 
(parse_ini_file), xml or even our own custom format in a small bunch of lines.

"Should be writable from PLA". Hmm. Yes, agreed. Could also be done with 
php-based config-files in theory, but this is clearly in favour of other 
approaches like ini.

"should be easy to revert/extract defaults from the code"
No problem with any of the solutions in my opinion.

"overriding defaults should be value-by-value"
No problem with any of the solutions in my opinion. If we continue using php 
variables, we could simply override default values with included user-values.

"there should be a clear convention regarding which configuration values have 
priority, depending on their source"
I guess this priority is obvious and therefore clear with any approach (from 
user perspective). From developer perspective, it would be better to 
encapsulate this so we don't have to check session, cookie and so on manually 
every time. Agreed.

Original comment by crazy4ch...@gmail.com on 26 Feb 2013 at 6:09

GoogleCodeExporter commented 9 years ago 
Ah, another thing that came to my mind:

We could store settings in a SQLite-DB. Just to discuss, I don't really like 
this solution very much myself ;-)

Original comment by crazy4ch...@gmail.com on 26 Feb 2013 at 6:12

GoogleCodeExporter commented 9 years ago 
Come on, let's not make it necessary to use PLA to edit PLA's settings :)  A 
text editor should be enough.

Ideally, *nano* (or notepad) should be enough —the basic tools you will have 
on a computer, through ssh, on the go, on a server with little software 
installed, on a NAS or mobile, etc.  Keep It Simple is always true.

Original comment by dreadnaut on 26 Feb 2013 at 6:16

GoogleCodeExporter commented 9 years ago 
haha ;-)
Sorry, maybe just forget my last post, it was more meant to provoke ;-)
(would make it pretty hard to reset the password for example ;-) )

Original comment by crazy4ch...@gmail.com on 26 Feb 2013 at 6:19

GoogleCodeExporter commented 9 years ago 
So I think we should either go for ini-files or php-files.
I don't think a custom-solution is a good idea. Always error-prone to reinvent 
the wheel and users get unsure about how to use it.

So currently I think ini-files might probably be the best solution for PLA.

Some more suggestions just for completeness:
- use Pear Config: http://pear.php.net/package/Config
Problem: Dependency on a comparably huge library. Pear often not installed.
- use JSON
Not so pretty (and user-friendly), but really nice to handle from PHP and other 
languages.

Another argument for the php-based solution:
It makes it possible to code a dynamic configuration. You could for example 
imagine passwords, languages or database-paths coming from some database. You 
could code this easily with some lines of php code.
With a configuration-file like an ini-file, you'd need to overwrite the 
ini-file from a cronjob or something like this.
(Not an important advantage because lot's of users don't do this kind of 
things, but I like flexible systems...)

Original comment by crazy4ch...@gmail.com on 26 Feb 2013 at 6:34

GoogleCodeExporter commented 9 years ago 
As, sorry, I took it too seriously :-p

And shocked by your message, I had somehow missed your previous post above O_O. 
I'll clarify just one point in response to that and then wait for what the 
others have to say ( please say something! anything! :) )

| "Should be writable from PLA". Hmm. Yes, agreed. Could also be done with
| php-based config-files in theory, but this is clearly in favour of other
| approaches like ini.

Php has var_export() which makes .php config files as easy as .ini ones. I 
don't like .ini files myself, nor the way parse_ini_files() behaves in many 
cases (I consider it broken and usually avoid it in my projects).

The main reason why I'd prefer a "non-php config" is error handling: incorrect 
code might give warnings, return null values, or results in Errors that stop 
execution —all stuff that we will have trouble handling. Even worse, these 
errors might be **invisible** depending on the local php configuration, and 
result in unexpected behaviour or a nice white page.

These are issues which would be present in the current version too, but I 
expect an external file to be more vulnerable.

Plus, the fact the current users are programmers doesn't mean that we should 
keep it difficult, if we don't need to. That would only ensure that future 
users will STILL be programmers, it doesn't exactly open up to less experienced 
users.

In any case, I suppose the questions are "do we need *php* config files? does 
it make things safer/easier/better along any dimensions? is there anything 
better?" disregarding the fact that we like them more or less :)

Original comment by dreadnaut on 26 Feb 2013 at 6:55

GoogleCodeExporter commented 9 years ago 
that's how I see it

Original comment by ykoro...@gmail.com on 26 Feb 2013 at 6:56

Attachments:

GoogleCodeExporter commented 9 years ago 
> The main reason why I'd prefer a "non-php config" is error handling: 
incorrect code 
> might give warnings, return null values, or results in Errors that stop 
execution 
> —all stuff that we will have trouble handling. Even worse, these errors 
might be 
> **invisible** depending on the local php configuration, and result in 
unexpected 
> behaviour or a nice white page
Yes, that's correct. I mean, we could turn display_errors on before including 
the external file and off afterwards to make config-errors appear, but this 
only solves part of the issue.

> "do we need *php* config files? does it make things safer/easier/better along 
any 
> dimensions? is there anything better?

Safer: Yes and No. "No" first: I think php-config files make things less safe, 
especially if the file is writable. var_export() makes it quite safe what *we* 
write, but we still have an executable file which is writable. Some security 
issue (of another script for example) could make it possible to write some code 
into it and execute it. This is not possible with ini-files or other 
non-executable files.

"Yes, php-files are safer": PHP-Code is executed when opened in a browser, not 
shown. Other files such as ini-files are shown. This is problematic as we store 
passwords and paths in there. So we cannot simply put some ini-file there 
without protection. And protection requires .htaccess to work for example. This 
would make another file, and .htacess doesn't work on lots of hosts. Clearly an 
issue we need to discuss.

Easier: Yes, I think so. Basically, it's an include to read and some 
var_exports (in a loop) to write. And as you said, parse_ini_file is not so 
pretty and there is no out-of-the-box php-function to write ini-files as far as 
I know, so writing would require some lines of code.

Better: Well, that's what we discuss.

@ykorotia: What's the advantage of arrays you think is important here? I mean, 
compared to variables.

Original comment by crazy4ch...@gmail.com on 26 Feb 2013 at 7:17

GoogleCodeExporter commented 9 years ago 
@it possible to write some code into it and execute it@
hehe. if admin is not noob he will give only read access to file by filesystem 
and that's it. if admin is noob, why should you care?

@What's the advantage of arrays you think is important here? I mean, compared 
to variables.@

MERGING .. and PHP is arrays, you don't have to parse anything, it's NATIVE, 
while all other formats are aliens.

INI files are kind of dinosaurs, it's MS-DOS.. are you serious about it, or 
it's kind of joke I don't know?

Original comment by ykoro...@gmail.com on 26 Feb 2013 at 8:24

GoogleCodeExporter commented 9 years ago 
If we end up encapsulating everything in a class, we will probably have a 
configuration arrays anyway. Or more than one (defaults, config, $_COOKIE, 
$_SESSION) which are merged depending on priority. It also reduces writing to a 
single var_export() and makes reading easier (include-d files are executed in 
the current scope; unless we include the file in the global scope, created 
variables will not be global and things become more complicated...)

Also, Just an idea for a possible hybrid .php solution which gives no output 
when run directly (except an error). Not very clean IMO, but at least we know 
it's possible.

-----
<?php return PLAConfig::load(__FILE__); ?>

# non-php config follows
password     12345
directory    dbs/
-----

Original comment by dreadnaut on 26 Feb 2013 at 8:30

GoogleCodeExporter commented 9 years ago 
Can you explain how it is going to work?

Original comment by ykoro...@gmail.com on 26 Feb 2013 at 8:36

GoogleCodeExporter commented 9 years ago 
Only these two points are important:
- if you 'return' from an included file, the remaining line of the file are not 
executed and therefore not sent to output;
- if you run the file without including it, php will halt execution because the 
class does not exist, sending only a "Fatal error" to output.

You can then build a .php file which contains non-php configuration.

Original comment by dreadnaut on 26 Feb 2013 at 8:43

GoogleCodeExporter commented 9 years ago 
> @it possible to write some code into it and execute it@
> hehe. if admin is not noob he will give only read access to file by 
filesystem and 
> that's it. if admin is noob, why should you care?
Because we were talking about being able to write to the file from PLA. So you 
could change a setting in PLA's GUI and save the configuration to the file. And 
then we need to make it writable, no matter how smart the admin is.

> @What's the advantage of arrays you think is important here? I mean, compared 
to variables.@
> MERGING .. and PHP is arrays, you don't have to parse anything, it's NATIVE, 
while 
> all other formats are aliens.
I meant PHP-arrays vs. PHP variables. You don't even need to merge anything 
with variables. You just load them in the correct order and if they are defined 
in the included file, they will overwrite the default ones. If one is not 
defined, the default is used. So merging is rather a disadvantage of arrays 
because it is overhead (which is not really true either, you can use 
overwriting if you use the right syntax as well). Variables are native as well.

> INI files are kind of dinosaurs, it's MS-DOS.. are you serious about it, or 
it's 
> kind of joke I don't know?
No, I was not joking about inifiles. Arrays are dinosours as well. Just because 
something is old, doesn't necessarily mean it's bad. If we argue like that, 
using php-files is the wrong way as well, we would need to store the config as 
xml in the cloud just to be up-to-date.

@dreadnaut:
Hmm. And in PLAConfig:load(), you parse this file? Why don't you just place a 
<?php exit; ?> in the first line and don't include it at all, but parse it 
straight away?

But I think people will get confused if the file-extension is .php and the 
content actually is something we parse ourself.

Original comment by crazy4ch...@gmail.com on 26 Feb 2013 at 9:51

GoogleCodeExporter commented 9 years ago 
@chris: nevermind, it was an intricate example of something that could be 
parsed, included, be php, be non-php, etc.

Anyway, I slept on it, and changed my mind a bit.

1) I realised that I *hate* when software overwrites my configuration files: 
comments are gone, the ordering messed up, etc. We should let configuration 
files be what they are, and that is read-only; we can find another place to 
store data, e.g. a different phpliteadmin.data.php.

2) Configuration should be a .php file for security and therefore php code; we 
can make it simple and readable anyway. Variables would probably be less error 
prone than arrays, and we can use something like compact() if we want to handle 
them as arrays.

We can start by simply including an optional external file, and later improve 
on the internal interface to configuration and conf. source preference that I 
mentioned previously.

Original comment by dreadnaut on 27 Feb 2013 at 1:23

GoogleCodeExporter commented 9 years ago 
Okay.
I can live very well with a read-only php-file included defining 
config-variables.
So we basically only move the declarations we already have into an external 
file. That's more or less what ykorotia initially proposed ;-)

But I think having a sample and a real config-file is not necessary.
I think we should just provide a config-file along with PLA and users edit this 
config file. If they mess it up, they can simply download the original config 
file again.
And if we introduce new config variables in new versions, we check with isset() 
if they are there and print a message if they are missing (and use a default as 
long as they are missing).

Everyone agrees?

Original comment by crazy4ch...@gmail.com on 27 Feb 2013 at 2:39

GoogleCodeExporter commented 9 years ago 
Since we are keeping the config section inside the main file that can be used 
as sample; however, it is common to have an external sample file to copy or for 
reference. I have no preference though.

For the external config file, I would avoid checking with isset() and allow for 
partial configuration. People will probably be happy with default values, but 
they might have a two-lines external config with custom values, e.g.

<?php
$directory = 'dbs/';
$password = '12345';

If new variables are introduced, people will notice them if/when they need 
them. We just need to write decent release notes :)

Original comment by dreadnaut on 27 Feb 2013 at 5:59

GoogleCodeExporter commented 9 years ago 
Okay, right.

>it is common to have an external sample file to copy or for reference 
Yes, but as we have a 1-file-approach, I guess spending another file on a 
sample config-file is not necessary.

So as we now seem to agree on the requirements, I'd say patches are welcome ;-)

Original comment by crazy4ch...@gmail.com on 27 Feb 2013 at 7:39

GoogleCodeExporter commented 9 years ago 
Do we want anything more complex than adding this around line 375?

$config_filename = './phpliteadmin.config.php';
if (is_readable($config_filename))
  include $config_filename;

Original comment by dreadnaut on 27 Feb 2013 at 7:52

GoogleCodeExporter commented 9 years ago 
I guess not. But I think we should really make clear to users that this file is 
included and overwrites settings defined in phpliteadmin.php. So I think we 
should place it above the language-constants around line 110, so users see it. 
And we should place some comments somewhere around lines 36 and 108 that tell 
the user that he might want to edit the config-file instead of the source file.

Especially users that know phpliteadmin will edit phpliteadmin.php and might be 
confused because their config-settings get overwritten with default-settings.

So maybe it really is a good idea to make PLA come with a sample config-file 
only that needs to be renamed to be used.
This also solves the "should be provide a sample additionally" question because 
the user can then decide whether he copies the sample or edits and renames it.

Original comment by crazy4ch...@gmail.com on 27 Feb 2013 at 8:03

GoogleCodeExporter commented 9 years ago 
here it is

Original comment by ykoro...@gmail.com on 27 Feb 2013 at 9:04

Attachments:

GoogleCodeExporter commented 9 years ago 
[deleted comment]
GoogleCodeExporter commented 9 years ago 
| Custom functions MUST BE configured/declared in phpliteadmin file directly!

Er, why?
Also, could you switch all double quotes to single quotes? They are a tiny bit 
faster :)

Original comment by dreadnaut on 27 Feb 2013 at 9:17

GoogleCodeExporter commented 9 years ago 
I did true copy-paste. Same quotes as in main file.

because you cannot redeclare functions. Of course, you can make 
function_exists(...) bla-bla. I did as I did. I cannot get your aproach with 
customs functions, so that's it

do what ever you want

regards

Original comment by ykoro...@gmail.com on 27 Feb 2013 at 9:22

GoogleCodeExporter commented 9 years ago 
> Also, could you switch all double quotes to single quotes? They are a tiny 
bit faster :)

LOL. Yeah, well, I thought so myself, but there is much discussion about this. 
Lots of people claim double quotes are even faster. For example have a look at 
these:
http://nikic.github.com/2012/01/09/Disproving-the-Single-Quotes-Performance-Myth
.html
http://phpbench.com/
http://micro-optimization.com/single-vs-double-quotes

I guess the real point why we should use single quotes for configuration is 
because people that don't know PHP might otherwise do something like this:
$cookie_name = "pla3412$thiswilldo{$strange}things";

Probably that's the reason why $cookie_name currently uses single quotes and 
all others use double quotes, but I am not sure.

What I think is more important is to use stuff consistently. We maybe should 
have a look at this sometime, but that's another issue.

> Custom functions MUST BE configured/declared in phpliteadmin file directly!
Yeah, indeed functions cannot be redeclared. Maybe we should put an "if 
(function_exists(..)" around the functions we provide as examples.
Or just say something like "make sure you don't declare the same function in 
phpliteadmin.php and phpliteadmin.config.php"

Original comment by crazy4ch...@gmail.com on 27 Feb 2013 at 10:21

GoogleCodeExporter commented 9 years ago 
| LOL. Yeah, well, I thought so myself, but there is much discussion about this.
| Lots of people claim double quotes are even faster.

I have seen those pages myself through the years, and they usually amount to 
bad science. You can check yourself: take the code from this 
http://micro-optimization.com/single-vs-double-quotes and run it. Then swap the 
calls to f1() and f2(), so that double quotes are tested before single quotes, 
and run it again.

However, the timing difference is so small that there is no practical use in 
forcing one style over the other, hence the smilie in my previous message, and 
this one :)

Single-quote strings are good practice for the reason you point out, and for 
static HTML strings where double quotes may appear for attributes.

About user functions: since we now have a sample configuration, do we still 
need sample functions inside the main code? We could move them to the sample 
config (together with the $databases sample array?) and reduce the 
phplisteadmin.php's size.

Original comment by dreadnaut on 27 Feb 2013 at 11:04

GoogleCodeExporter commented 9 years ago 
> swap the calls to f1() and f2(), so that double quotes are tested before 
single quotes, and run it again.
Yeah, I know people make mistakes like this. In fact it doesn't matter at all - 
it is not worth even thinking about this if it is only several nano seconds. If 
we write code where this would matter, we should use another language, not php.

> About user functions: since we now have a sample configuration, do we still 
need sample functions inside the main code? We could move them to the sample 
config (together with the $databases sample array?) and reduce the 
phplisteadmin.php's size.
Yeah, sounds reasonable.

Original comment by crazy4ch...@gmail.com on 27 Feb 2013 at 11:10

GoogleCodeExporter commented 9 years ago 
Ok, extending on ykorotia's: a patch to remove user functions from the main 
file and include the external config (where I swapped file_exists() for 
is_readable()) and a config file with a slightly different option ordering.

Also, we could change the $database config array to 'name' => 'filename'. I 
know the current format is used later, but this would make more sense and we 
can convert it to a sub-array later.

And if the key is an integer, we could use the filename as name, so people 
could write:

$databases = array(
 'myfile.sqlite',
 'othername.sqlite',
 'shortname' => 'verylongpath/loooongpath/longereven/sillyname.sqlite',
);

Original comment by dreadnaut on 27 Feb 2013 at 11:36

GoogleCodeExporter commented 9 years ago 
Uhm, attachment fail?

Original comment by dreadnaut on 27 Feb 2013 at 11:37

Attachments:

GoogleCodeExporter commented 9 years ago 
Thanks. I really like this solution. Feel free to commit it.

Regarding the change of $databases: Well, I guess only a few people actually 
still use this method. I agree that name => filename or filename only would be 
easier.

If you feel that it's worth it:
Please open a separate issue for it and copy the relevant posts there.
And if you like, write a patch for it. But please make it backwards compatible. 
So it should loop through the array and distinguish all three possibilities:
1. key is int and value is string => value is path, create name automatically 
from filename
2. key is int and value is array => old method used. Use name and path elements 
of the value-array. (if they don't exist, error)
3. key is string and value is string => use key as name and value as path
4. else (e.g. value is null): display error message (and continue looping?)

Original comment by crazy4ch...@gmail.com on 28 Feb 2013 at 9:56

GoogleCodeExporter commented 9 years ago

Original comment by crazy4ch...@gmail.com on 28 Feb 2013 at 9:56

GoogleCodeExporter commented 9 years ago 
This issue was closed by revision r345.

Original comment by dreadnaut on 1 Mar 2013 at 6:05

GoogleCodeExporter commented 9 years ago 
This issue was closed by revision r346.

Original comment by dreadnaut on 1 Mar 2013 at 6:06

GoogleCodeExporter commented 9 years ago 
Committed, in two parts because I forgot to move one of the two files :|

I also added a new wiki page where we should add proper documentation. I'll 
file a new issue for that, but you can find it here: 
http://code.google.com/p/phpliteadmin/wiki/Configuration

Original comment by dreadnaut on 1 Mar 2013 at 6:10

GoogleCodeExporter commented 9 years ago 
Thanks.

Original comment by crazy4ch...@gmail.com on 2 Mar 2013 at 5:35