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

[Docs] Removing paragraph about PDO similarity #4947

Merged
merged 1 commit into from
Nov 7, 2021
Merged

[Docs] Removing paragraph about PDO similarity #4947

merged 1 commit into from
Nov 7, 2021

Conversation

ThomasLandauer
Copy link
Contributor

According to https://www.doctrine-project.org/2020/11/17/dbal-3.0.0.html, this is not true anymore.

The major theme of DBAL 3.0 is the decoupling from PDO.

@greg0ire greg0ire requested a review from morozov November 5, 2021 17:47
@morozov
Copy link
Member

morozov commented Nov 5, 2021

I believe it's not the only obsolete PDO-related statement. Here's another one:

dbal/README.md

Line 10 in 44fb020

Powerful database abstraction layer with many features for database schema introspection, schema management and PDO abstraction.

@ThomasLandauer could you look through the rest of the documentation and see if there are more?

@ThomasLandauer
Copy link
Contributor Author

OK, I guess I caught most of them now ;-)
This caution box at https://www.doctrine-project.org/projects/doctrine-dbal/en/latest/reference/transactions.html#transaction-nesting I don't understand, so I left it:

Directly invoking PDO#beginTransaction(), PDO#commit() or PDO#rollBack() or the corresponding methods on the particular Doctrine\DBAL\Driver\Connection instance in use bypasses the transparent transaction nesting that is provided by Doctrine\DBAL\Connection and can therefore corrupt the nesting level, causing errors with broken transaction boundaries that may be hard to debug.

@greg0ire
Copy link
Member

greg0ire commented Nov 5, 2021

I think that box only made sense on 2.x, where methods from PDO where callable through a Driver\Connection instance:

class PDOConnection extends PDO implements ConnectionInterface, ServerInfoAwareConnection

On 3.x, inheritance has been removed and composition is used instead:

final class Connection implements ServerInfoAwareConnection
{
/** @var PDO */
private $connection;

EDIT: my bad, it still makes sense:

Directly invoking PDO#beginTransaction(), PDO#commit() or PDO#rollBack()

You can use getWrappedConnection():

public function getWrappedConnection(): PDO
{
return $this->connection;
}

or the corresponding methods

Or you can call those methods , they are proxied:

public function beginTransaction()
{
return $this->connection->beginTransaction();
}
/**
* {@inheritDoc}
*/
public function commit()
{
return $this->connection->commit();
}
/**
* {@inheritDoc}
*/
public function rollBack()
{
return $this->connection->rollBack();
}

I think the warning can be left as is.

@ThomasLandauer
Copy link
Contributor Author

OK, thanks!

@derrabus derrabus added this to the 3.1.4 milestone Nov 5, 2021
docs/en/reference/introduction.rst Outdated Show resolved Hide resolved
docs/en/reference/security.rst Outdated Show resolved Hide resolved
docs/en/reference/transactions.rst Outdated Show resolved Hide resolved
@morozov
Copy link
Member

morozov commented Nov 5, 2021

@ThomasLandauer once the work is complete, please squash all changes in one commit.

@ThomasLandauer
Copy link
Contributor Author

Sorry, I don't know how to do that :-( Contrary to what @greg0ire recommended at doctrine/orm#9160 (comment) I did it on GitHub's website again ;-)

@greg0ire
Copy link
Member

greg0ire commented Nov 6, 2021

Here is some help once you do the dance described in doctrine/orm#9160 (comment):

How to do that?

  1. git rebase -i origin/3.1.x, assuming origin is a git remote that points to this repository, and not your fork. If you're not sure what your remotes are, run git remote -vvv, there should be your fork and the holy/reference/base/origin/whatever-you-call-it repository.
  2. A window will show up with many lines, replace pick with fixup on every line but the first one
  3. Close your editor, git should do its magic, and you should end up with one commit
  4. Use git push --force to overwrite what you already push. Don't forget the --force option otherwise git will try to merge both things together.

@ThomasLandauer
Copy link
Contributor Author

