search

Buzzinoffbond / phpliteadmin

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

Clearing up global constants #210

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago 
Do we need DATATYPE, FUCTIONS and CUSTOM_FUCTIONS?

Datatypes simply stores an array, used a few times later in the code:
index.php:96:   define("DATATYPES", serialize($types));
index.php:1326: $types = unserialize(DATATYPES);
index.php:2644: $types = unserialize(DATATYPES);
index.php:2756: $types = unserialize(DATATYPES);

Functions is similar, and is only used together with CUSTOM_FUNCTION, through 
getUserFunctions:
index.php:100:  define("FUNCTIONS", serialize($functions));
index.php:2240: $functions = array_merge(unserialize(FUNCTIONS), 
$db->getUserFunctions());
index.php:2345: $functions = array_merge(unserialize(FUNCTIONS), 
$db->getUserFunctions());

This last one confuses me a bit...
index.php:101:           define("CUSTOM_FUNCTIONS", 
serialize($custom_functions));
classes/Database.php:41: $cfns = unserialize(CUSTOM_FUNCTIONS);
classes/Database.php:53: $cfns = unserialize(CUSTOM_FUNCTIONS);
classes/Database.php:66: $cfns = unserialize(CUSTOM_FUNCTIONS);

Custom/user functions are added to the database object in 
Database::__construct, but then a copy is store also in Database. No other use 
is made in the Database class, but they are requested from the main flow 
through getUserFunctions.

A patch is attached that would:
- dispose of DATATYPE and FUNCTIONS, renaming the corresponding arrays as 
$sqlite_datatypes and $sqlite_functions for clarity
- remove the code relative to CUSTOM_FUNCTIONS from Database::__construct
- remove addUserFunction and getUserFunctions from Database
- add an equivalent Database::registerUserFunction
- call registerUserFunction on database creation

Result: fewer constants, less data stored in Database, fewer function calls to 
move this data around.

Please double check this, since I might have missed something important :)

Original issue reported on code.google.com by dreadnaut on 6 Apr 2013 at 7:57

Attachments:

GoogleCodeExporter commented 9 years ago 
DATATYPES: Useless in my opinion. The only change it makes that it makes it 
_constant_ explicitely, i.e. cannot changed later on. But not much of a 
benefit. We should not mess with (i.e. overwrite) any config variable unless we 
have a very good reason.

FUNCTIONS: Here we mess with $functions as we overwrite it ;-) But yes, I 
agree, we can get rid of the constant. But we should not write to $functions 
when merging, but use some new variable for the merge with custom functions. 
You already clear this up with your patch.

CUSTOM_FUNCTIONS: Yeah. Database->fns seems useless as only a copy of 
CUSTOM_FUNCTIONS and never read.

Your patch looks good. Only two spaces for intention in registerUserFunction().

Only thing I see: You only call registerUserFunction once, but Database objects 
are created at multiple places. So this changes behaviour (in some cases, user 
functions don't get registered anylonger). These cases are:
- create new db
- export db as sql
- export db as csv
- import db

In some cases this might change behaviour noticeably. I.e. if a dump uses a 
user function, you cannot import it any longer.

Original comment by crazy4ch...@gmail.com on 22 Apr 2013 at 10:15

GoogleCodeExporter commented 9 years ago 
Right! We should register custom functions also when importing data, at least.
Attached is a new patch to fix that, and the two rogue spaces.

We still have 'FORCETYPE' and 'PAGE' around, but I'm leaving them for later. I 
don't feel confortable hacking at the Database class yet, but I'd like to clean 
up the constructor and remove all output from the class. Also, there are a lot 
of 'if($this->type==' around...

Original comment by dreadnaut on 24 Apr 2013 at 10:32

Attachments:

GoogleCodeExporter commented 9 years ago 
Looks good. Feel free to commit it.

Original comment by crazy4ch...@gmail.com on 26 Apr 2013 at 7:16

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

Original comment by dreadnaut on 30 Apr 2013 at 11:40

GoogleCodeExporter commented 9 years ago 
Thanks.

Original comment by crazy4ch...@gmail.com on 30 Apr 2013 at 11:45

GoogleCodeExporter commented 9 years ago 
Thank you for checking my code!

Original comment by dreadnaut on 30 Apr 2013 at 11:46

GoogleCodeExporter commented 9 years ago

Original comment by crazy4ch...@gmail.com on 2 Jan 2014 at 4:26