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

CRM-19463 Get E2E tests working on php7 #9268

Merged
merged 5 commits into from
Oct 20, 2016
Merged

Conversation

seamuslee001
Copy link
Contributor

@seamuslee001 seamuslee001 commented Oct 16, 2016

@totten I would appreciate your thoughts on this, This feels awfully strange way of solving a problem i ran into running E2E tests locally on php7.


@totten
Copy link
Member

totten commented Oct 16, 2016

The PR seems interesting in demonstrating where the problem lies, but it's a hard compatibility break. :(

@seamuslee001
Copy link
Contributor Author

@totten Yeh Tim, I'm not sure mate This solved the problem for me, Now arguably i think we should be trying to keep it as an array in the SOAP and just ensure its encoded utf-8 but i couldn't figure it out and I don't understand SOAP enough

@seamuslee001
Copy link
Contributor Author

@totten after some more googling i have come up with this and chucking some debugging in suggests that we are outputting in the same format from php5 to php7

@seamuslee001
Copy link
Contributor Author

Jenkins re test this please

$array[$key] = self::encode_items($value);
}
else {
$array[$key] = mb_convert_encoding($value, mb_detect_encoding($value, mb_detect_order(), TRUE), 'UTF-8');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@totten @colemanw do either of you know a way of doing this without using mb_ functions?

Copy link
Member

Choose a reason for hiding this comment

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

I wouldn't worry about alternatives to mb_* if the system-check displays a warning about mb_*

$array[$key] = self::encode_items($value);
}
else {
$array[$key] = mb_convert_encoding($value, mb_detect_encoding($value, mb_detect_order(), TRUE), 'UTF-8');
Copy link
Member

Choose a reason for hiding this comment

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

OK, this has the effect of changing how some return values are encoded in SOAP XML -- ie as xsd:int vs xsd:string. A quick fix seems to be:

diff --git a/CRM/Utils/Array.php b/CRM/Utils/Array.php
index 8537409..eaca889 100644
--- a/CRM/Utils/Array.php
+++ b/CRM/Utils/Array.php
@@ -1088,6 +1088,9 @@ class CRM_Utils_Array {
       if (is_array($value)) {
         $array[$key] = self::encode_items($value);
       }
+      elseif (is_int($value)) {
+        $array[$key] = $value;
+      }
       else {
         $array[$key] = mb_convert_encoding($value, mb_detect_encoding($value, mb_detect_order(), TRUE), 'UTF-8');
       }

Copy link
Member

Choose a reason for hiding this comment

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

Actually, there might be other primitive types. This seems better and also passes for me in PHP7:

diff --git a/CRM/Utils/Array.php b/CRM/Utils/Array.php
index 8537409..d82250c 100644
--- a/CRM/Utils/Array.php
+++ b/CRM/Utils/Array.php
@@ -1088,9 +1088,12 @@ class CRM_Utils_Array {
       if (is_array($value)) {
         $array[$key] = self::encode_items($value);
       }
-      else {
+      elseif (is_string($value)) {
         $array[$key] = mb_convert_encoding($value, mb_detect_encoding($value, mb_detect_order(), TRUE), 'UTF-8');
       }
+      else {
+        $array[$key] = $value;
+      }
     }
     return $array;
   }

@seamuslee001
Copy link
Contributor Author

jenkins re test this please

@totten
Copy link
Member

totten commented Oct 20, 2016

Tests passing. When I manually compared the XML outputted (http://php.net/manual/en/soapclient.getlastresponse.php) before and after the patch, it appeared equivalent.

@totten totten merged commit 62d798d into civicrm:master Oct 20, 2016
@seamuslee001 seamuslee001 deleted the CRM-19463 branch November 23, 2016 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants