Skip to content

docs: update DB transactions #6886

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

Merged
merged 13 commits into from
Dec 11, 2022
Merged

docs: update DB transactions #6886

merged 13 commits into from
Dec 11, 2022

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Nov 20, 2022

Needs #6919

Description

Checklist:

  • Securely signed commits
  • [] Component(s) with PHPDoc blocks, only if necessary or adds value
  • Unit testing, with >80% coverage
  • User guide updated
  • Conforms to style guide

@kenjis kenjis added testing Pull requests that changes tests only documentation Pull requests for documentation only 4.3 database Issues or pull requests that affect the database layer labels Nov 20, 2022
@sclubricants
Copy link
Member

How about a test with $this->db->transStrict(false); This would change these test outcomes right?

@kenjis
Copy link
Member Author

kenjis commented Nov 22, 2022

I don't know why Strict Mode is needed.

@kenjis
Copy link
Member Author

kenjis commented Nov 22, 2022

How about a test with $this->db->transStrict(false); This would change these test outcomes right?

Added. It works as expected.

But it seems the strict mode makes sense only when DBDebug is false.
Since v4.3.0, DBDebug is true by default, so if one query fails, an exception will be thrown and that's all.

Is the strict mode a safeguard that prevents all subsequent transactions from being executed if one transaction fails when there were no exceptions in PHP?

@sclubricants
Copy link
Member

Adding the transaction status helps to understand the tests better.

But it seems the strict mode makes sense only when DBDebug is false.

yes, then you can use $this->db->transStatus() to determine what to do.

I wonder if calling $this->db->transCommit(); or $this->db->transRollback() will then allow you to start new transactions again or if they will keep failing.

@sclubricants
Copy link
Member

sclubricants commented Nov 22, 2022

There is no resetTransStatus() method. In case you wanted to try a series of transactions again while in strict mode..

It can be accomplished like this however:

    public function resetTransAfterFailure()
    {
        $this->disableDBDebug();

        $builder = $this->db->table('job');

        $this->db->transStrict(true);

        // The first transaction group
        $this->db->transStart();

        $jobData = [
            'name'        => 'Grocery Sales',
            'description' => 'Discount!',
        ];
        $builder->insert($jobData);

        $this->assertTrue($this->db->transStatus());

        // Duplicate entry '1' for key 'PRIMARY'
        $jobData = [
            'id'          => 1,
            'name'        => 'Comedian',
            'description' => 'Theres something in your teeth',
        ];
        $builder->insert($jobData);

        $this->assertFalse($this->db->transStatus());

        $this->db->transComplete();

        $this->dontSeeInDatabase('job', ['name' => 'Grocery Sales']);


        $this->db->transStrict(false); // resets transaction status
        $this->db->transComplete(); // resets transaction status
        $this->db->transStrict(true); // return back to strick mode


        // The second transaction group
        $this->db->transStart();

        $jobData = [
            'name'        => 'Comedian',
            'description' => 'Theres something in your teeth',
        ];
        $builder->insert($jobData);

        $this->assertTrue($this->db->transStatus());

        $this->db->transComplete();

        $this->seeInDatabase('job', ['name' => 'Comedian']);

        $this->enableDBDebug();
    }

It might be helpful if you wanted to call a series a transactions another time.

$this->aMethodThatCallsASeriesOfTransactionsInStrictMode();

if ($this->db->transStatus() === false) {
    $this->db->resetTransStatus();
    $this->aMethodThatCallsASeriesOfTransactionsInStrictMode(); // try it again
}

OR

$this->aMethodThatCallsASeriesOfTransactionsInStrictMode();
$this->db->resetTransStatus();
$this->anotherMethodThatCallsASeriesOfTransactionsInStrictMode(); // try it again

The way it is when running in strict mode (default) once a failure occurs you can never run anymore.

@sclubricants
Copy link
Member

But it seems the strict mode makes sense only when DBDebug is false.

We should show how to turn debug off in the managing errors section.
https://codeigniter4.github.io/CodeIgniter4/database/transactions.html#managing-errors

@sclubricants
Copy link
Member

Can you add this method and documentation?

    /**
     * Reset transaction status - to restart transactions after strict mode failure
     */
    public function resetTransStatus(): BaseConnection
    {
        $this->transStatus = true;

        return $this;
    }

@sclubricants
Copy link
Member

We probably need a public method on BaseConnection to set debug mode.

@kenjis
Copy link
Member Author

kenjis commented Nov 23, 2022

We probably need a public method on BaseConnection to set debug mode.

Do you need DBDebug = false?
Since v4.3.0, DBDebug is true by default in all environments,
and I would like to deprecate DBDebug in the future.

Since PHP 8.0, PDO throws exceptions by default:
https://www.php.net/manual/en/pdo.error-handling.php

@kenjis
Copy link
Member Author

kenjis commented Nov 23, 2022

We should show how to turn debug off in the managing errors section.
https://codeigniter4.github.io/CodeIgniter4/database/transactions.html#managing-errors

I added a comment in the sample code.

Copy link
Member

@MGatner MGatner left a comment

Choose a reason for hiding this comment

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

A few typos, otherwise looks good!

@sclubricants
Copy link
Member

I added a comment in the sample code.

I really wish we had a method. $this->db->setDBDebug(false); You can't depend on the database config for handling these. Maybe you have it turned off in one environments and on in another. It kind of defeats the purpose of having transStatus() if it always throws an error.

