[REF] Clarify not-so-great code in the Group BAO #26074
Conversation
|
(Standard links)
|
| @@ -20,6 +20,7 @@ | |||
| */ | |||
|
|
|||
| return [ | |||
| // FIXME: This is arguably the worst name for a setting ever | |||
| 'is_enabled' => [ | |||
There was a problem hiding this comment.
@eileenmcnaughton maybe we could add a way to rename and alias settings? Probably trigger a deprecation when accessed by the old name.
There was a problem hiding this comment.
I believe Eileen did a rename/reformat a few years back for a setting called contribution_invoice_settings (which was apparently a pretty gnarly setting). A straight-up rename should be simpler, but the important thing is that it led to the methods SettingsBag::updateVirtual() and SettingsBag::createVirtual(). That would probably be the easiest place to inject an alias mechanism.
There was a problem hiding this comment.
@eileenmcnaughton I feel like its time to add noisy deprecation to contribution_invoice_settings
|
Thanks @mattwire! |
|
If we remove the parents in the CRM_Contact_BAO_Group::create function, I think this code will be redundant : https://github.com/civicrm/civicrm-core/blob/master/CRM/Group/Form/Edit.php#L372 |
|
@samuelsov yes, that's true. The code is all a bit complicated but please feel free to try a PR. |
Overview
Adds comments and a bit of tidying to some problematic code
Before
Problematic code in Group BAO; can only add parents but cannot remove them.
After
Better documented problematic code