Clearing up global constants

[dreadnaut]

Originally reported on Google Code with ID 210

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 :)

Reported by dreadnaut on 2013-04-06 19:57:22

<hr> * Attachment: constants.diff

[Christopher Kramer]

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.

Reported by crazy4chrissi on 2013-04-22 22:15:58

[dreadnaut]

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...

Reported by dreadnaut on 2013-04-24 10:32:49 - Status changed: Started

<hr> * Attachment: constants.diff

[Christopher Kramer]

Looks good. Feel free to commit it.

Reported by crazy4chrissi on 2013-04-26 19:16:33

[dreadnaut]

This issue was closed by revision r412.

Reported by dreadnaut on 2013-04-30 11:40:35 - Status changed: Fixed

[Christopher Kramer]

Thanks.

Reported by crazy4chrissi on 2013-04-30 11:45:42

[dreadnaut]

Thank you for checking my code!

Reported by dreadnaut on 2013-04-30 11:46:54

[Christopher Kramer]

Reported by crazy4chrissi on 2014-01-02 16:26:41 - Labels added: Target-1.9.5