Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[php8-compact][REF] Fix failing custom group test on php8 by better h… #20616

Merged
merged 3 commits into from
Jun 18, 2021

Conversation

seamuslee001
Copy link
Contributor

…andling strings in 2nd key of the extends array and also validating the child and main entity work

Overview

This fixes a failing unit test in CustomGroup API testing that is because we pass a string rather than an array as a 2nd param to the extends in API and it was somewhat not well handled. It does it by adding in validation on the subtype + entity combination

Before

testCustomGroupExtendsMultipleCreate test fails because php8 is harder on variable types in implode

After

testCustomGroupExtendsMultipleCreate passes on php8

ping @eileenmcnaughton @colemanw @totten @demeritcowboy

@civibot
Copy link

civibot bot commented Jun 16, 2021

(Standard links)

@civibot civibot bot added the master label Jun 16, 2021
@seamuslee001 seamuslee001 force-pushed the fix_custom_group_Test branch from a5ca284 to 1a8057c Compare June 16, 2021 03:01
@seamuslee001
Copy link
Contributor Author

This needs #20617 to be merged first to be able to pass

$registeredSubTypes = self::getSubTypes()[$params['extends']];
if (is_array($extendsChildType)) {
foreach ($extendsChildType as $childType) {
if (!array_key_exists($childType, $registeredSubTypes) && !in_array($childType, $registeredSubTypes, TRUE)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a bug in this validation. If you try to create a ParticipantRole group with a type through the form it fails because registeredTypes above has a key 'ParticipantRole' but $params['extends'] is 'Participant'

I tested this & it seems to work

--- a/CRM/Core/BAO/CustomGroup.php
+++ b/CRM/Core/BAO/CustomGroup.php
@@ -51,7 +51,9 @@ class CRM_Core_BAO_CustomGroup extends CRM_Core_DAO_CustomGroup {
       $extendsChildType = $params['extends_entity_column_value'];
     }
     if (!CRM_Utils_System::isNull($extendsChildType)) {
-      $registeredSubTypes = self::getSubTypes()[$params['extends']];
+
+      $b = self::getMungedEntity($params['extends'], $params['extends_entity_column_id'] ?? NULL);
+      $registeredSubTypes = self::getSubTypes()[$b];
       if (is_array($extendsChildType)) {
         foreach ($extendsChildType as $childType) {
           if (!array_key_exists($childType, $registeredSubTypes) && !in_array($childType, $registeredSubTypes, TRUE)) {
@@ -224,6 +226,24 @@ class CRM_Core_BAO_CustomGroup extends CRM_Core_DAO_CustomGroup {
     return CRM_Core_DAO::commonRetrieve('CRM_Core_DAO_CustomGroup', $params, $defaults);
   }
 
+  /**
+   * Get the munged entity.
+   *
+   * This is the entity eg. Relationship or the name of the sub entity
+   * e.g ParticipantRole.
+   *
+   * @param string $extends
+   * @param int|null $extendsEntityColumn
+   *
+   * @return string
+   */
+  protected static function getMungedEntity($extends, $extendsEntityColumn = NULL) {
+    if (!$extendsEntityColumn) {
+      return $extends;
+    }
+    return CRM_Core_OptionGroup::values('custom_data_type', FALSE, FALSE, FALSE, NULL, 'name')[$extendsEntityColumn];
+  }

@eileenmcnaughton
Copy link
Contributor

I hit a bug in r-run. I'm happy with the code once it passes r-run but want to note that it seems possible to change types (at least before fields are added) so I think r-run needs to include that & ensuring it's possible to change from a subtype-kinda-one to a main type & the reverse & the db columns are correctly updated

…andling strings in 2nd key of the extends array and also validating the child and main entity work
…te into apiv3 and change Custom Group form to use apiv3
@seamuslee001 seamuslee001 force-pushed the fix_custom_group_Test branch from ee185f7 to 92ef782 Compare June 18, 2021 04:43
@seamuslee001
Copy link
Contributor Author

thanks for the review @eileenmcnaughton I have applied your fix with a small modification and have also added 2 unit tests 1 covering the bug that you hit and 1 covering the changing of the entity when the custom group doesn't have any fields

@eileenmcnaughton
Copy link
Contributor

I did another r-run and was able to save ParticipantRole and change between types (including sub & non-sub types)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants