Add the ability to set a machine name for "settings_group"

Makes sense, sounds like a good idea to make this separately configurable. Will need an update path so existing sites remain unchanged.

Hi, @scott_euser! I plan to start working on this issue.
I suggest adding a new field to our entity with the key "fieldset_machine_name" and making it optional then making the same method as loadByFieldset() to access the settings in PHP files. How is this solution?

Hi, @scott_euser!
I tried to add the "fieldset machine_name" field to the entity but it looks useless. We don't have a relationship between labels and machine names. I think will be better to create a new entity and change the field type from a string into an entity reference. What do you think about it?

Thanks for all your work on this lately! Sounds good to me, I guess one challenge is going to be having a smooth upgrade path for existing sites.

I think it will be a very interesting challenge). Probably you have any ideas on how to add a machine name for "settings_group"?
We can discuss them and implement the best solution.

Hi bobi-mel,

Just thinking about this one, probably best to be a custom entity type, otherwise we get into permissions/rabbit hole requirements for Taxonomy Terms and start requiring additional modules I imagine.

That make sense to you as well?

I'm going to join the discussion to help @bobi-mel with this task.

Since a new entity will be defined need to think about backward compatibility.

1) There is no problem with creating group entities using available data using hook_post_update().
2) Need to update existing tests and create a few new ones for the new entity type.

BTW this feature must be released as a major update.

Hi, @scott_euser!
I continue working on this issue and tried to create a config entity with bundles. But I think a config entity would be enough because it provides the possibility to automatically convert from Cyrillic symbols to English for example 'Назва' => 'nazva'. I am doing a little cleanup for my solution.
No CTA, just FYI.

Makes sense, thanks for the update!

Just setting to needs work to reflect that work is in progress

Hi, @scott_euser!

I continue working on this task. In the new commit, I have done the following:
- removed useless the SiteSettingsGroup content entity.
- added the possibility of dynamically creating the SiteSettingsGroupType config entity.
- changed the SiteSetting table for displaying entity instead of the Fieldset text field.
- fix bugs that were displayed in logs.
- beta version of hook_update for adding new field Group, generating entities, and removing the Fieldset field as useless.

I faced some problems with data migration, but I am working on it and hope to finish as soon as possible.

Nice work! Sounds like you are most of the way there then!

Hi, @scott_euser @Panchuk
I finished work on the issue. Please check and test it.

Thanks for the work on this! I gave it a first spin, added some feedback.

I'll fix these remarks

Hi, @scott_euser!
I fixed and commented on your remarks

Hi Bobi-mel,

Thanks again for the work on this! Almost there, but if you have time would be great if you can figure out why the tests are failing to ensure status quo is maintained on 8.x-1.x branch. I knocked 2 off from the config install of the test module but the remaining 2 failures seem like legitimate failures that need looking into.

Thanks!
Scott

Ok I'll check it

Not quite sure why, but when I attempt to test this out, the entity type is not getting installed

Screenshot from admin/reports/status:

Perhaps you can take a look at installing this from an existing site and seeing what is going wrong?

For the tests, I believe the failure is because gitlab CI now uses concurrency and I think the clickLink() bits rely on previous tests having run (which is of course a bad idea). I fixed that in 2x and I'll backport that to here once we get the primary functionality of the group entity type working well.

BTW, still targeting this to 8.x-1.x branch. I will then deal with cherry-picking it into the 2.x branch and resolving conflicts.

Hi @scott_euser
I'll check it

Hi @scott_euser

I tried to reproduce this bug
it occurs only when doing the following combination of actions:
- create an entity on the 8.x-1.x branch
- go to the 3365584-add-the-ability branch and run the drush updb command (site_settings_update_10004)
- turn off the module and return to the 8.x-1.x branch again
- re-enable and create new site settings entities there
- go to the 3365584-add-the-ability branch and run the drush updb command. The same update hook 10004 is launched.
but it does not go through, also always fails even if you change its number.

