Using this module in combination with commerce promotions, makes it unable to save the promotion

Wrong default version on issue creation.

grevil → changed the visibility of the branch 3503328-using-this-module to hidden.

MR!19 simply removes the group call, which fixes the issue, but doesn't look very good IMO:

Please decide and review.

Static patch for the time being.

Thanks, I can confirm this major issue!

Hm we could check for status to make sure this does not happen? Just removing this will break integration for forms that have a checkbox status field like media for example.

@hydra right!

Like this?

@grevil could you also please test, if the new MR works? Then we could merge and release this! Thanks!

This looks good to me, if @grevil could confirm this works for his use-case I'll do a release with that imidiatly :)

No, this won't work, as the form has a "status" and also a group ("option_details" as described in the issue summary), so conditionally moving the field won't work.
This could be a commerce issue, that the form simply doesn't have a "footer" group. I'll check this.

I don't quite understand yet, why we move the "status" field to "footer". Gin itself moves the status field to "status" (See GinContentFormHelper).
And moving it there, I can at least save the form again, but the form element is still nowhere to be seen.

Furthermore, I don't see anything special about Commerce's "PromotionForm" it simply extends core's "ContentEntityForm" and doesn't do anything special with the "status" form element. They only move the "status" form element inside their "option_details" details wrapper here and that's it. Could that be a problem? Other than that, the theme call ($form['#theme'] = ['commerce_promotion_form'];) could potentially be another place, which results in this issue (see here )

@hydra do you have any idea, how this issue could get potentially fixed? I am out of ideas (pushing the "status" fix, so we can at least save the form again).

Static patch for the time being.

@grevil Yeah but gin is designed to optimize the NodeForm, not the ContentEntityForm, and NodeForm is doing this:
$form['status']['#group'] = 'footer';

Therefore in order to move the status checkbox field to the position, we need to move it to the footer group. See my screenshot from before:

Ah, I see, so through doing: $form['status']['#group'] = 'footer';(along with other code changes), gin can simply process the form as it would be a node form and display it as gin would display a node form.

But this still leaves the question, why this doesn't work for commerce's "PromotionForm"? Again, I don't see anything special about the PromotionForm, that could cause this incorrect behavior?
Maybe you see something I don't @hydra? Could you have a look at https://git.drupalcode.org/project/commerce/-/blob/3.0.x/modules/promoti...?

Hey @grevil, that made me curious. Some differences I could identify are:

The status field is of type "radios" instead of type "checkbox"
That might be something we could cover by making sure the form element type is correct.

The missing footer container
That could also easily been covered by providing it if non existent.

#tree TRUE
The PromotionForm set's the #tree property to TRUE, which actually breaks the #group functionality. I cannot see a reason right now why this has been done. And I am not sure we can just randomly change that value in gin_everywhere. I did a quick manual check and the form still works when I remove that, but I cannot assure it would not break anything - but have not many stacks in that either so very interested in your opinions!

Thanks for digging @hydra! I'll do some more testing soon. Maybe I can find a general solution for this issue through your helpful comment! 👍
Otherwise, we might have to create an issue on commerce's site, if we justify it correctly, we might get a chance. I really don't want to add too much form specific code in this lovely module.

Alright, I did some further testing and interestingly enough, the "status" form element is not even a standalone radios form element, but instead a container containing the radios form element!

I tried conditionally adding the footer group, which doesn't fix the problem unfortunately. Setting "#tree" to false inside the "PromotionForm" also doesn't do anything.

I don't really want to do a commerce specific fix here, where we add the status element to one of the dedicated commerce form groups, as this would just bloat the code.
Instead, I'd suggest going with your first suggestion and do a type check on the status form. This way the PromotionForm doesn't look super pretty, but at least it won't break any functionality.

Funnily enough, the "footer" group actually exists in the PromotionForm, so even if the status element is a container and not a checkbox, it should be theoretically possible to simply move it!

I suggest, we commit the MR as a quick fix, lower the priority of this issue and keep it open for the future. What do you think @hydra? Please review!

I am totally fine with your suggestion! Merged.

Maybe we find a better title for this issue if we leave it open?

Great @hydra! Are you planning to tag a new release containing this?

Regarding the title, something more general might make sense?

Yeah just did not have the time yesterday. 1.2.3 is out containing the hotfix.

Great stuff! Thank you, @hydra!