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

Status:Active» Needs review

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new9.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.

StatusFileSize
new9.29 KB
FAILED: [[SimpleTest]]: [MySQL] 58,585 pass(es), 3 fail(s), and 3 exception(s).
[ View ]
new778 bytes

Status:Needs work» Needs review

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

StatusFileSize
new9.99 KB
FAILED: [[SimpleTest]]: [MySQL] 58,688 pass(es), 3 fail(s), and 3 exception(s).
[ View ]
new1.58 KB

Documented.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new9.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.

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.

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new2.29 KB
new12.09 KB
PASSED: [[SimpleTest]]: [MySQL] 58,594 pass(es).
[ View ]

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

Patch no longer applies.

Status:Needs work» Reviewed & tested by the community
Issue tags:-Needs reroll
StatusFileSize
new12.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.

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.

Status:Needs work» Needs review
StatusFileSize
new13.64 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Status:Needs review» Needs work

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

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

Argh, blargh, what happened to the installer again?

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.

Status:Needs review» Reviewed & tested by the community

This was RTBC before it broke.

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

Title:Change notice: Change KeyValueFactory to use Settings instead of $confChange 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.

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?

Arrggh.. borked commit message.

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

I found it cleaner. I might be wrong.

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.