Skip to content

fix regression with unit regex, update dependencies and tooling - #4

Merged
zerodahero merged 26 commits into
zerodahero:mainfrom
matthewmcvickar:main
Sep 21, 2026
Merged

zerodahero merged 26 commits into
zerodahero:mainfrom
matthewmcvickar:main

Conversation

@matthewmcvickar

@matthewmcvickar matthewmcvickar commented Sep 21, 2026 •

Copy link
Copy Markdown

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 90012

the Normalizer->parse() method would return:

123 Main St #Los, Angeles, CA 90012

This was happening because the change to the regex added an empty alternative to the end of the pattern looking for a Unit prefix:

# from https://github.com/zerodahero/address-normalization/pull/3
- $this->unit_regexp = '(?:(su?i?te|p\W*[om]\W*b(?:ox)?|dept|apt|apartment|ro*m|fl|unit|box)\W+|\#\W*)([\w-]+)';
+ $this->unit_regexp = '(?:(su?i?te|p\W*[om]\W*b(?:ox)?|dept|apt|apartment|ro*m|fl|unit|box)\W+|\#\W*|)([\w-]+)';

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.

@zerodahero zerodahero left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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.

Comment thread tests/NormalizerTest.php
@matthewmcvickar

Copy link
Copy Markdown
Author

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.)

Comment thread .github/workflows/php.yml Outdated
Comment on lines +23 to +28
- { 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 }

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

This would also mean we'd need to keep the docblock annotations instead of the #[] ones.

@matthewmcvickar matthewmcvickar Sep 21, 2026 •

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

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.

@matthewmcvickar matthewmcvickar changed the title fix regression with Unit regex and multi-word Place names fix regression with unit regex, update dependencies and tooling Sep 21, 2026
Comment thread .github/workflows/php.yml Outdated
Comment thread tests/NormalizerTest.php Outdated
@zerodahero
zerodahero merged commit 895ffd5 into zerodahero:main Sep 21, 2026
4 checks passed
@zerodahero

Copy link
Copy Markdown
Owner

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!

@matthewmcvickar

Copy link
Copy Markdown
Author

Thank you for the quick review and merge! I appreciate the chance to contribute here and very happy this repo exists.

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 this pull request may close these issues.

2 participants