Fix a test case error: "Trying to access array offset on value of type null openstack_form_cloud_config_openstack_get_region()" (OpenStackCloudConfigTest)

Hi,

I have tried to fix the issue here. And as suggested, uploading the same patch file here to review.

diff -u b/modules/cloud_service_providers/openstack/openstack.module b/modules/cloud_service_providers/openstack/openstack.module
--- b/modules/cloud_service_providers/openstack/openstack.module
+++ b/modules/cloud_service_providers/openstack/openstack.module
@@ -293,8 +293,10 @@
   $response = OpenStackEc2Service::describeRegions([], $credentials);
 
   $region_name_response = [];
-  foreach ($response['Regions'] ?: [] as $regions) {
-    $region_name_response[] = $regions['RegionName'];
+  if (isset($response['Regions'])) {
+    foreach ($response['Regions'] ?: [] as $regions) {
+      $region_name_response[] = $regions['RegionName'];
+    }
   }

Note: The patch was as per the issue fixed for #3138080, I'd not recreated here.

@shobhit_juyal

Thank you for uploading the patch. Could you please re-create the patch? We've merged the patch at #3138080.

Working to upload patch

Hi @yas,

Here is the patch..

needs a review, unassigning myself.
Thanks

Thank you for the patches.

@all, I think we should merge the contents of both patches.

@shobhit_juyal's patch is great for fixing the PHP warning. @jigish.addweb's patch fills in the mockdata for the actual test.

@baldwinlouie

Thank you for your review. I also created the patch for the test case.

@shobhit_juyal

Thank you for re-creating the patch. The patch looks fixing the issue, and yes, actually the tests have been passed successfully however it might not fix the root cause. I guess the patch would only fix the issue by bypassing the test case. Our expectation is that OpenStack cloud service provider creation form validates if the parameter of the region is valid (= existing) or not by calling describeRegions() w/ the credential parameters if we pass the correct credentials.

@shobhit_juyal
@jigishaddweb

I added a test case for OpenStackCloudConfigTest::testCloudConfig() to make it error w/ a message 'Region is invalid. Please enter valid region.' by passing a wrong input parameter of the region while saving a new OpenStack cloud service provider. So could you please check the test w/ that patch?

@jigishaddweb

Yes, that's what I recognized. If my additional test case is correct (please review it carefully), the current code have something wrong so it should be fixed.

I re-created the patch for the additional test case so you can use it.

@jigishaddweb

Thank you for updating the patch. It looks good, so could you please change isset to !empty?

+++ b/modules/cloud_service_providers/openstack/openstack.module
@@ -121,19 +121,13 @@ function openstack_cloud_config_presave(CloudConfig $cloud_config) {
...
     if (isset($cloud_config->get('field_access_key')->value) && isset($cloud_config->get('field_secret_key')->value)) {

+++ b/modules/cloud_service_providers/openstack/openstack.module
@@ -293,8 +374,10 @@ function openstack_form_cloud_config_openstack_get_region(array $credentials = [
...
+  if (isset($response['Regions'])) {

Hi @yas

Here, I guess, isset is fine because it check the existence of the array's key.

+++ b/modules/cloud_service_providers/openstack/openstack.module
@@ -293,8 +374,10 @@ function openstack_form_cloud_config_openstack_get_region(array $credentials = [
...
+  if (isset($response['Regions'])) {

Thanks

@shobhit_juyal

Thank you for your review. We are trying to use !empty in any cases of the isset context in the entire Cloud family modules since isset and !empty is not exactly the same; and !empty will more fit to our expectation in most of cases.

Especially in the following case, we don't want to pass the variable $response['Regions'] at the if statement in case of $response['Regions'] = [].

+++ b/modules/cloud_service_providers/openstack/openstack.module
@@ -293,8 +374,10 @@ function openstack_form_cloud_config_openstack_get_region(array $credentials = [
...
+  if (isset($response['Regions'])) {

Moreover, we put ?: [] in foreach statement to avoid null pointer exception. e.g.

+    foreach ($response['Regions'] ?: [] as $regions) {

HTH

@jigishaddweb

Thank you for the update. It looks good to me now.

@baldwinlouie

What do you think?

@yas and @jigish, The patch looks good to me. I do have the following comment, but it can be another ticket for refactoring.

+++ b/modules/cloud_service_providers/openstack/openstack.module
@@ -280,6 +336,31 @@ function openstack_form_cloud_config_openstack_region_validate(array &$form, For
+function openstack_create_credential_file($cloud_context, $access_key, $secret_key) {
+  // Write the access key ID and secret out to an ini file.
+  $access_data = <<<EOF
+[default]
+aws_access_key_id = {$access_key}
+aws_secret_access_key = {$secret_key}
+EOF;
+  $credential_file = aws_cloud_ini_file_path($cloud_context);
+  $credential_directory = aws_cloud_ini_directory();
+  if (\Drupal::service('file_system')->prepareDirectory($credential_directory, FileSystemInterface::CREATE_DIRECTORY | FileSystemInterface::MODIFY_PERMISSIONS)) {
+    \Drupal::service('file_system')->saveData($access_data, $credential_file, FileSystemInterface::EXISTS_REPLACE);
+  }
+
+}
+

This can be another ticket, but I think this function can move up to aws_cloud module. aws_cloud has a similar function, and I think we can refactor this.

@baldwinlouie

Thank you for your review. That's a good point, so let's address it in another issue. I'll merge the patch to 8.x-1.x and 8.x-2.x and close this issue as Fixed.