When switching between branches and running the update hook 10004 for the first time, everything always goes as it should.
I think that during testing, this hook was launched several times.

Have you tried applying these changes to a database on which site_settings_update_10004 did not run?

Yeah I was rolling back my database for a local copy of a production client with >50 site settings entities to test it rather than just manually setting the installed version number

I'll try it from a fresh install like you tried to see if I can spot the difference

Okay, I think its getting close. I changed the target to 2x branch because I think ultimately its too much of a breaking change given we renamed fieldset to group here. I resolved conflicts by pulling in everything from the 2x branch into here + updated the new code provided by the branch to reference group instead of fieldset.

The problem that is occurring at the moment is that anyone other than admin cannot see the group. I did a first attempt at that with access handler but its not working correctly yet. @bobi-mel could you attempt to sort this? I believe sorting it out should resolve the test failures as the tests are failing as a result of the group not showing (and subsequent fails are hit as a result of that).

Added a follow-up for after: https://www.drupal.org/project/site_settings/issues/3400025

Ok, I'll fix it

Hi @scott_euser
Investigated changes were added in your commits, found and fixed another problem. Also, I can't reproduce the same bug as on the screenshot. Could you add more information on how to get this bug?

If you do a default site install, Core now ships with content editor role. If you grant just the access the site settings overview permission, then attempt view the site settings entity list, you cannot see the group.

When I run

./vendor/bin/phpunit -c core modules/contrib/site_settings/src/Tests/SiteSettingsUiTest.php --filter=testSiteSettingsUpdateGroup

Its in the final browser output of the error as well.

Okay as far as I can see, the reason the tests are failing is being further work is actually needed. From what I can see its:

1. site settings entity type needs to have the group as a config dependency
2. site settings sample data module config/install dir needs to be redone with the new config

I have added (1) to the merge request but (2) needs doing if bobi-mel or anyone is able to pick this up.

Hi @scott_euser
I have found and fixed the reason why the group names were not displayed. However, the test failed due to the following error 'Behat\Mink\Exception\DriverException:
There is no element matching XPath "descendant-or-self::*[@id = 'site-setting-11']/descendant-or-self::*/*[@class and contains(concat(' ', normalize-space(@class), ' '), ' view-group-table-column ')]/descendant-or-self::*/a" '

More details here

Could you please check the correctness of the test case, maybe it needs to be changed.

Hi @scott_euser
I investigated the 'testSiteSettingsUpdateGroup' test and found several mistakes. After fixing the tests, they passed successfully.
Please check MR.

Thanks for all the work on this and for sorting that test headache!

Okay I figured out the issue I was having testing this out on some sites. It works fine upgrade from Site Settings Entities that have been created, but any Site Setting Entity Type that has not yet been created then misses a Group. As a result, if there is 1 or more site setting not yet created, there is a fatal error after site_settings_update_10006 when you visit /admin/content/site-settings

TypeError: Drupal\site_settings\Plugin\SiteSettingsLoader\FlattenedSiteSettingsLoader::groupKey(): Argument #1 ($group) must be of type string, null given, called in /var/www/html/web/modules/contrib/site_settings/src/Plugin/SiteSettingsLoader/FlattenedSiteSettingsLoader.php on line 226 in Drupal\site_settings\Plugin\SiteSettingsLoader\FlattenedSiteSettingsLoader->groupKey() (line 394 of modules/contrib/site_settings/src/Plugin/SiteSettingsLoader/FlattenedSiteSettingsLoader.php).

I think instead of looping through the Site Settings we should loop through the Types, create the Groups via that, then load the Site Settings and update their Group to match the Group that the Type now has.

Re-running tests after merge from 2.0.x changes, manually retesting as well.

Thanks for the patience here! If we need to we can create follow-ups but manual testing + automated testing seems to work fine here. This was a big one, nice to finally get it in. Thanks particularly to @bobi-mel

HI! @scott_euser
Thank you for your support!