Perhaps a better way to handle this would be to have a parameter on transStart() or a separate method transError(true/false).

I looked back in my own code where I used transactions and found that I had implemented a workflow using transStatus() but I had a comment by $this->db->transComplete(); // throws error?.

We need an easy way to set the failure behavior other than the database config file.

@kenjis
Copy link
Member Author

kenjis commented Nov 23, 2022

@codeigniter4/database-team

Before v4.3.0, DBDebug was false in production environment, but true in other environments.
It is very bad practice, because many devs don't know the behavior will change only in production environment.
In develop environment, CI throws an exception but not in production environment.

Since v4.3.0, DBDebug is always true by default, and it has become nothing to do with debugging (showing errors).
It is just a flag to throw an exception or not. It is error mode.

It kind of defeats the purpose of having transStatus() if it always throws an error.

Yes, now in 4.3, transStatus() does not work at all unless you set DBDebug = false.
By default if a query error occurs, it will be automatically rolled back and an exception will be thrown.

DBDebug is error mode in 4.3, and there seems to be no need to change it at runtime.
I would like to remove the setting if possible. So I don't want to add a new method to change it.

Perhaps a better way to handle this would be to have a parameter on transStart() or a separate method transError(true/false).

I feel that such a way is necessary unless we remove manual transactions functionality.
It seems removing manual transactions is difficult.

@kenjis kenjis requested a review from MGatner November 23, 2022 23:41
@MGatner
Copy link
Member

MGatner commented Nov 24, 2022

I was strongly in favor of the DBDebug changes and @kenjis' explanation seems like the next logical steps. I've never actually used our trans____() methods so don't have a suggestion on how what the best implementation might be there. But generally it sounds like we need a separate flag for transaction error handling that isn't tied to our regular error handling.

And yes: let's plan to do away with DBDebug altogether. Firing a DatabaseErrorEvent or something should be plenty for developers, along with the new exception handling feature.

@kenjis kenjis mentioned this pull request Nov 25, 2022
3 tasks
@kenjis
Copy link
Member Author

kenjis commented Nov 25, 2022

I created an issue #6909

This PR is only for documentation update and adding test code.
Can I merge this? @codeigniter4/database-team

Copy link
Member

@michalsn michalsn left a comment

Choose a reason for hiding this comment

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

Shouldn't we also update the user guide if the general behavior in 4.3 has changed? If we're about to throw an exception every time, the examples in the user guide seem useless now.

I would like to see Transactions to be mentioned in the changelog. This is a critical change for someone who uses them.

@kenjis
Copy link
Member Author

kenjis commented Nov 25, 2022

Rebased.

@kenjis
Copy link
Member Author

kenjis commented Nov 25, 2022

Added about transactions in the changelog.

@kenjis kenjis requested a review from michalsn November 25, 2022 21:27
@kenjis
Copy link
Member Author

kenjis commented Nov 27, 2022

I added a sample code and updated some descriptions.

I also found that SQLite3, Postgres and OCI8 drivers throw ErrorException when a query error occurs
because of Error Handler. See #6912

if (error_reporting() & $severity) {
throw new ErrorException($message, 0, $severity, $file, $line);
}

So depending on the value of the error reporting setting, the ErrorException may not be thrown.

$this->db->transComplete();
} catch (DatabaseException $e) {
// Automatically rolled back already.
}
Copy link
Member Author

Choose a reason for hiding this comment

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

I still feel uncomfortable with this code.

@kenjis
Copy link
Member Author

kenjis commented Nov 27, 2022

Perhaps a better way to handle this would be to have a parameter on transStart() or a separate method transError(true/false).

See #6917

@michalsn
Copy link
Member

Well, I wasn't saying anything since everyone seemed to be on board regarding throwing exceptions, but... personally, I don't think that throwing exceptions for anything related to transactions has sense. This is why we have transStatus() - to handle errors.

So, for me #6917 is definitely a good way to solve this.

@kenjis kenjis mentioned this pull request Nov 27, 2022
3 tasks
@kenjis kenjis marked this pull request as draft November 28, 2022 00:36
@kenjis
Copy link
Member Author

kenjis commented Nov 28, 2022

So, for me #6917 is definitely a good way to solve this.

Yes, I also think it is necessary missing feature for v4.3.

kenjis and others added 13 commits November 30, 2022 09:03
Co-authored-by: sclubricants <vaughnb@scoil.com>
Co-authored-by: MGatner <mgatner@icloud.com>
All the current DB drivers throw an exception when a query error occurs.
See codeigniter4#6912

MySQLi driver sets `mysqli_report(MYSQLI_REPORT_ALL & ~MYSQLI_REPORT_INDEX)`
that throws mysqli_sql_exception.

SQLite3, Postgres and OCI8 drivers throw ErrorException because of Error Handler.

SQLSRV driver has different implementaion. It throws DatabaseException when the
query return value is false.
@kenjis kenjis merged commit b2e90d7 into codeigniter4:4.3 Dec 11, 2022
@kenjis kenjis deleted the db-transactions branch December 11, 2022 11:25
@kenjis
Copy link
Member Author

kenjis commented Apr 12, 2024

@sclubricants I sent #8767

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
database Issues or pull requests that affect the database layer documentation Pull requests for documentation only testing Pull requests that changes tests only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants