Updated: Comment #N

Problem/Motivation

The DatabaseLockBackend class/service still calls db_* procedural functions.

Proposed resolution

Inject the database connection as a dependency and add the database service in the DIC.

Remaining tasks

Patch, etc...

User interface changes

None

API changes

None

Files: 
CommentFileSizeAuthor
#12 2087931-12.patch5.21 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,841 pass(es).
[ View ]
#12 interdiff-2087931-12.txt597 bytesdamiankloip
#7 2087931-5.patch5.22 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 59,171 pass(es).
[ View ]
#7 interdiff-2087931-5.txt3.5 KBdamiankloip
#2 2087931-2.patch5.24 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 59,259 pass(es).
[ View ]
#2 interdiff-2087931-2.txt740 bytesdamiankloip
d8.DatabaseLockBackend-injection.patch4.51 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 58,899 pass(es), 2 fail(s), and 2 exception(s).
[ View ]

Comments

Status:Needs review» Needs work

The last submitted patch, d8.DatabaseLockBackend-injection.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new740 bytes
new5.24 KB
PASSED: [[SimpleTest]]: [MySQL] 59,259 pass(es).
[ View ]

Status:Needs review» Reviewed & tested by the community

yes!

Status:Reviewed & tested by the community» Needs work

+++ b/core/lib/Drupal/Core/Lock/DatabaseLockBackend.php
@@ -15,12 +16,23 @@
   /**
+   * The database connection.
+   *
+   * @var \Drupal\Core\Database\Connection
+   */
+  protected $connection;
+

Can we please name this $database instead? The code below...

+++ b/core/lib/Drupal/Core/Lock/DatabaseLockBackend.php
@@ -32,7 +44,7 @@ public function acquire($name, $timeout = 30.0) {
-      $success = (bool) db_update('semaphore')
+      $success = (bool) $this->connection->update('semaphore')

Is a lot less readable than the code before it. "db" is a crucial part of explaining what's going on. What's a "connection"?

λ d8 → ħ git 8.x* → grep "protected \$connection\;" . -rn | wc -l
42
λ d8 → ħ git 8.x* → grep "protected \$database\;" . -rn | wc -l
29

So connection seems to be more consistent

On the other hand though we do have \Drupal::database() and nothing on ControllerBase()

Status:Needs work» Needs review
StatusFileSize
new3.5 KB
new5.22 KB
PASSED: [[SimpleTest]]: [MySQL] 59,171 pass(es).
[ View ]

I don't really mind either way, doesn't really affect the readability in my eyes but here are those changes.

If this the new way to go, then we might want to start creating issues to convert the $connection usage. As #5 states the numbers, and $connection is more popular atm :)

Status:Needs review» Reviewed & tested by the community

well

'connection' imo breaks encapsulation ; why do I need to know or care that the database system uses a connection object to communicate with the database?

'connection' is a bad name because it does not tell me what am I connected to.

So, 'database' is a win.

Ok great, then looks like this will be the new 'thing' then.

+++ b/core/lib/Drupal/Core/Lock/DatabaseLockBackend.php
@@ -15,12 +16,23 @@
+   * @param \Drupal\Core\Database\Connection $connection

this should be $database

StatusFileSize
new597 bytes
new5.21 KB
PASSED: [[SimpleTest]]: [MySQL] 58,841 pass(es).
[ View ]

Good spot.

And it is still ready to go!

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Patch no longer applies.

Status:Needs work» Fixed

Looks like this was already committed.

Yep you're correct - committed 4e1d667

Issue tags:-Needs reroll

Tags

Status:Fixed» Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.