Project

General

Profile

Refactoring #2621

Database class API is too complex

Added by Anonymous over 8 years ago. Updated over 8 years ago.

Status:
New
Priority:
High
Assignee:
-
Start date:
24/01/2011
Due date:
% Done:

0%

Estimated time:

Description

The nested tree stuff does not belong to the Database class' natural responsibilities. It ought to be its own class.

Furthermore, the database class has way too many methods to do the same thing.

The "retrieve" group of methods is particularly large: retrieve_objects, retrieve_result, retrieve_object_set, retrieve_record_set, retrieve_max_sort_value, retrieve_next_sort_value, retrieve_record, retrieve_row, retrieve_object, retrieve_distinct (and count_objects, count_result_set, count_distinct could also be considered part of the retrieval group depending on how you look at it)

The API itself is also rather unclear; what's supposed to be for internal use and what isn't? There are almost no private or protected methods.


Files

mdb2.patch (6.67 KB) mdb2.patch Goulwen Reboux, 03/02/2011 11:30

Associated revisions

Revision 431fe03a (diff)
Added by Alex Aragon 9 months ago

add remove xss - ref #2621

History

#1

Updated by Anonymous over 8 years ago

To refactor the API itself I think it's important that no MDB2 objects leak out so as to make the abstraction complete. That means anything returning MDB2 objects should either be marked protected/private (since those can only be helper methods) or wrapped in Chamilo-specific classes.

This would also make it easier to move to PDO or some other abstraction layer when MDB2 finally dies.

#2

Updated by Hans De Bisschop over 8 years ago

Please keep in mind that PDO in and of itself is not an abstraction layer in the same way MDB2 is.

#3

Updated by Anonymous over 8 years ago

delete() and delete_objects() are identical. One should be removed, or they should be differentiated somehow.

#4

Updated by Goulwen Reboux over 8 years ago

When starting digging in the code, I got lots of warnings from MDB2 about Strict Standards when calling non static methods statically. I've done a patch, but I'm not sure it's OK to change this in Chamilo, as it's more the responsability of MDB2, but in the other hand, MDB2 activity is pretty low now. At least it allows to develop with default error reporting without having lots of message in the top of the page.

#5

Updated by Anonymous over 8 years ago

Goulwen Reboux wrote:

When starting digging in the code, I got lots of warnings from MDB2 about Strict Standards when calling non static methods statically. I've done a patch, but I'm not sure it's OK to change this in Chamilo, as it's more the responsability of MDB2, but in the other hand, MDB2 activity is pretty low now. At least it allows to develop with default error reporting without having lots of message in the top of the page.

First off, this is unrelated to the issue at hand; this should probably be a separate issue.

But more to the point, I ran into the same issue while writing tests for the Database class, and actually had to hack a workaround into the tests to temporarily disable the strict checking so the tests didn't fail prematurely on those issues, so I think fixing this is rather important. Your patch looks good to me and I think you should commit this, after filing a bugreport with this patch on the MDB2 issue tracker. Then you can refer to your MDB2 ticket number (and/or bugtracker URL) in the commit message.

My tests should help you to find these problems more easily, but unfortunately I still haven't been able to commit that because it's part of my Postgres support changes. I can't commit those until the new stable/dev different repos are set up, which should be by the end of this week I hear.

#6

Updated by Goulwen Reboux over 8 years ago

Yep sorry, I've started write a new issue, but before post it, I made a new research for "MDB2" and found the ticket. I assume wrongly that it's related.

I create a new ticket on MDB2 issue tracker and then will add a new ticket here with the patch (I don't have the commit access currently)

#7

Updated by Stefaan Vanbillemont over 8 years ago

  • Target version set to 7
#8

Updated by Stefaan Vanbillemont over 8 years ago

  • Project changed from Chamilo LCMS Connect to Repository
#9

Updated by Anonymous over 8 years ago

  • Project changed from Repository to Common
#10

Updated by Stefaan Vanbillemont over 8 years ago

  • Target version changed from 7 to 2.1.0
#11

Updated by Stefaan Vanbillemont over 8 years ago

  • Target version changed from 2.1.0 to 31
#12

Updated by Stefaan Vanbillemont over 8 years ago

  • Target version changed from 31 to Backlog (default)

Also available in: Atom PDF