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

[Core: Utility] Remove asArray function #4133

Closed
wants to merge 1 commit into from
Closed

[Core: Utility] Remove asArray function #4133

wants to merge 1 commit into from

Conversation

johnsaigle
Copy link
Contributor

Brief summary of changes

asArray() had a cleanup tag about deleting it and replacing its calling code with toArray instead since the functions are basically equivalent.

@johnsaigle johnsaigle added Cleanup PR or issue introducing/requiring at least one clean-up operation [branch] major labels Nov 20, 2018
Copy link
Collaborator

@HenriRabalais HenriRabalais left a comment

Choose a reason for hiding this comment

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

looks good!

@kongtiaowang kongtiaowang added the State: Needs work PR awaiting additional work by the author to proceed label Nov 26, 2018
@kongtiaowang
Copy link
Contributor

php lorisform_parser.php
PHP Fatal error: Uncaught TypeError: Return value of Utility::toArray() must be of the type array, string returned in /var/www/loris/php/libraries/Utility.class.inc:491

@johnsaigle johnsaigle added the State: Discussion required PR or issue that requires the resolution of a discussion with the relevant parties to proceed label Nov 27, 2018
@driusan driusan requested a review from xlecours November 27, 2018 18:07
@johnsaigle
Copy link
Contributor Author

johnsaigle commented Nov 27, 2018

Upon further review, these functions actually have quite different behaviour.

  • toArray() takes array|string. When given an associative array (i.e. one that does not have 0 as an array key) it returns a new numeric array containing the associative_array as its first element. IN the case of numeric arrays or strings, they simply get returned to the calling code unchanged.

  • asArray() converts scalar values to arrays, and returns arrays.

We can take two approaches here:

  1. Update the function docs to make these distinctions clear. At the very least we need to remove the cleanup note about asArray()
  2. Do a deeper refactor:
    • Change toArray() to toNumericArray() and ensure that only associative arrays are passed as parameters. Use TypeErrors or DeprecatedExceptions to prevent calling code from passing scalar values.

Thanks to @xlecours and @kongtiaowang for doing testing on this. @xlecours @driusan Do you have a preference as to what approach to use here?

@driusan
Copy link
Collaborator

driusan commented Nov 27, 2018

I prefer simplifying if possible (so approach #2), personally.. but you can always start by improving the docs

@johnsaigle
Copy link
Contributor Author

Closing this PR in favour of #4151

@johnsaigle johnsaigle closed this Nov 27, 2018
@johnsaigle johnsaigle removed the State: Needs work PR awaiting additional work by the author to proceed label Nov 27, 2018
driusan pushed a commit that referenced this pull request Dec 12, 2018
Following #4133 toArray() is being deprecated in favour of the more precise function associativeToNumericArray(). This function does the same thing as toArray() but only accepts and returns arrays.

This change reduces confusion between toArray and asArray as well as allows us to be more precise with the way we're using types.

The calling code has been updated to use the new function and issue a deprecation warning.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Cleanup PR or issue introducing/requiring at least one clean-up operation State: Discussion required PR or issue that requires the resolution of a discussion with the relevant parties to proceed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants