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

Discussion: Design of database functions #862

Open
M4LuZ opened this issue Jan 10, 2024 · 3 comments
Open

Discussion: Design of database functions #862

M4LuZ opened this issue Jan 10, 2024 · 3 comments
Labels

Comments

@M4LuZ
Copy link
Collaborator

M4LuZ commented Jan 10, 2024

Expected Behavior

While refactoring a file for the new DB class I noticed some points that I wanted to put up for discussion.
As adoption is currently in an early stage, this is the right point to see if some points should be changed

Current Behavior

Usefulness of functions that just return a field from object passed as parameter

The old db class has functions to return data and properties from the last query result.
This was stored internally in the class.
E.g. to fetch a result array from a query done:

$db->qry('statement'); //runs the query and stores the result internally 
$data = $db->fetch_array(); //gets next data row as array

I agree that the result(data) should not be stored in the database object buuut:
Now we still expose functions that provide the same functionality, although the functionality is not related to the database class / object at all.
e.g.

$mysqliStatement = $database->query('statement') //returns a mysqliStatement object as result
$mysqliResult = $database->getStatementResult($mysqliStatement); //this just calls $mysqliStatement->get_result() for us
$resultData = $mysqliResult->fetch_array() // now we are on our own again as there is no matching function in $database

I personally see the following issues with that:

  • Inconsistency(1): On the one hand functions are exposed that can be utilized to execute operations on native mysqli-classes. But by far not all of the ones needed are mapped.
    This provides a half solution and should either do none or all of the functionality previously provided for result processing
  • Inconsistency(2): I would also see an argument to place this functionality in the database class, if any additional work or transformation would happen that cannot be done within the object scope itself.
    But these are operations provided by and run completely within the scope of the mysqliStatement and mysqliResult classes respectively.
  • Inconsistency(3): The control flow changes within the standard operation. First we call a function from database to run an action with the result object, then we act on the object itself in the same regard.
  • No added value. E.g. function body of getStatementResult consists of return $statement->get_result();. This provides no benefit.

Parameter-array for query-functions

This is a subtle one that makes a fair amount of difference in effort for refactoring:
While query functions in the the old db class use a variable list of arguments (uses func_get_args ) the new functions expect all values as array. Means as part of the refactoring [] has to be added to every single query string instead of "just" replacing class & function name and occurrences of %int%, %string%
I do see that this may have advantages (such as having no mapping internally for statement execution), but want to put it up to discussion anyways due to the disadvantage being noticed.

Possible Solution

Object functions

All or nothing. I would personally remove them and document how to use the native mysqli functions
Or: Implement prior functions also to provide same feature set

@M4LuZ M4LuZ added the question label Jan 10, 2024
@andygrunwald
Copy link
Collaborator

Usefulness of functions that just return a field from object passed as parameter

Thats a fair concern.
Next to the "raw" methods that return internals of the MySQL functions, we also offer

public function queryWithFullResult(string $query, array $parameterValues = []): array
public function queryWithOnlyFirstRow(string $query, array $parameterValues = []): array|null

I assume those will be used more often than the raw functions.

The challenge right now:
Insert statements are also done by public function query(string $query, array $parameterValues = []): \mysqli_stmt.

To get the inserted ID, we use public function getStatementInsertID(\mysqli_stmt $statement): int|string.
Similar for UPDATE statements with public function getStatementAffectedRows(\mysqli_stmt $statement): int|string.

And how do we return an error? And how do we know what went wrong?
We can run them through an \Exception. Then, every DB needs a try{} catch().

Any thoughts?

Parameter-array for query-functions

The use of func_get_args has a tremendous disadvantage. It is dynamic programming and in most cases, we don't need dynamic programming. dynamic programming is executed via runtime and static analysis is mostly useless.

I personally see getting rid of func_get_args + friends as a big benefit, because it makes our source code more explicit. Yes, it might be a bit more work.

Possible Solution

I am not sure if I understand your suggestion here.

@M4LuZ
Copy link
Collaborator Author

M4LuZ commented Jan 13, 2024

Hmm, I noticed a few...misunderstandings in how the class is supposed to be used.
I'll continue to rewrite code and either come back with further comments or accept the current state as is ;)

@andygrunwald
Copy link
Collaborator

An alternative approach would be to offer methods for the particular INSERT and/or UPDATE usecase. Then we would not need to expose the "internals".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants