Re: [AppDB] big AppDB refactoring

Jonathan Ernst wrote:
> This patch is a big refactoring of the AppDB. Some files have been
> moved and renamed, others were simply deleted and many where fixed and
> improved. No functional differences (apart the fixed global vars).
>
> *The configuration (config.inc.php) has to be updated on the live server*
>
> CHANGELOG:
> - moved functional code out of procedural code
> - moved functional code out of object oriented code
> - moved classes in /include/classes/
> - moved functions libraries in /includes/functions/
> - deleted some no more needed files (content was duplicate or included in other files)
> - created a library for bugzilla related functions
> - moved some code from one library to another library (db related in /include/db.php, etc.)
> - improved the config file (using constants, ability to specify new parameters that would make it very easy to integrate the appdb on another site like ReactOS as they can replace references to winehq and bugs.winehq.org for example)
> - the debug parameter of the config file is now taken into consideration in the code (function isdebugging() has been improved)
> - fixed some obvious global vars bugs when encountered (so that the help system re-works and most broken admin functions are back)
> - replaced apidb occurences with appdb as it's the name of the application
> - replaced $apidb_root occurences with BASE constant because both were used and had the same value (so no more glbal $apidb_root needed)
> - updated TODO and CODING_STANDARD regarding these changes
> - overall improved security (because of global vars and using constants for the config so that it can't be changed using php injection)
>
> Files added:
> - include/classes/application.php
> - include/classes/menu.php
> - include/classes/screenshot.php
> - include/classes/tableve.php
> - include/classes/category.php
> - include/classes/query.php
> - include/classes/session.php
> - include/classes/user.php
> - include/functions/appdb.php
> - include/functions/date.php
> - include/functions/maintainer.php
> - include/functions/rating.php
> - include/functions/util.php
> - include/functions/banner.php
> - include/functions/db.php
> - include/functions/pn_buttons.php
> - include/functions/sidebar_admin.php
> - include/functions/vote.php
> - include/functions/bugzilla.php
> - include/functions/help.php
> - include/functions/sidebar_login.php
> - include/functions/comments.php
> - include/functions/html.php
> - include/functions/query.php
> - include/functions/sidebar.php
>
> Files deleted:
> - include/appbyvendor_inc.php
> - include/appdb.php
> - include/application.php
> - include/appversion_inc.php
> - include/banner.php
> - include/category.php
> - include/comments.php
> - include/db.php
> - include/html.php
> - include/maintainer.php
> - include/menu.php
> - include/parsedate.php
> - include/pn_buttons.php
> - include/qclass.php
> - include/query_inc.php
> - include/rating.php
> - include/session.php
> - include/sidebar.php
> - include/sidebar_admin.php
> - include/sidebar_login.php
> - include/tableve.php
> - include/user.php
> - include/util.php
> - include/vote.php
>
> File changed:
> - CODING_STANDARD
> - TODO
> - account.php
> - addcomment.php
> - appbrowse.php
> - appdbStats.php
> - appsubmit.php
> - appview.php
> - bugs.php
> - commentview.php
> - deletecomment.php
> - edituser.php
> - index.php
> - maintainerdelete.php
> - maintainersubmit.php
> - noteview.php
> - preferences.php
> - screenshots.php
> - search.php
> - stdquery.php
> - support.php
> - updaterating.php
> - updatevote.php
> - vendorview.php
> - votestats.php
> - admin/addAppFamily.php
> - admin/addAppNote.php
> - admin/addAppVersion.php
> - admin/addCategory.php
> - admin/addVendor.php
> - admin/adminAppQueue.php
> - admin/adminCommentView.php
> - admin/adminMaintainerQueue.php
> - admin/adminMaintainers.php
> - admin/deleteAny.php
> - admin/editAppFamily.php
> - admin/editAppNote.php
> - admin/editAppOwners.php
> - admin/editAppVersion.php
> - admin/editBundle.php
> - admin/editCategory.php
> - admin/editVendor.php
> - admin/index.php
> - admin/adminAppDataQueue.php
> - help/index.php
> - include/config.php.sample
> - include/header.php
> - include/incl.php
> - include/query_users.php

<gack> you _can't_ be serious.

- One patch to do one thing. For example each of the following should their own 
patch.

Change apidb_header() and apidb_footer() to appdb_header() and appdb_footer()
   Change apidb_fullurl() to appdb_fullurl
   Move include/tableve.php to include/classes/tableve.php
   Move include/application.php to include/classes/application.php
   ...

I know you probably will end up sending more than a dozen patches that way. 
However, I can't approve of a wholesale change, like this, in one go.

</gack>

The changes you propose are good ones. Just break them down.

--

Tony Lambregts

[AppDB] big AppDB refactoring

[Jonathan Ernst]

Hello,

I certainly agree that I could have separated apidb->appdb changes from
the rest (but replacing it is trivial and is functionnaly equivalent),
but I can't seriously make one patch for each file moved as it'll
require to patch every dependent files for each move.

The files in / import files in /include if I move one file from /include
to /include/classes I have to patch all files.

Then I have to wait until the patch is commited which will take one day
because of timezones.

Then I have to move the second file and re-change every single file that
includes it. and so on at least 25 times (because there are 25 moved
files) without counting the code moving between includes.

I will _try_ to split the patch in three or four smaller patches and I
hope we can find a way to make it in as it would allow for more
interesting (and smaller ;-) ) changes later.

