Allow Field Group Elements to be grouped

Josh Waihi → closed merge request !427

Thanks @Josh Waihi for your contribution. I've left some comments in the MR.

As this is a new feature that can only be added to the 2.x branch, please update the MR accordingly.

Also, this needs an update hook to initialize the new group value for existing ECA models; otherwise this is logging warning about undefined array keys.

jurgenhaas → closed merge request !428

I don't know how to approach the update hook here. Is this something you can add or give me some guidance on how to write it?

TIL when the target branch is changed in an issue, it also changes the diff/patch (which I was using in my composer patches for a project). So attaching a static patch for 1.1.x for reference (accepting it won't be added).

Thanks a lot @Josh Waihi, I've added two more comments in the MR.

I don't know how to approach the update hook here. Is this something you can add or give me some guidance on how to write it?

An example can be eca_update_8001() which loads all existing ECA entities. You could loop over all action plugins and if the FormAddGroupElement plugin is being used but doesn't have the group property in the configuration, add that as an empty string and save the config entity.

Having said that, it just comes to mind that we have the not widely known Drush command drush eca:update which goes through all ECA entities and updates them if necessary. Would you mind testing this, if it updates a model properly?

TIL when the target branch is changed in an issue, it also changes the diff/patch (which I was using in my composer patches for a project). So attaching a static patch for 1.1.x for reference (accepting it won't be added).

Patches should never be used from d.o, it is always strongly recommended to download them and apply them from a source that is fully under your control. There have been recent discussions, that the migration to GitLab may mean that all patch files will be gone. While that will break a lot of deployments, it is still under consideration due to that strong recommendation, that had always been in place.

I tried testing drush eca:update:

1. I updated my model config in YAML and removed the "group" configuration setting.
2. Then used drush config:import to persist that configuration into the database
3. Then I ran drush eca:update expecting it to update configuration in the database
5. Then I ran drush config:export expecting it to recreate the "group" configuration setting with an empty value

However, config:export did not introduce any changes indicating eca:update did not work as expected.

Update hook added. Ready for another review.

I tried testing drush eca:update

That drush command is not involved in any of the update hooks. That's for a different purpose.

What you need to test is to run drush updatedb and you should see, that it will execute the eca_form_update_8003 update hook. For that to work, you don't need to manually edit any yaml file, that should all be done by that hook.

Also, please fix the pipeline error before setting to "Needs review".

For that to work, you don't need to manually edit any yaml file, that should all be done by that hook.

I needed to revert my config so I could test a state where the hook would actually change the config.

Pipeline fixes done.

Whoops, wrong status

Thanks a lot @Josh Waihi for your patient work on this. I've reviewed it and will merge in a minute.

w00t! Thanks!

@jurgenhaas, can this also be merged into 2.0.x?

@Josh Waihi unfortunately not, as this is a new feature. The 2.0.x branch is for bug fixing only. But you can easily apply the diff of this commit to your composer project so that it automatically applies that change until 2.1.0 will be released.

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