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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Status: Needs review » Needs work

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

damiankloip’s picture

Status: Needs work » Needs review
FileSize
740 bytes
5.24 KB
ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

yes!

webchick’s picture

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"?

dawehner’s picture

λ 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

dawehner’s picture

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

damiankloip’s picture

Status: Needs work » Needs review
FileSize
3.5 KB
5.22 KB

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 :)

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

well

chx’s picture

'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.

damiankloip’s picture

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

dawehner’s picture

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

this should be $database

damiankloip’s picture

FileSize
597 bytes
5.21 KB

Good spot.

dawehner’s picture

And it is still ready to go!

alexpott’s picture

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

Patch no longer applies.

damiankloip’s picture

Status: Needs work » Fixed

Looks like this was already committed.

alexpott’s picture

Yep you're correct - committed 4e1d667

mcrittenden’s picture

Issue tags: -Needs reroll

Tags

Status: Fixed » Closed (fixed)

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