I'm finding that I can enable the Shippable trait just fine, it adds a Weight field to my product variations properly. However, the next time I re-save the Edit form, when the Shippable field is grayed out and pre-selected, it saves with no error but seems to de-select the Shippable option. The next time I visit the page, I have to re-select it and save, and then the following time it'll show up selected and grayed out again (but I'd better not re-save or the cycle continues!).

CommentFileSizeAuthor
#11 commerce-Preserve_enabled_traits-2845793-11.patch2.47 KBreplicaobscura

Comments

bmcclure created an issue. See original summary.

ransomweaver’s picture

ransomweaver commented

I can reproduce this. However it only happens if there are weight values in the DB table. Before any data is added there, it's not greyed out and it doesn't go away. So a bug to do with saving the field state when its disabled.

bojanz’s picture

bojanz commented
Assigned: Unassigned » bojanz

Investigating.

replicaobscura’s picture

replicaobscura
she/her
Primary language English
Location West Virginia
commented

I started looking into it a little bit myself to see if I could get anywhere quickly. So far, I can verify that the disabled field HTML seems proper:

<input data-drupal-selector="edit-traits-purchasable-entity-shippable" disabled="disabled" type="checkbox" id="edit-traits-purchasable-entity-shippable" name="traits[purchasable_entity_shippable]" value="purchasable_entity_shippable" checked="checked" class="form-checkbox">

And when not disabled (when I reload the page and it's unchecked again):

<input data-drupal-selector="edit-traits-purchasable-entity-shippable" type="checkbox" id="edit-traits-purchasable-entity-shippable" name="traits[purchasable_entity_shippable]" value="purchasable_entity_shippable" class="form-checkbox">

However, in CommerceEntityBundleFormBase::validateTraitForm(), the provided FormStateInterface says the value for the grayed-out checkbox is 0.

At first, this seems like some kind of bug in the Form API not respecting the value of a disabled, checked, checkbox. But if that were the case, I'd expect it to be a bigger issue than just me reporting it here, so I then thought maybe something else was altering that form state before it got to the validateTraitForm() code. But ProductVariationTypeForm::validateForm() simply calls validateTraitForm directly. The only difference I can find from when it works to when it doesn't is that it has `#disabled` set in the item when it fails to detect the value in the form state.

Is it possible this is a core bug, or is there a piece in Commerce I'm missing here that might be getting in the way? I guess it'd be simple enough to check for disabled traits in the $form array provided and populate those back into the FormStateInterface in the validate method, but should that really be necessary?

replicaobscura’s picture

replicaobscura
she/her
Primary language English
Location West Virginia
commented

Actually, unless the Form API has something in place for getting around this, it seems like disabled checkbox values aren't included in a POST at all:

http://stackoverflow.com/questions/4727974/how-to-post-submit-an-input-c...

So maybe a workaround actually is needed to re-add the disabled checkboxes to the form state?

bojanz’s picture

bojanz commented

Yes. This is a bug we inherited from the attribute checkboxes on the variation type form, and the variation type checkboxes on the attribute form. No test coverage on either side. The #disabled trick removes values from form values. We'll need to preserve the disabled values in another hidden field, then add those to get the final values in submit. Looking at creating a form element that we could use in all 3 places.

replicaobscura’s picture

replicaobscura
she/her
Primary language English
Location West Virginia
commented

Edit: Missed your comment, you discovered the issue already :)

replicaobscura’s picture

replicaobscura
she/her
Primary language English
Location West Virginia
commented

Should this issue be moved to Commerce core since it seems to relate to entity traits and not to the shipping trait specifically?

Also, does this present data loss when the issue occurs or is the field data preserved?

bojanz’s picture

bojanz commented
Priority: Normal » Critical

Yes, this causes data loss.
Since this issue was opened we stopped using traits for order types, but the problem persists on product variation types.
Leaving it here for now cause Shipping is affected the most.

replicaobscura’s picture

replicaobscura
she/her
Primary language English
Location West Virginia
commented

Here's a PR for Commerce that solves the issue for me: https://github.com/drupalcommerce/commerce/pull/667

Patch coming as well.

replicaobscura’s picture

replicaobscura
she/her
Primary language English
Location West Virginia
commented
Status: Active » Needs review
StatusFileSize
new commerce-Preserve_enabled_traits-2845793-11.patch2.47 KB

This patch is the same as the PR, and fixes the issue for me as far as I can tell.

bojanz’s picture

bojanz commented
Title: Shippable trait getting de-selected automatically » Entity trait getting de-selected automatically
Project: Commerce Shipping » Commerce Core
Component: Code » Other
bojanz’s picture

bojanz commented
Title: Entity trait getting de-selected automatically » [Data loss] Entity trait / product attribute / product variation type get de-selected automatically

The fix is good, applying it to the other two places with the same pattern.

  • bojanz committed 731948a on 8.x-2.x authored by bmcclure 
    Issue #2845793 by bmcclure, bojanz, ransomweaver: [Data loss] Entity...
bojanz’s picture

bojanz commented
Status: Needs review » Fixed

We really need to unify all 3 into the same form element, but this resolves the critical bug at least.

Status: Fixed » Closed (fixed)

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