What we have currently is:

      $module_config
        ->set("enabled.$module", $weight)
        ->set('enabled', module_config_sort($module_config->get('enabled')))
        ->save();

this could be simplified to:

      $module_config
        ->set("enabled.$module", $weight)
        ->call('module_config_sort')
        ->save();

With just adding

  public function call($callable) {
    $this->data = $callable($this->data);
    return $this;
  }

Simple and pretty. What's wrong with this? You didn't like this simplifcation but gave no reasoning as to why. The added code to Config is superb little and the code in module_enable() is prettier without a doubt.

Comments

sun’s picture

Assigned: sun » Unassigned

What's wrong with this?

1) There's only one consumer that has this use-case, and the consumer is able to solve its need without changing the API for everyone.

2) The proposed helper method only leads to a different syntax and does not actually ensure that the callable is always invoked for a certain config object [key], which would certainly change the situation, but since that's not the case, there is also no architectural benefit in terms of reliability to it.

3) The proposed call() method is too inflexible and will thus only work for this single use-case.

This won't fix for me, unless an actual need for such a method arises through other use-cases in further config conversions. But if that happens, then I'm pretty sure that we will actually want to find more targeted solutions.

chx’s picture

Basically anyone wanting to do anything before, without introducing a hook, this could be an easy and cheap solution. It's never a bad thing to be a bit more extensible, is it?

sun’s picture

I don't see what this proposal here has to do with "extensible" - there's also no correlation to hooks, nor does it present any kind of solution.

The only thing I see being introduced here is a bad design pattern, really. The proposed call() method is comparable to the magic __call() class method, except that the class is not in control over what gets called. Add to this that there's no concrete purpose and random callables can be called, and the callable is able to manipulate the value of a protected class property. That's what makes this proposal strange to look at. Normally, the caller would pass the object to a callable.

sun’s picture

Title: Add a call method to Config.php » Add a Config::__call() method
Status: Active » Closed (won't fix)
Issue tags: +Configuration system

I still think this 1) adds too much magic and 2) there are no consumers (and there should not be any consumers).

Trying to clean up the config system queue currently, so closing this. Feel free to re-open though.