paragraphs_replicate does not properly handle single-valued paragraphs fields, for which paragraphs returns NULL when empty (see paragraphs_field_property_get()), resulting in a fatal error when attempting to clone the field value.

Comments

ciss’s picture

Title: Fatal error when cloning an empty field of cardinality 1 » Fatal error when cloning entity with empty paragraphs field of cardinality 1
Status: Active » Needs review
StatusFileSize
new695 bytes

  • seanB committed 04ae80d on 7.x-1.x
    Issue #2478503 by ciss: Fatal error when cloning entity with empty...
seanb’s picture

Status: Needs review » Fixed

I added a fix in the dev version. Even though I couldn't reproduce this issue, I do see how paragraphs_field_property_get() could return NULL in some cases. I didn't like the field_info_field() call though. We could just check directly if NULL was returned and act on that.

ciss’s picture

Using field_info_field is the proper way to do it, since the only indicator should be the cardinality, not the actual value returned. Also, field_info_field() uses static caching and thus is a cheap operation.

Steps to reproduce:

  1. Enable translation.
  2. Create a node type and enable translation for it.
  3. Add a paragraphs field with cardinality 1.
  4. Create a new node, leave the field empty.
  5. Create a new translation for that node.
seanb’s picture

In your steps I miss the part where we replicate the content? I just tried adding a translation without replicating and replicating a node and changing the language after that. Still having problems reproducing it.

The paragraphs_field_property_get() can only return NULL when the cardinality == 1 and the cardinality is already checked there, so it feels unnessecary to check for this again? I do understand performance is not really an issue.

Does the new dev version fix your issue?

ciss’s picture

In your steps I miss the part where we replicate the content? I just tried adding a translation without replicating and replicating a node and changing the language after that. Still having problems reproducing it.

I forgot to mention that you need to enable paragraphs_i18n as well (bundled with paragraphs). If enabled, the items will be replicated in hook_node_prepare when loading the node form for the new (unsaved translation). The issue should also be reproducable by simply replicating a node via replicate_ui. All you need is a paragraphs field with a cardinality of one, and no items in it. Before that patch this would result in array(null), which would cause a fatal error in the clone statement right at the top of replicate_clone_entity().

The paragraphs_field_property_get() can only return NULL when the cardinality == 1 and the cardinality is already checked there, so it feels unnessecary to check for this again? I do understand performance is not really an issue.

The point is to not make assumptions about the returned values and instead use the information available for that field. It's all about DX. In the end though it is your decision.
By the way, an even safer way to handle this would be to do array_filter($old_items) before iterating over them or instanceof checks in the foreach. That way even array(null) wouldn't matter, and as an added bonus FALSE would also be handled (if for some reason the referenced entity would be missing).

Does the new dev version fix your issue?

I can't test it right now, but looking at the change I'm certain that it fixes the issue.

seanb’s picture

Thanks for the extra feedback. The commit seems to fix the issue.

If we really don't want to assume anything about returned values we can't really be sure we only get a NULL value when the field cardinality is 1 (a multi value field might also return NULL). I think we have it covered for now.

Let me know if there are any other issues.

Status: Fixed » Closed (fixed)

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