This patch has a lot of @todos and has never been actually run. I did php -l on it only.

Files: 
CommentFileSizeAuthor
#23 1649238-23.patch35.91 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 94 pass(es).
[ View ]
#21 1649238-21.patch35.53 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#17 1649238-16-exportablebase.patch16.88 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#16 ctools.config.16.patch21.45 KBsun
PASSED: [[SimpleTest]]: [MySQL] 24 pass(es).
[ View ]
#16 interdiff.txt18.2 KBsun
#14 1649238-13.patch10.96 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 24 pass(es).
[ View ]
#8 1649238-implement-config-exportable-controller.patch10.63 KBmerlinofchaos
PASSED: [[SimpleTest]]: [MySQL] 24 pass(es).
[ View ]
implement-config-system.patch11.33 KBmerlinofchaos
PASSED: [[SimpleTest]]: [MySQL] 24 pass(es).
[ View ]

Comments

merlinofchaos’s picture

For the record there's at least one major bug in the patch. :)

damiankloip’s picture

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();
}
damiankloip’s picture

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

merlinofchaos’s picture

I have to catch a plane soon so this is not a full response but I wanted to address:

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?

sun wanted to be able to leverage the config object, so actually storing it on the exportable and making it accessible is the best way. Almost everyone else involved thinks that the DatabaseController will actually go away and not return, though I still am hoping to keep the system abstract enough to make it possible anyway, but some people are saying that even the possibility of it existing is bad. We'll have to see. Anyhow, we should do our best to accommodate that one for now.

Your other abstraction ideas seem pretty reasonable. Anything moved to the Base should then be part of the interface.

damiankloip’s picture

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.

damiankloip’s picture

Issue tags:+VDC

.

merlinofchaos’s picture

Hmm. $data->get() might work. We need to retain the granularity so that each schema entry has its own get() call, but that probably isn't actually affected by that I don't *think*. So that is worth trying out.

merlinofchaos’s picture

StatusFileSize
new10.63 KB
PASSED: [[SimpleTest]]: [MySQL] 24 pass(es).
[ View ]

Updated patch after those methods moved.

damiankloip’s picture

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

damiankloip’s picture

Status:Needs review» Needs work
+++ 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).

damiankloip’s picture

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?

merlinofchaos’s picture

The config object replaces the array and because it's an object, it has to be maintained throughout its lifetime. So it's a bit more complex than just using an array.

That said, there is value to making sure that config object exists and is usable at a low level.

sun’s picture

Issue tags:+Configuration system

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.

damiankloip’s picture

Status:Needs review» Needs work
StatusFileSize
new10.96 KB
PASSED: [[SimpleTest]]: [MySQL] 24 pass(es).
[ View ]

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

damiankloip’s picture

Status:Needs work» Needs review
sun’s picture

Status:Needs work» Needs review
StatusFileSize
new18.2 KB
new21.45 KB
PASSED: [[SimpleTest]]: [MySQL] 24 pass(es).
[ View ]

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

damiankloip’s picture

Status:Needs review» Needs work
StatusFileSize
new16.88 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
+++ 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 :)

damiankloip’s picture

@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).

merlinofchaos’s picture

I think we need a discussion about whether or not it's worthwhile to keep the DatabaseController or not, and what that means. Keeping it will limit our design decisions, even if we keep it only in theory. i.e: In my view, if we 'keep' DatabaseController it will always live outside of core.

The pros: Flexibility and the ability for some things that exist in today's world to port forward more sanely. The biggest issue I have is that if it turns out CMI simply isn't suitable for something in today's world, it has a transition point. Without this, that thing might not be portable to D8 ever. The downside to this line of thinking is that I don't actually know of anything, for sure.

However, CMI is going to enhance performance problems that export.inc already has. This shows up in Page Manager in particular once you cross a threshold of a couple hundred pages. Because you can't fetch a list of pages that is paged (i.e, only fetch 25 or so at a time) you have an increasing memory problem whenever you have to fetch all of the page manager pages. This is something that can be avoided with Database stuff.

That said, we can also fix this problem with database synchronization, where the database is basically considered a cache. However, this is a hard problem to solve completely.

merlinofchaos’s picture

I think the best path forward for now:

Let's do our best to keep DatabaseController a possibility until such time as keeping that possible impacts the design too negatively. One thought I had is to have a base interface and a Config specific interface; the base interface makes no assumptions about what the data will be, and the Config specific interface assumes a $config object. THat would allow Database to then do the same thing, out in contrib.

The primary reason I want to keep this is to hedge our best. export.inc is used in a lot of places and I would like the upgrade path to be possible.

That said, what I really want to do is encourage everything to move to Config and use database synchronization. To that end, we need to ensure that db sync is solid. See http://drupal.org/node/1670370#comment-6221342 for the current work on that.

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new35.53 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 1649238-21.patch, failed testing.

damiankloip’s picture

StatusFileSize
new35.91 KB
PASSED: [[SimpleTest]]: [MySQL] 94 pass(es).
[ View ]

This patch removes some files too.

damiankloip’s picture

Status:Needs work» Needs review
effulgentsia’s picture

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?

moshe weitzman’s picture

tim.plunkett’s picture

Status:Needs review» Closed (duplicate)

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