Skip to content

fix: Ensure unmatched optional capture group is treated as null in parseRowColumnSelection - #578

Merged
nyamsprod merged 2 commits into
thephpleague:masterfrom
zigzagdev:fix/parse-row-column-selection
Aug 1, 2026
Merged

fix: Ensure unmatched optional capture group is treated as null in parseRowColumnSelection#578
nyamsprod merged 2 commits into
thephpleague:masterfrom
zigzagdev:fix/parse-row-column-selection

Conversation

@zigzagdev

Copy link
Copy Markdown
Contributor

Summary

What I have done

  • Added PREG_UNMATCHED_AS_NULL flag to the preg_match call in parseRowColumnSelection()
  • Without this flag, PHP sets unmatched named capturing groups to '' (empty string) rather than leaving them absent, causing $found['end'] ?? null to return ''instead of null
  • This bug caused single-number selections (e.g. row=1, col=2) to always fail validation when combined with semicolon-separated expressions (e.g. row=1;3, col=1;3)

Where I have changed

  • src/FragmentFinder.php: Add PREG_UNMATCHED_AS_NULL to preg_match and simplify $end assignment from $found['end'] ?? null to $found['end']
  • src/TabularDataReaderTestCase.php: Add test cases covering following cases
    • Multiple single-number row/column selections (row=1;3, col=1;3)
    • Mixed single and range selections (row=1;3-5)
    • All-invalid single selections (row=0;0)
    • Mixed valid/invalid selections — matchingFirst returns partial results, matchingFirstOrFail throws FragmentNotFound

Test Plan

Expression What is verified
row=1;3 Two single-number row selections both resolve correctly without a range endpoint
row=1;3-5 A single-number selection and a range selection can coexist in one expression
col=1;3 Two single-number column selections both resolve correctly without a range endpoint
row=0;0 All-invalid single selections yield null from matchingFirst and throw from matchingFirstOrFail
row=0;3 (×2) When valid and invalid selections are mixed, matchingFirst returns only the valid result while matchingFirstOrFail throws FragmentNotFound

Test Cases Result

screen shot 2026-05-29 8 57 44

@nyamsprod

Copy link
Copy Markdown
Member

@zigzagdev sorry for the late reply I was busy. I am on the fence about this PR not because it is bad but because the feature itself is not on a good shape itself it requires fixing that can only happen once we moved to v10. I will try to review your PR later and see if I can still merge it. Either way thanks for the contribution.

@zigzagdev

Copy link
Copy Markdown
Contributor Author

@nyamsprod

Thank you for the feedback! I understand the feature needs design improvements in v10.

I'll keep this PR open and wait for v10. When you're ready to review or if there's anything I should adjust based on the v10 direction, please let me know.
I appreciate you taking the time to consider this contribution.

@nyamsprod
nyamsprod merged commit 1e7bb18 into thephpleague:master Aug 1, 2026
9 checks passed
@nyamsprod

Copy link
Copy Markdown
Member

@zigzagdev

I will merge your fix. No need to wait for v10. This part of the code will nevertheless be fix when I start working on v10 in the meantime this fixes an issue with the current implementation so let's just do that

Thanks for the contribution

@zigzagdev
zigzagdev deleted the fix/parse-row-column-selection branch August 2, 2026 04:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants