Remove hardcoding #group for the status field

pradhumanjain2311 → made their first commit to this issue’s fork.

Can you please test this with what we're working on in ✨ Move Action buttons to sticky header Fixed ?

tested it on the dev version with the suggested changes from ✨ Move Action buttons to sticky header Fixed
nothing has changed (I also noticed that the sidebar switch is hidden in the ellipsis, it seems it shouldn't be like that)

@maks

tested it on the dev version with the suggested changes from #3356717: Move Action buttons to sticky header
nothing has changed

Okay thanks for verifying

(I also noticed that the sidebar switch is hidden in the ellipsis, it seems it shouldn't be like that)

That is intentional

We have experienced this same issue in farmOS for some time where the entity status field is not a checkbox. I solved it in other ways (our own version of GinContentForm so to say) but agree with the proposed fix to only move the status when it is a checkbox.

Furthermore, I'm running into this issue on *simple forms* (not entity forms, not gin content forms) after ✨ Move Action buttons to sticky header Fixed . With this change and status field in a form is moved out of the form to the actions area. Not only is this unexpected, but it also results in this status field not being included in the form submission (I believe because the status is moved without the additional template changes that content forms receive?)

I think the fix is two parts:
1. Only move the status field on gin content forms
2. Only move the status field when it is a checkbox

I've opened a new MR with these changes. It also includes some changes to simplify GinContentFormHelper logic and also likely increase performance (don't invoke thegin_content_form_routes hook multiple times!!!)

I've rebased this MR onto the latest 8.x-3.x, there was only one minor conflict with the last commit with this MR. Still need a review on this

Not able to Replicate this issue on my local.

Checked with installing examples module ( https://www.drupal.org/project/examples → ) also, But not able to reproduce it. Let me know if other specific configuration is needed, thanks!

Hi @maninders, appreciate you trying to recreate this. The important thing is that the form you are testing has a form field called status. I'm not sure if there is a custom form in the Examples module that fits this criteria. Testing likely requires creating a custom form.

I tested with the latest 8.x-3.x and confirmed the same issue is happening. There were some changes in GinContentForm so I've rebased my MR 446 onto the latest 8.x-3.x. Again, the full diff for this MR is a little ugly, please review the 4 commits individually to better understand the changes. I changed the order of these commits to make them easier to review.

The first two commits fix the issues previously described in this issue.

The third commit in the MR has the most changes because it combines two if statements into one and un-indents code from a nested if statement to simplify relevant logic in GinContentHelper::formAlter. These are the two if statements before changes:

    // Sticky action buttons.
    if ($this->stickyActionButtons($form, $form_state, $form_id) || $this->isContentForm($form, $form_state, $form_id)) {
      // Action buttons.
      if (isset($form['actions'])) {

In the last commit, instead of calling isContentForm() and invoking `hook_gin_content_for_routes` 3 separate times we can save the result in a variable and reference this value as needed.

Ultimately, the logic in the two above if statements are simplified to:

if (($use_sticky_action_buttons || $is_content_form) && isset($form['actions'])) {

I mentioned in Slack that I might open a separate issue with these additional changes that are not necessary to fix the issue. But because these changes would either conflict with or be dependent on the fix in this MR, it is a much easier contribution to include them here. If maintainers still consider this out of scope then please only consider the first two commits from my MR. Cheers

Yes, agreed. While it is literally off-topic, it really makes sense to optimize this code altogether. I've looked through and I'm happy with it, so setting to RTBC.

@paul121 maybe we should also open a follow-up issue to optimize \Drupal\gin\GinContentFormHelper::isContentForm as we should probably cache the result of that fairly expensive method.

maybe we should also open a follow-up issue to optimize \Drupal\gin\GinContentFormHelper::isContentForm as we should probably cache the result of that fairly expensive method.

Oh interesting! I haven't done something like this before. Curious how you would go about that.

saschaeggi → changed the visibility of the branch 3440148-remove-hardcoding-group to hidden.

Thanks Paul for your work on this 👏