This is fixed and also a lot of code duplication is nuked from KeyValueExpirableFactory.

Files: 
CommentFileSizeAuthor
#21 2098111_21.patch13.83 KBchx
PASSED: [[SimpleTest]]: [MySQL] 58,907 pass(es). View
#19 2098111_19.patch13.64 KBchx
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#15 2098111_15.patch12.3 KBchx
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View
#13 2098111_13.patch12.09 KBchx
PASSED: [[SimpleTest]]: [MySQL] 58,594 pass(es). View
#13 interdiff.txt2.29 KBchx
#10 2098111_10.patch9.99 KBchx
FAILED: [[SimpleTest]]: [MySQL] 58,648 pass(es), 1 fail(s), and 5 exception(s). View
#8 interdiff.txt1.58 KBchx
#8 2098111_8.patch9.99 KBchx
FAILED: [[SimpleTest]]: [MySQL] 58,688 pass(es), 3 fail(s), and 3 exception(s). View
#5 interdiff.txt778 byteschx
#5 2098111_5.patch9.29 KBchx
FAILED: [[SimpleTest]]: [MySQL] 58,585 pass(es), 3 fail(s), and 3 exception(s). View
#3 2098111_3.patch9.22 KBchx
FAILED: [[SimpleTest]]: [MySQL] 27,401 pass(es), 67 fail(s), and 43 exception(s). View
kvf_class_constants.patch5.82 KBchx
FAILED: [[SimpleTest]]: [MySQL] 21,704 pass(es), 190 fail(s), and 286 exception(s). View

Comments

chx’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, kvf_class_constants.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
9.22 KB
FAILED: [[SimpleTest]]: [MySQL] 27,401 pass(es), 67 fail(s), and 43 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 2098111_3.patch, failed testing.

chx’s picture

FileSize
9.29 KB
FAILED: [[SimpleTest]]: [MySQL] 58,585 pass(es), 3 fail(s), and 3 exception(s). View
778 bytes
chx’s picture

Status: Needs work » Needs review
dawehner’s picture

This looks really promising.

  1. +++ b/core/lib/Drupal/Core/KeyValueStore/KeyValueExpirableFactory.php
    @@ -14,30 +14,11 @@
    -  }
    +  const DEFAULT_SERVICE = 'keyvalue.expirable.database';
    +
    +  const SPECIFIC_PREFIX = 'keyvalue_expirable_service_';
    +
    +  const DEFAULT_SETTING = 'keyvalue_expirable_default';
    +
    
    +++ b/core/lib/Drupal/Core/KeyValueStore/KeyValueFactory.php
    @@ -13,6 +14,12 @@
     
    +  const DEFAULT_SERVICE = 'keyvalue.database';
    +
    +  const SPECIFIC_PREFIX = 'keyvalue_service_';
    +
    +  const DEFAULT_SETTING = 'keyvalue_default';
    +
    

    These constants are great!
    It would be cool to document the different meaning of these different constants.

  2. +++ b/core/lib/Drupal/Core/KeyValueStore/KeyValueFactory.php
    @@ -25,12 +32,17 @@ class KeyValueFactory {
     
    +  /**
    +   * @var \Drupal\Component\Utility\Settings
    +   */
    +  protected $settings;
     
       /**
        * @param \Symfony\Component\DependencyInjection\ContainerInterface $container
        */
    -  function __construct(ContainerInterface $container) {
    +  function __construct(ContainerInterface $container, Settings $settings) {
         $this->container = $container;
    +    $this->settings = $settings;
       }
    

    Some docs here and there would be cool

chx’s picture

FileSize
9.99 KB
FAILED: [[SimpleTest]]: [MySQL] 58,688 pass(es), 3 fail(s), and 3 exception(s). View
1.58 KB

Documented.

Status: Needs review » Needs work

The last submitted patch, 2098111_8.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
9.99 KB
FAILED: [[SimpleTest]]: [MySQL] 58,648 pass(es), 1 fail(s), and 5 exception(s). View

Well, that's hopeful -- now only the tests themselves are broken.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Let's hope it will just pass.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2098111_10.patch, failed testing.

chx’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
2.29 KB
12.09 KB
PASSED: [[SimpleTest]]: [MySQL] 58,594 pass(es). View
alexpott’s picture

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

Patch no longer applies.

chx’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
12.3 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2098111_15.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

#15: 2098111_15.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 2098111_15.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
13.64 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed. View

Status: Needs review » Needs work

The last submitted patch, 2098111_19.patch, failed testing.

chx’s picture

Status: Needs work » Needs review
FileSize
13.83 KB
PASSED: [[SimpleTest]]: [MySQL] 58,907 pass(es). View

Argh, blargh, what happened to the installer again?

chx’s picture

Accidentally, KeyValueFactory became a rather generic factory that we might want to (in a followup) move up to Component... there's nothing in there that is keyvalue bound. We could, for example, change cache settings to use the same factory and structure instead of just a single cache array. We could unify queue too just needs to split off the reliable queue factory into a separate class.

chx’s picture

Status: Needs review » Reviewed & tested by the community

This was RTBC before it broke.

alexpott’s picture

Title: KeyValueFactory is using $conf instead of Settings » Change notice: Change KeyValueFactory to use Settings instead of $conf
Priority: Normal » Major
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record
chx’s picture

Title: Change notice: Change KeyValueFactory to use Settings instead of $conf » Change KeyValueFactory to use Settings instead of $conf
Priority: Major » Normal
Status: Active » Fixed
Issue tags: -Needs change record

We agreed the followup will be enough documentation.

tstoeckler’s picture

I personally think it is rather pointless, to inject 'settings' when we already inject the service_container anyway, out of which we could pull the settings. Is there any specific reason to do that?

alexpott’s picture

Arrggh.. borked commit message.

Re-committed 714f9e3 and pushed to 8.x. This time with the correct commit message - sorry chx.

chx’s picture

I found it cleaner. I might be wrong.

tstoeckler’s picture

OK, that's fine by me. Just wanted to know if there's some general standard that I missed. Thanks for the answer!

Status: Fixed » Closed (fixed)

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