Problem

  • Configurable things that can be managed through modules will be converted to configuration objects, but there is no consistent pattern for new configurable module API thingies yet.

Goal

  • Design a concept, (abstract) base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc).

Terminology

Configuration object
Means a bare, low-level Config object, as returned by config('foo.bar'). Config objects do not invoke module hooks at any point in time; as such, they behave identically to variable_get() and variable_set().
Configurable
Means a higher-level data object/model/bundle, which is usually based on a class. Examples:

...and many more.

Each configuration thingie belongs to a dedicated module API. E.g., the Taxonomy Vocabulary API, the Node Type API, the Contact Category API, the Image Style API, etc. Any code that interacts with a configuration thingie, interacts with the respective API instead of the bare configuration object.

Details

  • Each module that allows to "manage" a certain type of configuration is acting on configuration objects, but encloses them in a higher-level module API.
  • The module API most notably involves the invocation of load/presave/insert/update/predelete/delete hooks to allow other modules to react to the operation.
  • The module API might also contain further adjustments and business logic — e.g., instantiation of configured plugins for the configuration thingie, so that it is fully operational.
  • However, looking at module APIs of configuration thingies throughout Drupal core, there is a common pattern of straightforward business logic that repeats itself across the board.
  • Therefore, the goal is to design an interface and base implementation that works for the >90% use-case of configuration thingies (thus simplifying their implementation), but at the same time, leaving the option for contributed modules to implement something more advanced.
  • Common denominator:
    • ID (machine name)
    • Label (for the UI)
    • ->get(), ->set(), ->save(), ->delete()
    • ->isNew(), ->original (for handling changes of dependent data [e.g., fields, entities] in updates)

Notes

  • Configuration thingies are NOT part of the configuration system.

Relates issues

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

Status: Active » Needs review
Issue tags: +Needs architectural review
FileSize
24.87 KB

Here is the proposed abstract ConfigThingieBase class and ConfigThingieInterface.

The base class is extended by the configurable ConfigTest thingie of config_test.module, which is visible at the end of the patch.

I'd be thankful if you only comment on the ConfigThingie name, if you spent at least 5 minutes thinking about the naming problem and you have a real/concrete naming suggestion. Thanks ;)

This patch leverages changes of the following issues, so it cannot be tested yet:
#1666632: Add Config::isNew() to allow code to determine whether a config object already exists
#1609760: hook_image_style_*() is not invoked for image styles upon Image module installation

Anyway, architectural design reviews are highly appreciated!

This issue blocks a full range of conversion issues, so we should try to come to a conclusion ASAP. Due to that, we should also try hard to defer/postpone any advanced topics that can be added separately to follow-up issues.

sun’s picture

Issue summary: View changes

Updated issue summary.

fgm’s picture

Re: abstract vs interface. Yes, it allows specifying implementations that take an implementation of the interface as a parameter without limiting actual parameters to inherit from the abstract base class, because all that really matters is the interface itself (if it quacks like a duck...).

kim.pepper’s picture

Having everything inherit from a single ConfigThingy base class is code smell IMHO. It fails the basic "X is a Y" versus "X has a Y" semantic test. e.g. "Vocabulary has a configuration" versus "Voculabulary is a configurable thing". It's a classic inheritance versus composition design pattern showdown. I believe Inheritance should be reserved for more type-based classifications (e.g. chair is a kind of furniture).

Kim

sun’s picture

FileSize
4.15 KB
25.08 KB

Removed base class specific methods from ConfigThingieInterface.

  1. The intention is to have a common interface for all configurable thingies.
  2. Modules may base their configurable thingies on the ConfigThingieBase base implementation, in which case they just extend the abstract base class. (as demonstrated for the ConfigTest thingie in this patch)
  3. There may be modules, for which the base class is not suitable. These modules can just implement the interface. That is also the reason for why __construct() is not part of the interface.

@kim.pepper: A vocabulary will be configuration, wrapped in a configurable thingie class in D8. I.e., a vocabulary does not have configuration, but "a vocabulary is a configurable thing" is in fact correct.

sun’s picture

Issue summary: View changes

Updated issue summary.

fago’s picture

on configuration entities:

Even though the intention is to deliver a DX that is mostly consistent with entities (but not identical), calling them "configuration entity" would be blatantly wrong. Suggestions highly welcome!

As already discussed over at http://drupal.org/node/1552396#comment-6050216, I still fail to see why "configuration entities" would be blatantly wrong. I mean, conceptually it are entities (as entities just means thingies) and implementation wise it would make a lot of sense to me as we could re-use all of the existing CRUD interface and code that we need for all that configuration objects anyway.

So, if you want to discuss/explore that option further, I'd be happy to. But don't worry I won't start derailing this issue by fighting for that, although I think it's a direction that's worth exploring. Also, I'd appreciate a solution like that as I think another entity storage controller implementation in core (=config) that doesn't implement lots of optional entity stuff could help a lot on making sure the entity api works well for integration any kind of data objects.

sun’s picture

@fago: Personally, I did not ditch that possible direction yet, and am interested in investigating it. However, it involves a lot more factors (dependencies, unavailable methods, etc), which won't be easy to resolve and really require their own in-depth discussion.

For now, the plan of attack is to keep things moving in the right direction, so we can unblock the huge stack of conversions to configurable thingies that are required to happen. Thus, keeping the ConfigThingieInterface in line with Entity API not only happens for excellent DX, but also for keeping that door open. :)

As soon as this patch is RTBC/committed, I'll resurrect the work on #1552396: Convert vocabularies into configuration - which will kinda serve as real world example and template for how to convert all these things.

effulgentsia’s picture

Issue tags: +VDC

Conceptually, I like this a lot.

For naming, would it make sense to eventually move Drupal\Core\Config to Drupal\Component\Config? We'd need to remove the dependencies on drupal_*() functions, like drupal_array_get_nested_value(), and we'd also need to move Drupal\Core\Database to Drupal\Component\Database, but I think that's desired anyway. If we did this, then would it make sense for ConfigThingie to be Drupal\Core\Config? In other words, Drupal\Component\Config becomes a low-level configuration object while Drupal\Core\Config becomes a high-level config object designed for Drupal usage. To me, this would make sense if we plan to drop direct usage of the low-level object, and have all of core (except the internals of the config system itself) use the high-level object always.

Would be great to get this in ahead of the various code sprints happening at the Midwest Drupal Summit, including the ones for Views, so adding the VDC tag. I'm hoping that merlinofchaos or another Views expert can review this with an eye towards that.

David Strauss’s picture

I'm looking over CMI-related issues while @heyrocker is in town. I'm all for adding utility interfaces and base classes (or even mixins!) that demonstrate best practices, shorten development time, and gently encourage more consistency. This appears to be exactly that, so +1. Not sure this is "critical," though.

effulgentsia’s picture

Category: feature » task
Priority: Critical » Major

Not sure this is "critical," though.

I don't even know if there's a functional difference between a major and critical feature request. Changing this to a major task, because it's not so much a feature request in itself, but rather, getting it done soon will speed up and improve the conversion of Views and lots of things in core to the config system.

sun’s picture

#5: config.thingie.5.patch queued for re-testing.

sun’s picture

Issue summary: View changes

Updated issue summary.

sun’s picture

@effulgentsia: Took me some nights to think through your proposal. Here's what I think:

  1. Removing dependencies on procedural Drupal functions from Config and moving it into Drupal\Component\Config would probably a good idea either way. So +1 on that. I don't see Database as a potential blocker for that, since it merely means that DatabaseStorage would stay where it is.
  2. However, the idea of having both Drupal\Component\Config and Drupal\Core\Config makes me feel uncomfortable. Here's why:
    • DX: Even in the sentence above, I was tempted to clarify the second Config with "(for configurable thingies)" but did not, in order to give a nice example that hopefully clarifies the confusion this would cause. ;)
    • Configuration forms for simple settings will still use config() and thus Config directly. (which one? :))
    • The ambiguity would be misleading from a technical perspective, too. As already noted in the issue summary:

      Configuration thingies are NOT part of the configuration system.

      Configurable thingies are really much more comparable to entities. They are not configuration themselves. Like entities in the entity concept, they are just an attempt to standardize (and thus simplify) the business logic and management of configuration that can appear multiple times, whereas each incarnation follows a defined schema and behavior.

      If you will, they are variants/derivatives of a certain plugin type. ;) [but erm... if desired, let's pleeease discuss that joke in a separate follow-up issue :P]

      However, the essence is that a ConfigThingie just happens to wrap a Config object. If we'd decide that this was a bad idea and Config shouldn't and wouldn't be involved at all, then it would still be a ConfigThingie.

      Config just happens to be involved, but is definitely not the driving factor. Alas, as with the introduction of entities in D7 (which wasn't really planned in the first place, just a very late aftermath to fields in core), we could have introduced this standardized ConfigThingie concept years ago already. ;) If we had, then it would use database tables or perhaps even variables under the hood today.

      The only difference to today's proposal is that we want to convert everything that's not an entity (and not state) into the configuration system for D8, so as to have a clear & consistent differentiation between content vs. configuration vs. state.

But thanks a lot for the suggestion! :)

Meanwhile, I'd almost prefer to go ahead with this patch as is. That would be a huge benefit, as it means we can start convert a whole lot of stuff (since time is slowly but certainly running out). There's nothing easier than renaming a class with git.

cosmicdreams’s picture

Surely a better name than ConfigThingie can be conjured. How about:

  • EntityConfigHelper
  • ConfigHelper
  • ConfigAdapter
  • NodeConfigHelper
  • VocabConfigHelper
  • XConfigHelper (where X is the entity type)
sun’s picture

Out of those choices, I'd prefer ConfigEntity ;)

dixon_’s picture

I think I like what @fago is suggesting, i.e. making use of the Entity API and implement Drupal\Core\Config as an entity storage controller instead.

That said, things are still changing a lot for the Entity API and if we find this to be too much of a drastical change, I won't derail this issue either.
The patch in #5 is definitely a step in the right direction and a very good improvement. Also it would make a move to Entity API easier later on, if it turns out to be a good idea.

dixon_’s picture

As for the name, how about ConfigWrapperInterface and ConfigWrapperBase?

Implementations would be something like NodeTypeConfigWrapper. To me this naming is really straigh forward, and conveys what it does quite well.

dixon_’s picture

Status: Needs review » Needs work

To motivate my naming suggestion a bit more...

However, the essence is that a ConfigThingie just happens to wrap a Config object. If we'd decide that this was a bad idea and Config shouldn't and wouldn't be involved at all, then it would still be a ConfigThingie.

+++ b/core/lib/Drupal/Core/ConfigThingie/ConfigThingieInterface.php
@@ -0,0 +1,117 @@
+  /**
+   * Sets the configuration object for this configuration thingie.
+   *
+   * @param Drupal\Core\Config\Config $config
+   *   The configuration object containing the data for this configuration
+   *   thingie.
+   *
+   * @return Drupal\Core\ConfigThingie\ConfigThingieInterface
+   *   This configuration thingie (suitable for chained method calls).
+   */
+  public function setConfig(Config $config);

This sort of assumes that we will be wrapping config objects, doesn't it?

dixon_’s picture

Status: Needs work » Needs review

Didn't intend to change the status...

damiankloip’s picture

I read this issue yesterday and had a think about it last night, The solution I came up with was ConfigWrapper, but when I came on here it's already been suggested :)

Basically, I agree with dixon_.

sun’s picture

FileSize
3.02 KB
24.76 KB
  1. Sorry, #17 obviously revealed a bug in the interface. ;) Thanks, @dixon_!

    dd9b369 Fixed Config/base class implementation specific methods appear on interface.

  2. Regarding Entity API integration, see #7 - we can investigate that topic after introducing this initial architecture, but it should not be a prerequisite.

    I'm happy to do so (and I'm also happy to explore a potential integration with the new Plugin system), but the super massive follow-up work after this patch lands will actually be to convert existing thingies to the new pattern/standard. We really need to work on the actual conversions ASAP, since all of them will technically be API changes. Once that is in place, possibly moving from there will be a lot easier.

  3. On the actual implementation names, I was actually planning to use simple and specific custom names like we do for entity types in D8; e.g., ImageStyle, NodeType (or ContentType), FilterFormat (or TextFormat), View, etc. — i.e., not using any "ConfigThingie" (or whichever) suffix.
cosmicdreams’s picture

I'm eager to help with #20.2

effulgentsia’s picture

I think ConfigWrapper and the suggestions in #13 are too implementation-centric. It would be like calling Entity an EntityStorageControllerWrapper, or if we still kept entities tied to database storage, a DatabaseTableWrapper. What's important about ConfigThingie isn't that it uses the Config system for storage, but that it represents an abstract object: whatever it is that image styles, node types, and views have in common, just the same way that Entity represents whatever it is that nodes, comments, and users have in common.

Even if we manage to get done all of the things we want to in EntityAPI so that it will be technically possible to make Entity a base class for image styles, node types, views, nodes, comments, and users, I think we're still going to want some term for classifying image styles, node types, and views in one set, and nodes, comments, and users in another set. Where the former represents objects that are "configuration like" (primarily intended for developing on a dev site and pushing to prod), and the latter represents objects that are "content like" (primarily expected to be volatile on prod). I say "primarily", because of course, there's lots of small sites that update configuration on prod, and lots of big sites that deploy content from staging to prod, but still, I think there's some important conceptual difference between configuration and content, even if we're still grappling with how to express that difference and with some gray areas.

At least for now, for better or worse, Entity is our term for the content-like objects. So until we come up with another term for those, I'm reluctant to rename ConfigThingie to ConfigEntity.

So, having no better suggestion, I'm okay with ConfigThing or ConfigThingie as a placeholder, and a follow up for coming up with alternate terminology.

I can't tell from the comments here if anyone's reviewed the actual code. If you did and found nothing that needs work (other than terminology concerns), please say so. Thanks. I'll also try to review this soon.

sun’s picture

+1000 for #22 - thanks @effulgentsia! That basically just explains what I tried to say in #12.2.3 in a better way :)

webchick’s picture

Sorry but we really, really can't use ConfigThing or ConfigThingie. We need to commit this with actual documentation describing what it does, and we can't punt on this important requirement to later.

One thought is just the word "Configurable" (as a noun) like we have "Exportable." It's, granted, a little weird.

But that would turn it into:

