"During creation, when clicking "add another" on a multiple cardinality field, entities $sid internally is changed. "
Well, it's kind of hard to replicate, but.. Modules I'm using:

Entityform
Workflow 7.x-2.x-dev using 'Workflow Field'
Workflow fields 7.x-1.x-dev with some patches: #2612674: Author is not calculated properly for entities
#2148935: Make module compatible with workflows defined through "Workflow field" module

So, Let's say that I have an entityform with 2 states: "(creation)" & "New" & it's properly configured so author can make the transition (creation)-> New.

And the entityform has a simple field (let's say a text field) with unlimited cardinality (so we have the "add another" button).

Now, workflow fields is configured to allow the author to edit it during creation, but not when the workflow is set to "New".

So.. during creation, if the author clicks on the "Add another" button, suddenly the field disappears.

After some debugging, I've figured out that when the form is being created, the sid is set to "creation"'s value (as expected), but when the author presses the "add another" button, the sid returned from workflow_node_current_state is set to "New".

I know that this only causes an issue in workflow_fields, but I think that if entity hasn't been created yet (it's "is_new" variable is set to TRUE), the expected behaviour of workflow_node_current_state function is to return creation stage sid.

CommentFileSizeAuthor
#17 workflow-wrong_sid_during_creation_add_another-2612702-17.patch679 byteszekvyrin
#10 workflow-wrong_sid_during_creation_add_another-2612702-10.patch564 byteszekvyrin
#9 workflow-wrong_sid_during_creation_add_another-2612702-7.patch624 byteszekvyrin
#8 workflow-wrong_sid_during_creation_add_another-2612702-7.patch624 byteszekvyrin
#2 workflow-wrong_sid_during_creation_add_another-2612702-2.patch645 byteszekvyrin

Comments

Zekvyrin created an issue. See original summary.

zekvyrin’s picture

zekvyrin commented
StatusFileSize
new workflow-wrong_sid_during_creation_add_another-2612702-2.patch645 bytes
zekvyrin’s picture

zekvyrin commented
Status: Active » Needs review
johnv’s picture

johnv
Primary language Dutch
commented
Status: Needs review » Needs work

2 things with this patch:
- Please try the same on a 'normal' Add entity page.
- Your patch is above determination of the field_name. Why?

Hope to hear soon from you.

johnv’s picture

johnv
Primary language Dutch
commented

Also, please install very latest version.

johnv’s picture

johnv
Primary language Dutch
commented

And also, can you conform to the $sid = _creation... pattern?

or doesn't it work in that case?

zekvyrin’s picture

zekvyrin commented

With your questions I did some additional digging with this.
I tried today's dev, issue still existed in my case.

Then, I tried to replicate this on a node. The issue wasn't there. That made me wonder why...

First of all, "normal" (node,users) & other entities have some differences.. is_new attribute doesn't exist in $form['#entity'] variable
on node/add page, although it does appear in entityform submission creation page.

After some additional digging, I found out why this is happening:

The '$entity' variable doesn't contain the workflow field when
- creating a node
- trying to add another item (during node's creation)
- creating a new

But (during Entityform Submission's creation) when trying to add another item, the '$entity' variable contains the workflow field and had the "New" state's sid, so line 695 (current dev version) returns a value in $sid.

Indeed there is no reason for it to be above field_name determination, although because it already existed (field_name was given as argument), I didn't notice it earlier. So with this patch I've uploaded (it's just a temporal fix) I'm just actually moving field_name after re-setting it. Although instead of forcing it there to return a value, we could also alter the final if when there isn't any current state.
But maybe some additional debugging is required before applying it.

I can try and test it in another entity if you want (planning to do it when I have some more time). Maybe instead of force-setting it there (where is_new), it would be better to not have a value in entity variable at first place (if that can be changed), so maybe some additional digging is required. But I would need some help probably to find where it does that and if we can change it.

On the other hand the reasoning "If the entity hasn't be saved yet, force (creation) sid" seems pretty valid to me as well.

zekvyrin’s picture

zekvyrin commented
StatusFileSize
new workflow-wrong_sid_during_creation_add_another-2612702-7.patch624 bytes

Sorry forgot to add the patch in previous comment

zekvyrin’s picture

zekvyrin commented
StatusFileSize
new workflow-wrong_sid_during_creation_add_another-2612702-7.patch624 bytes

Also uploading an alternative patch: added the condition when entity is new in the final if-clause.

zekvyrin’s picture

zekvyrin commented
StatusFileSize
new workflow-wrong_sid_during_creation_add_another-2612702-10.patch564 bytes

Damn :/ Ignore #9 (it's actually the same as before), this is the alternative.

johnv’s picture

johnv
Primary language Dutch
commented

:-)

FYI, one hour ago, I fixed this: #2613204: WSOD when adding/editing a non-node entity with workflow_field
It is also about non-node entities. Please use latest dev version of tomorrow.

johnv’s picture

johnv
Primary language Dutch
commented

Patch #10 doesn't change the logic, does it?
What is the result of this code: 

    $wrapper = entity_metadata_wrapper($entity_type, $entity);
    // Get State ID for existing nodes (A new node has no sid - will be fetched later.)
    // and normal node, on Node view page / Workflow history tab.
    $sid = $wrapper->{$field_name}->value();
johnv’s picture

johnv
Primary language Dutch
commented

I guess the same happens when you have a normal node, and press the Preview button.

johnv’s picture

johnv
Primary language Dutch
commented

IMO you can use this in your code:
if ((!$sid) || (!empty($entity->is_new) )) {

For the preview mode:
if ( !$sid || !empty($entity->is_new) || !empty($entity->in_preview) ) {

I guess we do not need creation_state, but workflow_node_previous_state

zekvyrin’s picture

zekvyrin commented

#11) Ok, will do tomorrow

#12) No, not changing anything results exactly the same every time. I just added it to conform (like you said in #6).

#13) Also, will try to test it tomorrow

#14) I guess you're right about the code. I haven't met a situation when "is_new" is set, but is false (and I don't think there should be).

But why "previous"? This only happens in creation (for me). I have also test it in other states (e.g. when on "new") and the problem doesn't exist there (ok not with using node's "preview")

johnv’s picture

johnv
Primary language Dutch
commented

In your case, previous and creation is the same, but not in preview.

zekvyrin’s picture

zekvyrin commented
StatusFileSize
new workflow-wrong_sid_during_creation_add_another-2612702-17.patch679 bytes

Hello john,

I tested everything today with latest dev.
Yes issue still exists on that dev, and also you were right about node preview.

I made a new patch using workflow_node_previous_state and including $entity->in_preview in the if clause, as you said in #14.

zekvyrin’s picture

zekvyrin commented
Status: Needs work » Needs review
johnv’s picture

johnv
Primary language Dutch
commented
Title: During creation, when clicking "add another" on a multiple cardinality field, entities $sid internally is changed. » workflow_node_current_state is not correct in some use cases.
Issue summary: View changes
johnv’s picture

johnv
Primary language Dutch
commented
Version: 7.x-2.x-dev » 7.x-2.6
Status: Needs review » Fixed

Fixed in 2.6+dev

johnv’s picture

johnv
Primary language Dutch
commented
Version: 7.x-2.6 » 8.x-1.0-beta1
Status: Fixed » Patch (to be ported)

Needs changes in D8, too.

johnv’s picture

johnv
Primary language Dutch
commented
Version: 8.x-1.0-beta1 » 7.x-2.6
Status: Patch (to be ported) » Fixed

Thi sis done in 8.x-1.0-beta1+dev.
Back to lowest version.
(apparently, d.o. updates for commits are behind.)

Status: Fixed » Closed (fixed)

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

johnv’s picture

johnv
Primary language Dutch
commented