Announcement

  •  » Engine
  •  » Database improvements

#31 2009-11-17 20:44:48

nicolas
Former Piwigo Team
2004-12-30
1232

Re: Database improvements

grum wrote:

I just wonder if we cannot use POO to do this....

I think it could have be better for an intellectual point of view and for a complexity of code. But for performance I think functionnal approach is better.  And the "migration" would be simplier.

In a second step we can easily improve code and use POO if we want to.

Offline

 

#32 2009-11-20 16:18:25

nicolas
Former Piwigo Team
2004-12-30
1232

Re: Database improvements

With my last commit ([Subversion] r4325), I almost remove all mysql_* functions from core code.

I don't solve plugins problem and I let code as it was. Any idea on how to solve that ? Should we add a param to plugin to say if it allow multiple database engine  ?

Next step :
I will start to propose postgresQL on installation stage ([Bugtracker] ticket 1255). I will have to improve some sql code.

Offline

 

#33 2009-11-21 14:54:46

rvelices
Former Piwigo Team
2005-12-29
1960

Re: Database improvements

nicolas, i have a doubt about your recent commit
mysql doc it says

Code:

[LIMIT {[offset,] row_count | row_count OFFSET offset}]

i didnt test your commit, but it seems to me that the parameters are inversed when using OFFSET

Offline

 

#34 2009-11-21 20:40:07

nicolas
Former Piwigo Team
2004-12-30
1232

Re: Database improvements

rvelices wrote:

nicolas, i have a doubt about your recent commit
mysql doc it says

Code:

[LIMIT {[offset,] row_count | row_count OFFSET offset}]

i didnt test your commit, but it seems to me that the parameters are inversed when using OFFSET

Thanks. You're absolulty right. I read the documentation too quickly.
Fix [Subversion] r4334

Last edited by nicolas (2009-11-21 20:56:56)

Offline

 

#35 2009-11-23 13:04:37

rub
Former Piwigo Team
Lille
2005-08-26
5019

Re: Database improvements

About [Bugtracker] ticket 928 and [Subversion] r4280.

Nicolas, I  don't understand your way about "$conf['users_table']" and "define 'USERS_TABLE'".

If I have a config_local.inc.php, it's necessary to do again a "define('USERS_TABLE', $prefixeTable.'users');" ?

=> that invoked problem for all gallery in production

Normaly, with $conf only a new value of $conf value is necessary.

Offline

 

#36 2009-11-23 14:10:14

nicolas
Former Piwigo Team
2004-12-30
1232

Re: Database improvements

I don't understand the problem. I don't there's no problem !

Before my commit, when you want an external authentification, you modify include/config_local.inc.php in such a way :

Code:

$conf['users_table'] = 'your_specific_user_table';

That modification is propagated in include/constants.php to define USERS_TABLE constant.

After my commit, you allways modify include/config_local.inc.php but in that way :

Code:

define('USERS_TABLE', 'your_specific_user_table');

The constant is not override in include/config_local.inc.php because it already exists. So I know that some website which used external authentification will be broken but we could had a warning on upgrade. That modification will be only in a major release.

Is it ok for you ?

Offline

 

#37 2009-11-23 14:55:19

rub
Former Piwigo Team
Lille
2005-08-26
5019

Re: Database improvements

I know that technically there are no problem.

Point 1:
---------
The problem, it's for user:
  o You increase the complexity level of configuration
  o You change "rules" of local_config

For me, config_local:
  o must be the more simple in order to be used by user witch not know php
  o param of config files must be define on table #_config or with other system

With you method, it's not possible to switch all the value on table #config for example.

Personnaly, the $conf should not be changed.


Point 2:
---------
Other thing, in other part of code, I prefer to use $conf['users_table'] than USERS_TABLE.
Simply, in order to make link between $conf['users_table'] and $conf['user_fields'].

But it's just my preference...

Offline

 

#38 2009-11-23 16:17:37

nicolas
Former Piwigo Team
2004-12-30
1232

Re: Database improvements

I'm not sure to really understand your message.
Before my commit, to use external authentification you must add an entry $conf['users_table'] in include/config_local.inc.php
After my commit, you always modify the same file. You not add an entry in $conf but you define a constant. I don't think the gap of complexity is so large.

For me config_local file is here to override default behavior of piwigo.

Offline

 

#39 2009-11-23 16:40:58

rub
Former Piwigo Team
Lille
2005-08-26
5019

Re: Database improvements

nicolas wrote:

I'm not sure to really understand your message.

You have understood it right.

nicolas wrote:

Before my commit, to use external authentification you must add an entry $conf['users_table'] in include/config_local.inc.php
After my commit, you always modify the same file. You not add an entry in $conf but you define a constant. I don't think the gap of complexity is so large.

For a developer the gap is not complexy. But for a user than can be more difficult.
But it's the mixt of entry un $conf and define constant what I don't like.

nicolas wrote:

For me config_local file is here to override default behavior of piwigo.

config_local, it's not define_local
plugins is here to o override default behavior of piwigo.


But, it's just my point of view... Team your opinion?

Last edited by rub (2009-11-23 16:42:14)

Offline

 

#40 2009-11-23 21:44:01

VDigital
Former Piwigo Team
Paris (FR)
2005-05-04
17680

Re: Database improvements

I'd just read las five post and for me, it's not clear enough.

Rub,
Could you remind me the full old and new syntax?

$conf['users_table']"  = ...
and "define('USERS_TABLE'.....


More could explain the problem of people who had never coded in php and are using the shared user table.
What could happen after migration?
Does the upgrade process able to change?

Gotcha, (after Rub answers), give us a beginner view point.

Thxs to all


Piwigo.com: Start and run your own photo gallery. Signup and get 30 days to try for free, no commitment.
8-)

