Make paragraphs module working with field_group version 3.x with support for the field_layout module

Thanks for creating an issue for it. Any chance we can get a patch? We would also need tests for the change.

The attached patch should do the trick.
It needs patch #11 from https://www.drupal.org/node/2878359

Support for common scenarios need test coverage. We already have around field group, so it should be easy to add tests for field_layout as well.

Triggering test bot once..

With my patch, paragraphs in forms (node/edit etc) do not work.
Paragraphs in the frontend (and ui_patterns with field groups for that matter) work.

It needs work.

Your patch is still using #pre_render. With the fieldgroup patch, you need #process.

An updated patch. But not fully working yet. Tabs are loaded, but fields are not in it.

Triggering test bot.

Opened this related issue: #2922650: Move field_group integration of paragraphs to field_group module.

The test needs work, but for anyone reviewing the patch, you also need the field_group patch at #11 #2878359: Field groups are not compatible with field layout.

Please test with the latest patch in #33 of #2878359: Field groups are not compatible with field layout for me that finally fixes this mad problem. Please review and feedback. You can find my feedback in #35 of the other issue.

Test bot will not be able to test this completely I think because both patches are needed in combination?

Tested with patch #36 and #33 on #2878359: Field groups are not compatible with field layout and in both cases, while the error messages are gone, I'm hitting an issue where the field_group inside the Paragraph type disappears. If I make the paragraph field 'required' and configure the paragraph widget to auto-create the first instance, then the field_group inside the paragraph gets moved outside the paragraph field, to the main node creation form on /node/add/page.

An image to explain the above comment ^

Without either the patch from this issue nor the patch from #2878359 the field groups on the paragraph type works perfectly fine even with field layout module enabled, but breaks when you add a field group to the node itself.

Sadly I have to confirm that we encountered the same problem like described in #13 / #14.

@jwilson: Please retry with #52 from #2878359: Field groups are not compatible with field layout and provide some new feedback :)

I can finally confirm that #7 works in combination with #57 from #2878359: Field groups are not compatible with field layout!
So this can be set RTBC if the failing tests are due to the old fieldgroup version.

How shell we proceed here to finally fix this issue and tests?
Is there a way to run the tests cleanly in Drupal.org CI based on the combination of patches? Does somebody know?

I can confirm the issue is fixed too.
Sadly, we are going to need to wait until Field Group is patched to be able to run our automated tests.
We should wait a new Field Group release anyway or we are going to break BC if this patch is commited before the other one.

We're using the patch in combination with the field groups patch since month now without any further problems. I'll set this RTBC and this should be committed together with the #2878359: Field groups are not compatible with field layout.

Update: The patch of #2878359: Field groups are not compatible with field layout was just committed to DEV. You can commit this patch. Next step: Check if we can move this to field_group.

Happy to see integrations fixed and improved, but we meanwhile have tests for all (other?) contrib modules we integrate with.

Can we simply have a test that checks this is doing what it should, combined with field_group.

Not sure about committing and release sync. Are you saying this was before already integration code that is now broken with Dev? That would require us to sync a next release.

What confuses me even more is that field_group was just majorly refactored in the related issue but there was zero test coverage added / updated...

Hi, I've tested the patch and now I can use field_group with Paragraphs module. Thanks!

Patch applies cleanly against Paragraphs 8.x-1.5, seems to work properly with Field Group 8.x-3.x-dev. Paragraph fields now nested properly under field groups, only tested with Field Group types Details and HTML Element.

Changed issue title, now patch is committed in the field_group module and there is a new version (RC1).

Changed the issue summary because the patch is now committed in the new released version.

I'm a bit confused, could somebody confirm this assumption

This patch is only needed when using paragraphs, field_groups and field_layout at the same time. When field_layout isn't used this patch is not required?

Also the fact that this patch throws 4 errors specifically on field_groups makes me a bit nervous.

@Spokje At one time ( back in January ), I needed this patch for field_group, even though I was not using field_layout, but I was using layout_builder. I don't recall the exact situation, but I am no longer using the patch, probably an update to either field_group or layout_builder subsequently fixed the underlying issue I was experiencing. So, in my case, this patch is not needed.

@scottsawyer Thanks, really appreciate the elaborate explanation :)

Can we please have feedback from more folks, if this patch really isn't needed anymore?

You can see a test example here: https://www.drupal.org/project/field_group/issues/2994053
Did you try vertical tabs WITHOUT this patch?

I haven't had a chance to test without the patch, but FWIW, the release notes for Field Group 3.0-rc1 specifically warn paragraphs users to apply this patch:

Important for paragraph users: Make sure you apply the following paragraphs patch: https://www.drupal.org/project/paragraphs/issues/2907094

For me, vertical tabs work without the patch.

This is the combination used:

Core: 8.6.16
Field Groups: 3.0.0-rc1
Paragraphs: 1.8.0

Main difference, besides module versions, between me and the example provided in #31 seems to be that I'm not using Core Field Layout.
(Which was the main reason behind my initial question in #28)

This patch is mandatory for me:

Core 8.7.2
Paragraphs 1.8.0
Field Groups 3.0.0-rc1
Field Layout or Layout Builder are not enabled

Neither simple HTML elements like container div's nor vertical or horizontal tabs are working without it. Applying the patch fixes all my problems.

I have the same exact setup as @hudri and the patch is also necessary for me. HTML elements with vertical tabs (haven't tested horizontal) aren't working, but it works with the patch.

Core 8.7.2
Paragraphs 1.8.0
Field Groups 3.0.0-rc1
Field Layout or Layout Builder are not enabled

Patch is not working for me
Drupal 8.7.2 / Paragraphs 1.x-dev / Field Group 3.0-rc1

Attached is an updated patch that works against latest stable of paragraphs (1.8).

The main difference is that this patch actually checks which implementation of field_group is available and implements the correct one. So it works with both field_group 1.x and 3.x.

I will leave this ad "Needs work" because we still need some test.

Same situation as #35, using:
- Core 8.7.2
- Paragraphs 1.8.0
- Field Group 3.0.0-rc1
Field Layout or Layout Builder are not enabled

Support for common scenarios need test coverage. We already have around field group, so it should be easy to add tests for field_layout as well.

@miro_dietiker, the problem here is not that we don't have a test for field_layout (field_layout is not needed to reproduce this bug). The problem is that we need to test this with Field Group 3.0.0-rc1. Because in version 3, field_group_form_pre_render has been replaced with field_group_form_process.

Added a patch that should work for field_group 2 & 3.

@weseze seems like we both added a patch to support both versions at the same time. I've hidden mine as you added a test to check if the method exists.

Not sure why we'd need extra tests, this should already be covered?

I'm flagging #37 as rtbc as tests are green and I don't see how we could write a test for field_group 3.0 rc1.

I can confirm, that patch #37 applies cleanly against the latest dev of paragraphs and solved the problem. In our case, before applying the patch, the fields inside of a details element were gone on form display.

Core 8.7.3
Paragraphs 8.x-1.8+9-dev
Field Group 8.x-3.0-rc1
Field Layout and Layout Builder are not installed

Same here. Tested the last submitted patch (#39) and everything works great with it.

Lol stupid testBot is using #39 and removing the status from the issue...

@weseze maybe commit your patch again for the testBot and issue status ?? ;d

Rerolled #37. Credit should go to @weseze.

👍

Tested ... Patch in #7 is working.
Thank you :)

Confirming RTBC of #48. Commit finally?

Patch in 48 gives me the following error when using paragraphs with horizontal tabs in field group :

Notice: Undefined index: widget in Drupal\paragraphs\Plugin\Field\FieldWidget\InlineParagraphsWidget->massageFormValues() (line 1329 of modules/contrib/paragraphs/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php).

I've put a patch to fix this, hope this is done right.
Sorry if I'm holding up getting this released :)

Seems weird to add a fix in a line that follows this comment:

      // @todo: Maybe we should actually delete it here?

i wonder what the testbot would say if we actually remove it.

@andybroomfield, could it be that the notice originates in a missing configuration setting or are you using any other patches?

+++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
@@ -1320,8 +1325,9 @@ class InlineParagraphsWidget extends WidgetBase {
+      elseif(isset($widget_state['paragraphs'][$item['_original_delta']]['mode']) && ($widget_state['paragraphs'][$item['_original_delta']]['mode'] == 'remove' || $widget_state['paragraphs'][$item['_original_delta']]['mode'] == 'removed')) {

If we do change it, we should use === instead of ==

Yes, was a bit strange.
@mpp This is on a vanilla set up when I tested the patches, though I'm getting the error on a couple of sites.
It happens when I save the node.
so if I have a paragraph type that uses field group and horizontal tabs, then I save the node, the error message occurs.

I am using Drupal 8.7.3 and Paragraphs 1.6.0 and the patch in #52 fixes the problem for me. Thanks andy!

I tried to do a blame of the code to see why this piece of code was introduced but the commit message didn't contain a reference to a drupal.org issue. Would have been nice to have a reference to an issue somewhere.

      // If our mode is remove don't save or reference this entity.
      // @todo: Maybe we should actually delete it here?
      elseif(isset($widget_state['paragraphs'][$item['_original_delta']]['mode']) && ($widget_state['paragraphs'][$item['_original_delta']]['mode'] == 'remove' || $widget_state['paragraphs'][$item['_original_delta']]['mode'] == 'removed')) {
        $item['target_id'] = NULL;
        $item['target_revision_id'] = NULL;
      }

Let's see what testbot says if we remove it.

Removing the "Needs tests" tag as:
- I don't see how we could write a test for a release candidate (field_group 3.0 has a rc1)
- I don't know of any module that writes tests to cover future versions of other modules?
- There are tests for field_group 2.x and they're green

@miro, Please inform me if you believe we should still pursue tests and if there's a way to do it.

> - I don't see how we could write a test for a release candidate (field_group 3.0 has a rc1)
> - There are tests for field_group 2.x and they're green

field_group 1.x, actually, according to composer.json:

"drupal/field_group": "~1.0",

There is no field_group 2.x. What's not possible is having a test for both 1.x and 3.x. But we can pick one version, also an rc, we just need to update the version constraint to what we want. Our existing test will then simply run with that version (and might need to be updated, not sure)

Since 3.x is the only supported version, it makes sense to update to that and test with that.

Thanks for your input @berdir!

I'll see if I can update the tests to use 3.x.

Updated the dependency for field_group to 3.x since it's currently the only supported version.

This time without the redundant/deprecated(?) code:

      // If our mode is remove don't save or reference this entity.
      // @todo: Maybe we should actually delete it here?
      elseif(isset($widget_state['paragraphs'][$item['_original_delta']]['mode']) && ($widget_state['paragraphs'][$item['_original_delta']]['mode'] === 'remove' || $widget_state['paragraphs'][$item['_original_delta']]['mode'] === 'removed')) {
        $item['target_id'] = NULL;
        $item['target_revision_id'] = NULL;
      }

Looks like the name key is required in 3.x
Let's see if this fixes the tests.

That should have been 'region', not 'name'.

Testing with field_group 3.x-dev

FYI: #68 worked for me together with #3 from #2986704: Incompatibility with inline entity form

+++ b/src/Plugin/Field/FieldWidget/InlineParagraphsWidget.php
@@ -1319,12 +1324,6 @@ class InlineParagraphsWidget extends WidgetBase {
       }
-      // If our mode is remove don't save or reference this entity.
-      // @todo: Maybe we should actually delete it here?
-      elseif($widget_state['paragraphs'][$item['_original_delta']]['mode'] == 'remove' || $widget_state['paragraphs'][$item['_original_delta']]['mode'] == 'removed') {
-        $item['target_id'] = NULL;
-        $item['target_revision_id'] = NULL;
-      }
     }

this seems unrelated?

this seems unrelated?

"The check to see if mode is present is due to field group tabs."

An extra isset() check with the above comment was introduced in #52 so it would seem to be related. I could not find any history as to why this was introduced in the first place, there's no test for it and the comment mentions to remove the code.

I think that was an unrelated change, but that doesn't make removing it related :)

#3068880: Undefined index: widget updated that check, so I think there's no reason to remove it here. What's the problem with the notices, is that a field_group or something on our side?

I think there's no reason to remove it here.

Reverted that.

What's the problem with the notices, is that a field_group or something on our side?

I think it's a field_group issue.

Alright, that sounds like the test is blocked on field_group (and also that 3.x isn't very stable :-/).

One option would be to just commit the changes and then postpone committing the test update until that issue is fixed?

One option would be to just commit the changes and then postpone committing the test update until that issue is fixed?

Agreed, I removed the composer requirement for field_group 3.x.

Committed. I'd be fine with closing this and opening a follow-up for the tests.

Patch at #76 works. Thanks

F

@berdir, I created a new issue to write the tests.

I installed the dev version of paragraphs and it works for me. Thanks.

Users of field_group, see comment #12 + #13! You may need that additional patch.