key_value table is only used by a core service but it depends on system install

+++ b/core/modules/system/system.install
@@ -855,71 +855,6 @@ function system_install() {
diff --git a/core/modules/system/tests/fixtures/HtaccessTest/access_test.module.orig b/core/modules/system/tests/fixtures/HtaccessTest/access_test.module.orig

index e69de29..0000000
diff --git a/core/modules/system/tests/fixtures/HtaccessTest/access_test.php.orig b/core/modules/system/tests/fixtures/HtaccessTest/access_test.php.orig

These look unrelated...

I see a few oddities. For example,

+    if ($uuid = $this->uuid()) {
+      $matching_entities = $storage->getQuery()
+        ->condition('uuid', $this->uuid())
+        ->execute();
+      $matched_entity = reset($matching_entities);
+      if (!empty($matched_entity) && ($matched_entity != $this->id()) && $matched_entity != $this->getOriginalId()) {
+        throw new ConfigDuplicateUUIDException("Attempt to save a configuration entity '{$this->id()}' with UUID '{$this->uuid()}' when this UUID is already used for '$matched_entity'");
+      }
     }

I guess the idea of setting $uuid is that you planned to use the value later instead of making another call to the uuid() method.

This pattern is used twice:

+    $try_again = FALSE;
+    try { ...
+    }
+    catch (\Exception $e) {
+      // If there was an exception, try to create the table.
+      if (!$try_again = $this->ensureTableExists()) {
+        // If the exception happened for other reason than the missing bin
+        // table, propagate the exception.
+        throw $e;
+      }
+    }
+    // Now that the bin has been created, try again if necessary.
+    if ($try_again) {
+      $this->set($key, $value);
+    }

I think you can eliminate the $try_again variable completely by moving $this->set($key, $value); inside the catch block. IMO this makes the control flow easier to follow, which is a good thing to worry about when a function calls itself from a catch block!

    try {
// ...
    }
    catch (\Exception $e) {
      // If there was an exception, then try to create the table.
      if ($this->ensureTableExists()) {
        // Now that the bin has been created, try again.
        $this->set($key, $value);
      }
      else {
        // If the exception happened for other reason than the missing bin
        // table, then propagate the exception.
        throw $e;
      }
    }

I will look at it some more tomorrow.

I forgot to say: I was looking at KeyValueStore/DatabaseStorage.php in my previous comment. Here is another little complaint:

        $schema_definition = $this->schemaDefinition();
        $database_schema->createTable($this->table, $schema_definition);

You can combine those into one line and still stay under 80 characters.

Funny indentation:

      // If another process has already created the batch table, attempting to
      // recreate it will throw an exception. In this case just catch the
      // exception and do nothing.
    catch (SchemaObjectExistsException $e) { // ...
    }

Comparing schemaDefinition() in KeyValueStore/DatabaseStorage.php and KeyValueStore/DatabaseStorageExpirable.php, I see

        'name' => [
          'description' => 'The key of the key-value pair. As KEY is a SQL reserved keyword, name was chosen instead.',

in one and

        'name' => [
          // KEY is an SQL reserved word, so use 'name' as the key's field name.
          'description' => 'The key of the key/value pair.',

in the other. I would rewrite DatabaseStorageExpirable/schemaDefinition() as

  public function schemaDefinition() {
    $schema = parent::schemaDefinition();

    $schema['description'] = '...';
    $schema['expire'] = [
      // ...
    ];
    $schema['indexes'] = [
      // ...
    ];

    return $schema;
  }

This would fix the inconsistencies that are already there, and it would prevent future inconsistencies.

This looks wrong: don't you want use Drupal\Core\KeyValueStore\DatabaseStorage;?

--- a/core/modules/system/src/Tests/Path/UrlAliasFixtures.php
+++ b/core/modules/system/src/Tests/Path/UrlAliasFixtures.php
@@ -2,6 +2,7 @@
 
 namespace Drupal\system\Tests\Path;
 
+use Drupal\Core\Config\DatabaseStorage;
 use Drupal\Core\Database\Connection;
 use Drupal\Core\Path\AliasStorage;
 
@@ -89,7 +90,7 @@ public function tableDefinition() {
     $schema = system_schema();
 
     $tables['url_alias'] = AliasStorage::schemaDefinition();
-    $tables['key_value'] = $schema['key_value'];
+    $tables['key_value'] = DatabaseStorage::schemaDefinition();

Do we really need to keep multi-line array syntax here?

--- a/core/modules/system/src/Tests/Update/DbDumpTest.php
+++ b/core/modules/system/src/Tests/Update/DbDumpTest.php
@@ -84,7 +84,6 @@ protected function setUp() {
 
     // Create some schemas so our export contains tables.
     $this->installSchema('system', [
-      'key_value_expire',
       'sessions',
     ]);
     $this->installSchema('dblog', ['watchdog']);

The attached patch does a few things:

• Restore the removed files as pointed out in #14.
• Implement the cosmetic changes from my comments #17 and #18, plus one or two other changes suggested by CodeSniffer.
• Change the use directive in UrlAliasFixtures.php as I suggested in #18.

The removed files seem to cause one of the test failures for the patch in #14. I am getting additional failures when I run that test locally, so I am curious to see what the testbot does now.

@dawehner points out that some of my other suggestions affect code already in core, so I will open a couple of follow-up issues to suggest those changes.

It already needs a re-roll!

Failing two tests. I think one is easy to fix. I am attaching an interdiff with my previous patch.

I was right: we are down to one failing test.

After adding some extra lines to the failing test, I get the following message when running it locally:

Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupal.simpletest974603key_value_expire' doesn't exist: DELETE FROM {key_value_expire} WHERE (expire < :db_condition_placeholder_0); Array ( [:db_condition_placeholder_0] => 1462765698 ) in system_cron() (line 1285 of /var/www/drupalvm/drupal/core/modules/system/system.module).

This is the extra dblog message that makes the test fail. The offending line in system.module is

    \Drupal::service('keyvalue.expirable.database')->garbageCollection();

So I think the problem is that KeyValueStore\KeyValueDatabaseExpirableFactory::garbageCollection() is trying to delete entries from a table that does not exist.

That is as far as I plan to go with this.

@dawehner: Yes, and I decided to give it a try myself.

Since the DatabaseStorage::catchException() method is neither public nor static, I copied it (replacing $this->table with 'key_value_expire') to KeyValueDatabaseExpirableFactory and used it in the catch block. The DbLog test now passes locally; let's see if the testbot gets the same result.

While working on this, I noticed that some of the code comments had been copied from the BatchStorage class. Since that is part of this issue, I updated the comments. This is a signal that we should formalize this pattern for lazy table creation.

Another thing I noticed is that the table name 'key_value_expire' appears in two places: the constructor for DatabaseStorageExpirable and in KeyValueDatabaseExpirableFactory::garbageCollection(). (After this patch, it appears in another method in that class.) I think that string should be used in just the factory class, passed to the DatabaseStorageExpirable constructor. In fact, the only reason that constructor exists seems to be to override the default value of $table from the DatabaseStorage constructor, so if we pass 'key_value_expire', then we can eliminate the child constructor. Unless someone tells me I am missing something, I will open an issue to fix this.

This is a signal that we should formalize this pattern for lazy table creation.

There's a really old issue where that was being done - #2371709: Move the on-demand-table creation into the database API

@almaudoh: Thanks for the reference.

I am moving this back to NW even though it is passing tests. I searched for key_value and key_value_expire in the code base. I found a few places, mostly in tests, where there are direct database queries. For example, (drupal-8.views_entity_reference_plugins-2429191.php):

$installed = $connection->select('key_value')
  ->fields('key_value', ['value'])
  ->condition('collection', 'entity.definitions.installed')
  ->condition('name', 'node.field_storage_definitions')
  ->execute()
  ->fetchField();

Probably these should be instantiating the appropriate DatabaseStorage class instead. Maybe in a follow-up issue? Or maybe they really should talk to the database directly?

In MigrationCreationTrait::getLegacyDrupalVersion() there is something similar, and I think this one should stay as is.

There are several tests that do something like this in their setUp() methods (DbLogFormInjectionTest.php):

    $this->installSchema('system', ['key_value_expire', 'sequences']);

I think the right thing to do is remove 'key_value_expire' from the array, and I think it should be done as part of this issue. But maybe some or all of these should be creating the tables indirectly by instantiating the appropriate class.

Also, there are several gzipped files that directly reference these database tables: core/modules/system/tests/fixtures/update/*.php.gz.

The patch from #31 ran afoul of #2687897: Convert system module's kernel tests to NG, which moved the tests to a new location. Don't take my word for it, ask git:

$ git show --stat=200 20a0802
commit 20a080288f7342d6fecff46bf7c1228675ea84c6
Author: Nathaniel Catchpole <catch@35733.no-reply.drupal.org>
Date:   Mon May 23 12:37:36 2016 +0100

    Issue #2687897 by dawehner, jhedstrom, catch, jibran, Berdir, klausi: Convert system module's kernel tests to NG
...
 core/modules/system/{src/Tests => tests/src/Kernel}/Block/SystemMenuBlockTest.php                                    |   4 +-
...
 core/{modules/system/src/Tests/Common => tests/Drupal/KernelTests/Core/Asset}/AttachedAssetsTest.php |   9 +-

That makes it easy to re-roll:

$ git co -b foo 20a0802~1
$ git apply -v --index 2664322-31.patch
$ git commit
$ git merge 8.2.x

The resulting patch (attached) applies to the current head of 8.2.x or 8.3.x. I do not think an interdiff is appropriate in this case.

I checked the interdiff attached to #31. It does affect only tests, and it does remove 'key_value_expire' from the array of modules to be enabled for the test, as advertised. In appropriate places, the setUp() method does not do anything else, so it is removed completely.

I again searched the code base for key_value and key_value_expire. I found one new test that installs the module in its setUp() method. I removed that: a second patch and an interdiff are attached. Let's see what the testbot has to say!

The 33-a.patch is so much bigger...

@almaudoh: Thanks for catching that. After making sure that my patch applied against 8.3.x, I made a diff against 8.2.x, so I accidentally included the differences between 8.2.x and 8.3.x in that patch.

I am hiding the bad patch and uploading a replacement. The interdiff on #33 shows the differences between 2664322-33.patch and 2664322-35.patch.

Just a reroll

Reverted unrelated changes in #37.1 and #37.3

Also raised #2787737: Finalize return value for KeyValueStoreInterface::getMultiple() for #37.2

Reverted some leftovers of unrelated changes in File.php. Also added the #2787737: Finalize return value for KeyValueStoreInterface::getMultiple() issue link in the todo.

This change is looking pretty good. One thing that makes me hesitate is what happened when we made the change for url_alias. However I don't think that is a concern here because the state table will definitely exist by the end of the install. But still we should check that once this patch is applied requests to regular routes like node/1. Since if we're getting exceptions here then we'll slow down the critical path.

+++ b/core/lib/Drupal/Core/Config/DatabaseStorage.php
@@ -181,7 +181,7 @@ protected function ensureTableExists()  {
+  public static function schemaDefinition() {

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigEntityBase.php
@@ -323,12 +323,14 @@ public function preSave(EntityStorageInterface $storage) {
-    $matching_entities = $storage->getQuery()
-      ->condition('uuid', $this->uuid())
-      ->execute();
-    $matched_entity = reset($matching_entities);
-    if (!empty($matched_entity) && ($matched_entity != $this->id()) && $matched_entity != $this->getOriginalId()) {
-      throw new ConfigDuplicateUUIDException("Attempt to save a configuration entity '{$this->id()}' with UUID '{$this->uuid()}' when this UUID is already used for '$matched_entity'");
+    if ($uuid = $this->uuid()) {
+      $matching_entities = $storage->getQuery()
+        ->condition('uuid', $uuid)
+        ->execute();
+      $matched_entity = reset($matching_entities);
+      if (!empty($matched_entity) && ($matched_entity != $this->id()) && $matched_entity != $this->getOriginalId()) {
+        throw new ConfigDuplicateUUIDException("Attempt to save a configuration entity '{$this->id()}' with UUID '$uuid' when this UUID is already used for '$matched_entity'");
+      }

Are these unintended changes or are they because we've got tests for key value entity storage?

Other than that looks good to me

works for me

That change looked odd to me as well, uploading a patch without the hunk (essentially patch -p1 -R #45) to see what fails exactly.

Nice - I think we should open an issue to add a SchemaDefinitionInterface so we can discover all the services which have schema for reporting and schema comparison.

+++ b/core/lib/Drupal/Core/Config/DatabaseStorage.php
@@ -181,7 +181,7 @@ protected function ensureTableExists()  {
    */
-  protected static function schemaDefinition() {
+  public static function schemaDefinition() {
     $schema = array(
       'description' => 'The base table for configuration data.',
       'fields' => array(

Why is this change from protected to public in this patch? Looks unrelated.

Still looking for an answer to #58.

Guess you forgot the --binary option again :) Same patch as in #65

Since #58 has been addressed, back to RTBC.

Committed 5571193 and pushed to 8.3.x. Thanks!

It turns out that this broke saving config entities without UUIDs. For example:

    $values = [
      'formId' => $form_id,
      'captchaType' => 'default',
      'status' => FALSE,
    ];
    $captcha_point = new CaptchaPoint($values, 'captcha_point');
    $captcha_point->trustData()->save();

From #2808087: Improve captcha_install(), correctly create dynamic captcha points, move static captcha points to default config

The problem is that when \Drupal\Core\Config\Entity\ConfigEntityBase::preSave() fires it assumes $this->uuid() returns a UUID but creating and saving config entities as above by passes the uuid assignment in entity storage create. We need to work out what to do here.

This also introduced a test failure on PHP7/Postgres so we should ensure any subsequent patch is tested against all environments.

For some reason the connection class is trying to use PDOStatement instead of the statement class. This throws the HY00: General error: user-supplied statement does not accept constructor arguments error. I am not sure why the driver or our DBAL is switching back to PDOStatement.

All three queries being run, the watchdog insert query, the key_value store select query, and the table exists query are being run with PDOStatement. Everything else is using the defined statement class.

In C: https://github.com/php/php-src/blob/master/ext/pdo/pdo_dbh.c#L411

ConnectionFailureTest calls Database::closeConnection();, which resets ATTR_STATEMENT_CLASS to PDOStatement. And then DBlog's custom connection thing might not be going through the right pathways to set it again.

I think the bug lies in the general Database/Connection classes rather than Watchdog module because Watchdog module shouldn't need to know about the intricacies of setting up the connection correctly to use Statement. It should be happening correctly in the constructor for a driver class, but it's not?

Yes, quite confusing.

This patch is probably not the correct approach, but to show that it's the problem.

Maybe a race condition in PDO object destruction and now since that test is doing more queries it uses an existing PDO object? Checking the statement class inside Dblog shows that it's Statement, but then checking inside prepareQuery shows that it's PDOStatement. If it is a race condition it would theoretically affect any driver using statement class.

I had some weird changes locally and did not apply the patch in #77 correctly. Hiding my patch and interdiff.

The SQLite fail is valid as that driver doesn't override statementClass, which might be better if it did. I wanted to apply this generally because I have a sneaking suspicion that it could apply across the board in some rare cases. Or that we are hiding the error somewhere in exception handling.

I will apply the patch on a clean checkout and redo.

Try with a clean patch.

Also an experimental patch that if it fails deserves its own issue. Edit: I expect sqlite to behave oddly, but we'll see if mysql and pgsql display an actual issue.

If I look at your statement-class-test.patch I see that you are trying to get the Statement class with the getAttribute() method. Looking at the PHP documentation for this, there are a number of PDO constants that can be used. And \PDO::ATTR_STATEMENT_CLASS is not one of them. So the construction that you are using is not supported by PHP. Maybe unofficial but not according to their manual (http://php.net/manual/en/pdo.getattribute.php). The constant \PDO::ATTR_STATEMENT_CLASS can only be used with the method setAttribute() to: "Set user-supplied statement class derived from PDOStatement. Cannot be used with persistent PDO instances. Requires array(string classname, array(mixed constructor_args)).". This is also according to the PHP manual. AFAIK a PDOStatement class is the result that you get after you run a PDO query.

@dawehner, I retested #77 for php7 sqlite and got the same error as in #84 for php7 sqlite. I queued up #84 (pgsql and sqlite) and #77 (sqlite only) tests again in case these errors in #77 and #84 were random test failures.

Thanks, @daffie. It looks like it's a php docs bug. It's possible that it only applies on certain drivers: "Note that some database/driver combinations may not support all of the database connection attributes." Connection::__construct sets the attribute to use Statement class from a PDO Query, not PDOStatement. Drupal switches back to PDOStatement during Connection::destroy.

Edit: #77 and #84 still fail on SQLite (php7). Probably the same for PostgreSQL.

PHP 7 isn't passing at the moment due to a PHP bug: https://www.drupal.org/node/2813981.

So i guess #84 is working for 5.5 and 5.6 mysql, sqlite, and pgsql. That's the patch that only adds a check in pgsql to ensure that Statement is the statement class instead of PDOStatement.

Retesting on php7. So how do we want to proceed? Commit #84 as is? Or take those new parts to a new issue?

+    // the connection.
+    $statementClass = $this->connection->getAttribute(\PDO::ATTR_STATEMENT_CLASS);
+    if ($statementClass[0] <> $this->statementClass) {
+      $this->connection->setAttribute(\PDO::ATTR_STATEMENT_CLASS, array($this->statementClass, array($this)));
+    }

We should use !== instead of <>. Didn't realise that still works in php :)

Just the nitpicks. I guess I can RTBC this as I haven't made any changes since the revert.

It's good that the workaround works, but do we understand what the race condition is? And is it reproducible enough to post a PHP/PDO bug report for?

@catch, I don't have any specific data, but I think this is an application-level condition, not a PHP/PDO bug.

$connection is held open if there are any references to the object even when the destroy command is issued on it. I'm not sure where that could be coming from in the code base. A debug_zval_dump would help to prove that this is the case, but that doesn't help to figure out where it could be.

I added debug_zval_dump into ::destroy to see what it usually comes up with. During the test run of ConnectionFailureTest it is set to 2 reference counts in that method. Most likely those references are destroyed quickly in most use cases of database, but maybe not when Dblog is doing it?

I don't think we should add drivercode to cover a bug that's only reproducible in a test that's specifically messing with the database connection, or do it in this issue. Can we split this discussion out to a new issue and postpone this one?

The patch from #67 was committed (5571193) and then reverted (eaaea5d for two reasons:

1. This breaks saving config entities without UUIDs (#73).
2. There are test failures with PHP7 and some database drivers (#74).

The first problem is addressed in #77, and later patches (up to #94) addressed the second problem.

There have been several suggestions (at least #85 and #102) to handle the second problem in a separate issue and to postpone this issue. I am updating the "Remaining Tasks" section of the issue summary.

I just created #2847855: Incorrect statement class caused by race condition when closing and opening the connection, added it as a related issue, and marked this issue as Postponed. Is there any way (other than this comment) to indicate that the related issue and the Postponed status are connected?

I did my best to describe the problem, but do not be shy about updating #2847855 if you have a better handle on it.

In order to make reviews easier, I am attaching two interdiffs:

• interdiff-67-77.txt shows the differences between the patch in #67 (which was committed and then reverted) and the one in #77.
• interdiff-77-94.txt shows the differences between the patches in #77 and #94.

In fact, interdiff-67-77.txt is essentially the same as the interdiff attached to #74. (We used different methods to generate the interdiffs.) I think the only difference between #74 and #77 is that the second one was correctly generated with git diff --binary and the first without the --binary option.

The patch in #94 corrects a few typos in documentation comments. The code change that it introduces should be made in the new issue #2847855: Incorrect statement class caused by race condition when closing and opening the connection.

I think that further work on this issue should start with the patch in #77 (along with fixing the typos as in #94), which may need to be re-rolled.

Here is an attempt to re-roll the patch from #77, with the typo corrections from #94. Because of the changes in HEAD, I do not see an easy way to generate an interdiff. :-(

There was a merge conflict in EntityAutocompleteElementFormTest::setUp():

   protected function setUp() {
     parent::setUp();
 
-    $this->installSchema('system', ['key_value_expire']);
     \Drupal::service('router.builder')->rebuild();
 
     $this->testUser = User::create(array(

That snippet now looks like

   */
  protected function setUp() {
    parent::setUp();

    $this->installSchema('system', ['key_value_expire']);
    $this->installEntitySchema('entity_test_string_id');
    \Drupal::service('router.builder')->rebuild();

    $this->testUser = User::create(array(

so it is easy to see why the patch did not apply. Also easy to apply it manually.

The other conflict was in ConfigEntityStorageTest::testSaveMismatch(). This is more complicated, since that test was rewritten in #2824165: Remove brittleness from ConfigEntityStorageTest. But maybe the only reason that change was in the patch was to fix failing tests, in which case we may not have to deal with it in this issue.

I am curious to see which tests fail this time around. ;) So I am temporarily moving from Postponed to NR.

Here is a new patch that, instead of giving up on on merge conflicts in ConfigEntityStorageTest , leaves that test completely alone. I expect that this will fix the test failures with MySQL.

I also removed the change to the dblog tests. If that fixes the test failures with Postgres, then we will have to re-think the idea that those are not really related to this issue.

@dawehner, it has been interesting!

I have attached an interdiff for the changes between #109 and #111. I already described the two small changes in #111.

Now that the testbot has spoken, I am moving this issue back to Postponed. I think the next step is to answer the question (#80, #96) why this patch triggers the PostgresQL error. In other words, we need to work on #2847855: Incorrect statement class caused by race condition when closing and opening the connection.

Changing the implementation of the PostgreSQL version of the method Schema::tableExists to the same as the MySQL version fixes the problem in #2847855: Incorrect statement class caused by race condition when closing and opening the connection. Lets see if it does the same for this issue.

Let's give the testbot a workout. I expect PostgreSQL to work with various versions of PHP, but I do not know what to expect with SQLite.

See my comments in #111 and the interdiff in #114: only half of the changes I added in #111 should be kept.

As I suggested on #2847855: Incorrect statement class caused by race condition when closing and opening the connection I think that fixing tableExists() should be done in a separate issue, but I am really excited to see those test passes on this issue!

I would also like to address some of the comment from #17 before this issue is closed.

I did some debugging (reported on one of the related issues) and I now think we can fix this issue. We discovered some new issues while working on this, but this issue is no longer blocked by any of them. See my "fail" patch at #2847855-10: Incorrect statement class caused by race condition when closing and opening the connection, which reproduces the PostgreSQL test failure that has been blocking this issue.

This is why the test fails (answering @dawehner's question in #80):

1. The Connection class has the wrong statementClass, leading to an exception.
2. The patches on this issue change the exception handling in KeyValueStore\DatabaseStorage::getMultiple(). Previously, all exceptions were ignored. Earlier patches on this issue propagated the exception unless it seemed to be caused by a missing (to-be-lazy-loaded) database table.
3. Different implementations of tableExists() give different results, which explains why the test fails only for PostgresQL.

I created #2847855: Incorrect statement class caused by race condition when closing and opening the connection to follow up on #1.
I just created #2850292: Improve exception handling indirectly in KeyValueStore\DatabaseStorage::getMultiple() method to follow up on #2.
@daffie has posted patches here and on #2847855: Incorrect statement class caused by race condition when closing and opening the connection related to #3. I suggest dealing with that problem in another issue: perhaps create a new one.

The attached patch reverts all changes to getMultiple(). Locally, this fixes the ConnectionFailureTest failure with PostgresQL. Like the patch in #111, it reverts all changes to ConfigEntityStorageTest because #2824165: Remove brittleness from ConfigEntityStorageTest already did a better job of fixing that test. Because of all that, I have attached an interdiff comparing to the re-roll in #109, not to one of the more recent patches.

I also plan to do some code cleanup once I have a patch that passes testing.

Here is a patch simplifying some of the try/catch blocks as I suggested in my review in #17. Unless I am missing something, this patch is functionally equivalent to the one in #116.

I have a little more cleanup to do, but this is tricky enough that I want to provide an interdiff that is not cluttered up with any other changes.

Ouch, I made two mistakes. One is the mistake that @almaudoh kept fixing on earlier patches: I forgot to use the --binary option when generating the patches in #116 and #117. Then I accidentally committed changes to composer.json and composer.lock in #117.

The attached patch fixes those problems and gives an interdiff against what #117 was supposed to be.

As the interdiff shows, this only does some cleanup: avoid mixing aray(...) and [...] syntax in the same file, in order to follow Drupal coding standards.

This is the last cleanup I have in mind, although I still plan to take another look at the patch.

Hiding patches (but not interdiffs) as explained in #118.

It does not help to introduce a syntax error. :-(

Again, I attach a patch and an interdiff against what I meant to upload in #117.

I need some sleep. That should give the testbot time to finish.

Sorry, I keep tweaking this.

I do not like the pattern

  public function foo(...) {
    try {
      // This is where the real work goes.
    }
    catch (\Exception $e) {
      if (...) {
        $this->foo(...);
      }
      else {
        throw $e;
      }
    }
  }

It is bad enough that we are using exceptions for control flow. (Where did I see a policy against that?) If the if(...) does the wrong thing, then we could be caught in an infinite loop. The attached patch replaces this pattern (twice each in DatabaseStorage and DatabaseStorageExpirable) with the pattern that I see in Batch\BatchStorage and Cache\DatabaseBackend:

  protected function doFoo(...) {
    // Do something useful.
  }

  public function foo(...) {
    try {
      return $this->doFoo(...);
    }
    catch (\Exception $e) {
      if (...) {
        $this->doFoo(...);
      }
      else {
        throw $e;
      }
    }
  }

Actually, my pattern is a little different from what I see in those two classes. As in #117, I simplify the control flow, removing the $try_again flag. With the simpler control flow, I think we do not need so many comments on the control flow, so I have removed several comment lines.

@dawehner: No, we do not have to wait for fixes to pgsql. See my comment in #116. And see all the green from the testbot. It needs a reroll, probably because of the short array syntax.

That was a nasty reroll. I hope the testbot approves! I think the only conflicts were #2776975: March 3, 2017: Convert core to array syntax coding standards for Drupal 8.3.x RC phase (as expected) and #2854529: Fix Drupal.Scope.MethodScope - all methods should have scopes.

+++ b/core/tests/Drupal/KernelTests/Core/KeyValueStore/StorageTestBase.php
@@ -94,6 +94,11 @@ public function testCRUD() {
+    $result = $stores[0]->getMultiple(array());
+    $this->assertEquals(array(), $result);

The testbot now complains about this not using the new array syntax.

Thanks for checking that! I was so worried about fixing the merge conflicts that I neglected to check the files where the merge was clean.

Here is a new version of the patch, and an interdiff.

Patch still applies do we have any feedback pending here?

+++ b/core/modules/views/tests/fixtures/update/views.view.test_boolean_filter_values.yml
@@ -1,4 +1,5 @@
 langcode: en
+uuid: 8C0C65A8-7099-4E90-9F6C-C5EE7BD3445D
 status: true

Also:

>>> \Drupal\Component\Uuid\Uuid::isValid('8C0C65A8-7099-4E90-9F6C-C5EE7BD3445D');
=> false

UUID's need to be lowercase.

From #73:

It turns out that this broke saving config entities without UUIDs.

From #74:

This also introduced a test failure on PHP7/Postgres ...

I think we fixed the problem from #74, but we still need to fix the problem from #73. If I were sure of myself, then I would set this back to NW. Maybe the right thing to do is to add a failing test, and then the testbot will set the issue back to NW.

I see some recent activity on #2721271: Simplify exception handling for lazy-load pattern. If that issue gets in, then this patch will need a reroll.

Should we also postpone this issue on #2850292: Improve exception handling indirectly in KeyValueStore\DatabaseStorage::getMultiple() method?