Offline

 

#41 2009-11-23 21:50:06

plg
Piwigo Team
Nantes, France, Europe
2002-04-05
13786

Re: Database improvements

rub wrote:

Point 2:
---------
Other thing, in other part of code, I prefer to use $conf['users_table'] than USERS_TABLE.
Simply, in order to make link between $conf['users_table'] and $conf['user_fields'].

But it's just my preference...

I agree with your point 2!

Even if that's my code, I've always find it odd to define USER_TABLE based on a configuration variable that can be overwritten.

In some case, it's dangerous to have USERS_TABLE defined as "forum_users" instead of "piwigo_users". For example, in a migration task: if we want to upgrade the piwigo users table, we must take care of altering $prefixeTable.'users' and not USERS_TABLE.

In my opinion, as suggested by rub, I would find cleaner to have:

* USERS_TABLE == piwigo_users
* $conf['users_table'] == forum_users

To have a more general opinion about the use of constants for database table...

- pros: no need to have a "global $conf" at the beginning of every function performing a database query
- cons: with each new table, we have to declare it in include/constants.php

Propositions:

1) only have DBPREFIX in constants => 'select * from '.DBPREFIX.'images'
2) no constant at all => global $conf; 'select * from '.$conf['dbprefix'].'images'
3) no change with 2.0 implementation => 'select * from '.IMAGES_TABLE

My opinion: proposition 1 is nice at function level and for code maintenance, but DBPREFIX would remain a constant based on a configuration parameter, which is not really how I see a constant. Proposition 2 is how I was coding until Piwigo 1.3 (I remember it was a bit painful to always remember the "global $conf" at the beginning of the function).

Offline

 

#42 2009-11-23 21:59:41

nicolas
Former Piwigo Team
2004-12-30
1232

Re: Database improvements

VDigital wrote:

I'd just read las five post and for me, it's not clear enough.

Rub,
Could you remind me the full old and new syntax?

in include/constants.php you had :

Code:

if (!defined('USERS_TABLE'))
 define('USERS_TABLE', $conf['users_table']);

And you must modify the file include/config_local.inc.php:

Code:

$conf['users_table']  = 'my_users_table';

After my commit ([Subversion] r4280) :
in include/constants.php you had :

Code:

if (!defined('USERS_TABLE'))
 define('USERS_TABLE', $prefixeTable.'users');

The table is defined like the others tables.
If you want to add an external authentification you must add in your include/config_local.inc.php:

Code:

define('USERS_TABLE', 'my_users_table');

rub said that include/config_local.inc.php must only contains $conf but we can add functions, log class (that I do). The idea of that files is to add modifications to piwigo that will not be deleted or override by piwigo update.
I think the gap in syntax is very small.

Offline

 

#43 2009-11-23 22:10:52

nicolas
Former Piwigo Team
2004-12-30
1232

Re: Database improvements

I prefer solution 3 because we are independent of prefix and table name. We could change tables name and prefix without modify code !

But I think your answer plg give no response to the problem. Could we modify/define constants in config_local.inc.php ?



Sorry, I made an edit instead of quote...

Last edited by VDigital (2009-11-23 22:47:10)

Offline

 

#44 2009-11-23 22:42:23

VDigital
Former Piwigo Team
Paris (FR)
2005-05-04
17680

Re: Database improvements

nicolas wrote:

If you want to add an external authentification you must add in your include/config_local.inc.php:

Code:

define('USERS_TABLE', 'my_users_table');

rub said that include/config_local.inc.php must only contains $conf but we can add functions, log class (that I do). The idea of that files is to add modifications to piwigo that will not be deleted or override by piwigo update.
I think the gap in syntax is very small.

Very small for you (and can say me too), but for beginners ?

Piwigo 2.0.x: Ok no problem
Upgrade
Piwigo 2.1.0: Connection failures, could you confirm?

(We are speaking about 20 users maybe more. If the lasted to upgrade do it about 3 years later, are you sure to remind that change in less than 10 minutes).
Why is it complex to keep the old way?


Piwigo.com: Start and run your own photo gallery. Signup and get 30 days to try for free, no commitment.
8-)

Offline

 

#45 2009-11-23 22:42:44

plg
Piwigo Team
Nantes, France, Europe
2002-04-05
13786

Re: Database improvements

nicolas wrote:

I prefer solution 3 because we are independent of prefix and table name. We could change tables name and prefix without modify code !

OK. I'm not very satisfied with the 'select * from '.IMAGES_TABLE syntax but it obviously has some advantages.

nicolas wrote:

But I think your answer plg give no response to the problem.

I wanted your opinion about my alternative propositions on the use of constants before answering :-)

nicolas wrote:

Could we modify/define constants in config_local.inc.php ?

Thank you for reminding the before/after coding syntax.

I agree with rub about having only $conf['key'] in include/config_local.inc.php, no define. I also anticipate about a plugin to read/write config_local.inc.php, so we need to keep the syntax as coherent as possible.

In constants.php I don't really like the "trick" to check if the table has already been defined. I think we should just have:

Code:

define('CATEGORIES_TABLE', $prefixeTable.'categories');
define('COMMENTS_TABLE', $prefixeTable.'comments');
...
define('USERS_TABLE', $conf['users_table']);

This is the situation we had before [Subversion] r2096 + [Subversion] r2104 (see [Bugtracker] ticket 748). I have the feeling that the "if(!defined(USERS_TABLE))" trick was added only for the prepare_conf_upgrade() function, and I think it should have been made in a simpler way.

Offline

 
  •  » Engine
  •  » Database improvements

Board footer

Powered by FluxBB

github twitter newsletter Donate Piwigo.org © 2002-2024 · Contact