It's impossible to submit no value for a field that has a default value

Indeed, I can reproduce with radios, checkboxes, but actually also for any field type and widget (e.g simple textfield widget for text or number fields).

1. Set a default value on a field
2. Go to the creation form for an entity using that field
3. Remove the pre-filled value (FAPI #default) and leave the field empty, save
4. The entity has the default value for the field.

This happens in field_default_insert(). Since #392696: Save default values on insert, default values get added on "entity insert" when the incoming $entity has no $entity->field_name property. That was to save default values on programmatic entity saves.
Later on, the 'Translatable fields' patch changed the condition, and default values get added also when
isset($entity->field_name[$langcode]) && count($entity->field_name[$langcode]) == 0

Yet $entity->field_name = array($langcode => array()) is exactly what comes out of any field that was left empty.

I cannot say I specifically remember the reasoning behind the code added by the TF patch, but it looks like we should rather check for !isset($entity->field_name[$langcode]).

We'll also need tests for this :-/.

The last submitted patch, field_default_value-1253820-1.patch, failed testing.

Aw, this is pretty tricky: while introducing the new language key level the rule of thumb was "if in the old code !isset($entity->field_foo) is valid in the new code, then !isset($entity->field_foo[$langcode]) should be equally valid". Clearly this does not work here.

The code specifies nothing for field_foo, which happens to be configured with a default value, which should then be inserted...

This cannot work unless we are able to define a default value for the field language too:

I don't think this needs to be a configurable value like the actual value, but it cannot be the default site language. It should be the entity language but we have no such concept in core atm. Anyway I'm going to post a clean up task issue, in order to respond to the many requests for a reliable field_get_items()/field_set_items() pair, that should introduce it. I'll crosspost it here as soon as I write it.

The alternative in the case of a non existing field property might be creating a default value for every available language, but I ain't sure this is a desirable default behavior.

I am afraid this cannot always work: suppose only untranslatable fields were provided, we would have no clue about which language pick up. IMO the test should be changed in the following way:

I am afraid this cannot always work: suppose only untranslatable fields were provided, we would have no clue about which language pick up.

I might be missing something, but I'm not sure that's an issue - we're only dealing with programmatic creation of new entities here. The incoming $entity definition, containing the explicit data provided by the caller, is all we need to care about.

Proposal (assuming we're dealing with 'field_a', configured with a default value) :
- If field_a is not translatable, make sure it gets a value for 'und' language (filling in the default value if not)
- If field_a is translatable, make sure it has a value (filling in the default value if not) in every language present in any of the explicit field values present in the $entity (that is, every language explicitly 'opened' by the incoming $entity definition) - including 'und'.
If no language open (or if $entity has no explicit field value whatsoever) , make sure at least 'und' gets a value.

Marked #1052070: If a field has a default value, it cannot be empty on insert. as duplicate.

Sorry for the delay, I had a busy fortnight.

#10 will make it so that on a programmatic creation of an $entity spanning several languages, only the main language will receive default values. That sounds a bit unintuitive / inconsistent. Then again, I'm a bit baffled by the topic, so if that's the best we can do...

What about the following (modified from my #9 to address your #10) :
Assuming we're dealing with 'field_a', configured with a default value,
- If field_a is not translatable, make sure it gets a value for 'und' language (filling in the default value if not)
- If field_a is translatable, make sure it has a value (filling in the default value if not) in every language present in any of the explicit field values present in the $entity (that is, every language explicitly 'opened' by the incoming $entity definition) - not including 'und', but at least including the entity_language().

field_default_insert() is the place. I guess it could use a static var to extract the languages present in the incoming $entity on the 1st call (we can separate different entities because the new $entity being saved gets a nid before field_default_insert() gets called)

I'm leaving tomorrow for 3 weeks away from my coding env, so I probably won't be able to patch this myself, though :-/.

I'll look into a test.

Attached exposes the bug.

Re-rolled @xjm's test from #22 against HEAD.

The last submitted patch, field_default_value-1253820-24.patch, failed testing.

The last submitted patch, default-value-1253820-27.patch, failed testing.

Working on this.

The last submitted patch, field-default_value-1253820-31.patch, failed testing.

Just thinking out loud, but:
We now have entity_create(), that's used on both programmatic entity creation, and as a preliminary step when showing an "entity add" form.
I'm wondering if this would be the correct place to initialize default values.

Then both flows (programmatic creation & form creation) then become basically the same:
"here's a fresh entity with defaults prepopulated, you can now edit it".

This seems much more sound that what we currently do (populate defaults as default widget values in creation forms + force add them at save time, which clashes).

This being said, I haven't really looked at what the implementation might look like...

The last submitted patch, field-default_value-1253820-35.patch, failed testing.

We now have entity_create(), that's used on both programmatic entity creation, and as a preliminary step when showing an "entity add" form.
I'm wondering if this would be the correct place to initialize default values.

I think so as well. For the Entity Field API we have #1777956: Provide a way to define default values for entity fields - let's sure to get it right there. Then for d8 we can inherit the solution by basing field API on the Entity Field API.

I agree that this hook is a good idea - I already thought about doing that also. Imho it should be hook_entity_create() and follow the entity+$entity_type hook pattern. "Act on entity creation." ?

There is a typo in "intially".

Still, I think it would be preferable to have default values applied via Entity Field API and keep the hook for any custom alterations of them.
Still, we could apply all default values there meanwhile?

Generally, I think the patch is a good improvement so I guess it's a good idea to do this now, while improving things with Entity Field API in a second step. Thus, setting to needs work for the hook name and docs.

Also, per #31, I think we cannot fix this in D7 unless we do very radical changes to the current behavior, which I guess we don't want to do at this point of D7 lifecycle.

Ups, I missed #41. Here's a new one and an interdiff against #35.

This should fix the latest failures hopefully.

These need parameter typehints (no changes yet).

Attached is #46 with a test-only patch including my original test and other test hunks.

The last submitted patch, field-default_value-1253820-46.patch, failed testing.

This is directly copied from field_default_insert() but doesn't really reflect the new code flow now - this happens on entity_create(), so before and independently of both form submits and programmatic saves, and before an $entity->$field_name can even "be provided" ?

Right! Removed all that obsolete stuff and simplified the PHP doc as now there is not much to say about it. Fixed also missing type hints.

Also, do we really need those checks in the function now ? :

Yes. When calling entity_create() you can pass an array of initial values, without those checks the default values here might override those. There is a new test covering this.

Probably related: I'm not sure about field_populate_default_values() as a standalone function.

The main use case I have in mind is supporting the ability of populating defaults when adding a translation: in that case you already have an existing entity object, but you need to fill-in the defaults for the new language.

I'm guessing in the Entity API, we will need to either introduce entity translation CRUD hooks or make entity translations entities, see #1810370: Entity Translation API improvements.

@fago already gave his ok in #40, except for two minor things fixed immediately afterwards :)

Sorry, he didn't review the last entity API changes.

Here is a patch adding the missing PHP docs.

Seems to be an unrelated change? Howsoever, I'm not sure the new code is better than the old one. Maybe just convert it to multiple lines using if()/else()?

It's needed to make a test pass: previously if you created an entity using a non exisiting language code the language() method returned false, which made lots of things go wrong.

I see. In entityNG we have this workaround for the same problem:

Can we unify the work-a-rounds? LANGUAGE_NOT_SPECIFIED sounds reasonable.
Also could we just use if/else instead of ?: - imho having multiple lines of ?: statements is too much.

+ * This can be used to set initial values for the term, for instance to
+ * provide defaults.

Is there a difference between initial values and default values?? If not, let's just use "This can be used to set custom default values for entity fields."

Thanks, improvements look good!

Well, actually "initial" sounds slightly more generic to me: the new hook could be used to provide a value just in a particular circumstance. In this case I wouldn't call it a default value.

I see, ok.

Sry for not coming up with this previously, but I just realized that the hook description might be confused by devs with hook_comment_insert(). Let's clarify this better. Also, what we are acting on is the entity (object) so what about that:

Then, another thing we missed previously: field_entity_create() should check whether the entity is fieldable.

Works for me - I'd just remove the "just" as we do not use it other hook descriptions either.

Thanks - looks good.

I think these should be generically in entity.api.php as hook_ENTITY_TYPE_create()

While this is a good suggestion, it's not what we do currently. We should follow the current way of doing things and address this question in a general issue for all entity_* hooks, i.e. #1824184: Remove and replace entity type specific API hook documentation with generic entity API hook documentation.

ok, back to RTBC then.

What's not stated in the docs or clear from the name 'foo' is that this is only needed for entity fields/properties that aren't managed by field.module.

Also this example and the ones for the specific entity type hooks (hook_node_create(), etc.) assume EntityNG, but that's not stated anywhere, so if someone copies this documentation into a module they're working on now, it won't work. Perhaps a @todo would help clarify.

Unless someone beats me to it, I'll give the docs here a crack in a couple hours to keep this moving.

Added @todo.

Works for me.

#75: field-default_value-1253820-75.patch queued for re-testing.

The last submitted patch, field-default_value-1253820-75.patch, failed testing.

hm

Alex and I actually spent a bit of time walking through this the other day, and I think it's good to go.

Committed and pushed to 8.x. Thanks!

We should add a change notice for this.

Any reason that invokeHook() wasn't added to ConfigStorageController as well?

Here is a fix for #86. Btw, I think we should provide an abstract common ancestry to config and content entities when working on storage-agnostic entities and the Storage API.

Edit: I meant config and content entity storage controllers, obviously :)

Nope, I just meant that it's fired on every entity_create() but not when instantiating the object on entity load. Tried to make the first paragraph more clear.

Hm. Seems like it should be possible to write a test for #86/#88, no?.

@chx:

In #90 I was referring to this:

http://drupal.org/node/1891514/revisions/view/2523892/2523960

Ok, I removed the "first time" terminology since it is misleading. Hope now it looks better.

The attached patch provides test coverage for #88. There's also a test only version to prove it.

Hm. Seems like it should be possible to write a test for #86/#88, no?.

True - that said we already have an EntityCRUDTestCase which tests for hook invocations. We should test this hook there also.

Let's see what the bot thinks about this.

#100 addresses @webchick's and @fago's concerns so back to RTBC.

Awesome, thanks!

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

related? #1500214: Optional radio button value saves as default value, if N/A selected

I thought that per #31 we agreed that we cannot fix this in D7 without making very radical changes.