Closed (fixed)
Project:
Replicate paragraphs
Version:
7.x-1.x-dev
Component:
Code
Priority:
Critical
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
27 Apr 2015 at 12:08 UTC
Updated:
15 May 2015 at 21:04 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
ciss commentedComment #3
seanbI 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.
Comment #4
ciss commentedUsing 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:
Comment #5
seanbIn 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?
Comment #6
ciss commentedI 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 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).
I can't test it right now, but looking at the change I'm certain that it fixes the issue.
Comment #7
seanbThanks 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.