In http://drupalcode.org/project/ubercart.git/commitdiff/04ff7d6 config()->set() calls were replaced with setData(). I am not sure this is the correct approach, and may be the reason that the StoreTest now fails. setData() overwrites the entire config data array, so e.g. in the case of config('system.mail') in uc_store_install() I think this might overwrite the interface.default values? Even in StoreSettingsForm it is possible that another module might want to extend the uc_store.settings config in some way, but setData() here will wipe out any other settings.

Comments

TR’s picture

That's not how it's supposed to work. The documentation recommends using setData() instead of set() whenever multiple values need to be set. Which makes sense, since setting them all at once should greatly reduce the overhead, and is similar to the way other Drupal APIs work, like with node_load() and node_load_multiple().

But looking at the source code for Config, I've found at least three big problems with Config. You're right that the current implementation of setData() is totally overwriting the data. What's more, it's not parsing the data; if we do config('system.mail')->set('interface.uc_cart', $value) then set() splits the config name on the '.' and stores the config internally in an associative array, like array('interface' => array('uc_cart', $value)). But setData() exposes the internal (implementation-specific) data structure used by Config, and requires the caller to know about and manipulate that data structure. In other words, you can't just setData(array('interface.uc_cart' => $value)) you would have to do setData(array(array('interface' => array('uc_cart', $value))) instead. Thus, setData() is pretty much worthless to anyone in its current implementation. This reminds me of the whole problem with the mail_system variable in D7, but worse because *all* configs are exposed to this now, and because D8 relies so heavily on configs.

I've also found a number of OO problems with the Config class, and I will be writing some tests and patches and pursuing this in the core queue. I'm sure that will take a good long time to push through.

In the meantime, I see no option but to revert the commit and go back to using set(). I'll take care of that later today.

longwave’s picture

You are expected to understand the mapping of dotted keys to arrays somewhat, as it is valid to do both $config->set('parent.child', 'value') and $config->set('parent', array('child' => 'value')) although again there is a subtle difference - the second one wipes out any siblings under the parent key, whereas the first does not. This has already been the cause of mistakes in core such as #2146971: SimpleTest should override all mail system implementations with TestMailCollector

TR’s picture

Yes. I think it stems a lot from lack of familiarity with OO by many core developers. The protected $data variable is an implementation detail of the Config class, and should *never* be exposed to the end user. Allowing setData(array $data) to simply overwrite the $data variable without any checks allows anyone to subvert the "protected" qualifier and breaks encapsulation. The data structure used by the Config class to store its instance data should be invisible to the end user - this would allow the internal implementation to change without affecting any code that used the Config class. That's extremely important for reuse and for encapsulation. In the end, D8 is still passing around a lot of non-type-checked and non-documented arrays of values, rather than representing these data structures properly with objects.

The more I delve into D8 the more I'm disappointed with what has been done in core. Yes it's a big step, but it's a big step that has been taken with poor form.

TR’s picture

And as an aside, part of the dysfunction that is D8 is that 'private' variable and methods violate the Drupal the coding standards. 'private' has an important role - to hide *implementation* details from the end user and from subclasses. Crell, despite his knowledge and experience and talent, doesn't seem to realize this. 'private' is a very important concept that is forbidden in Drupal's concept of OO. Me, I've been programming OO for 25 years, and I see Drupal is finally coming around to embracing *some* of the concepts that have been accepted by professional computer scientists in that time. It's personally frustrating trying to deal with people who insist on re-inventing concepts that were well-honed decades ago. Watching people rediscover development best practices is like watching grass grow. And it's painful writing patches when you know that you're just putting a bandage on a fundamentally flawed structure.

So in this instance, $data should probably be 'private'. That way, if the settings/getting of config values ever becomes a performance bottleneck, the Config class could replace its internal array structure with a linked list or a red/black tree or some other advanced data structure to improve performance without requiring any other changes in core or contrib or in the Config API. Likewise, making it 'private' would force us to think about what the API for Config should be, and would force us to make rational and usable set() and (e.g.(e.g.) setMultiple() functions.

Anyways, that's my rant for the day.

TR’s picture

Status: Active » Fixed

Commit reverted by 1de0d0d. Let me know if there are any side-effects I missed.

Status: Fixed » Closed (fixed)

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