Use of "iterable" Type May Need Re-Evaluation Across the Repo

[Grunet]

Describe your environment Describe any aspect of your environment relevant to the problem, including your php version (php -v will tell you your current version), version numbers of installed dependencies, information about your cloud hosting provider, etc. If you're reporting a problem with a specific version of a library in this repo, please check whether the problem has been fixed on master.

The repo opened up in Gitpod (so the dev environment?)

Steps to reproduce I haven't reproduced the problem directly in the repo yet, but it was pointed out to me that there are many instances in the repo where something is typed as an "iterable" but treated like an ordinary array , which may not always function as intended.

For example, on this occasion, "empty" is called on the iterable type, but if that iterable is something like an exhausted generator, it counterintuitively won't return "true" (as I tried to reproduce in this fiddle)

What is the expected behavior? For the specific case above, an iterable that doesn't have any content remaining should be considered the same as an empty array, not the opposite

What is the actual behavior? For the specific case above, passing in an exhausted generator would cause it to skip the initial check and proceed to the rest of the function I think

Additional context There was a long discussion around part of this in Slack that may be valuable to review

[bhaibel]

@Grunet Reading this and the thread for your PR #412, it sounds like you believe #412 makes a dent in the issue but doesn't completely solve it. This issue is also really open-ended.

You might not know what next steps for smoking out all other potential bugs would look like. But if you have even kind of a place you would suggest starting, would you mind leaving that as a breadcrumb for other developers? Right now this bug is written in a way where it seems hard to start work on, and anything that makes it easier to work will help it actually get worked.

[Grunet]

For sure. IMO a first step could be to figure out how many of the usages of the "iterable" type are actually part of the public API surface for the library vs being part of a method marked "public" (since PHP doesn't have a concept of "internal" methods or similar AFAIK it's hard for me at least to tell what isn't accessible to consumers, by design or otherwise).

For example, Github's find references feature is telling me the AbstractExporter::export method (which types its input as "iterable") is currently only used in 3 places in the repo, all of which seem to just be passing an array. So if it also isn't directly exposed to consumers, it (probably) doesn't suffer from this concern, and maybe could just be updated to have the input be typed as "array" to avoid being misleading.

If there are some cases where an "iterable" type is given to a consumer-facing input, I think it would then become a question of either

• Changing the type to "array" to simplify the repo's handling (though this could also be a breaking change in theory)
• Updating the repo's handling to cover arbitrary "iterable" type inputs appropriately

I don't have a good (or any...) sense of the repo to know which is more common, nor do I know of any automated PHP dependency graph analysis tools that could maybe help cut down on manual effort for figuring this out (though would be interested if there are some).

Sorry that was just kind of a dump, but if that seems like an appropriate next step for this I can condense it and update the issue to mention that abbreviated version.

[tidal]

A quick fix for this issue would be to cast the iterator to an array in cases where array behavior is expected but an iterable given. PHP's function iterator_to_array would do the job here.

[tidal]

@Grunet Did you plan to work on this? Otherwise we can add a help-wanted label.

[Grunet]

Nope not at the moment. Required a bit more familiarity with the library overall than I had/still have to properly tackle I think. And yeah the label seems useful to me.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[amber0612]

@tidal I can take this up

[amber0612]

One fix that i tried is this. I believe it will be more efficient solution.