As I'm not an expert, if you have advices on how to split it more
efficiently I'm all open as you know.

See you,

<gack> you _can't_ be serious.
> - One patch to do one thing. For example each of the following should their own
>
> patch.
>
> Change apidb_header() and apidb_footer() to appdb_header() and appdb_footer()
>    Change apidb_fullurl() to appdb_fullurl
>    Move include/tableve.php to include/classes/tableve.php
>    Move include/application.php to include/classes/application.php
>    ...
>
> I know you probably will end up sending more than a dozen patches that way. 
> However, I can't approve of a wholesale change, like this, in one go.
>
> </gack>
>
> The changes you propose are good ones. Just break them down.
>
> --
>
> Tony Lambregts

[AppDB] big AppDB refactoring

[Chris Morgan]

I'd feel bad if this issue hadn't come up a few times in the past.  Sending in 
large patches that do multiple things is silly.  Even worse is that these are 
fundamental changes to where things are located but we haven't even decided 
there is a problem with how things are now or even whether things are better 
after these changes.

I looked the patch over, you move include/user.php to include/classes/user.php 
but you didn't just move the class, you moved the class AND functions that 
call in to the user class.  Why bother separating things into functions and 
classes if this isn't even the case in the patch?

I wish we could have at least talked about this patch before you went through 
the effort to make these changes.  I can't see any reason why we should 
separate functions and classes.  Right now the name of the php file indicates 
what the contents of the file are.  Lets talk about where we are going with 
this and look at some examples of how these changes would improve things 
before we decide to make them.

Chris

On Saturday 18 December 2004 3:16 pm, Jonathan Ernst wrote:
> Hello,
>
> I certainly agree that I could have separated apidb->appdb changes from
> the rest (but replacing it is trivial and is functionnaly equivalent),
> but I can't seriously make one patch for each file moved as it'll
> require to patch every dependent files for each move.
>
> The files in / import files in /include if I move one file from /include
> to /include/classes I have to patch all files.
>
> Then I have to wait until the patch is commited which will take one day
> because of timezones.
>
> Then I have to move the second file and re-change every single file that
> includes it. and so on at least 25 times (because there are 25 moved
> files) without counting the code moving between includes.
>
> I will _try_ to split the patch in three or four smaller patches and I
> hope we can find a way to make it in as it would allow for more
> interesting (and smaller ;-) ) changes later.
>
> As I'm not an expert, if you have advices on how to split it more
> efficiently I'm all open as you know.
>
> See you,
>
> <gack> you _can't_ be serious.
> > - One patch to do one thing. For example each of the following should
> >
> > their own patch.
> >
> > Change apidb_header() and apidb_footer() to appdb_header() and
> > appdb_footer() Change apidb_fullurl() to appdb_fullurl
> >    Move include/tableve.php to include/classes/tableve.php
> >    Move include/application.php to include/classes/application.php
> >    ...
> >
> > I know you probably will end up sending more than a dozen patches that
> > way. However, I can't approve of a wholesale change, like this, in one
> > go.
> >
> > </gack>
> >
> > The changes you propose are good ones. Just break them down.
> >
> > --
> >
> > Tony Lambregts

[AppDB] big AppDB refactoring

[Jonathan Ernst]

Le samedi 18 décembre 2004 à 19:31 -0500, Chris Morgan a écrit :
> I looked the patch over, you move include/user.php to include/classes/user.php 
> but you didn't just move the class, you moved the class AND functions that 
> call in to the user class.  Why bother separating things into functions and 
> classes if this isn't even the case in the patch?

I moved user into classes because it is a class and removed functions
that are not method of this class into another library. A class is a
class and shouldn't contain other code I think.

See:
http://www.phpfreaks.com/tutorials/35/1.php
for example

Don't you ever tried to do $current->loggedin() in the past before to
see that loggedin() was not a method ?
> I wish we could have at least talked about this patch before you went through 
> the effort to make these changes.  I can't see any reason why we should 
> separate functions and classes.  Right now the name of the php file indicates 
> what the contents of the file are.  Lets talk about where we are going with

The names don't tell you if it's a class or not and are not clear enough
in my opinion. It's no wonder why many things in the code are made "by
hand" while functions already exist in the include directory (sql
queries, things in includes/html are barely used, etc). Some files have
an _inc suffix which is redundant as they are in a directory named
include... There are also too many files and maybe we could improve this
situation (people not using or finding usefull functions) by
reorganising it (that was the purpose of my patch and it's huge because
deleting files and adding new files make diffs huge).
> this and look at some examples of how these changes would improve things 
> before we decide to make them.

I thought that the include directory was not exactly well organised; you
have files in it that have different purposes. Some are just plain
includes (headers, footers), others are classes and others provide
functions. Last but not least some mix classes and functions and you
even have some files in / that provide usefull (sometimes duplicated
functions).

As to discuss it you know that it's hard for me to be woken up at nights
to discuss every thing so I thought I'll simply do the changes, test it
the whole day (and be confident that it makes much more good than bad
and that everything works just the same or better)

I think that we could decide alltogether if you think my arguments for
reorganising the includes/functions is valid or not and if it is we
could start moving it.

Finally I saw you committed my config changes and I'm glad you saw that
it might be an improvement over the current situation. I'll send a new
patch that _only_ takes advantage of the new config file.

Thanks for your time and efforts,
Jonathan