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

Support from Acquia helps fund testing for Drupal Acquia logo

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

Status: Needs review » Needs work

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

chx’s picture

FileSize
9.29 KB
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
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

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

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

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

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.