Fix: prevent fatal error during form migration

[pauloiankoski]

Resolves GIVE-243

Description

After version 3.3.0, form migrations began to fail with a fatal error. Upon investigation, I discovered that under certain conditions, the conditionals for executing the Funds & Designations step were attempting to count the elements of a nullable variable, resulting in the fatal error.

This pull request resolves the issue by replacing the count function with an !empty check. This fix will work for both nullable variables (returning false) and arrays.

Affects

Form Migrations

Testing Instructions

1. Create a v2 form
2. Migrate it

Pre-review Checklist

• Acceptance criteria satisfied and marked in related issue
• Relevant @unreleased tags included in DocBlocks
• Includes unit tests
• Reviewed by the designer (if follows a design)
• Self Review of code and UX completed

[glaubersilva]

@pauloiankoski I left a few comments suggesting changes in the conditions to make sure it will work with arrays.

Another solution would be to handle this problem inside the getFundsAndDesignationsAttributes() method.

If you take a look at line 650, you will see this: $options[0]

The problem here is that the index 0 of the $options var may not exist in certain scenarios, we can return an empty array instead of a null value. We also can return an empty array in the 651 line instead of a null value.

[pauloiankoski]

@glaubersilva What's the practical difference between is_array($fundsAndDesignationsAttributes['options']) && count($fundsAndDesignationsAttributes['options']) > 0 and !empty($fundsAndDesignationsAttributes['fund'])?

I implemented with !empty as we don't need the exact number of elements those arrays contains but only if they have elements (using count in a large set can be less performant than !empty). Also, I didn't use the is_array as our implementation will either return null or an array and the !empty will catch the nullable return as false.

[glaubersilva]

@pauloiankoski You are right. Seems that this is some internalized practice by me too many years ago that does not make sense anymore...

I remember that at some point in the past, I had some kind of problem using the empty() method with arrays (maybe with some old PHP version - not sure), so I set it in my mind to always use the count() method to prevent any problems.

But to be honest, revisiting it now I can't remember or reproduce any problem using it. This way, I believe we are ready to QA.

Thanks for the discussion!

About the fatal error during the migration, I wasn't able to reproduce it locally using the PHP 7.4.30 version. So if it was happening only in specific versions, we need to inform the QA team which version should be used to test this fix.

[pauloiankoski]

Thank you, @glaubersilva zito!

[rickalday]

Passed manual QA tests.