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

Fix: explode() parameter type compatibility issue in CatalogItemsV20220401Api #831

Open
burakaktna opened this issue Dec 16, 2024 · 1 comment

Comments

@burakaktna
Copy link

burakaktna commented Dec 16, 2024

Issue Description

In PR #700 (merged on Jun 10), a change was introduced to use explode() for parameter validation in CatalogItemsV20220401Api. While the PR author noted "it would make more sense if these parameters were typed as arrays to begin with, but making a change like that would break existing implementations", the implementation actually caused the breaking change it aimed to avoid.

This change was released in v5.10.3 and is now strictly enforcing string type for its second parameter in explode(). Previously, this parameter accepted both string and array types, which allowed for more flexible usage. This change has caused TypeErrors in our production environment:

TypeError: explode(): Argument #2 ($string) must be of type string, array given

Impact

Expected Behavior

The function should maintain backwards compatibility by accepting both string and array types as it did previously. This flexibility was a key feature that many implementations rely on and was the original intention in PR #700.

Current Behavior

The function now strictly accepts only string type, throwing a TypeError when an array is provided, which contradicts the PR #700's goal of avoiding breaking changes.

Why This Matters

  1. Backwards Compatibility: Many existing implementations rely on the dual type acceptance, which was the explicit goal of PR Fix incorrect parameter checks inside searchCatalogItemsRequest #700
  2. Test Coverage: This incident highlights the importance of comprehensive test coverage for parameter types and edge cases
  3. Breaking Changes: Such type restrictions should be introduced with proper deprecation notices

Proposed Solution

Modify the parameter type definition to accept both string and array types, restoring the functionality that PR #700 intended to preserve. This ensures existing implementations continue to work without requiring extensive revisions.

Additional Context

The issue occurs in:
/vendor/jlevers/selling-partner-api/lib/Api/CatalogItemsV20220401Api.php at line 856

The change affects three parameter validations:

  • marketplace_ids
  • identifiers
  • keywords

This issue demonstrates the critical importance of:

  • Maintaining backwards compatibility as explicitly intended
  • Comprehensive test coverage
  • Careful consideration of type restrictions in public APIs
burakaktna added a commit to burakaktna/selling-partner-api that referenced this issue Dec 16, 2024
- Add getArraySize helper method for consistent counting
- Maintain array handling in query parameters
- Fix backwards compatibility from PR jlevers#700

Fixes jlevers#831
burakaktna added a commit to burakaktna/selling-partner-api that referenced this issue Dec 17, 2024
- Add getArraySize helper method
- Maintain array handling in query parameters
- Fix backwards compatibility from PR jlevers#700

Fixes jlevers#831
@jlevers
Copy link
Owner

jlevers commented Dec 27, 2024

@iajrz could you take a look at this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants