Currently, when using this module in combination with commerce promotions. I am unable to save the promotion, because the "Status" field disappears from the sidebar, and the following error happens:
Error
1 error found: Status
The reason for this is the following code in gin_everywhere.module line 100:
$form['status']['#group'] = 'footer';
I guess this was used to put the form element at the bottom of the sidebar? But it doesn't work in Drupal 10.3 anymore.
The original `#group` value of the status field is "option_details". If we simply remove this line, everything works again, but it doesn't look very good.
| Comment | File | Size | Author |
|---|---|---|---|
| #16 | patch-16.patch | 562 bytes | grevil |
| #9 | Screen Shot on 2025-01-30 at 16:40:18.png | 412.19 KB | hydra |
| #7 | patch-7.patch | 498 bytes | grevil |
| #6 | 2025-01-30 13_06_53-Clipboard.png | 28.94 KB | grevil |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #4
grevil commentedWrong default version on issue creation.
Comment #6
grevil commentedMR!19 simply removes the group call, which fixes the issue, but doesn't look very good IMO:Edit: It doesn't anymore. @anybody changes to the MR were incorrect, but the original change also isn't valid. (Original change: https://git.drupalcode.org/project/gin_everywhere/-/merge_requests/19/di...)
Please decide and review.
Comment #7
grevil commentedStatic patch for the time being.
EDIT: Don't use this patch, I accidentally made a patch including the changes by @anybody which doesn't change anything. This was the original change: https://git.drupalcode.org/project/gin_everywhere/-/merge_requests/19/di... (which is also incorrect)
Comment #8
anybodyThanks, I can confirm this major issue!
Comment #9
hydra commentedHm 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.
Comment #10
hydra commentedComment #11
anybody@hydra right!
Like this?
Comment #12
anybody@grevil could you also please test, if the new MR works? Then we could merge and release this! Thanks!
Comment #13
hydra commentedThis looks good to me, if @grevil could confirm this works for his use-case I'll do a release with that imidiatly :)
Comment #14
grevil commentedNo, 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.
Comment #15
grevil commentedI 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 to see the template file).@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).
Comment #16
grevil commentedStatic patch for the time being.
Comment #17
hydra commented@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:
Comment #18
grevil commentedAh, 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...?
Comment #19
hydra commentedHey @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!
Comment #20
grevil commentedThanks 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.
Comment #21
grevil commentedAlright, 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.
Comment #22
grevil commentedFunnily 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!
Comment #24
hydra commentedI am totally fine with your suggestion! Merged.
Comment #25
hydra commentedMaybe we find a better title for this issue if we leave it open?
Comment #26
anybodyGreat @hydra! Are you planning to tag a new release containing this?
Regarding the title, something more general might make sense?
Comment #27
hydra commentedYeah just did not have the time yesterday. 1.2.3 is out containing the hotfix.
Comment #28
grevil commentedGreat stuff! Thank you, @hydra!