While working on #1831774: Config import assumes that 'config_prefix' contains one dot only, I realized that breakpoint.module is the only place in core other than storage controllers and the storage controller test that directly translates config object names into config entity IDs.
Also in there are calls like
config_get_storage_names_with_prefix()
drupal_container()->get('config.storage')->listAll()
entity_get_info() x 2
entity_load_multiple() x 2

Those all scream to be contained in a storage controller.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

This is still crazypants.

RainbowArray’s picture

My understanding is that the main change needed here is to create a storage controller. Would that also resolve the issue of translation of config object names into config entity IDs?

If there's an example of a similar conversion to a storage controller, I'd be glad to give this a try.

RainbowArray’s picture

Status: Active » Needs review

I worked with timplunkett on IRC to set up a patch to take care of this move. There's a decent chance this breaks and will need work. Let's test and see!

RainbowArray’s picture

Uploading the patch: usually pretty useful.

Status: Needs review » Needs work

The last submitted patch, 4: breakpoint-group-storage-controller-3.patch, failed testing.

tim.plunkett’s picture

  1. +++ b/core/modules/breakpoint/breakpoint.module
    @@ -6,6 +6,7 @@
     use Drupal\breakpoint\Entity\Breakpoint;
    +use Drupal\breakpoint\BreakpointGroupStorageControllerInterface;
     use Drupal\breakpoint\Entity\BreakpointGroup;
     use Drupal\Core\Config\Entity\ConfigStorageController;
    

    I'm guessing some of these use statements can be removed now.

  2. +++ b/core/modules/breakpoint/breakpoint.module
    @@ -49,7 +50,7 @@ function breakpoint_help($path, $arg) {
    +  \Drupal::entityManager()->getStorageController('breakpoint_group')->_breakpoint_delete_breakpoints($theme_list, Breakpoint::SOURCE_TYPE_THEME);
    
    +++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Controller/BreakpointGroupStorageController.php
    @@ -0,0 +1,76 @@
    +  function _breakpoint_delete_breakpoints(array $list, $source_type) {
    
    +++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Controller/BreakpointGroupStorageControllerInterface.php
    @@ -0,0 +1,26 @@
    +  public function _breakpoint_delete_breakpoints(array $list, $source_type);
    

    This should be renamed to something like deleteBreakpointsBySource or something?

  3. +++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Controller/BreakpointGroupStorageController.php
    @@ -0,0 +1,76 @@
    +    $ids = \Drupal::configFactory()->listAll('breakpoint.breakpoint_group.' . $source_type . '.');
    +    $entity_manager = \Drupal::entityManager();
    ...
    +        $breakpoint_ids = \Drupal::service('config.storage')->listAll('breakpoint.breakpoint.' . $source_type . '.' . $breakpoint_group->id() . '.');
    

    We should implement EntityControllerInterface and use that for injecting these services.

  4. +++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Controller/BreakpointGroupStorageController.php
    @@ -0,0 +1,76 @@
    +   * Remove breakpoint groups from all disabled themes or uninstalled modules.
    
    +++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Controller/BreakpointGroupStorageControllerInterface.php
    @@ -0,0 +1,26 @@
    +   * Remove breakpoints from all disabled themes or uninstalled modules.
    

    "Removes"

  5. +++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Controller/BreakpointGroupStorageController.php
    @@ -0,0 +1,76 @@
    +    $breakpoint_groups = $this->loadMultiple();
    +    foreach ($breakpoint_groups as $breakpoint_group) {
    +      if ($breakpoint_group->sourceType == $source_type && $breakpoint_group->source == $group_id) {
    +        $breakpoint_group->delete();
    +      }
    +    }
    

    There is a possibility we can rewrite this to use loadByProperties, but that should wait for the patch to pass first.

  6. +++ b/core/modules/breakpoint/lib/Drupal/breakpoint/Entity/BreakpointGroup.php
    @@ -18,6 +18,9 @@
    + *   controllers = {
    + *     "storage" = "Drupal\breakpoint\BreakpointGroupStorageController",
    + *   }
      *   entity_keys = {
      *     "id" = "id",
    

    You're missing a comma after the } the line before entity_keys, which is why this failed to install.

RainbowArray’s picture

Status: Needs work » Needs review
FileSize
9.47 KB
4.35 KB

I must have messed up the patch last night, as it didn't cleanly apply to 8.x. I did a reroll of my own work (shouldn't make patches at 2 a.m. I guess), and then made the changes above except for the third suggestion, as I wasn't sure how to do that.

Hopefully this moves things in the right direction.

We made this patch in a demo on contributing to d8 at the Twin Cities Drupal Open House.

Status: Needs review » Needs work

The last submitted patch, 7: breakpoint-group-storage-controller-7.patch, failed testing.

RainbowArray’s picture

Status: Needs work » Needs review
FileSize
9.41 KB
4.35 KB

I uploaded the wrong patch.

Status: Needs review » Needs work

The last submitted patch, 9: breakpoint-group-storage-controller-1915272-9.patch, failed testing.

RainbowArray’s picture

Status: Needs work » Needs review
FileSize
9.41 KB
667 bytes

Comma error caused the failure, I think.

Status: Needs review » Needs work

The last submitted patch, 11: breakpoint-group-storage-controller-11.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
11.59 KB
7.06 KB

Ahh, this will always cause problems:
You had Drupal\breakpoint\BreakpointGroupStorageController but core/modules/breakpoint/lib/Drupal/breakpoint/Controller/BreakpointGroupStorageController.php

Extra "Controller" in there.

I also fixed up the DI, we need the breakpoint entity type and storage controller.

Status: Needs review » Needs work

The last submitted patch, 13: breakpoint-1915272-13.patch, failed testing.

tim.plunkett’s picture

13: breakpoint-1915272-13.patch queued for re-testing.

tim.plunkett’s picture

Status: Needs work » Needs review
RainbowArray’s picture

Since this is passing tests, what's next? Do we need some manual testing that deleting breakpoints still works?

tim.plunkett’s picture

Hmmm, #2080823: Create API to discover config entities' soft dependencies and use this to present a confirm form on module uninstall completely removes every line from this patch. Maybe this is just a dupe of that one?

alexpott’s picture

Status: Needs review » Closed (duplicate)

I agree with #18 - this will become unnecessary once config entity dependencies are managed properly.