Implement CMI Exportable Controller

Just looking at the patch, a few ideas to kick around:

• Should we move load, loadMultiple, and loadAll methods into the ControllerBase class? They are just wrapper/convenience methods anyway. loadExportables et al is all that needs to be overridden on extending classes
• Agree with your comment that we should move the getSchema method to the base class
• Maybe we should look at refactoring how exportables are loaded and cached, then we could move the static caching to the ControllerBase class and let extending classes controll the loading?
• Unless there is a reason I have missed; I think unpack should always be passed an array of data. Could we just do a $config->get() and pass everything in? or is that not good/has issues?
• Sounds like the issue regarding not being able to get the name of the config object is big/urgent issue :)

Was this the bug you spotted?

public function delete(array $keys) {
  $exportable->getData()->save();
}

... and, so that I hopefully don't forget; on both Config and Database Controllers we are setting the type in the constructor, but this is already being set in the ControllerBase constructor.

Ok, I kind of guessed we need to keep the config object. That makes sense. I think what I was think was still storing the config object in $this->data but when instead of passing just the config object to unpack, it makes more sense to me to do this in the exportable constructor:

$this->unpack($data->get())

Then unpack can always be an array and we keep the config object too. Wouldn't it be more efficient to just get everything at one rather than call get() for each schema field too? Sorry if I'm exercising CMI ignorance :)

I can look at putting the abstractions in separate patches? Would be good to break things up a bit.

.

Is it just me, or does it seem crazy that you can't get the actual config name from the object you have loaded?

+++ b/lib/Drupal/ctools/ConfigExportableController.phpundefined
@@ -8,8 +8,226 @@
+      if (isset($data[$field])) {
+        $config->set($field, $data[$field]);
+      }

This will only set the field on the config object if the value is not NULL. Is this what we want? If we have NULL values from the schema fields, don't we still want those stored on the config object? currently isset() will not be true for nulls, sorry if this is the intended.

+++ b/lib/Drupal/ctools/ConfigExportableController.phpundefined
@@ -8,8 +8,226 @@
+    foreach ($this->schema['fields'] as $field => $field_info) {
+      $exportable->$field = $config->get($field);

I know it would be ideal to have pack/unpack to be mirrors of each other but it seems like it would be a good idea to just pass an array (using ->get()on the config object) and use the existing unpack method. I think what I am trying to get at is; won't it be faster to just get all the config values once, rather than calling get() on each value? Stuff will be loaded alot more than it will be saved. So in the Exportable constructor we would do $this->unpack($data->get()) instead of just $this->unpack($data).

(I know I already mentioned this but thought it might be good to get it written in the issue).

Also, the pack method is setting the values on the config object. and not just returning an array. Is this the route we are taking now? Pack isn't just to extract the values anymore. I thought this would be something done more when saving is done rather than packing? (Not saying this is wrong at all, just clarifying :) )

We probably need to give setStatus a bit of attention too, I guess this will just be handled with the $disabled reserved key on the exportable being set on the config object?

On the more simple things - this patch seems to predate the config rearchitecture patch (which introduced ->getName() and ->setName(), which is one of the @todos in that patch), and also the addition of ->isNew() (eliminating !empty() checks on ->get()).

I'm also not sure whether the additional caching mechanism in $this->cache is needed. We have a general issue for checking/improving the config system performance with #1605124: Configuration system performance, which is currently postponed on the idea of introducing a cache backend/plugin that supports LRU logic; i.e., statically caching only a limited amount of config objects, which are re-requested within a single request. In general though, we slightly postponed this topic in order to focus on the bare architecture first - at this point, I wouldn't recommend to implement a custom caching behavior here.

Lastly, I'd love if we could get these changes here aligned with #1668820: Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc) as much as possible, in order to at least ensure a common/shared direction. Ideally, we'd combine the two approaches entirely, of course, although I believe that this won't be really possible until the config system supports all of the additional feature-set for handling enabled/disabled/overridden configuration. That said, I might be mistaken with that.

Here is a slightly updated patch, based on the patch from #8.

Main changes of note:

• Changed $address property to $prefix
• Changed the delete() method on the ConfigContoller to work with keys, as that is what we currently have in the interface.
• Added reserved keys to unpack() method and made $disabled property public
• Added a simplified setStatus() method (Not sure if this is right entirely, but I think this might be the direction we need to go in). Also removed some of the status logic from the Exportable constructor and let unpack handle this for now.

I haven't used the getName/setName() methods yet anywhere, It would be good to discuss the naming of views with the prefix and what is stored as the id etc...

I was also thinking (as were other people) that the config system would handle caching. So when this can be removed we should do it.

It would be good to fall in line with the Configurable Thingies stuff, In my mind this should wait a bit though, It would be good to get some momentum moving this stuff to using the config system. This could be changed later if needed? At the moment Our implementation is quite different. I definitely like the idea though :)

