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

No usage of AbstractCommand::requireTransaction() #782

Open
Tigrov opened this issue Nov 22, 2023 · 3 comments
Open

No usage of AbstractCommand::requireTransaction() #782

Tigrov opened this issue Nov 22, 2023 · 3 comments

Comments

@Tigrov
Copy link
Member

Tigrov commented Nov 22, 2023

Protected AbstractCommand::requireTransaction() method should be public or removed.

If it will be public possible usage

$command->requireTransaction()->execute();

Or if not use requireTransaction()

$db->transaction(fn () => $command->execute());
@vjik
Copy link
Member

vjik commented Nov 22, 2023

Actually, this method don't allow mark command for run in transaction with default isolation level. ->requireTransaction() don't run command in transaction.

CommandInterface do not contain requireTransaction method. Suggest remove requireTransaction and isolationLevel from implementations. For run in transcation can be used $db->transcation(). If there is a request for requireTransaction() then we can add it in next major version to CommandInterface.

@Tigrov
Copy link
Member Author

Tigrov commented Nov 23, 2023

This method has one advantage. If the logger or profiler is configured on the same db connection, the logger and profiler entries will be rolled back along with the failed query if use $db->transaction(fn () => $command->execute());

This method allows you to complete a transaction for a command only.

But I would not use a logger and profiler in production or need to configure them on different storage / db connection. I'm more inclined that this method is redundant, at least until today it was not used and there were no problems (in yii2 the same).

@vjik
Copy link
Member

vjik commented Nov 23, 2023

@darkdef @terabytesoftw What do you think? Can we remove requireTransaction() method?

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

No branches or pull requests

2 participants