+abstract class ConfigurableBase implements ConfigurableInterface {

And of course "whatever it is that image styles, node types, and views have in common" is that they are configurables.

Note: This is probably a horrible idea. But I agree with Alex we need to get out of using the class names to describe the implementation and more into getting the class name to describe what it does. Also agreed that "Entity" is out because it's over-loaded to mean something else already.

Dries’s picture

Status: Needs review » Needs work

I think it is a reasonable idea, but I think it has limited re-usability. There will be various "configuration objects", but I don't believe all of these will or have to be ConfigThingies. I tried to explain this below:

+++ b/core/lib/Drupal/Core/ConfigThingie/ConfigThingieBase.phpundefined
@@ -0,0 +1,232 @@
+  /**
+   * Implements Drupal\Core\ConfigThingie\ConfigThingieInterface::get().
+   */
+  public function get($property_name) {
+    return $this->config->get($property_name);
+  }
+
+  /**
+   * Implements Drupal\Core\ConfigThingie\ConfigThingieInterface::set().
+   */
+  public function set($property_name, $value) {
+    return $this->config->set($property_name, $value);

I don't want my classes that implement ConfigThingie to use a generic set and get function. Instead of using set('foo', 'bar'), it is almost always better to do setFoo('bar'). In other words, the ConfigThingie promotes bad design. Maybe these should be protected functions instead.

+++ b/core/lib/Drupal/Core/ConfigThingie/ConfigThingieBase.phpundefined
@@ -0,0 +1,232 @@
+  /**
+   * Implements Drupal\Core\ConfigThingie\ConfigThingieInterface::delete().
+   */
+  public function delete() {
+    // Allow modules to react prior to deleting the configuration.
+    module_invoke_all($this->getEventBasename() . '_predelete', $this);
+
+    // Delete the configuration object.
+    $this->config->delete();
+
+    // Allow modules to react after deleting the configuration.
+    module_invoke_all($this->getEventBasename() . '_delete', $this);

More importantly, I'm not sure it makes sense to invoke '_predelete' and '_delete' hooks at this level. If a Block class derived from ConfigThingie overwrites the delete() function, Block.delete() most likely wants to do some of its own clean-up before calling ConfigThingie.delete() (e.g. remove something from the database), and call its own '_predelete' hook before it does so. Hence, we'd be calling two '_predelete' hooks which would be bad.

The same is true for save(). I don't want to wrap any of the save related hooks around the saving of the config object only; I want to wrap it around all my saving logic. Hence, I think the notion of a ConfigThingie _may_ be flawed. Or at least, it may be flawed as something that we plan to re-usable often.

Because this isn't blocking any work, I don't think it is 'major', but I'll leave it at 'major' for now.

Gábor Hojtsy’s picture

Because this isn't blocking any work, I don't think it is 'major', but I'll leave it at 'major' for now.

Just responding to this, the main goal of this patch is to have a standard for "item based config" like vocabularies, contact forms, aggregator sources, etc, and this would be a pre-requisite to convert those, so maybe even higher than a major? :) It would be great to be able to move those forward as well.

effulgentsia’s picture

the main goal of this patch is to have a standard for "item based config" like vocabularies, contact forms, aggregator sources, etc, and this would be a pre-requisite to convert those

Given some of the feedback in #25, would it make sense to move forward on some of those items without the base class and interface, and then, when we have real code for a couple specific use cases, then make the case for this issue? At which point, it should be easier to address the API questions posed in #25.

sun’s picture

We already have experience with the approach of not defining patterns/standards upfront.

This issue is already the conclusion of various other issues that attempted to convert stuff into configurable thingies.

Furthermore, the underlying idea is to take the unique chance in Drupal's history to standardize these things. This idea apparently is in line with the original goals for D8: cleanly separating between content (entities) and configuration. In essence, D8 developers should be able to say: "Either I'm dealing with an EntityInterface, or I'm dealing with a ConfigThingieInterface."

That said, the entire idea behind ConfigThingieInterface is to keep it as identical as possible to EntityInterface. D8's current EntityInterface and upcoming Property API actively introduces and uses ->get() and ->set() methods.

That is why I object to @Dries' first review issue. In general, it appears to me that setFoo('bar')-style setters and getters are OO purisms that primarily exist in the Java world, but not really in the PHP world (and once you've seen a full-blown class of such setters + getters in Java, you seriously ask yourself why these 300 lines of code are necessary). In any case, the goal is not to introduce something completely different; the goal is consistency.

@Dries makes a very good point in his second issue though, and I will try to address that.

tim.plunkett’s picture

FileSize
21.4 KB
28.61 KB

I liked webchick's suggestion of "Configurable" enough to go ahead and switch everything around. It make sense to me, and makes the code seem more real by not saying "thingie".

Leaving at needs work for whatever sun is cooking up, this was just a straightforward renaming.

Jose Reyero’s picture

Though I like the idea of standardizing somehow the way config objects are built, I fully agree with @Dries concerns, so I think needing to extend some base config class is not a good idea.

I think we should focus on the process on 'how config objects are built / managed' and at most implement some interface, which will let you extend any kind of object easily to be 'configurable', that is to be handled in some standard way through the config system. How this may look like:

interface ConfigurableObject {
   function getConfigType();  // 'image.style', 'node.type', etc...
   function getConfigName();
   function getConfigObject();
   static function buildFromConfig();
}

Then we could have some generic builder for configuration objects that may look like:

/**
 * @returns ConfigurableObject
 */
function config_get_object($name, $class) {
   $config = config($name);
   return call_user_func(array($class, 'buildFromConfig'), $config);
}

function config_save_object(ConfigurableObject $object) {
   $object->getConfigObject()->save();
   // Invoke hooks here, example of hook
   $type = $object->getConfigType();
   $type = str_replace('.', '_', $type);
   module_invoke_all('config_object_' . $type . '_save', $object);
   // This will invoke hook_config_node_type_save(), hook_config_image_style_save(), etc....
}

And if a Drupal object that is configurable is passed around you can always map it to the config object by using the interface methods like $object->getConfigType() ('node.type'), $object->getConfigName() ('node.type.blog').

Still while we are deciding how the interface looks like, it won't do any harm to move our Stdclass objects and arrays to named classes like 'NodeType', 'ImageStyle', etc... so they will be known classes that can later implement that interface.

Because we are needing badly to move 'old objects / arrays' into the config system so we can work out on localization or other features, and also because this will eventually impact most of Drupal core, that works with configurable data all the time, I think it makes sense to keep this as 'major'

Gábor Hojtsy’s picture

BTW I assume this concept also blocks the CMI conversion of menus and menu items as well, right? No? :)

merlinofchaos’s picture

This is still an implementation without a separate controller object (as with entity), which leads to the following problems:

1) There's no generic load routines. In order to load you first load a config object and then plug that into a configurable. This will end up causing a significant amount of duplicated code; plus there's no load multiples, etc.

2) This exposes the storage mechanism too much. When dealing with a view, for example, nobody dealing with that view is going to be interested in the underlying config object.

3) Since the config object is in the constructor, creating a new one requires creating a new config first. I'm not sure that's the right workflow for creating a new Configurable.

sun’s picture

Issue tags: +API addition

Thanks for the feedback! Highly appreciated.

1) There's an interface and a base class (the latter suitable for the 90% using Config under the hood).
2) If a view/etc wants a different storage mechanism for whatever reason, it can implement the interface, not extending the base class.
3) The constructor was intentionally left out of the interface, since you already stated that before. ;)

Anyway. :)

i) Taking the entirety of feedback into account,
ii) Sleeping over all of this for some more nights, and
iii) Taking #1609418: Configuration objects are not unique into account, and lastly,
iv) Seeing the potential of #1498270-2: Meta data for configuration and #1346214: [meta] Unified Entity Field API

I'm actively in the process of wrapping my head around the fundamentally different alternative of rebasing Configurables onto the entity system. That is, because the entirety of the architecture that is being asked for is provided by the entity system already. The only part that actually differs is the final storage engine and storage controller that takes over the CRUD operations for Configurables (compared to "regular entities" [whatever that is]).

This is only underlined by the fact that all of the Configurables live in database tables today already. Furthermore, the entity system in D8 even aims to support entities that are stored remotely (possibly even read-only). In the end, Configurables really just have a different/custom storage engine/controller under the hood. The only more obvious challenge is to get the string IDs of Configurables in line and supported in the entity system, which might require different code paths here and there (not verified though).

Jose Reyero’s picture

The problem is we may really need to expose the underlying config object to modules. Let's imagine Comment module which wants to get / set related content type settings.

The Comment module needs to either be able to grab the full config object or to use some standard NodeType object's method to access comment options related to a content type.

So we could use the full config object

$node_type->getConfigObject()->get('comment.default_mode');

Or we could use some generic getter/setter methods (the same we need to read, we may need to write properties)

$node_type->getConfigProperty('comment.default_mode');

In any case we need some method for modules to be able to stick and read later their config properties into bigger objects that are read from configuration thus Drupal Objects created out of config need to be extendable somehow.

sun’s picture

Category: task » feature
peterx’s picture

#25 and #28. Set methods are good because you can validate the settings instead of letting people put unusable settings direct into attributes. How will configuration handle a setting that is not usable and what happens when we extend? What happens when my module Two extends your module One and a user enters a setting rejected by module One?

The Entity Property API mentions validation but not errors or feedback of error information to the user.

fgm’s picture

@peterx : breakage ensues. One of the tenets of drupal has been 'we don't babysit broken code', so if application 2 injects improper data into application 1, we do not expect any kind of predictable result, to limit the volume and complexity of running code.

merlinofchaos’s picture

Unfortunately I think we've grown past the point where "We don't babysit broken code" is a useful tenet. When we can validate without adding a burden to the system, we absolutely should. And when we fail, we should fail meaningfully, IMO.

fgm’s picture

@merlinofchaos : this is a very significant statement you are making here. Not that I necessarily object to it, but it has been a recurring objection against most patch suggestions looking toward improved fault detection and resilience, for years.

So if we are changing our policy regarding this point - and the major inflections in core development for D8 would tend to support this - it this needs to be widely announced, not just mentioned in this issue, but at least in the "core announcements" group, I think.

tim.plunkett’s picture

Throwing typed exceptions is not babysitting broken code. It's responsible coding.

peterx’s picture

The use case I am thinking is for settings where a module is extending classes and trying to set something and you would expect some validation. If A accepts an integer then B extends A and requires an upper limit of 99 then C extends B and sets a lower limit of 1 them my module tries to set a value of 150, what happens?

Or are we drawing a line and not letting non core extend core?

xjm’s picture

Category: feature » task

Moving back to task because this is a critical blocker for VDC.

damiankloip’s picture

What about #1649238: Implement CMI Exportable Controller too? I think that may be currently closer to what we need.

effulgentsia’s picture

Category: task » feature

The Comment module needs to either be able to grab the full config object or to use some standard NodeType object's method to access comment options related to a content type...thus Drupal Objects created out of config need to be extendable somehow.

Is this true? Should the Article node type's comment settings be in the 'node.type.article' config object? Or in a separate 'comment.SOMETHING.article' config object? And if the latter, do we expect a $node_type Configurable object to provide unified access to the information in both the underlying 'node.type.article' config object as well as the underlying 'comment.SOMETHING.article' config object and whatever other per-module config objects exist that provide additional settings to node types?

I don't know if we want to go down the road of a concept for a Configurable being composed of multiple config objects, but if we do, I don't think either #29 or #1649238: Implement CMI Exportable Controller support that. #33's suggestion of making Configurable a special kind of Entity might support that though, and that might not be that big a stretch from the direction #1649238: Implement CMI Exportable Controller is heading in.

If we don't want to go down the road of a Configurable being composed of multiple config objects, then how do we deal with the Comment module extending Node Type settings use case?

effulgentsia’s picture

Category: feature » task
sun’s picture

Status: Needs work » Needs review
FileSize
39.12 KB

Introducing: the Configurable entity.

This makes much more sense.

Especially from a developer perspective, the 100% parity with entities is very appealing.

And from an architectural perspective, it's one custom thing less to maintain.

AFAICS, it resolves all review/feedback issues that came up so far. It also seems to be pretty much in line with the work that happened in CTools — obviously with a couple of differences, since there is no "overridden" concept in the configuration system yet; which I don't want to make part of this issue/patch, because that stuff really requires its own in-depth discussion.

What do you think? :)

Status: Needs review » Needs work

The last submitted patch, config.entity.46.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
2.07 KB
40.95 KB

00a8328 Fixed NodeStorageController is futzing with $entity->isCurrentRevision property directly.

Status: Needs review » Needs work

The last submitted patch, config.entity.48.patch, failed testing.

Jose Reyero’s picture

I think simple is beautiful. (So this is ugly).

Then about 'babisitting broken code' / validate everything, I don't think that is the point. I think Drupal is big because your module does its thing, and it lets my module do whatever so basically 'you care about your data, do not fuck (aka validate) mine, thanks'.

So if another module wants / needs to stick some more data into some configuration, that should be ok, we just try to stop users from doing stupid things. But we should allow developers to do stupid things.

sun’s picture

Well. Yes, this adds flexibility (and complexity), but I'm definitely not the one who asked for that.

However, this has one huge benefit from a macro-level perspective: There's only one system you have to learn.

Furthermore, all Configurables directly benefit from each and every entity system improvement that's going to happen; be it the Property API, be it entity-level validation, encapsulated entity form controllers, etc.pp. — there's a lot.

In the end, we'd essentially have to re-invent all of these things for Configurables, just because they're not using the entity system, although they could - as this patch proves.

The patch actually passed; looks like I overlooked a stray method call that only exists in the config-next branch. Will fix that in the next re-roll.

fgm’s picture

@Jose Reyero: nicely put re. babysitting vs validation

Something else: Configurable, being an adjective, should be an interface name, not part of the name of a class. Not important regarding the concept, but makes for some rather weird names : Configurable(Interface|Base).

sun’s picture

Status: Needs work » Needs review
FileSize
6.63 KB
40.82 KB

0c0a9e0 Fixed handling of Configurable renames, and save() doesn't return SAVED_NEW/_UPDATED.
8e8f12a Added Helper callback for uasort() to sort Configurable entities by weight and label.
de1f470 Fixed config_test entity form does not follow standard for entity forms.
de11924 Removed irrelevant Config\StorageInterface::exists() test assertion change.

sun’s picture

gdd’s picture

As a developer I do not find the 100% parity with entities appealing. What I find appealing is a simple system that does only what it needs to and nothing more. I do not find consistency across subsystems a benefit if it means that systems that could be simple are made complex, and if it means that we are trying to shoe-horn ideas into things that weren't meant to support them.

I basically completely reject this approach.

sun’s picture

Hum. I suspect I failed in communication...?

Can we clarify what exactly is (technically/architecturally) problematic with this approach/patch? In order to move forward, I need to know and understand any concerns.

I'm perfectly aware that there are 1) still a couple of glitches and needless lines of code in this patch and also 2) some open @todos, but overall, the entire discussion on this and related topics pretty much led to the conclusion that this approach is just the logical conclusion and architecturally, the most sensible direction to progress into.

The approach taken fully addresses @Dries' concerns in #25 and seems to be architecturally in line with #1649238: Implement CMI Exportable Controller, whereas the main difference to previous patches in here is that there's a clear and clean separation between the CRUD controller for Configurables[/"Exportables"] and the Configurable entity/Thingie™ itself.

The essential conclusion being drawn is that it would be a bad idea to re-invent that entire wheel from scratch. The amount of architectural and functional requirements that are being shared between "entity entities" and "configuration entities" are really big and only leave a trivially small amount of systematic differences. The differences boil down to the storage engine that supplies the data and are almost exclusively covered by ConfigStorageController, which hopefully helps to prove that the approach taken builds upon a clean and straightforward subsystem.

There are certainly many rooms for simplification in this approach, and I'd actually have a full stack of them already, but did not include them, in order to not run into scope-creep.

I will definitely amend that the effects of #1018602: Move entity system to a module - in hindsight - are detrimental for this use-case of entities (as it essentially requires a config.module dependency for every module that uses Configurable). However, that change was an architectural conclusion that happened before the PSR-0 and Drupal\Core conversions, and I could very well see us moving the entity system "back" into a Drupal\Core component - which would inherently allow us to also move Configurable* back into Drupal\Core and thus remove the config.module dependency.

In the end, there is a very large common denominator between entities and Configurable entities. The entity system is progressing in many different ways already, and the Configuration system queue doesn't fall short of issues that essentially want and try achieve exactly the same. I already mentioned a few of them in #51, but the rather dominant hurdle is to understand that

1) all of this logic is outside of the configuration system itself,

2) we're trying to manage multiple "instances" of an entity class (which follows a certain "type" or "schema", each),

3) if you ignore the [default] storage for a moment, then you realize that the entity system was architecturally designed for exactly this purpose,

4) there is no difference aside from the storage controller being used.

I hope this helps to understand the train of thought some more. I'm happy to further discuss and collaborate on detailed questions.

fago’s picture

In the end, we'd essentially have to re-invent all of these things for Configurables, just because they're not using the entity system, although they could - as this patch proves.

Oh yes, thanks for doing so! The entity system - as our primary data API - is designed to work with *any* storage backend, so the configuration system naturally can be one.

The entity system is storage-agnostic, but moreover one of it's goal is the ability to be able to use it for incorporating remote-data, i.e. cases where you cannot control the data structure. That's why it cannot assume much and why all the features built into entities stay optional - only storage is required. For example, entities storing relational data (as relation.module or e.g. the node workflow state entity planned in the wscci format sprint) probably won't leverage an entity-render system, so probably won't configuration entities.

The essential conclusion being drawn is that it would be a bad idea to re-invent that entire wheel from scratch. The amount of architectural and functional requirements that are being shared between "entity entities" and "configuration entities" are really big and only leave a trivially small amount of systematic differences.

Yep, and I think there is a lot that "configuration entities" could benefit from being entities:

  • A proofed to work well hook-interface for *reacting* on changes. I consider this highly important for configuration. E.g. field API needs to know when a node type is renamed...
  • A clear and transparent layer for applying caching, e.g. cache all node types.
  • Modular extensibility solved - imho that's something we need for configuration objects as well. Modules adding settings to a node type, should be added to and get stored via $node_type->save();.
  • Avoid dividing configuration and content unnecessarily. Yes, we need to handle configuration differently, but on the upper level the commonalities should be common. Actually, that's what heyrocker's core conversation back in SanFran was all about (if I've not gotten it totally wrong): The line between content and configuration is sometimes blurry, so keeping tools compatible to both is a big win.

That said, I see the following wins in the common API:

  • It would allow us to reuse EntityFormControllers as introduced in #1499596: Introduce a basic entity form controller for configuration. While we could implement the Property API for config objects as well, having entities means that we could reuse the whole form controller and would save d8mi a ton of work! Also it gets us embeddable (while staying extensible) configuration forms - think apps taking existing configuration UI and embedding it elsewhere.
  • While not done yet, config could benefit from the entity validation API as well: #1696648: [META] Untie content entity validation from form validation. E.g. I'd assume that when importing configuation it needs to be validated independently from a form submission. Still we want to leverage the same logic there and avoid implementing it another time for forms. Same problem - same solution?
effulgentsia’s picture

Although the patch in #53 needs work, I'm +1 to the idea of:

  • Image styles, node types, views, and similar kinds of objects being entities (i.e., implementing EntityInterface, whether directly or by inheriting from Entity and/or ConfigEntity base classes), and
  • Just like all other core entities, passing all CRUD operations to a separate 'storage controller' object, and for the storage controller to handle all of the integration with the Config system, and
  • For ConfigStorageController to invoke Drupal hooks as similarly as possible (i.e., with similar names and at similar points in the operations) as DatabaseStorageController does for content-like entities (nodes, comments, etc.).

What I like about this is:

  • It addresses the original need of this issue, which is to invoke hooks in response to config imports (i.e., when 'image.style.large.yml' is deployed from dev to prod, the prod site needs to invoke hooks so that image module can empty the public://styles/large directory, media module can empty out text/filter caches containing rendered <img> tags with obsolete width/height attribute values, etc.).
  • The decoupling of high-level object and its storage controller matches the Exportables architecture used by CTools (at least at a high level), so will ease the conversion of Views and similar modules, and will allow Views and/or CTools to implement a storage controller that supports the existing CTools concept of "overridden" even if we decide against such a thing for CMI in general.
  • It allows for experimentation of different strategies for how to combine multiple low-level config objects into a single high-level thingie. For example, our entity storage controller architecture allows for each field to be stored in a separate table (in relational databases) or for the entire entity to be stored in a single document (in document-oriented databases). Similarly, per #44, we don't know yet whether we want to put comment-related settings for the article node type into the 'node.type.article' config object or into a 'comment.SOMETHING.article' config object. By leveraging the EntityInterface/EntityStorageControllerInterface architecture, we can vary that choice without impacting the NodeType API.
  • #56 and #57 list additional cool things we get to reuse.

However, heyrocker, sun, chx, and I had a call to discuss this, and identified some possible concerns worth considering:

  • Terminology. Per #22, it's useful to distinguish content-like things from configuration-like things. If we use the Entity architecture for both, then what do we call each of the subgroups?
  • Some things currently in EntityInterface, Entity, and entity.module might not make any conceptual sense for configuration thingies. For example, bundles, UUIDs, and URIs. Then again, maybe we will want to add UUIDs and URIs to configuration thingies, who knows.
  • EntityInterface currently has language() and translation() methods and a $langcode parameter in get() and set(), but I don't know how well this meshes with the config language handling in #1616594: META: Implement multilingual CMI.
  • Do fields and EntityFieldQuery make sense for configuration thingies? In theory, adding images or tags to node types and views would be cool and has occasionally been requested, but are we ready to open the door to that?
  • Are there risks of circular dependencies? For example, node_entity_info() calls node_type_get_names(), but if node_type_get_names() requires loading $node_type objects which themselves require hook_entity_info() to be complete, that's a problem. Or, if we allow configuration thingies to become fieldable, but fields require configuration stored in config thingies, that's also a potential problem. However, we think that these kinds of problems can be solved by making sure that node type names and field configurations can be fetched from low-level config objects, and not only from the high-level configuration thingies.

Overall, I think the benefits outweigh the concerns, but I'm interested in other feedback, particularly from people working on VDC, and Jose or others who can raise additional concerns or alternate solutions.

peterx’s picture

Re config and entity language as a use case. Language has und, the users language, and the site language. When selecting an entity, you get the the user version or the site version or the und version. In config you typically want the site config then add the user specific config. Selecting entities based on language is one area where config could work in a more useful way.

Re config and versioning as a use case. Versioning of config would be good for displaying sites back at a point in time and keep some lawyers happy. Use the delta in entities. The delta can be set to a date. Would be excellent for any config related to what visitors see or can do.

gdd’s picture

I would like to back off a bit from my initial strong reaction which was mostly driven by

1) The Entity system brings a lot to the table that we don't need and which I'm worried adds overhead or unnecessary implementation complexity. Fieldability is one example that was brought up. On the other hand, we also realized that something like the Enabled/Disabled flag on a View may be useful as a property or field.

2) Over the years I've seen a lot of situations where someone says 'Oh look, I have an object over here and it is 80% of what I want to do, and some extra stuff but we can just ignore it, and we can add in our 20% on top and it will save a bunch of time' and what it ends up doing is making both implementations more complicated and brittle as you get into implementations. By this time we've lost a month and now we're in bad shape because our schedule is getting very tight right now.

3) A concern that we already had two implementations under discussion and now we're adding a third to the mix which is just going to prolong this discussion even longer. See schedule concerns above.

All that said, I'm not really an expert on the entity system, so if the parties involved address these issues and there is general consensus around it I don't really have a problem with it as long as we can get moving on things fast. I definitely want to get input from people like merlinofchaos and yched, since they represent very large use cases around this functionality. I'll also try and take some time and dig into the patch and implementation more closely before DrupalCon.

fago’s picture

Great to see we finally have a constructive discussion here! :-)
Adding my thoughts to the points raised:

Terminology. Per #22, it's useful to distinguish content-like things from configuration-like things. If we use the Entity architecture for both, then what do we call each of the subgroups?

Configuration entity sounds good to me, for the rest - I don't know. I don't think that everything not being configuration is content, but that clearly depends on the definition of content. E.g. I'd not call an entity storing a flag typical site content.

Some things currently in EntityInterface, Entity, and entity.module might not make any conceptual sense for configuration thingies. For example, bundles, UUIDs, and URIs. Then again, maybe we will want to add UUIDs and URIs to configuration thingies, who knows.

Yep, all of those are optional, so methods may return NULL or similar. That's the case for some other entities as well, still we have them in the interface as most entities have them.

Do fields and EntityFieldQuery make sense for configuration thingies? In theory, adding images or tags to node types and views would be cool and has occasionally been requested, but are we ready to open the door to that?

Good question. On fields, the message module already does so - it uses fields on the (exportable) message type (without field UI though). That's very useful there, but it clearly poses problems when entity reference fields should be imported/exported. My take is that with property api and #1497374: Switch from Field-based storage to Entity-based storage, "fieldable" isn't the big question any more. The question is whether we allow users customly adding fields via field UI, what we probably won't for configuration. Still, I think it's better to leave out "fieldable" for now as it adds lots of complexity and weight to entity load (as it is now).

On querying - not sure what the needs for querying configuration entities are and whether it can query the DB or has to go for files (...). But implementing it using the entity-query interface surely makes sense, but wouldn't be fun until "EFQ-query backends" have received some love. So for now, I guess it would be fine to implement custom find methods on the storage controller and possibly move it to efq later on.

Are there risks of circular dependencies? For example, node_entity_info() calls node_type_get_names(), but if node_type_get_names() requires loading $node_type objects which themselves require hook_entity_info() to be complete, that's a problem.

Yes, that kind of problems definitly exist but are solvable.
For once, directly accessing storage is always a way around it. Furthermore, (future) symfony-ication of the entity system should make it able to re-use entity-controllers without having globally built entity info.

However, I think bundle-info should be moved out of hook entity info anyway what would solve the problem as well. Maybe we can even replace it by a pointer to bundle-entities (node -> node types) and so automatically solve #1374116: Move bundle CRUD API out of Field API!

sun’s picture

Quick response on two particular points:

- #58: UUIDs: Meanwhile the question for me is not whether Configurables need UUIDs, but instead, how we can enforce them. They are the only way to properly resolve the problem of renamed configuration when staging/importing configuration. The entity system has built-in support for them. This patch leverages that and adds UUIDs to the ConfigTest entity already, which I've verified and works really fine.

- #61: Entity info bundles: Yep, when I looked into the issue of defining bundles for another "content" entity, I had exactly the same thought — whether the 'bundles' property couldn't just simply point to another entity info key and be done with it.

Third, improving and simplifying the way in which configuration entity types are registered in hook_entity_info() was one of the things I purposively did not include in this patch (as mentioned in #56). Essentially, I'm thinking of possibly introducing a separate info hook (or class annotation) for registering config entity types, with the primary purpose of enforcing/standardizing a couple of entity info properties; for example:

$defaults = array(
  'storage controller class' => 'ConfigStorageController',
);
$enforce = array(
  'entity keys' => array(
    'id' => 'id',
    'label' => 'label',
    'uuid' => 'uuid',
  ),
  'fieldable' => FALSE,
  'bundles' => array(),
);

However, I'd highly prefer to discuss and do that in a separate follow-up issue.

tim.plunkett’s picture

FileSize
23.52 KB
104.75 KB

Posted the patch from our discussion, details to follow.

tim.plunkett’s picture

Assigned: sun » tim.plunkett
Status: Needs review » Needs work

Missed a couple of use statements.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
473 bytes
104.79 KB

Maybe only the one?

tim.plunkett’s picture

FileSize
104.8 KB

Fixed 4 instances of type hinting, it should fix those last couple exceptions.

fgm’s picture

This should fix the remaining save() signature inconsistencies.

tim.plunkett’s picture

FileSize
119.04 KB

Unless I'm missing something, you can't do this:

interface Foo {
}

interface Bar extends Foo {
}

interface Thing {
  function stuff(Foo $object);
}

class Spam implements Thing {
  function stuff(Foo $object) { }
}

class Eggs extends Spam {
  function stuff(Bar $object) { }
}

Fatal error: Declaration of Eggs::stuff() must be compatible with that of Thing::stuff()

Therefore, this becomes a bigger patch.

tim.plunkett’s picture

FileSize
77 KB

I accidentally added in the patch from #1588422: Convert contact categories to configuration system, sorry about that!

tim.plunkett’s picture

FileSize
76.35 KB

Also the interdiff from #1588422-58: Convert contact categories to configuration system...

Sorry for all the noise.

cosmicdreams’s picture

Wow, so we're green on this now?!?!?!?!

tim.plunkett’s picture

FileSize
10.27 KB
83.28 KB

Berdir pointed out we needed to do the same thing to EntityStorageControllerInterface, but he, xjm, and I agreed that StorageControllerInterface is better than StorableStorageControllerInterface.

Hopefully this doesn't cause more exceptions.

Berdir’s picture

Some feedback and thoughts below about the current page. A few things are obvious given that this is just a proof of concept patch, e.g. that we still task about entity in the code and comments in most cases but I mentioned that more because I'm not those cases not sure to what we should actually change it...

Leaving at needs review as this is what the issue needs.

+++ b/core/modules/config/config.api.phpundefined
@@ -31,12 +31,12 @@
-  // Only configurable thingies require custom handling. Any other module
+  // Only configurable entities require custom handling. Any other module

@@ -60,13 +60,25 @@ function MODULE_config_import_create($name, $new_config, $old_config) {
 function MODULE_config_import_change($name, $new_config, $old_config) {
-  // Only configurable thingies require custom handling. Any other module
+  // Only configurable entities require custom handling. Any other module

Does that comment need to be updated too? I have no clue to what, though. Is there a noun equivalent for storable?

+++ b/core/modules/config/config.api.phpundefined
@@ -31,12 +31,12 @@
-  $config_test = new ConfigTest($new_config);
+  $config_test = entity_create('config_test', $new_config->get());

@@ -60,13 +60,25 @@ function MODULE_config_import_create($name, $new_config, $old_config) {
+  $config_test = entity_load('config_test', $id);

I guess it would make sense to rename/duplicate/whatever entity_*() as well?

+++ b/core/modules/config/lib/Drupal/config/ConfigStorageController.phpundefined
@@ -0,0 +1,346 @@
+class ConfigStorageController implements EntityStorageControllerInterface {

The patch introduces StorableInterface, but the controller still has entity.

Since we don't need two different interfaces there (right?), we could just remove the Entity prefix and name it StorageControllerInterface. It's still within Drupal\entity, though.

I've talked with @sun about moving the entity system into a component (Drupal\Core\Entity), which might make sense because there is no way it will ever in any way be optional and when we moved entity.inc to entity.module, the components concept didn't exist yet.

If we have that, we could also introduce Drupal\Core\Storable and put the generic stuff in there.

+++ b/core/modules/config/lib/Drupal/config/ConfigStorageController.phpundefined
@@ -0,0 +1,346 @@
+  public function load($ids = array(), $conditions = array()) {

#1184272: Remove deprecated $conditions support from entity controller removes the $conditions thing from load() and instead introduces loadbyProperties() and loadRevision(). Atleast loadByProperties() most likely won't make sense for configuration, so maybe that means we might want to have an extended controller interface for entities.

Also, lot's of the code and comments in this class still reference entities, but I'd suggest wait with fixing that until the references issue is commited.

+++ b/core/modules/config/lib/Drupal/config/ConfigStorageController.phpundefined
@@ -0,0 +1,346 @@
+    $class_info = new \ReflectionClass($entity);
+    foreach ($class_info->getProperties(\ReflectionProperty::IS_PUBLIC) as $property) {
+      $name = $property->getName();

I guess this change means that configuration will use the same property API as entities hopefully will, so we can get that information from there.

Because we might want to support extendable configuration (e.g. additional options on node types, has there been any discussion about that?) which could work with properties.

+++ b/core/modules/config/lib/Drupal/config/ConfigurableBase.phpundefined
@@ -0,0 +1,102 @@
+ * Defines a base configurable entity class.
+ */
+abstract class ConfigurableBase extends StorableBase implements ConfigurableInterface {

Same here, lot's of "entity" in the text will that become "configuration"?

tim.plunkett’s picture

Issue tags: +revisit before beta

There are still 129 counts of 'entity' in core/modules/config/, and yes that is a problem.

Renaming entity_* procedural functions is something we didn't talk about at all.

I covered the EntityStorageControllerInterface in #77.

I think the reason for not moving the Storable classes to Drupal\Core\Storable is because if we decide it's a needless abstraction, it's less work to undo later. And if it DOES prove useful, we can move it.

We 100% want properties. Absolutely.

webchick’s picture

xjm/Jess spent about 2 hours walking me through this tonight, bless her heart. :) What follows are my initial impressions/thinking, NOT based on a code review, and NOT "the official word of webchick" or whatever.

She basically outlined two approaches: one that has Configurable just another type of entity, and one that abstracts the methods / properties in common (e.g. CRUD functions, etc.) between Configurables and Entities into a "Storable" class from which they both derive. She also explained that those in the room (most of whom are in this issue) generally favoured the second option.

My initial gut reaction is that if we're going to make Configurables a type of entity (which I'm not totally convinced is a good idea, but seems to have pretty broad support here), we should just do that and make it a type of entity. While we gained a lot of cool functionality by abstracting content-providing modules into content types (D5), and content types, users, taxonomy, etc. into entities (D7), I'm not sure I see the concrete benefit gained by this extra "Storable" level of abstraction. It seems to introduce more obscure terminology, and make the class hierarchy more difficult to understand for newcomers. Whereas just making them first-class entity types directly, with certain features turned off (e.g. revisions, fieldability) keeps things simple and consistent across the board, and it also brings us back to the original founding statement of the CMI initiative back in 2011 (which I completely agreed with) that there are no hard-and-fast rules around what is configuration and what is content; it totally differs (and needs to be configurable) on a site-per-site basis (fwiw, I think Greg no longer is an advocate of this approach, though).

However, there are some legitimate concerns raised as downsides to the first option, such as it being harder to find a list of just configuration (vs. content) for export without using some janky flags, and the brainf*ck of trying to explain what entities are now that entities are no longer content. There's also the political/community concern that raising the "there is no hard line" argument again will de-rail this issue for another several weeks/months, and Views in Core needs this ASAP. What xjm and folks would like to do is advocate for the "Storable" approach with minimum changes possible, which could potentially be revisited if after gaining some experience with this, we decide really does make sense to move "up the chain."

Some action items:

1) I see concerns like "Performance" being raised against this or the other option, which IMO should be fairly easy to just benchmark and get a number so we know what we're looking at, rather than hand-waving. (e.g. what's the performance hit of loading a fieldable vs. non-fieldable entity? Is it actually zero?)

2) I've tagged this as "Issue summary initiative." I've seen a Google Doc which outlaid a lot of this, and I think this needs to be copied/re-purposed into the issue for the benefit of people not at DrupalCon. Particularly a crisp description of the options, a pros/cons list, and a list of who was consulted.

3) Let's time-box this discussion; there are already patches coded for both approaches, we just need to pick one and move forward.

tim.plunkett’s picture

FileSize
83.71 KB

Just keeping the patch fresh after #1184272: Remove deprecated $conditions support from entity controller, still didn't update it for #78.

chx’s picture

My rusty one and a half cents: the following things make no sense on configurables IMO:

  1. Bundles
  2. EntityFieldQuery
  3. Fieldable

Status: Needs review » Needs work

The last submitted patch, drupal-1668820-81.patch, failed testing.

catch’s picture

I just chatted to xjm and effulgentsia about this in person, then caught up on a week or so of backscroll.

For me I like the Storable interface since I think we'll be adding a lot more things to the entity system (like #1026616: Implement an entity render controller) that won't make sense for configuration thingies.

While it's an extra level in the class hierarchy, it's not going to be in people's faces when developing modules IMO - same as most people working with entities don't (or shouldn't) need to worry about custom storage controllers and similar either. And it also means we won't have date formats or image styles in hook_entity_info() which would feel very, very wrong.

Anonymous’s picture

i'm late to the party, but here goes.

how about option 4) don't invent thingies, and add lower level reuse when we really know what that will be.

discussed this with timplunkett in #drupal-contribute, and i came away feeling like we're putting code reuse way above a design that is easy to talk and think about.

as far as i can see, this discussion makes me consider things like:

- "a view is the same type of thing as an image style as a node type as a taxonomy vocabulary" (you know, is-a and has-a and inheritance and stuff)

- should a thingie (which is the type of thing that a view, an image style, a node type and a taxonomy vocabulary is) be fieldable? or support EFQ? or bundles?

at that point, i think we've already lost, because our design is way more complicated to think about than the problem we're trying to solve.

end rant, continue on.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
83.69 KB

Status: Needs review » Needs work

The last submitted patch, drupal-1668820-86.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
83.69 KB

I missed the change to public function load(array $ids = NULL);

Status: Needs review » Needs work

The last submitted patch, drupal-1668820-88.patch, failed testing.

fago’s picture

For me I like the Storable interface since I think we'll be adding a lot more things to the entity system (like #1026616: Implement an entity render controller) that won't make sense for configuration thingies.

I'd see this things like entity render controllers more as separate system that build upon entities (and we want all of them to be optional). Thus, entities really are about storing and retrieving data, the rest are just APIs that integrate with them.

So as discussed, what bothers me about that Storable abstraction is that I see no difference between an Entity and a Storable. Still, I think that the approach of introducing it right now (with the EntityInterface being empty) and revisiting before release whether Storable is different is a way forward we can go.

However, there are some legitimate concerns raised as downsides to the first option, such as it being harder to find a list of just configuration (vs. content) for export without using some janky flags, and the brainf*ck of trying to explain what entities are now that entities are no longer content.

Differentiating configurable entities from other entities is easy: add a flag to hook_entity_info() that tells us whether it's configuration or not. The very same way, we'll have a hard distinction between config and non-config anyway as config uses CMI as storage backend.

In regard to the brainf*ck, I agree that this will be confusing to people. The way I see it, it's the understanding of what an entity is that "needs work" in the community anyway and teaching them entities = content doesn't help it as content maps to renderable entities, but an entity is not necessarily renderable as it isn't necessarily fieldable. E.g. at the wscci format sprint we planned for storing flags for entities as another entities, just as flag module could store its values using an entity. Or such as relation module stores information about other entities as a "flag" entity. So entity = renderable doesn't get it independently from configuration. Entity = a thingie we can load and save gets it though.

The other point to the brainf*ck is that this "entity = content" gave entity an drupal specific meaning that doesn't map to the rest of the world. As from my understanding the term "entity" very well maps to or even equals thingie, what would make it natural for a "node type", a "rule" or a "view" to be one as those all are thingies.
Then other systems like doctrine and/or the symfony community as well as the whole java world also uses the term "entity" for storables. So I'd love to see us fixing our definition instead of carrying on with inventing yet another weird drupalism.

tim.plunkett’s picture

I made a branch in my core sandbox for rerolling this patch, because its getting ridiculous. I can give access to whoever and post patches when needed, but its getting in the way of architectural review.

http://drupal.org/sandbox/tim.plunkett/1698392

Differentiating configurable entities from other entities is easy: add a flag to hook_entity_info()

Please, no more flags to a procedural info hook.

tim.plunkett’s picture

Eating my own words, sun suggested converting the static method getConfigPrefix to another flag in the info hook, for later conversion to annotations.

Patch to come later.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
83.76 KB

See attached.

damiankloip’s picture

Status: Needs review » Needs work

Not sure if this is in or out of scope of this patch.

+++ b/core/modules/config/config.api.phpundefined
@@ -59,14 +59,28 @@ function MODULE_config_import_create($name, $new_config, $old_config) {
+  // @todo Make this less ugly.
+  list($entity_type) = explode('.', $name);

Is $entity_type = strtok($name, '.'); any less ugly?

tim.plunkett’s picture

Status: Needs work » Needs review

We have an @todo to clean up that specific code (as well as other places), I'll be opening followups on the train tomorrow.

damiankloip’s picture

I'll propose that then then :)

Sorry, meant to not leave it as 'needs work'.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Per the conversation with Dries, webchick, fago, alexpott, dixon_, and sun, this is RTBC.
I'll be posting follow-ups tomorrow.

Commit message:
'Issue #1668820 by sun, xjm, tim.plunkett, fgm: Concept, base class, and interface for configurable objects.'

gdd’s picture

Assigned: tim.plunkett » Dries

Assigning to dries

chx’s picture

Assigned: Dries » chx
Status: Reviewed & tested by the community » Needs work

Let me hold the patch for a few minutes.

chx’s picture

Assigned: chx » Dries
Status: Needs work » Reviewed & tested by the community

While #72 is confusing, I can offer this as an explanation to the weird mega-type-hint-renaming going on here:

interface StorablesController {
  function presave(StorableInterface $storable);
}

interface ConfigurableController extends StorablesController {
  function presave(ConfigurableInterface $configurable);
}

interface StorableInterface {
}

interface ConfigurableInterface extends StorableInterface {
}

Results in PHP Fatal error: Declaration of ConfigurableController::presave() must be compatible with StorablesController::presave(StorableInterface $storable)

We love PHP.

Berdir’s picture

Quote from http://php.net/manual/en/language.oop5.interfaces.php

The class implementing the interface must use the exact same method signatures as are defined in the interface. Not doing so will result in a fatal error.

http://stackoverflow.com/questions/8862846/php-oop-implementation-must-b... also covers this (yes it does chx, even though you didn't believe me ;))

I also confirmed that this works just fine in Java. PHP--.

chx’s picture

Nope, neither is relevant. Those are about classes implementing interfaces. This is about interfaces extending interfaces.

Edit: this does not affect, in any way, that the patch is, indeed ready.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

Dries and I had talked earlier after the impromptu BoF we had at DrupalCon Munich and he gave approval for me to commit this ahead of him. Since I am sitting in the Westin lobby with xjm and sun, it seemed like a good time for a review. :D

Here's my Dreditor review. I don't think any of this holds up the patch at all. Mostly documentation, a few missing/weird comments, and one possible bug.

Follow-ups I think need to be filed are:

1) In many places we moved from one easy to read line like $config->save() to 40 lines of un-readable stuff that says "clean this up" :) Let's make sure we do that.

BLAH BLAH BLAH BALH BALH

2) I think that FALSE / NULL thing on $ids is actually a bug.

I got word from the team that they'll sic tim.plunkett on this on the train on the way up to Paris. ;)

Committed and pushed to 8.x. WOOHOO.

+++ b/core/modules/config/config.api.phpundefined
@@ -59,14 +59,28 @@ function MODULE_config_import_create($name, $new_config, $old_config) {
+function hook_config_import_change($name, $new_config, $old_config) {
...
-  $config_test = new ConfigTest($new_config);
-  $config_test->setOriginal($old_config);
+
+  // @todo Make this less ugly.
+  list($entity_type) = explode('.', $name);
+  $entity_info = entity_get_info($entity_type);
+  $id = substr($name, strlen($entity_info['config prefix']) + 1);
+  $config_test = entity_load('config_test', $id);
+
+  $config_test->original = clone $config_test;
+  foreach ($old_config->get() as $property => $value) {
+    $config_test->original->$property = $value;
+  }
+
+  foreach ($new_config->get() as $property => $value) {
+    $config_test->$property = $value;
+  }

Can we add some comments about what the heck is going on here?

The old code made fine and decent sense, and adequately explained what was going on here. The new code makes absolutely no sense. :(

+++ b/core/modules/config/config.api.phpundefined
@@ -89,8 +103,8 @@ function MODULE_config_import_change($name, $new_config, $old_config) {
+function hook_config_import_delete($name, $new_config, $old_config) {

@@ -100,8 +114,10 @@ function MODULE_config_import_delete($name, $new_config, $old_config) {
-  $config_test = new ConfigTest($old_config);
-  $config_test->delete();
+  list($entity_type) = explode('.', $name);
+  $entity_info = entity_get_info($entity_type);
+  $id = substr($name, strlen($entity_info['config prefix']) + 1);
+  config_test_delete($id);

Same deal here. :(

+++ b/core/modules/config/config.api.phpundefined
@@ -100,8 +114,10 @@ function MODULE_config_import_delete($name, $new_config, $old_config) {
+  list($entity_type) = explode('.', $name);
+  $entity_info = entity_get_info($entity_type);
+  $id = substr($name, strlen($entity_info['config prefix']) + 1);
+  config_test_delete($id);

Since this is basically the same code as above, should we pull this into a helper function of some kind?

I asked xjm this and she said something something something annotations something something. :P

+++ b/core/modules/config/lib/Drupal/config/ConfigStorageController.phpundefined
@@ -0,0 +1,356 @@
+   * @param $ids
+   *   An array of entity IDs, or FALSE to load all entities.
...
+    if ($ids === NULL) {

The docs say it's checking for FALSE, the code says it's checking for NULL. one of these is wrong. :)

Also, why on earth would you want to load *all* entities? Egads.

+++ b/core/modules/config/lib/Drupal/config/ConfigStorageController.phpundefined
@@ -0,0 +1,356 @@
+  protected function buildQuery($ids, $revision_id = FALSE) {

Could we throw some comments in here to explain better what's happening here? I kind of get it, but it'd be easier if there were two or three sentences throughout the function to just act as short-hand.

+++ b/core/modules/config/lib/Drupal/config/ConfigurableBase.phpundefined
@@ -0,0 +1,102 @@
+  /**
+   * Overrides Entity::isNew().
+   *
+   * EntityInterface::enforceIsNew() is not supported by configurable entities,
+   * since each Configurable is unique.
+   */
+  final public function isNew() {
+    return !$this->id();
+  }
+
+  /**
+   * Overrides Entity::bundle().
+   *
+   * EntityInterface::bundle() is not supported by configurable entities, since
+   * a Configurable is a bundle.
+   */
+  final public function bundle() {
+    return $this->entityType;
+  }
...
+  public function get($property_name, $langcode = NULL) {

If these aren't supported by configurables, why are they in ConfigurableBase?

+++ b/core/modules/config/lib/Drupal/config/ConfigurableBase.phpundefined
@@ -0,0 +1,102 @@
+  public static function sort($a, $b) {

http://api.drupal.org/api/search/8/_sort_ :(

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigConfigurableTest.phpundefined
@@ -0,0 +1,79 @@
+      'label' => 'Thongie',

Thongie? ;)

+++ b/core/modules/config/lib/Drupal/config/Tests/ConfigImportTest.phpundefined
@@ -31,6 +31,33 @@ class ConfigImportTest extends WebTestBase {
+    unset($GLOBALS['hook_config_test']);

$GLOBALS grumble grumble! I brought this up to sun, and he pointed out it's not really possible to fix this in a test that's touching the same request. Hrm.

+++ b/core/modules/config/tests/config_test/config_test.moduleundefined
@@ -1,50 +1,303 @@
 <?php

Missing a @file declaration (novice task)

+++ b/core/modules/config/tests/config_test/config_test.moduleundefined
@@ -1,50 +1,303 @@
-  $new_config->save();
+  // @todo Make this less ugly.
+  list($entity_type) = explode('.', $name);
+  $entity_info = entity_get_info($entity_type);
+  $id = substr($name, strlen($entity_info['config prefix']) + 1);
+  $config_test = entity_load('config_test', $id);
+
+  $config_test->original = clone $config_test;
+  foreach ($old_config->get() as $property => $value) {
+    $config_test->original->$property = $value;
+  }
+
+  foreach ($new_config->get() as $property => $value) {
+    $config_test->$property = $value;
+  }
+
+  $config_test->save();

Yet another one of these. :(

+++ b/core/modules/config/tests/config_test/config_test.moduleundefined
@@ -1,50 +1,303 @@
-  $old_config->delete();
+  // @todo Make this less ugly.
+  list($entity_type) = explode('.', $name);
+  $entity_info = entity_get_info($entity_type);
+  $id = substr($name, strlen($entity_info['config prefix']) + 1);
+  config_test_delete($id);

And another. :(

+++ b/core/modules/config/tests/config_test/lib/Drupal/config_test/ConfigTest.phpundefined
@@ -0,0 +1,30 @@
+
+  public $id;
+
+  public $uuid;
+
+  public $label;

Need PHPDoc on these.

fago’s picture

* Defines a base entity class.
*
* Default implementation of StorableInterface.

* This class can be used as-is by simple entity types. Entity types requiring
* special handling can extend the class.

This comment describes very well how the code looks like now. It's a storable API that only talks about entities. If we decide to keep this when we revisit it before release, this will need a big clean-up.

tim.plunkett’s picture

Assigned: Dries » tim.plunkett

We had no working wi-fi on the train, but yes, I am working on this today.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
9.5 KB

This addresses all of the feedback in #103.

Opening a bunch of true follow-up issues, I will post them here (and in the issue summary).

aspilicious’s picture

+ * @file
+ * @todo.

:)

tim.plunkett’s picture

Yes, because it's the first function in the file and I have no idea what else might go in there, and I need to talk to heyrocker about what else the config.module would be used for before I summarize what it will be used for :)

cosmicdreams’s picture

Is this still the issue where we make suggestions on what to rename ConfigThingie once we're happy with the patch?

If so I have another attempt to name this something that doesn't cause a knee-jerk reaction:

ConfigMediator

There's even a formal definition of the software development pattern:
http://en.wikipedia.org/wiki/Mediator_pattern

The description provide sounds like it's in the ballpark of what the code we've developed intends to do. Which is encapsulate how various "things" can communicate with the new Config object in order to make their management easier.

I'd also be ok with ConfigManager (even though the books don't call that out as an "official" pattern.)

cosmicdreams’s picture

In /core/modules/lib/Drupal/config/ConfigurableBase.php there is a function, sort, that has two parameters ($a, and $b). Should we declare the type of these objects? It's a public function and each of the parameters require the existence of a "weight" property. OK it doesn't "require" them, but it immediately checks for their existence and then moves onto whatever it supposed to do. Which apparently is to process the label of the object even though it doesn't check for the existence of that label property first.

Couldn't this code be handled better if we declare the type of the arguments and throw an error if those objects aren't provided?

Otherwise this seems RTBC to me.

effulgentsia’s picture

Is this still the issue where we make suggestions on what to rename ConfigThingie once we're happy with the patch?

Per #103, the initial patch has been committed, and the name chosen was Configurable, not ConfigThingie.

If so I have another attempt to name this something that doesn't cause a knee-jerk reaction: ConfigMediator

I applaud the attention to clear naming, but this isn't a mediator pattern: it's a straight up base class. An ImageStyle or a NodeType or a View is to Configurable what Node and Comment are to Entity. What's a little unusual about the name is we're using Configurable as a noun rather than as an adjective. This is following the precedent that CTools started with the name Exportable, but I think it's tripped up a few people already, since in daily English, "configurable" is an adjective.

cosmicdreams’s picture

Cool, I'll fix the description if the calls for naming are closed. The description still says,

Note: "Configuration thingie" obviously is a temporary term. Even though the intention is to deliver a DX that is mostly consistent with entities (but not identical), calling them "configuration entity" would be blatantly wrong. Suggestions highly welcome!

While it is true that suggestions may still be welcome, the decision is made and if Configurable should be renamed, that should be done in a separate issue.

cosmicdreams’s picture

Issue summary: View changes

Updated issue summary.

cosmicdreams’s picture

Title: Concept, base class, and interface for configurable Thingies (e.g., node types, image styles, vocabularies, views, etc) » Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc)

rename issue to reflect chosen name.

cosmicdreams’s picture

Issue summary: View changes

Reflect the chosen name, Configurable for the name of the thingies.

sun’s picture

Thanks! :)

I'd like to move the config_extract_id_...() stuff into a separate issue, because I want to investigate possible solutions some more:
#1760358: Provide a way to extract the ID from a config object name

Doing so inherently resolves the @file...@todo review issue mentioned in #107 for this patch.

The other changes look fine to me to do as a straight follow-up patch/commit on this issue.

cosmicdreams’s picture

Status: Needs review » Reviewed & tested by the community

Agree

sun’s picture

Same as #106 but without the config_extract_id_*() stuff.

sun’s picture

FWIW, since the topic (also) came up as part of this discussion, I've created:
#1760786: Move entity system "back" into a Drupal\Core component

effulgentsia’s picture

sun’s picture

FileSize
2.37 KB

FWIW, somewhere in the epic rename and re-roll action since #52 the changes of attached interdiff got lost :-/

HEAD is only able to pass currently, because the entire renaming/abstraction to Storable* caused ConfigurableBase to inherit from the wrong (empty) base class.

I'll re-incorporate the essential change in #1626584: Combine configuration system changes to verify they are compatible and also try to improve it...

Anyway, latest patch in here is still RTBC.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Thanks, those all look like good clean-ups.

Committed and pushed to 8.x!

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

tim.plunkett’s picture

Title: Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc) » Change notice: Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc)
Priority: Major » Critical
Status: Closed (fixed) » Active
Issue tags: +Needs change record

How did we not have one of these?

xjm’s picture

xjm’s picture

Assigned: tim.plunkett » xjm

I'll start the change notice.

webchick’s picture

We're once again over the critical task threshold. Please try and find time to write up documentation for your API change ASAP so your fellow contributors' features aren't blocked. Thanks.

xjm’s picture

Title: Change notice: Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc) » Concept, base class, and interface for Configurables (e.g., node types, image styles, vocabularies, views, etc)
Priority: Critical » Major
Status: Active » Fixed
Issue tags: -Needs issue summary update, -Needs architectural review, -Needs change record

Finally added a change notice. Sorry for the delay; I didn't look at it because I was told someone else was working on it, but that person apparently didn't. :)

I think #1761040: Rename Storable, Entity, and Configurable to Entity, ContentEntity, and ConfigEntity should probably also have a change notice for the addition of ContentEntity, which I'll add separately.

xjm’s picture

Assigned: xjm » Unassigned

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

alexrayu’s picture

I have had installation fail a few time because the empty $entities object was passed on install to array_filter() at ViewStorageController.php.

alexrayu’s picture

Status: Closed (fixed) » Needs review
plach’s picture

Status: Needs review » Closed (fixed)

Please open a new issue and link it from here.

tim.plunkett’s picture

Ah! @alexrayu you found the bug for #1895100: Remove pre-existing compiled PHP code when re-installing Drupal core, thank you so much.

claudiu.cristea’s picture

Seems that configurables having ID as string are not fieldable due to integer nature of entity_id in field tables (field_data_*, field_revision_*). Is this tackled somewhere or needs a new issue ticket?

Or I miss something :)

amateescu’s picture

amateescu’s picture

Issue summary: View changes

Updated issue summary.

catch’s picture

Issue tags: -revisit before beta

Untagging, doesn't need revisiting or at least not in this issue.

catch’s picture

Issue tags: -revisit before beta

Untagging, doesn't need revisiting or at least not in this issue.