Problem/Motivation
From #3174662: Encapsulate \PDOStatement instead of extending from it.
The Statement*
classes have public properties like $dbh
and $allowRowCount
, historically to support tight integration with PDO.
With #3174662: Encapsulate \PDOStatement instead of extending from it, the PDO integration was relaxed, and we could revisit that in order to implement a more clean approach with protected properties and getters/setters.
Proposed resolution
move $dbh and $allowRowCount to the magic getter for BC with deprecation trigger, make an alternative set of protected properties for that, introduce getters (and setters if necessary), and use them across - will have to touch the other StatementInterface implementations
Remaining tasks
User interface changes
API changes
The method Drupal/Core/Database/Connection::prepareStatement() has a new parameter called $allow_row_count
, which is a boolean value and default to FALSE. When set to TRUE allows the method Drupal\Core\Database\StatementInterface::rowCount() return the number of affected rows in the query.
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|
Issue fork drupal-3177660
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
- 3177660-remove-public-properties changes, plain diff MR !68
Comments
Comment #2
mondrakeComment #3
mondrakeOn this.
Comment #4
mondrakeLet's see how much we fail.
Comment #5
andypostQueued pgsql, because only mysql got mass fail
Comment #6
mondrake#5 it's expected, only deprecation for now
Comment #7
mondrakeComment #8
mondrakeWonder if it's worth deprecating $dbh and $allowRowCount also in StatementEmpty and StatementPrefetch, and adding the new methods to StatementInterface
Comment #10
andypostEnable may return
$this
so this code could be simplified with chainingOTOH it's not clear to me how this flag works
Comment #11
mondrake#10: that flag was introduced in #2146733: Select queries should not use rowCount() to calculate number of rows...
Personally I find this toggle to be code babysitting, if one wants to use rowCount on selects and shoot their own foot, be it... and in any case the toggle is little protection, one could always enable it and so back to starting blocks.
Comment #12
andypostI suggest to mark both methods
@internal
as this is a way to set custom flag on wrapper to allow getting number of affected rows (which has lots of bugs on different DBs)It's normally used to prevent consumer of trying to use rowCount on select-based queries
Comment #13
andypostand that's only ugly way to pass this flag from connection (calling code) to statement wrapper - to flag the only case when row count allowed
Comment #14
mondrake#13 if it’s really necessary we may consider to pass it in the constructor of the statement
Comment #15
andypostAs I see it done this way specifically to set it exactly after execution if requested by result type
Comment #16
mondrake#15: I see, but why? We can pass
$options['return']
to the StatementInterface object on construction, since we know what type of query we are doing at that moment... at least in today's Drupal. Largely revised patch, interdiff wouldn't help.Comment #17
mondrakeComment #18
andypostThat looks much more cleaner but leads to code duplication in constructors
I think it was public to prevent change every constructor, no idea how to make it different way
Comment #20
mondrakeFixes.
Comment #21
mondrakeComment #22
mondrakeComment #23
daffie CreditAttribution: daffie commentedPatch looks good!
Why do we use the variable
$name
? We know what its value is!I think when we touch method with a patch and the method has no docblock we should add one.
Why do we do this change? AFAIK
Database::RETURN_STATEMENT
is the default value.9.1.0 should become 9.2.0
Comment #24
anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commentedAddressed 23.1, 23.2, 23.3, 23.4.
Comment #26
anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commentedComment #28
anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commentedComment #29
daffie CreditAttribution: daffie commentedAll the changes in the patch from comment #28 look good. Only point #23.5: "We are missing a number of deprecation tests." still needs to be done.
Comment #30
mondrakeYes, and some todos to fix. Working on this.
Comment #31
mondrakeComment #33
mondrakeComment #34
daffie CreditAttribution: daffie commentedCould you explain why this check is necessary and why can it be removed in D10?
@mondrake: You are properly right that working with merge requests is going to be better then with patches. Only I do not know how to get my comment on the merge request to this issue queue.
Comment #35
mondrake#34.2 we need to write a CR before completing that. The deprecation tests are there, now with the deprecation moved from annotation to method you can expect patterns instead of exact strings.
Comment #36
mondrakeComment #37
daffie CreditAttribution: daffie commentedThe patch looks good, only my point #34.2 is still open.
Comment #38
mondrakeComment #39
mondrakeNot reviewable atm.
Comment #40
mondrakeComment #41
daffie CreditAttribution: daffie commentedI like the current solution. For me it the way to fix this issue.
Comment #42
mondrakeComment #43
daffie CreditAttribution: daffie commentedQuick review:
Comment #44
mondrake#43.1 As long as this is not leading to it being used in
Connection::query
, I think it could stay in until #3185269: Introduce Connection::lastInsertId and deprecate the 'return' query option and Database::RETURN_* constants#43.2 I would suggest so, yes, but in #3185269: Introduce Connection::lastInsertId and deprecate the 'return' query option and Database::RETURN_* constants
Comment #45
mondrakeComment #46
mondrakeAdded CR and adjusted the MR.
Comment #47
daffie CreditAttribution: daffie commentedBack to needs work.
Comment #48
mondrakeComment #49
alexpottcore/tests/Drupal/KernelTests/Core/Database/StatementWrapperLegacyTest.php is skipped for sqlite so I think we don't have any coverage of the new code in StatementPrefetch or StatementEmpty.
Comment #50
daffie CreditAttribution: daffie commentedAdded a new comment on the MR.
Comment #51
mondrakeThanks.
Comment #52
mondrakeFrom discussion in Slack:
@daffie
Comment #53
mondrakewell in the MR we are no longer using Connection::query to execute the query, but the Statement directly. So per se we are taking a different route here, even if we had that option set it would be ineffective in here. And yes, we should deprecate it IMHO since in core it is not really doing anything at the moment.
No. What I fancy is a 'ExceptionHandler' class in the db/driver namespace, that both Connection::query and operation::execute methods could invoke, using the ::getDriverClass mechanism. Then Connection::handleQueryException() could be deprecated. That will help normalizing exception management per driver.
Comment #54
mondrakeFiled #3186934: Introduce an ExceptionHandler class in the database API, deprecate Connection::handleQueryException.
Comment #55
daffie CreditAttribution: daffie commentedComment #56
mondrakeComment #57
daffie CreditAttribution: daffie commented@mondrake: Why postening this issue? I am ready to give it a RTBC.
Comment #58
mondrake#57 yes, sorry I should have explained. I think doing #3186934: Introduce an ExceptionHandler class in the database API, deprecate Connection::handleQueryException first would improve the patch here, by reducing the code duplication when dealing with exceptions.
Comment #59
mondrakeFiled #3189680: Deprecate the 'throw_exception' option in the Database API.
Comment #60
mondrakeReroll, and updates after commit of #3186934: Introduce an ExceptionHandler class in the database API, deprecate Connection::handleQueryException
Comment #61
mondrakeBack to NR.
Comment #62
mondrakeReplied in MR.
Comment #63
mondrakeRerolled
Comment #64
mondrakeRerolled and removed changes to StatementEmpty, now deprecated, so no longer needed.
Comment #65
andypostFiled 10.x issue for added TODOs - #3210310: Adjust Database API to remove deprecated Drupal 9 code in Drupal 10
Fixed test (
StatementEmpty
no longer affected by the patch)Added full namespace to deprecations
Comment #66
daffie CreditAttribution: daffie commentedThe patch looks good to me.
Just 1 minor nitpick.
After that one it is RTBC for me.
Comment #67
mondrakeComment #68
daffie CreditAttribution: daffie commentedThe patch looks good to me.
The 2 class variables are now protected.
There is a BC layer for the 2 class variables.
There are deprecation messages for the 2 class variables.
The Is and the CR look good.
For me it is RTBC.
@mondrake: Thanks.
Comment #69
andypostAdded strict comparison but have no idea hot to trigger sqlite testing on MR
Comment #70
mondrake@andypost you need to wait a few minutes after pushing the commit to branch for the integration to check thta the branch is mergeable. Then, an ‘add test’ link appears and you can select test configs
to run as usual.
Comment #71
andypost@mondrake thank you! now waiting for results...
Comment #72
mondrakeComment #73
andypostI used to rebase changes as solving merge conflics was too log
Comment #74
mondrakethanks
Comment #76
mondrakeComment #78
alexpottA couple of things added to a review on gitlab...
Comment #79
mondrakeThanks @alexpott, replies in MR
Comment #80
mondrakeFiled #3211866: Upsert::execute() return values are inconsistent.
Comment #81
daffie CreditAttribution: daffie commentedThe testbot is failing the MR.
Comment #82
mondrakeI tend to prefer Upsert::execute changes at version 42, given latest findings - in reality now in HEAD we're returning the number of affected rows (which is returned by the Connection::query() call with Database::RETURN_AFFECTED 'return' option), even if the variable is misnamed. With the latest changes (removing the Database::RETURN_AFFECTED 'return' option from Upsert), we're returning the Statement instead.
Or we can postpone this issue on the hardening to be done in #3211866: Upsert::execute() return values are inconsistent.
Comment #83
andypostSomehow pgsql and sqlite failed
Comment #84
daffie CreditAttribution: daffie commentedThe current MR is (partly) fixing #3211866: Upsert::execute() return values are inconsistent. Therefore to me, we need to do the other issue first.
Comment #85
mondrakeRerolled and now adjusted after #3211866: Upsert::execute() return values are inconsistent.
Comment #86
andypostOnly sqlite test failed
Comment #87
mondrakeSomething's broken with $allowRowCount in StatementPrefetch, on it.
Comment #88
mondrakeLooks like you cannot really use
rowCount()
withStatementPrefetch
when a SELECT is executed. In SQLite it would return 0, which is not the count of rows. So here changed the test to execute an UPDATE. It does not matter for the test itself.Comment #89
andypost@mondrake yes, that's why affected rows should be internal (implementation details) it applies only for non-select queries
I wondered that it returns number of results for Mysql and PG
Comment #90
daffie CreditAttribution: daffie commented2 remarks for now. Will do another review tomorrow.
Comment #91
mondrakesee commnets in MR
Comment #92
daffie CreditAttribution: daffie commentedBack to you @mondrake.
Comment #93
mondrake@daffie indeed, I should have noticed that :)
Fixed and also changed the code touched in SQLite's Connection:nextId to use square brackets syntax.
Comment #94
daffie CreditAttribution: daffie commented@mondrake: Almost there. Just one fix is wrong.
Comment #95
mondrakeWhoooops... thanks @daffie, fixed.
Comment #96
daffie CreditAttribution: daffie commentedNow it all looks good to me.
Comment #97
mondrakeComment #98
mondrakeLast MR version tested with DruDbal experimental driver and BC now seems there.
Comment #99
daffie CreditAttribution: daffie commentedAll the changes look good to me.
We shall have to wait for D10 to remove those
$options['return'] = Database::RETURN_AFFECTED;
.Back to RTBC.
Comment #100
alexpottI moved the deprecations to 9.3.0 on commit because 9.2.0 is in alpha and we shouldn't be added new deprecations in that branch.
Committed 201212c and pushed to 9.3.x. Thanks!