Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Consistent DB enum function #5952

Conversation

homersimpsons
Copy link
Contributor

Fixes #1813

Description of the Change
Make all DB enum functions consistent

About the following requirement from the task:

In addition a third argument for limit clauses should be added to the basic functions and the class implementations

Should I just update the DbConn::enum and DbConn::enum_fields?

Release Notes
N/A

Comment on lines +207 to +211
/**
* @param string|null $where_clause
* @param string|null $order_clause
* @return list<BoincUser>
*/
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the typing. If we target recent php versions I can just add it directly in the function signature (except for the generic return).

@@ -29,7 +29,7 @@

function create_category($orderID, $name, $is_helpdesk) {
$q = "(orderID, lang, name, is_helpdesk) values ($orderID, 1, '$name', $is_helpdesk)";
$db = BoincDB::get();
$db = BoincDb::get();
Copy link
Contributor Author

@homersimpsons homersimpsons Dec 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The case was incorrect. PHP is case-insensitive but it is better to use the correct casing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to split this file to have 1 file per class for OOP. I'm not sure that this is possible with the current setup though.

* @param string|null $where_clause
* @param string|null $order_clause
* @return list<BoincProfile>
*/
static function enum($where_clause=null, $order_clause=null) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only occurence of $where_clause=null, should I keep it? Or should I check for usages?

Comment on lines +315 to 318
static function enum($where_clause, $order_clause=null) {
$db = BoincDb::get();
return $db->enum('team_delta', 'BoincTeamDelta', $where_clause);
return $db->enum('team_delta', 'BoincTeamDelta', $where_clause, $order_clause);
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to have this function in a trait?

@AenBleidd AenBleidd changed the title ♻️ Consistent DB enum function Consistent DB enum function Dec 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot wasn't able to review any files in this pull request.

Files not reviewed (5)
  • html/inc/boinc_db.inc: Language not supported
  • html/inc/db_conn.inc: Language not supported
  • html/ops/create_forums.php: Language not supported
  • html/user/get_project_config.php: Language not supported
  • html/user/server_status.php: Language not supported
@AenBleidd
Copy link
Member

@davidpanderson, please review

Copy link

codecov bot commented Dec 14, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 10.73%. Comparing base (c67393e) to head (6c38fbe).
Report is 10 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff            @@
##             master    #5952   +/-   ##
=========================================
  Coverage     10.73%   10.73%           
  Complexity     1068     1068           
=========================================
  Files           280      280           
  Lines         36619    36619           
  Branches       8489     8489           
=========================================
  Hits           3930     3930           
  Misses        32300    32300           
  Partials        389      389           

@homersimpsons homersimpsons force-pushed the refactor/consistend-db-enum-functions branch from 207e56c to 6c38fbe Compare December 14, 2024 21:37
@davidpanderson
Copy link
Contributor

There's a better way to do the DB API, but a this point there's no
reason to change what we have.
Let's not change the coding style.

@homersimpsons
Copy link
Contributor Author

There's a better way to do the DB API, but a this point there's no reason to change what we have. Let's not change the coding style.

Can you update your vision on #1813? I may be able to tackle this.

@davidpanderson
Copy link
Contributor

It's possible to use inheritance so that the table-specific classes (BoincUser etc.)
share the same enum(), lookup() etc. functions.
This would greatly shrink the code.
However, our current policy with the PHP code is: if it ain't broken don't fix it.

Something that would be worth doing: make sure that SQL injection attacks are impossible.
The DB APIs are often called with arguments that are user-supplied: GET/POST args and cookies.
String args are generally 'sanitized' with escape_string(), but:

  • numeric args should be sanitized too, e.g. with intval()
  • right now it's vague whether sanitization is the job of the caller or the API;
    this should be nailed down.

I can create an issue if you'd like.

@homersimpsons
Copy link
Contributor Author

right now it's vague whether sanitization is the job of the caller or the API;

If we take example from PDO, DBAL and Eloquent they all favor parameterized queries. So it is the responsibility of the caller to use parameters and the API guarantee that those cannot be used as SQL injection.
If I recall correctly this also has the advantages that some SQL engine will be able to parse / analyse the SQL query only once instead of doing it every time there is a parameter that changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

DB layer enum functions are inconsistent
3 participants