Trying to move this forward...

Attached patch makes ExportableCrudTest work - both for DatabaseExportableController and ConfigExportableController, although both only get to line 134, at which the test tries to call ->get() on a Exportable object that is supposed to live in the database, but for some reason does not. Since both controllers are failing on the same line, I stopped there.

However, the most important part of this patch is rather this comment I left in the code of unpack():

    // @todo As long as DatabaseExportableController exists,
    //   Exportable::$data has to be an array. The current code looks like there
    //   is an indecision on whether DatabaseExportableController or alternative
    //   implementations are still going to be needed in the future. In any case,
    //   as long as that concept exists, the Exportable class cannot decide on
    //   the data format.
    //   If that remains to be the case, then all invocations of config() should
    //   be removed from unpack() and pack(), $config changed back to $data, and
    //   config() and Drupal\Core\Config\Config should only be involved in
    //   load(), save(), and delete().
    //   In the opposite case, DatabaseExportableController should be deleted.

HTH

+++ b/lib/Drupal/ctools/ConfigExportableController.phpundefined
@@ -8,8 +8,346 @@
+    $this->schema = drupal_get_schema($this->info['schema']);

I think it was correct before, but $info['schema'] would be an array of the fields that the exportable packs/unpacks etc... so no call to drupal_get_schema. Atleast that's how I saw it working for config.

+++ b/lib/Drupal/ctools/ConfigExportableController.phpundefined
@@ -8,8 +8,346 @@
+    $defaults = $this->getDefaultExportables($args);

We don't want to do this on the config implementation do we? So this was intentionally left out. Wont the config system manage the rest for us? or are you suggesting reading the files directly?

It's really not up to me, but I would personally like to see the implementation stay abstracted enough so we can keep the database controller too. I also mentioned some stuff in #10 of this post about the pack/unpack data, that might be applicable. Not sure if we can.

I was playing with the idea of going back to an ExportableBase class and extend this as we need. Here is a patch I created yesterday, It's just a rough idea. So please don't take it overly serious :)

@sun, also I think if we do keep both controllers, refactoring the tests to have a base we should definitely do.

edit: also, with the get/set methods, you think move to storing everything in $data. Does this still store the config object too? (sorry don't have time to look properly).

Here is a new version, I have spent quite alot of time on this today, so now the majority of the tests are working - we have config and database test classes. I have added some default test config exportables and exportable info to accommodate these.

This still uses the ExportableBase class as I think this may be the best way forward on this.

I'm sure there is still work to do, and some stuff here may need tidying (certainly reviewing).

This patch removes some files too.

What do you folks think of #1668820-53: Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc) and the comments that follow? I think that issue has taken a direction (and quite possibly some code) very in line with this one, but maybe there's something I'm missing? I think the main thing being proposed there that hasn't been part of the scope of this issue is to make Exportables/Configurables/WhateverOtherNameWeComeUpWith literally be entities with entity storage controllers rather than entity-like things with entity-like storage controllers. Is there any reason why that would present a problem for Views / CTools or is otherwise a bad idea?

#1668820: Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc) was committed. hopefully we are unblocked here?

This is a duplicate of #1760188: Improve Wizard method default_display_options which was committed! Yay.