updateObject method in DatabaseDriver.php always returns true

[renekreijveld]

Steps to reproduce the issue

Can only be reproduced if a database update fails.

Expected result

The updateObject method runs the query and should return the result of the execute() statement.

Actual result

The updateObject method runs the query and regardless of the result of the execute statement, it always returns true.

System information (as much as possible)

See the implemention of the updateObject method in PgsqlDriver.php.

Additional comments

Although the execute statement might trigger an exception, it would be better if the updateObject method itself returns a boolean value that represents the result of the update.
It might also be good to look at the insertObject method in DatabaseDriver.php. That too always returns true, whether or not the execute statement failed or not.

[richard67]

The code change which caused this issue was once made with PR #33 by @wilsonge .

However, the PgsqlDriver still has similar code as the (abstract) base driver had before that change: https://github.com/joomla-framework/database/blob/3.x-dev/src/Pgsql/PgsqlDriver.php#L1041-L1044

And the doc blocks of the "execute" method in both the (abstract) base driver and the PDO driver say that the "execute" method returns boolean.

That seems inconsistent to me.

@wilsonge Any idea what's the right way?

[pjdevries]

In my opinion a choice has to be made between:

• either the return of a boolean value that can be tested for the success of a method invocation,
• or no return value at all and throw an exception if anything goes wrong.

[laoneo]

I would always go for exceptions.

[pjdevries]

There are certain ground rules for the choice between the use of exceptions or the use of return values to determine the outcome of programming logic. I have to admit that I didn't know them by heart, so I asked ChatGPT. Its answer confirms what I thought I remembered:

1. Use Return Values for Expected, Controllable Outcomes

• Validation and Regular Control Flow: If a function is checking for an expected condition or validation (e.g., checking if a user input is valid, or if a file exists), use return values to indicate the outcome. For example, returning false or null when a file isn’t found is generally preferable over throwing an exception.
• Non-Critical Failures: When a function can fail in a controlled way that the calling code can handle easily (like returning false if a user ID isn’t found), a return value is usually more appropriate.

2. Use Exceptions for Unrecoverable, Unexpected Errors

• Unexpected Scenarios: Use exceptions when something goes wrong that the code wasn’t expected to handle in regular operation (e.g., database connection issues, invalid configurations, unexpected data types). In these cases, an exception signals a serious problem that you may not want to handle immediately.
  Critical Errors and Abort Logic: If you want to stop the code execution immediately due to a failure in a critical operation (like failing to allocate resources), use an exception.

3. Avoid Using Exceptions for Control Flow

• Efficiency: Exceptions are more computationally expensive than return values. Avoid using them to manage regular logic flows, like loop breaking or if-else conditions, as this can lead to performance bottlenecks and confusing code.
• Readability: Code that uses exceptions sparingly is generally more readable. Reserve exceptions for actual errors or exceptional states, and use return values for regular flow control.

4. Document and Consistently Implement

• Clarity for Other Developers: Document whether a function returns a special value (e.g., null or false) or throws an exception when an error occurs. Consistent practices in a codebase help other developers understand and anticipate behavior without surprises.
• Standardize: Create a convention across your codebase to distinguish between return values and exceptions. For example, if a function is expected to throw an exception on error, it should always do so rather than sometimes returning an error code or value.

5. Leverage PHP's Built-in Exception Types

• Use Specific Exceptions: PHP has built-in exceptions (e.g., InvalidArgumentException, RuntimeException) that help signal specific issues. Using these makes your error handling more specific and informative.
• Create Custom Exceptions for Clarity: When dealing with custom logic, create custom exception classes (e.g., DatabaseConnectionException) to clearly communicate the context of an error.

The thing I struggle with most myself, is 3. But I think consistency, as mentioned in 4, is an important one iin the context of this issue.

[HLeithner]

The decision about exception has already been made a long time ago.

In this case, I would expect, the return true is only for b/c reasons.
if we change the return signature to void would means that all code would fail which is does an if($db->updateObject()), also all inherited driver classes would fail.

Only way to solve this would be a new method, not sure if it's worth to do this.

[renekreijveld]

Why not do it the same as in PgsqlDriver.php?

// Set the query and execute the update.
$this->setQuery(sprintf($statement, implode(',', $fields), implode(' AND ', $where)));

return $this->execute();

If execute() fails you still have the exception thrown but also a return false.

[pjdevries]

In my opinion it still is a bad idea to use different ways of trying to achieve the same thing. It is confusing. b/c is one of the reasons why these confusing constructs remain.

[HLeithner]

because you will never came to the return because the exception changes the execution path, which lead to a wrong developer expectation.

You as developer would expect that in case of an error you get false, which is actually not true. You would need to check the docs to see that you get an exception instead of an false.

[richard67]

I've just checked the "execute" method of the DatabaseDriver, which is inherited e.g. my the MySQLi driver, and the "execute" method of the PdoDriver, which is inherited by the PgsqlDriver. Both of them return true in case of success, but in case of error they throw exceptions and they never return false, so the "return true" in these methods seems to be just there for b/c.

So it seems the change from PR #33 for the MySQLi driver was right, and it should be done in the same way in the PgsqlDriver.