Here's what I'm getting after exiting nano :-(

CONFLICT (modify/delete): lib/Doctrine/ORM/Persisters/OneToManyPersister.php deleted in HEAD and modified in e50f77a78... Corrected method names; the interface already used SQL, the files still used Sql in method names. Version e50f77a78... Corrected method names; the interface already used SQL, the files still used Sql in method names of lib/Doctrine/ORM/Persisters/OneToManyPersister.php left in tree.
CONFLICT (modify/delete): lib/Doctrine/ORM/Persisters/ManyToManyPersister.php deleted in HEAD and modified in e50f77a78... Corrected method names; the interface already used SQL, the files still used Sql in method names. Version e50f77a78... Corrected method names; the interface already used SQL, the files still used Sql in method names of lib/Doctrine/ORM/Persisters/ManyToManyPersister.php left in tree.
error: could not apply e50f77a78... Corrected method names; the interface already used SQL, the files still used Sql in method names
Resolve all conflicts manually, mark them as resolved with
"git add/rm <conflicted_files>", then run "git rebase --continue".
You can instead skip this commit: run "git rebase --skip".
To abort and get back to the state before "git rebase", run "git rebase --abort".
Could not apply e50f77a78... Corrected method names; the interface already used SQL, the files still used Sql in method names

The first line in nano looked like the others - are you sure to keep "pick" there?

@greg0ire
Copy link
Member

greg0ire commented Nov 6, 2021

I tried the rebase locally, and apparently it contains commits that are not yours:

p 8ddf46b60 [Raphael Stolt @ 11 days ago] Fixes spelling
p 5ed8c9b98 [Michael Voříšek @ 2 weeks ago] MariaDb1027Platform class should be non-final
p 07540a0b2 [Sergei Morozov @ 13 days ago] Update Psalm to 4.11.2
p 194448094 [Sergei Morozov @ 2 weeks ago] Rework negation of the special characters in SQL parser
p 9d38a4865 [Sergei Morozov @ 13 days ago] Handle PCRE error in the SQL parser
p a354bbb7a [Sergei Morozov @ 11 days ago] Workflow for closing stale pull requests
p 81c54e8ca [Sergei Morozov @ 7 days ago] Fix connection leaks in Oracle functional tests
p 317c05620 [Sergei Morozov @ 7 days ago] Use CONCAT() with SQL Server to concatenate strings
p 0ce5d7892 [Sergei Morozov @ 6 days ago] Do not mark issues as stale
p 3d2f79f2d [Thomas Landauer @ 27 hours ago] Removing paragraph about PDO similarity
p c3721d990 [Thomas Landauer @ 23 hours ago] Update README.md
p 2de7b5b5a [Thomas Landauer @ 23 hours ago] Update README.md
p 8a5d0173e [Thomas Landauer @ 23 hours ago] Update introduction.rst
p 6cf002580 [Thomas Landauer @ 23 hours ago] Update data-retrieval-and-manipulation.rst
p a6edade26 [Thomas Landauer @ 23 hours ago] Removing "This is a Doctrine 2.1 feature"
p c54601f54 [Thomas Landauer @ 23 hours ago] Update query-builder.rst
p 4732b690e [Thomas Landauer @ 23 hours ago] Update security.rst
p 363fbd6c5 [Thomas Landauer @ 23 hours ago] Update transactions.rst
p 5226d0112 [Thomas Landauer @ 22 hours ago] Update transactions.rst
p 3ca8ba648 [Thomas Landauer @ 9 hours ago] Update introduction.rst
p 9b6ac8f50 [Thomas Landauer @ 9 hours ago] Update security.rst
p c8c0487ab [Thomas Landauer @ 9 hours ago] Update transactions.rst

If that's also your case, please try getting to this state:

p 3d2f79f2d [Thomas Landauer @ 27 hours ago] Removing paragraph about PDO similarity
f c3721d990 [Thomas Landauer @ 23 hours ago] Update README.md
f 2de7b5b5a [Thomas Landauer @ 23 hours ago] Update README.md
f 8a5d0173e [Thomas Landauer @ 23 hours ago] Update introduction.rst
f 6cf002580 [Thomas Landauer @ 23 hours ago] Update data-retrieval-and-manipulation.rst
f a6edade26 [Thomas Landauer @ 23 hours ago] Removing "This is a Doctrine 2.1 feature"
f c54601f54 [Thomas Landauer @ 23 hours ago] Update query-builder.rst
f 4732b690e [Thomas Landauer @ 23 hours ago] Update security.rst
f 363fbd6c5 [Thomas Landauer @ 23 hours ago] Update transactions.rst
f 5226d0112 [Thomas Landauer @ 22 hours ago] Update transactions.rst
f 3ca8ba648 [Thomas Landauer @ 9 hours ago] Update introduction.rst
f 9b6ac8f50 [Thomas Landauer @ 9 hours ago] Update security.rst
f c8c0487ab [Thomas Landauer @ 9 hours ago] Update transactions.rst

I just did and there were no conflicts.

If you want to learn more about git rebase, please watch https://www.youtube.com/watch?v=uI1V7771plw&t=814s

@ThomasLandauer
Copy link
Contributor Author

Thanks! I think it worked out this time. Last time, the nano file contained hundreds (thousands?) of lines; this time only ~10.

@greg0ire
Copy link
Member

greg0ire commented Nov 6, 2021

I think you may want to change your commit message now (with git commit --amend) since your changes are more than just about removing a paragraph.

morozov
morozov previously approved these changes Nov 6, 2021
@ThomasLandauer
Copy link
Contributor Author

OK, thanks! :-)

@derrabus derrabus merged commit e05d1ab into doctrine:3.1.x Nov 7, 2021
@ThomasLandauer ThomasLandauer deleted the patch-1 branch November 7, 2021 15:28
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants