Follow-up from #1777956: Provide a way to define default values for entity fields: we missed the ConfigStorageController to provide the same default values functionality as DatabaseStorageController. Ideally the logic should be consolidated in a storage controller base class, so this is postponed on #1978870: Add an EntityStorageControllerBase.

Comments

fago’s picture

Issue tags: +Entity Field API

tagging

tim.plunkett’s picture

Status: Postponed » Active
sun’s picture

Both referenced issues are done by now:
#1777956: Provide a way to define default values for entity fields
#1978870: Add an EntityStorageControllerBase

Is this issue still relevant today?

In case it is, it sounds like it should be more severe than normal.

sun’s picture

Priority: Normal » Major
Issue summary: View changes
tim.plunkett’s picture

Issue tags: +beta blocker

I am now in the process of actually writing some more advanced config entity types, and the divergence between content entities and config entities is unfortunate.

I really wish issues like #1777956: Provide a way to define default values for entity fields had not been allowed in while neglecting half of the codebase...

Regardless, we need to finish the work that was started.

yched’s picture

Well, content entities and config entities de facto are very different...
Content entities are made of Fields, Config entities aren't, and I don't see that happening anytime soon.

And Fields are the level at which default values are handled for Content entities. Thus, not applicable / reusable for config entities.

tim.plunkett’s picture

It wasn't part of TypedData when I originally reviewed it. That happened during the course of the issue.

I'm working around this using postCreate(), but that is not intuitive.

Berdir’s picture

As @yched said, it made no sense to handle this in there, it's completely different.

Instead of postCreate(), why not use preCreate() ? There you have an array of values and can just do $values += $defaults. Make that a default pattern and have protected $defaults = array() + $values += $this->defaults in ConfigEntityBase::preCreate() and you should be done?

chx’s picture

I'd like to work on this but even when looking at the linked lissue, I have no idea what to work on. Please fill out the template. Thanks!

xjm’s picture

Just checking -- did this get discussed with @alexpott before it was tagged as a beta blocker?

I agree that this seems like a problematic thing and potentially beta-blocking -- but not sure how it fits in the big picture currently.

Berdir’s picture

I'm actually just as confused as @chx. Unlike content entities, which don't have protected/public properties anymore, config entities do, so why can't we define defaults there? If that doesn't work, can't we just fix that?

Will try to look into it, but having an actual example for "advanced config entity types" and the problems related to default values would be useful.

And yes, not sure why this is a beta blocker, whatever solution we come up here, it won't affect storage and shouldn't contain ground-breaking API changes unless I can't see the problem?

tim.plunkett’s picture

I have this in a patch:

  /**
   * {@inheritdoc}
   */
  public function postCreate(EntityStorageControllerInterface $storage_controller) {
    parent::postCreate($storage_controller);

    // @todo Use self::applyDefaultValue() once https://drupal.org/node/2004756
    //   is in.
    if (!isset($this->weight)) {
      $this->weight = $this->isDefaultSearch() ? -10 : 0;
    }
  }

That can't be done with properties.
Now sure, I can continue to do this, but it is disappointing when you find applyDefaultValue() on ContentEntityBase but not on ConfigEntityBase.

effulgentsia’s picture

Removing "Entity Field API" tag, since this issue is about config entities, not entity fields.

Removing "beta blocker" tag, because:
- it wasn't approved as such by a core committer
- the issue is major, not critical
- if what ends up happening is adding an applyDefaultValue() method on ConfigEntityBase, then I don't see any harm in that landing after beta

it is disappointing when you find applyDefaultValue() on ContentEntityBase but not on ConfigEntityBase

Adding the DX tag for that. It seems to be even more of a WTF than that in that is ContentEntityBase::applyDefaultValue() ever called? FieldableDatabaseStorageController::create() calls applyDefaultValue() on fields only, never on the entity itself. Seems like the only reason applyDefaultValue() is on ContentEntityBase at all is to satisfy TypedDataInterface. Perhaps that can be removed as part of #2002138: Use an adapter for supporting typed data on ContentEntities though? Or, if we think an entity-wide applyDefaultValue() method is useful to have as a separate concept from other (pre|post)create() logic, then yes, we should make its availability, signature, and time at which it's called, consistent across all entity types.

Berdir’s picture

Content entities do not have default values on their own. Their fields do. So yes, applyDefaultValue() there solely exists because it has to implement TypedDataInterface, so that will go away.

Dynamic default values like your use case are special and it's the same for content entities, see for example Node::preCreate() and Item::postCreate() (they're also not consistent, as you can see :)). Note that those are not as dynamic as your use case (I don't know what exactly defines the default search) but only affect a single field, we're working on moving that to a special field type in #2145103: Provide non-configurable field types for entity created, changed and timestamp fields.

And for non-dynamic default values, it should be as simply as protected $weight = 10; for config entities?

So.. sure, we could call applyDefault() from postCreate() but that wouldn't change anything in your code, just move it to a different method? Can't see what we could possibly do here :)

fago’s picture

And for non-dynamic default values, it should be as simply as protected $weight = 10; for config entities?

The difference with defining defaults on the class is that those defaults will be applied for imported or loaded entities as well. I think that might be ok, just noting that it is there.
I agree with berdir that this would be the most natural place to add those defaults, while others can go into preCreate().

Seems like the only reason applyDefaultValue() is on ContentEntityBase at all is to satisfy TypedDataInterface. Perhaps that can be removed as part of #2002138: Use adapters for supporting typed data though?

Right - I think it would best to remove it on entity level as it's not necessarily complete, e.g. it doesn't invoke the entity methods or the hook.

Berdir’s picture

Status: Active » Closed (works as designed)

I still have no idea what we could do differently here.

- Content entities have no default values, only fields have. Config entities have no counterpart for this.
- applyDefaultValues() was always bogus and unused and no longer exists.
- There is preCreate() and postCreate()
- You can also set default values directly for the properties, which is not possible for content entities, with the limitation that it is also applied to existing entities until overriden by the stored value.

More than enough options IMHO, I really wouldn't know what else to do.