fix regression with unit regex, update dependencies and tooling - #4
Conversation
zerodahero
left a comment
There was a problem hiding this comment.
Looks like this change should do it--I'm not finding any cases it regresses or fails to match. We should stay consistent with the repo and use data providers though.
|
I have also updated the GitHub Action configuration so that it runs properly, and now tests the library on PHP 8.0, 8.1, 8.2, 8.3, 8.4, and 8.5. (It runs the PHPUnit tests only on PHP 8.4 and 8.5, as the latest versions of PHPUnit only support PHP <=8.4.) |
| - { php-version: '8.0', run-tests: false } | ||
| - { php-version: '8.1', run-tests: false } | ||
| - { php-version: '8.2', run-tests: false } | ||
| - { php-version: '8.3', run-tests: false } | ||
| - { php-version: '8.4', run-tests: true } | ||
| - { php-version: '8.5', run-tests: true } |
There was a problem hiding this comment.
This effectively removes support for 8.0, 8.1, 8.2, and 8.3. I'm fine dropping 8.0 and 8.1, but 8.2 and 8.3 are still in maintenance mode (at least for a few more months for 8.2).
As long as the new config can work for phpunit 11, we should be able to set the composer version to ^11|^12|^13 to be able to support v11 and PHP 8.2.
There was a problem hiding this comment.
This would also mean we'd need to keep the docblock annotations instead of the #[] ones.
There was a problem hiding this comment.
Great points! The docblock annotations are ignored starting in v12, when the #[DataProvider] attribute was added. The test file now has both, which I think is OK (and I've added a comment explaining) until you drop support for PHP 8.2.
|
All green and merged in! New release cut as 3.1.0 (minor bump since we dropped some PHP versions). Thanks for the fix and for the quick follow through on notes! |
|
Thank you for the quick review and merge! I appreciate the chance to contribute here and very happy this repo exists. |
This PR fixes an issue in the previous PR that would interpret the first word of a multi-word Place (city) name (e.g. 'Los Angeles') as part of the unit.
This PR also updates PHPUnit to the latest version and updates its config file and syntax accordingly.
This PR also updates the GitHub Action so that it runs successfully and tests the library on all supported versions of PHP.
The Bug
Given the address:
123 Main Street, Los Angeles, CA 90012the
Normalizer->parse()method would return:123 Main St #Los, Angeles, CA 90012This was happening because the change to the regex added an empty alternative to the end of the pattern looking for a Unit prefix:
With an empty prefix, the
([\w-]+)pattern will find the first word of a multi-word City and erroneously interpret it as a Unit.The Fix
I've refactored the one-line Unit regex into a multi-line one and added code comments to make it easier to read and understand what it's doing.
The main change here is changing the last alternative from empty to
(?=[\w-]*\d), which allows a unit prefix to be 1) a word ('Apt' or 'Suite' etc.), 2) a pound sign (#), or 3) nothing, as long as the unit contains a number. Cities don't start with numbers, so we can safely assume the next word is part of the City.