Dev #1
Dev #1
Conversation
|
Warning Rate limit exceeded@eldadfux has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 15 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (5)
WalkthroughAdds a complete email handling subsystem: an Email value object with validation, classification (free/disposable/corporate), formatting and canonicalization; multiple canonical provider implementations (Gmail, Outlook, Yahoo, iCloud, ProtonMail, Fastmail, Generic); several validators (Email, EmailLocal, EmailDomain, EmailCorporate, EmailNotDisposable); a CLI importer for disposable/free domain lists and data files under data/; PHPUnit tests covering Email, validators, and providers; CI workflows, linting/config files, README, CONTRIBUTING, and Code of Conduct; and composer configuration. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Rationale: Large, heterogeneous addition (40+ new files) combining dense business logic (Email class, import CLI), multiple provider-specific implementations that require per-file verification, and extensive tests — review requires focused checks across distinct areas rather than simple repetitive changes. Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
🧹 Nitpick comments (21)
CODE_OF_CONDUCT.md (1)
61-61: Simplify wordy phrase for clarity."With regard to" can be concisely shortened to improve readability and professionalism.
Apply this diff to simplify:
-obligated to maintain confidentiality with regard to the reporter of an incident. +obligated to maintain confidentiality regarding the reporter of an incident..github/workflows/codeql-analysis.yml (1)
17-20: Consider using latest Composer 2.x image.The
composer:2.6image is pinned to a specific minor version. Consider usingcomposer:2to automatically get the latest Composer 2.x patches, or update to a more recent version if 2.6 is outdated.- name: Run CodeQL run: | - docker run --rm -v $PWD:/app composer:2.6 sh -c \ + docker run --rm -v $PWD:/app composer:2 sh -c \ "composer install --profile --ignore-platform-reqs && composer check".github/workflows/test.yml (1)
8-11: Consider allowing PHP nightly to fail.Including PHP nightly in the test matrix is good for forward compatibility testing, but nightly builds can be unstable and may cause CI failures on code that's otherwise correct. Consider using
continue-on-erroror a separate non-blocking job for nightly.Add a matrix configuration to allow nightly failures:
strategy: + fail-fast: false matrix: php-versions: ['8.1', '8.2', '8.3', 'nightly'] + include: + - php-versions: 'nightly' + experimental: trueThen add to the steps section before running tests:
- name: Run tests run: composer test continue-on-error: ${{ matrix.experimental || false }}composer.json (1)
41-41: Avoid using dev version constraint for PHPStan.The
1.9.x-devconstraint pulls the latest development commit from the 1.9 branch, which may include breaking changes or bugs. Use a stable version constraint instead."require-dev": { "phpunit/phpunit": "^9.3", "laravel/pint": "1.25.*", - "phpstan/phpstan": "1.9.x-dev" + "phpstan/phpstan": "^1.12" },Note: PHPStan 1.12 is the latest stable release as of recent updates. Check for the current latest version if needed.
.github/workflows/linter.yml (1)
17-20: Pin Docker image version for reproducibility.The
composerimage without a version tag defaults tolatest, which can change over time and break CI reproducibility.- name: Run Linter run: | - docker run --rm -v $PWD:/app composer sh -c \ + docker run --rm -v $PWD:/app composer:2 sh -c \ "composer install --profile --ignore-platform-reqs && composer lint"README.md (1)
219-253: Consider using proper markdown headings instead of bold text.Several section titles use bold text (e.g., "Update All Domains" on line 219) instead of proper markdown headings. For better document structure and accessibility, use heading syntax.
Example:
-**Update All Domains** +#### Update All Domains ```bash # Preview changes without committing php import.php allphpunit.xml (1)
1-10: Add XML schema declaration for PHPUnit configuration.The configuration is missing the XML schema declaration. For PHPUnit 9+, it's recommended to include the schema reference for proper validation and IDE support.
Apply this diff to add the schema:
-<phpunit backupGlobals="false" +<?xml version="1.0" encoding="UTF-8"?> +<phpunit xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" + xsi:noNamespaceSchemaLocation="vendor/phpunit/phpunit/phpunit.xsd" + backupGlobals="false" backupStaticAttributes="false" bootstrap="vendor/autoload.php"data/free-domains-manual.php (1)
9-10: Update or remove the placeholder timestamp.The "Last updated" timestamp appears to be a placeholder value. Update it to reflect the actual date or remove it if not needed.
data/sources.php (1)
9-10: Update or remove the placeholder timestamp.The "Last updated" timestamp appears to be a placeholder value. Update it to reflect the actual date or remove it if tracking in version control is sufficient.
tests/Validator/EmailCorporateTest.php (1)
21-35: Use assertTrue/assertFalse for booleans.Prefer assertTrue/assertFalse over assertEquals(true/false, …) for readability.
Also applies to: 37-57, 59-77, 80-101, 103-122
src/Emails/Validator/EmailDomain.php (1)
32-45: LGTM; consistent with other validators.Logic is sound: type guard + Email->isValid() && hasValidDomain().
All validators repeat the same guard/try/catch. Consider extracting a small helper/trait to reduce duplication.
src/Emails/Validator/EmailLocal.php (1)
32-45: LGTM; matches intended behavior.Validates string input, constructs Email, and checks hasValidLocal().
Same DRY opportunity as other validators (shared guard/instantiation).
tests/EmailTest.php (1)
24-43: Optional: prefer assertTrue/assertFalse in tests.For boolean checks, assertTrue/assertFalse improve readability and failure messages over assertEquals(true/false, …).
Also applies to: 45-55, 57-68, 70-80, 82-140, 142-163, 165-171, 213-384
tests/Validator/EmailLocalTest.php (1)
27-33: Use dedicated assertions and data providers.
- Replace assertEquals(true/false, …) with assertTrue/assertFalse for clarity.
- Consider a dataProvider to collapse repetitive cases.
- Add boundary tests for local length: 64 (pass) and 65 (fail).
Apply example diff for one block:
- $this->assertEquals(true, $validator->isValid('test@example.com')); + $this->assertTrue($validator->isValid('test@example.com'));Also applies to: 39-45, 51-57, 63-63, 70-70, 77-77
tests/Validator/EmailTest.php (1)
27-35: Prefer assertTrue/assertFalse and add edge cases.
- Swap assertEquals(true/false, …) for assertTrue/assertFalse.
- Consider data providers for valid/invalid sets.
- Add boundary tests: max domain length (253) and near-limit labels; include IDN/punycode if supported.
Example:
- $this->assertEquals(false, $validator->isValid('user@example..com')); + $this->assertFalse($validator->isValid('user@example..com'));Also applies to: 41-60, 66-71, 78-78, 85-85, 92-92
src/Emails/Email.php (2)
215-229: Provider/subdomain logic ignores public suffixes.Using last two labels breaks on multi-part TLDs (e.g., example.co.uk). Prefer a PSL-backed approach via Utopia\Domains\Domain.
Sketch:
$dom = new \Utopia\Domains\Domain($this->domain); $provider = method_exists($dom, 'getRegisterable') ? $dom->getRegisterable() : $dom->getDomain(); // adjust to actual API $sub = str_ends_with($this->domain, $provider) ? rtrim(substr($this->domain, 0, -strlen($provider)), '.') : '';If Domain exposes a different method name for the registerable domain, confirm and adjust.
Also applies to: 234-243
185-201: Harden domain list loading and speed up lookups.
- Guard includes with file_exists to avoid warnings on first run.
- Convert arrays to associative sets once for O(1) membership checks.
Example:
- if (self::$disposableDomains === null) { - self::$disposableDomains = include __DIR__.'/../../data/disposable-domains.php'; - } - return in_array($this->domain, self::$disposableDomains); + if (self::$disposableDomains === null) { + $file = __DIR__.'/../../data/disposable-domains.php'; + $list = file_exists($file) ? include $file : []; + self::$disposableDomains = array_fill_keys($list, true); + } + return isset(self::$disposableDomains[$this->domain]);(Apply similarly for free domains.)
import.php (4)
312-315: Add timeouts and a UA on network calls.Explicit timeouts and a deterministic User-Agent reduce hangs and rate-limit issues.
If supported by Utopia\Fetch\Client:
$response = $client->fetch( url: $sourceConfig['url'], method: \Utopia\Fetch\Client::METHOD_GET, timeout: 15, connectTimeout: 5, headers: ['User-Agent' => 'utopia-emails-import/1.0'] );Confirm exact option names for Client; I’ll adjust accordingly.
Also applies to: 358-361, 406-408, 453-455, 499-501, 562-565
681-725: Use atomic writes and locking for config files.Prevent torn writes on interruption.
Apply:
- $result = file_put_contents($configFile, $configContent); + $tmp = $configFile.'.tmp'; + $result = file_put_contents($tmp, $configContent, LOCK_EX); + if ($result === false) { + throw new \Exception("Failed to write temp config file: {$tmp}"); + } + if (!@rename($tmp, $configFile)) { + @unlink($tmp); + throw new \Exception("Failed to atomically move temp file into place: {$configFile}"); + }
730-776: Avoid including generated file to validate syntax.Including executes the file; use php -l or token_get_all to lint instead.
If exec is available, prefer:
- Run php -l $filepath and check exit code; or
- Use token_get_all(file_get_contents($filepath)) to ensure it parses without executing.
783-826: Guard percentage math against zero and show totals.Slight hardening and clearer output.
- If count($domains) === 0, short-circuit with a message.
- Also print total domains once to anchor percentages.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (26)
.github/workflows/codeql-analysis.yml(1 hunks).github/workflows/linter.yml(1 hunks).github/workflows/test.yml(1 hunks).gitignore(1 hunks)CODE_OF_CONDUCT.md(1 hunks)CONTRIBUTING.md(1 hunks)README.md(1 hunks)composer.json(1 hunks)data/disposable-domains-manual.php(1 hunks)data/free-domains-manual.php(1 hunks)data/sources.php(1 hunks)import.php(1 hunks)phpunit.xml(1 hunks)pint.json(1 hunks)src/Emails/Email.php(1 hunks)src/Emails/Validator/Email.php(1 hunks)src/Emails/Validator/EmailCorporate.php(1 hunks)src/Emails/Validator/EmailDomain.php(1 hunks)src/Emails/Validator/EmailLocal.php(1 hunks)src/Emails/Validator/EmailNotDisposable.php(1 hunks)tests/EmailTest.php(1 hunks)tests/Validator/EmailCorporateTest.php(1 hunks)tests/Validator/EmailDomainTest.php(1 hunks)tests/Validator/EmailLocalTest.php(1 hunks)tests/Validator/EmailNotDisposableTest.php(1 hunks)tests/Validator/EmailTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (12)
tests/Validator/EmailNotDisposableTest.php (2)
src/Emails/Validator/EmailNotDisposable.php (5)
EmailNotDisposable(13-66)isValid(32-45)getDescription(20-23)getType(62-65)isArray(52-55)src/Emails/Email.php (1)
isValid(129-132)
src/Emails/Validator/EmailNotDisposable.php (1)
src/Emails/Email.php (3)
isValid(129-132)isDisposable(183-190)
src/Emails/Validator/EmailLocal.php (5)
src/Emails/Email.php (3)
isValid(129-132)hasValidLocal(137-160)src/Emails/Validator/Email.php (5)
getDescription(20-23)isValid(32-45)isArray(52-55)getType(62-65)src/Emails/Validator/EmailCorporate.php (4)
getDescription(20-23)isValid(32-45)isArray(52-55)getType(62-65)src/Emails/Validator/EmailDomain.php (4)
getDescription(20-23)isValid(32-45)isArray(52-55)getType(62-65)src/Emails/Validator/EmailNotDisposable.php (4)
getDescription(20-23)isValid(32-45)isArray(52-55)getType(62-65)
src/Emails/Validator/Email.php (2)
src/Emails/Email.php (2)
isValid(129-132)src/Emails/Validator/EmailCorporate.php (1)
isValid(32-45)
tests/Validator/EmailLocalTest.php (2)
src/Emails/Validator/EmailLocal.php (5)
EmailLocal(13-66)isValid(32-45)getDescription(20-23)getType(62-65)isArray(52-55)src/Emails/Email.php (1)
isValid(129-132)
tests/Validator/EmailCorporateTest.php (3)
src/Emails/Validator/EmailCorporate.php (5)
EmailCorporate(13-66)isValid(32-45)getDescription(20-23)getType(62-65)isArray(52-55)src/Emails/Email.php (1)
isValid(129-132)tests/Validator/EmailNotDisposableTest.php (1)
test_invalid_disposable_email(34-53)
src/Emails/Email.php (4)
src/Emails/Validator/EmailCorporate.php (1)
isValid(32-45)src/Emails/Validator/EmailDomain.php (1)
isValid(32-45)src/Emails/Validator/EmailLocal.php (1)
isValid(32-45)src/Emails/Validator/EmailNotDisposable.php (1)
isValid(32-45)
src/Emails/Validator/EmailCorporate.php (3)
src/Emails/Email.php (3)
isValid(129-132)isCorporate(207-210)src/Emails/Validator/EmailDomain.php (1)
isValid(32-45)src/Emails/Validator/EmailNotDisposable.php (1)
isValid(32-45)
src/Emails/Validator/EmailDomain.php (5)
src/Emails/Email.php (3)
isValid(129-132)hasValidDomain(165-178)src/Emails/Validator/Email.php (5)
getDescription(20-23)isValid(32-45)isArray(52-55)getType(62-65)src/Emails/Validator/EmailCorporate.php (4)
getDescription(20-23)isValid(32-45)isArray(52-55)getType(62-65)src/Emails/Validator/EmailLocal.php (4)
getDescription(20-23)isValid(32-45)isArray(52-55)getType(62-65)src/Emails/Validator/EmailNotDisposable.php (4)
getDescription(20-23)isValid(32-45)isArray(52-55)getType(62-65)
tests/EmailTest.php (1)
src/Emails/Email.php (17)
get(89-92)getLocal(97-100)getDomain(105-108)getDomainOnly(113-116)getLocalOnly(121-124)isValid(129-132)hasValidLocal(137-160)hasValidDomain(165-178)isDisposable(183-190)isFree(195-202)isCorporate(207-210)getProvider(215-229)getSubdomain(234-243)hasSubdomain(248-251)normalize(256-259)getFormatted(264-279)
tests/Validator/EmailDomainTest.php (2)
src/Emails/Validator/EmailDomain.php (5)
EmailDomain(13-66)isValid(32-45)getDescription(20-23)getType(62-65)isArray(52-55)src/Emails/Email.php (1)
isValid(129-132)
tests/Validator/EmailTest.php (2)
src/Emails/Email.php (2)
isValid(129-132)src/Emails/Validator/Email.php (5)
isValid(32-45)getDescription(20-23)getType(62-65)isArray(52-55)
🪛 actionlint (1.7.8)
.github/workflows/codeql-analysis.yml
11-11: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/test.yml
14-14: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
.github/workflows/linter.yml
11-11: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 GitHub Actions: Tests
tests/Validator/EmailNotDisposableTest.php
[error] 27-27: Failed asserting that false matches expected true.
tests/Validator/EmailCorporateTest.php
[error] 54-54: Failed asserting that true matches expected false.
[error] 64-64: Failed asserting that true matches expected false.
tests/EmailTest.php
[error] 36-36: Failed asserting that true matches expected false.
[error] 78-78: Failed asserting that true matches expected false.
[error] 1-1: Failed asserting that exception message '{$email} must be a valid email address' contains ''invalid-email' must be a valid email address'.
[error] 1-1: Failed asserting that exception message '{$email} must be a valid email address' contains ''user@example@com' must be a valid email address'.
[error] 1-1: Failed asserting that exception message '{$email} must be a valid email address' contains ''@example.com' must be a valid email address'.
[error] 1-1: Failed asserting that exception message '{$email} must be a valid email address' contains ''user@' must be a valid email address'.
[error] 337-337: Test free email providers failed for provider: tutanota.com
[error] 360-360: Test disposable email providers failed for provider: tempmail.org
🪛 LanguageTool
CODE_OF_CONDUCT.md
[style] ~27-~27: Try using a synonym here to strengthen your wording.
Context: ...vances * Trolling, insulting/derogatory comments, and personal or political attacks * Pu...
(COMMENT_REMARK)
[style] ~61-~61: ‘with regard to’ might be wordy. Consider a shorter alternative.
Context: ...s obligated to maintain confidentiality with regard to the reporter of an incident. Further de...
(EN_WORDINESS_PREMIUM_WITH_REGARD_TO)
CONTRIBUTING.md
[style] ~90-~90: Consider using a different verb for a more formal wording.
Context: ...tests pass 2. Run code style checks and fix any issues 3. Run static analysis and f...
(FIX_RESOLVE)
[style] ~91-~91: Consider using a different verb for a more formal wording.
Context: ...x any issues 3. Run static analysis and fix any issues 4. Update documentation if n...
(FIX_RESOLVE)
🪛 markdownlint-cli2 (0.18.1)
README.md
219-219: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
228-228: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
240-240: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
249-249: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
330-330: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
335-335: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (10)
CODE_OF_CONDUCT.md (1)
1-75: Code of Conduct document structure and content are solid.The document follows the well-established Contributor Covenant v1.4 template and appropriately establishes clear community standards, responsibilities, and enforcement mechanisms. Attribution and links are correct. Once the placeholder email is filled in, this will provide good governance for the project.
CONTRIBUTING.md (1)
1-181: LGTM! Comprehensive contributing guidelines.The contributing guidelines are well-structured and cover all essential aspects: getting started, development setup, testing, code style, standards, PR/issue guidelines, workflow conventions, and community guidelines. This provides clear expectations for contributors.
pint.json (1)
1-10: LGTM! Pint configuration is appropriate.The Laravel Pint configuration uses a sensible preset and rule set for consistent code formatting.
data/disposable-domains-manual.php (1)
1-14: LGTM! Manual configuration file structure is correct.The empty array is appropriate for a manual configuration file that will be populated as needed. The placeholder date in the comment (2024-01-01) is acceptable for initial setup and will be updated when domains are added through the import system.
src/Emails/Validator/Email.php (1)
32-45: LGTM!The validation logic correctly delegates to the
src/Emails/Validator/EmailNotDisposable.php (1)
32-45: LGTM!The validation logic correctly checks both email validity and disposable status. Exception handling is appropriate.
tests/Validator/EmailDomainTest.php (1)
23-82: LGTM!Comprehensive test coverage for the EmailDomain validator, including valid domains, invalid formats, edge cases, non-string inputs, and validator metadata. The inline comment at line 42 correctly documents the expected behavior for consecutive hyphens.
data/sources.php (1)
23-50: All remote source URLs are accessible and operational.Verification confirms that all five remote domain lists return HTTP/2 200 status codes, indicating they are properly maintained and available for use in the configuration.
tests/EmailTest.php (1)
33-39: The suggested fix targets the wrong file and won't resolve the issue.The review comment recommends removing
example.comfromdata/disposable-domains-manual.php, butexample.comis not present in that file. The entry exists indata/disposable-domains.php(line 22765), which is auto-generated and will be overwritten on the next update.To fix the failing test (line 36:
$this->assertEquals(false, $email->isDisposable())), add an override todata/disposable-domains-manual.phpto excludeexample.comfrom the disposable domains list, or update the auto-generation source to exclude it.Likely an incorrect or invalid review comment.
src/Emails/Validator/EmailCorporate.php (1)
32-45: Code is correct; tests will fail without data file updates.The EmailCorporate validator logic is sound. However, tests explicitly expect
user@tutanota.comanduser@tempmail.orgto fail validation (return false), but they currently aren't in the domain lists, causing them to be treated as corporate. Before merging, ensure either:
- tutanota.com is added to data/free-domains.php, and
- tempmail.org is added to data/disposable-domains.php
Or, if these tests are incorrect, update them.
…amps, and improve exception message formatting
…ed performance and consistency
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
import.php (3)
31-35: Critical: Fix incorrect GitHub raw URL (still present despite past review).The URL still contains the incorrect
/refs/heads/path segment. The correct raw GitHub URL format ishttps://raw.githubusercontent.com/{owner}/{repo}/{branch}/{path}.Apply this fix:
'wesbos' => [ 'name' => 'Wes Bos Burner Email Providers', - 'url' => 'https://raw.githubusercontent.com/wesbos/burner-email-providers/refs/heads/master/emails.txt', + 'url' => 'https://raw.githubusercontent.com/wesbos/burner-email-providers/master/emails.txt', 'configFile' => CONFIG_DIR.'/disposable-domains-wesbos.php', ],
67-114: Critical: Refactor to return bool instead of calling Console::exit() (still not fixed).The helper function still calls
Console::exit()directly (lines 78, 86, 95, 112), which breaksupdateAllDomains()when called sequentially. IfupdateDisposableDomains()exits early (e.g., line 95 when up-to-date),updateFreeDomains()never executes.Refactor to return
bool:-function updateDisposableDomains(bool $commit, bool $force, string $source): void +function updateDisposableDomains(bool $commit, bool $force, string $source): bool { Console::title('Disposable Email Domains Update'); Console::success('Utopia Emails disposable domains update process has started'); try { $sources = $source ? [$source => DISPOSABLE_SOURCES[$source] ?? null] : DISPOSABLE_SOURCES; $sources = array_filter($sources); if (empty($sources)) { Console::error('No valid sources found'); - Console::exit(1); + return false; } $allDomains = fetchAllSources($sources, 'disposable'); $currentDomains = loadCurrentConfig('disposable-domains.php'); if (empty($allDomains)) { Console::error('Failed to fetch disposable email domains or list is empty'); - Console::exit(1); + return false; } Console::info('Fetched '.count($allDomains).' disposable email domains from all sources'); showDomainStatistics($allDomains); if (! $force && isConfigUpToDate($currentDomains, $allDomains)) { Console::success('Disposable email domains are already up to date'); - Console::exit(0); + return true; } Console::info('Changes detected:'); Console::info('- Previous domains count: '.count($currentDomains)); Console::info('- New domains count: '.count($allDomains)); if ($commit) { saveConfig('disposable-domains.php', $allDomains, 'Disposable Email Domains'); Console::success('Successfully updated disposable email domains configuration'); } else { Console::warning('Changes not yet committed to config file. Please provide --commit=true argument to commit changes.'); Console::info('Preview of changes:'); showPreview($currentDomains, $allDomains); } + return true; } catch (\Throwable $e) { Console::error('Error updating disposable email domains: '.$e->getMessage()); - Console::exit(1); + return false; } }Apply the same pattern to
updateFreeDomains()(lines 119-166).
867-869: Update CLI actions to handle bool return values after helper refactor.Once the helper functions are refactored to return
bool, the CLI actions must capture the return value and callConsole::exit()with the appropriate exit code.Apply after refactoring helpers:
->action(function (bool $commit, bool $force, string $source) { - updateDisposableDomains($commit, $force, $source); + $success = updateDisposableDomains($commit, $force, $source); + Console::exit($success ? 0 : 1); });Apply the same pattern to the
free(lines 878-880),all(lines 888-890), andstats(lines 896-898) actions.
🧹 Nitpick comments (1)
src/Emails/Email.php (1)
225-239: Consider Public Suffix List for accurate provider extraction.The current implementation assumes the provider is always the last two domain segments, which fails for multi-level TLDs like
.co.uk. For example,user@mail.company.co.ukwould returnco.ukinstead ofcompany.co.uk.Consider using a Public Suffix List library (like
getPublicSuffix()from the Utopia Domains package if available) for accurate domain parsing across all TLDs, or document this limitation in the method's docblock.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.github/workflows/linter.yml(1 hunks).github/workflows/test.yml(1 hunks)data/disposable-domains-manual.php(1 hunks)data/free-domains-manual.php(1 hunks)import.php(1 hunks)src/Emails/Email.php(1 hunks)tests/EmailTest.php(1 hunks)tests/Validator/EmailCorporateTest.php(1 hunks)tests/Validator/EmailNotDisposableTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- data/disposable-domains-manual.php
- .github/workflows/test.yml
- tests/Validator/EmailCorporateTest.php
🧰 Additional context used
🧬 Code graph analysis (3)
src/Emails/Email.php (4)
src/Emails/Validator/EmailCorporate.php (1)
isValid(32-45)src/Emails/Validator/EmailDomain.php (1)
isValid(32-45)src/Emails/Validator/EmailLocal.php (1)
isValid(32-45)src/Emails/Validator/EmailNotDisposable.php (1)
isValid(32-45)
tests/Validator/EmailNotDisposableTest.php (2)
src/Emails/Validator/EmailNotDisposable.php (5)
EmailNotDisposable(13-66)isValid(32-45)getDescription(20-23)getType(62-65)isArray(52-55)src/Emails/Email.php (1)
isValid(129-132)
tests/EmailTest.php (1)
src/Emails/Email.php (17)
get(89-92)getLocal(97-100)getDomain(105-108)getDomainOnly(113-116)getLocalOnly(121-124)isValid(129-132)hasValidLocal(137-160)hasValidDomain(165-178)isDisposable(183-190)isFree(195-207)isCorporate(212-220)getProvider(225-239)getSubdomain(244-253)hasSubdomain(258-261)normalize(266-269)getFormatted(274-289)
🔇 Additional comments (1)
src/Emails/Email.php (1)
212-220: Remove unreachable dead code and misleading comment.Lines 214-216 are unreachable. The condition
$this->isFree() && $this->isDisposable()can never be true becauseisFree()(line 202) returnsfalsewhenisDisposable()istrue. The logic on line 219 is correct and sufficient.Remove the dead code:
public function isCorporate(): bool { - // If domain is both free and disposable, prioritize free classification - if ($this->isFree() && $this->isDisposable()) { - return false; // It's free, not corporate - } - return ! $this->isFree() && ! $this->isDisposable(); }Likely an incorrect or invalid review comment.
…ovider-specific handling; remove deprecated sources.php file and update README badges for CI workflows.
…Checks and modify steps to use actions/checkout@v4 for improved performance.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (19)
composer.json (1)
37-37: Consider pinning PHPStan to a stable version.The
1.9.x-devconstraint points to a development branch, which may introduce instability or unexpected changes during development. For more predictable builds, consider using a stable version constraint.- "phpstan/phpstan": "1.9.x-dev" + "phpstan/phpstan": "^1.12"tests/Normalizer/Providers/GenericTest.php (1)
76-77: Use strict assertions for exactness (no type juggling).Prefer assertSame over assertEquals for scalars/arrays in these checks.
- $this->assertEquals($expectedLocal, $result['local'], "Failed for local: {$inputLocal}@{$inputDomain}"); - $this->assertEquals($expectedDomain, $result['domain'], "Failed for domain: {$inputLocal}@{$inputDomain}"); + $this->assertSame($expectedLocal, $result['local'], "Failed for local: {$inputLocal}@{$inputDomain}"); + $this->assertSame($expectedDomain, $result['domain'], "Failed for domain: {$inputLocal}@{$inputDomain}");- $this->assertEquals([], $domains); + $this->assertSame([], $domains);Also applies to: 91-91
src/Emails/Normalizer/Providers/Generic.php (3)
16-20: PHPMD: unused parameter in supports() — suppress intentionally.The interface requires the parameter; add a suppression to keep static analysis clean.
- public function supports(string $domain): bool + /** + * @SuppressWarnings(PHPMD.UnusedFormalParameter) + */ + public function supports(string $domain): bool { // Generic provider supports all domains return true; }
27-35: Avoid double strpos() call; compute once.Small micro-optimization and clarity.
- // Check if there's plus addressing - $hasPlus = strpos($normalizedLocal, '+') !== false && strpos($normalizedLocal, '+') > 0; + // Check if there's plus addressing + $plusPos = strpos($normalizedLocal, '+'); + $hasPlus = $plusPos !== false && $plusPos > 0;
39-42: Consider normalizing domain case (defensive).If upstream doesn’t guarantee lowercase domains, return strtolower($domain) to keep behavior consistent across providers. If Email normalizes domain beforehand, ignore.
- 'domain' => $domain, + 'domain' => strtolower($domain),src/Emails/Normalizer/Providers/Protonmail.php (2)
21-24: Make supports() case-insensitive.Defensively lowercase the input to avoid rejecting domains with unexpected casing.
- return in_array($domain, self::SUPPORTED_DOMAINS, true); + return in_array(strtolower($domain), self::SUPPORTED_DOMAINS, true);
26-46: Tidy unused parameter + micro-optimization + verify dot rule.
- Suppress PHPMD for unused $domain in normalize().
- Compute strpos once.
- Please confirm the “remove dots only when plus is present” rule for Proton (policy varies by provider) and that canonicalizing proton.me/pm.me to protonmail.com is desired.
- public function normalize(string $local, string $domain): array + /** + * @SuppressWarnings(PHPMD.UnusedFormalParameter) + */ + public function normalize(string $local, string $domain): array { // Convert to lowercase $normalizedLocal = $this->toLowerCase($local); - // Check if there's plus addressing - $hasPlus = strpos($normalizedLocal, '+') !== false && strpos($normalizedLocal, '+') > 0; + // Check if there's plus addressing + $plusPos = strpos($normalizedLocal, '+'); + $hasPlus = $plusPos !== false && $plusPos > 0;tests/Normalizer/Providers/GmailTest.php (1)
69-70: Prefer assertSame for strict comparisons.Stricter checks improve test signal and prevent subtle type coercions.
- $this->assertEquals($expectedLocal, $result['local'], "Failed for local: {$inputLocal}@{$inputDomain}"); - $this->assertEquals($expectedDomain, $result['domain'], "Failed for domain: {$inputLocal}@{$inputDomain}"); + $this->assertSame($expectedLocal, $result['local'], "Failed for local: {$inputLocal}@{$inputDomain}"); + $this->assertSame($expectedDomain, $result['domain'], "Failed for domain: {$inputLocal}@{$inputDomain}");- $this->assertEquals(['gmail.com', 'googlemail.com'], $domains); + $this->assertSame(['gmail.com', 'googlemail.com'], $domains);Also applies to: 82-82
tests/Normalizer/Providers/OutlookTest.php (1)
79-80: Tighten assertions with assertSame.Use strict comparisons for these scalar/array assertions.
- $this->assertEquals($expectedLocal, $result['local'], "Failed for local: {$inputLocal}@{$inputDomain}"); - $this->assertEquals($expectedDomain, $result['domain'], "Failed for domain: {$inputLocal}@{$inputDomain}"); + $this->assertSame($expectedLocal, $result['local'], "Failed for local: {$inputLocal}@{$inputDomain}"); + $this->assertSame($expectedDomain, $result['domain'], "Failed for domain: {$inputLocal}@{$inputDomain}");- $this->assertEquals($expected, $domains); + $this->assertSame($expected, $domains);Also applies to: 93-93
src/Emails/Normalizer/Providers/Gmail.php (2)
26-36: Silence PHPMD for unused$domainparameter in normalize()Signature must match the abstract method; suppression avoids false positives.
public function normalize(string $local, string $domain): array { + /** + * @SuppressWarnings(PHPMD.UnusedFormalParameter) + */ // Convert to lowercase $normalizedLocal = $this->toLowerCase($local);
31-36: Minor: remove plus before removing dots (skip work on discarded suffix)Same result, slightly less work for inputs with long +tags.
- // Remove all dots from local part - $normalizedLocal = $this->removeDots($normalizedLocal); - - // Remove plus addressing (everything after +) - $normalizedLocal = $this->removePlusAddressing($normalizedLocal); + // Remove plus addressing (everything after +) + $normalizedLocal = $this->removePlusAddressing($normalizedLocal); + + // Remove all dots from local part + $normalizedLocal = $this->removeDots($normalizedLocal);src/Emails/Normalizer/Provider.php (1)
68-71: Use mb_strtolower for consistency with constructorConstructor lowercases with mb_strtolower; mirror it here to avoid locale/Unicode edge cases.
- return strtolower($local); + return \mb_strtolower($local);src/Emails/Normalizer/Providers/Outlook.php (1)
29-36: Silence PHPMD for unused$domainparameter in normalize()Matches abstract signature; not needed by this provider.
public function normalize(string $local, string $domain): array { + /** + * @SuppressWarnings(PHPMD.UnusedFormalParameter) + */ // Convert to lowercase $normalizedLocal = $this->toLowerCase($local);tests/EmailTest.php (1)
28-43: Prefer assertTrue/assertFalse over assertEquals(true/false)Improves readability and PHPUnit diffs.
Example:
- $this->assertEquals(true, $email->isValid()); + $this->assertTrue($email->isValid()); - $this->assertEquals(false, $email->isDisposable()); + $this->assertFalse($email->isDisposable());(Apply similarly throughout this test class.)
Also applies to: 74-80, 90-116, 122-140, 158-163, 167-171, 217-225, 229-247, 251-263, 269-291, 295-305, 311-339, 358-383
src/Emails/Email.php (5)
300-305: Simplify: derive support by provider resolution (avoid duplicate scan)No need for a separate scan; reuse provider selection.
- public function isNormalizationSupported(): bool - { - return $this->isDomainSupported($this->domain); - } + public function isNormalizationSupported(): bool + { + return ! ($this->getProviderForDomain($this->domain) instanceof Generic); + }
327-336: DRY provider initializationThe same list is built in two methods. Centralize to one initializer.
@@ - protected function getProviderForDomain(string $domain): Provider + protected static function getProviders(): array { - if (self::$providers === null) { - self::$providers = [ - new Gmail, - new Outlook, - new Yahoo, - new Icloud, - new Protonmail, - new Fastmail, - ]; - } - - foreach (self::$providers as $provider) { - if ($provider->supports($domain)) { - return $provider; - } - } - - // Return generic provider if no specific provider found - return new Generic; + if (self::$providers === null) { + self::$providers = [new Gmail, new Outlook, new Yahoo, new Icloud, new Protonmail, new Fastmail]; + } + return self::$providers; } @@ - protected function isDomainSupported(string $domain): bool + protected function getProviderForDomain(string $domain): Provider { - if (self::$providers === null) { - self::$providers = [ - new Gmail, - new Outlook, - new Yahoo, - new Icloud, - new Protonmail, - new Fastmail, - ]; - } - - foreach (self::$providers as $provider) { - if ($provider->supports($domain)) { - return true; - } - } - - return false; + foreach (self::getProviders() as $provider) { + if ($provider->supports($domain)) { + return $provider; + } + } + return new Generic; }Also applies to: 353-361, 338-346, 351-371
200-221: Use strict in_array for domain membership checksAvoid type juggling; data files are strings.
- return in_array($this->domain, self::$disposableDomains); + return in_array($this->domain, self::$disposableDomains, true); @@ - if (in_array($this->domain, self::$freeDomains) && $this->isDisposable()) { + if (in_array($this->domain, self::$freeDomains, true) && $this->isDisposable()) { return false; // It's disposable, not free } @@ - return in_array($this->domain, self::$freeDomains); + return in_array($this->domain, self::$freeDomains, true);
229-235: Minor: comment mismatch and redundant guardComment says “prioritize free,” but system prioritizes disposable; the guard is redundant given the final return.
- // If domain is both free and disposable, prioritize free classification - if ($this->isFree() && $this->isDisposable()) { - return false; // It's free, not corporate - } + // A domain that is free or disposable is not corporate
240-254: Keep as optional refactor—multi-level TLD limitation is valid but not blockingThe analysis confirms the concern:
getProvider()does incorrectly handle multi-level TLDs (e.g.,example.co.jp→ returnsco.jpinstead ofexample.co.jp). However, verification reveals:
- No multi-level TLD test cases or active usage in the codebase
- All current tests pass with standard 2-3 label domains
- Function is used only for display/formatting, not critical business logic
- Current implementation works correctly for its actual use cases
The suggested PSL-based parsing is a valid improvement for broader email domain handling, but remains optional since the limitation doesn't impact current functionality.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
README.md(1 hunks)composer.json(1 hunks)src/Emails/Email.php(1 hunks)src/Emails/Normalizer/Provider.php(1 hunks)src/Emails/Normalizer/Providers/Fastmail.php(1 hunks)src/Emails/Normalizer/Providers/Generic.php(1 hunks)src/Emails/Normalizer/Providers/Gmail.php(1 hunks)src/Emails/Normalizer/Providers/Icloud.php(1 hunks)src/Emails/Normalizer/Providers/Outlook.php(1 hunks)src/Emails/Normalizer/Providers/Protonmail.php(1 hunks)src/Emails/Normalizer/Providers/Yahoo.php(1 hunks)tests/EmailTest.php(1 hunks)tests/Normalizer/Providers/FastmailTest.php(1 hunks)tests/Normalizer/Providers/GenericTest.php(1 hunks)tests/Normalizer/Providers/GmailTest.php(1 hunks)tests/Normalizer/Providers/IcloudTest.php(1 hunks)tests/Normalizer/Providers/OutlookTest.php(1 hunks)tests/Normalizer/Providers/ProtonmailTest.php(1 hunks)tests/Normalizer/Providers/YahooTest.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/Normalizer/Providers/IcloudTest.php
🧰 Additional context used
🧬 Code graph analysis (16)
src/Emails/Normalizer/Providers/Protonmail.php (1)
src/Emails/Normalizer/Provider.php (8)
Provider(10-72)supports(15-15)normalize(24-24)toLowerCase(68-71)removePlusAddressing(39-47)removeDots(52-55)getCanonicalDomain(29-29)getSupportedDomains(34-34)
src/Emails/Normalizer/Provider.php (8)
src/Emails/Normalizer/Providers/Fastmail.php (4)
supports(21-24)normalize(26-46)getCanonicalDomain(48-51)getSupportedDomains(53-56)src/Emails/Normalizer/Providers/Generic.php (4)
supports(16-20)normalize(22-43)getCanonicalDomain(45-49)getSupportedDomains(51-55)src/Emails/Normalizer/Providers/Gmail.php (4)
supports(21-24)normalize(26-41)getCanonicalDomain(43-46)getSupportedDomains(48-51)src/Emails/Normalizer/Providers/Icloud.php (4)
supports(21-24)normalize(26-46)getCanonicalDomain(48-51)getSupportedDomains(53-56)src/Emails/Normalizer/Providers/Outlook.php (4)
supports(24-27)normalize(29-41)getCanonicalDomain(43-46)getSupportedDomains(48-51)src/Emails/Normalizer/Providers/Protonmail.php (4)
supports(21-24)normalize(26-46)getCanonicalDomain(48-51)getSupportedDomains(53-56)src/Emails/Normalizer/Providers/Yahoo.php (4)
supports(25-28)normalize(30-53)getCanonicalDomain(55-58)getSupportedDomains(60-63)src/Emails/Email.php (2)
normalize(281-284)getCanonicalDomain(310-320)
tests/Normalizer/Providers/FastmailTest.php (2)
src/Emails/Normalizer/Providers/Fastmail.php (5)
Fastmail(15-57)supports(21-24)normalize(26-46)getCanonicalDomain(48-51)getSupportedDomains(53-56)src/Emails/Normalizer/Provider.php (4)
supports(15-15)normalize(24-24)getCanonicalDomain(29-29)getSupportedDomains(34-34)
src/Emails/Normalizer/Providers/Icloud.php (2)
src/Emails/Normalizer/Provider.php (8)
Provider(10-72)supports(15-15)normalize(24-24)toLowerCase(68-71)removePlusAddressing(39-47)removeDots(52-55)getCanonicalDomain(29-29)getSupportedDomains(34-34)src/Emails/Email.php (2)
normalize(281-284)getCanonicalDomain(310-320)
src/Emails/Normalizer/Providers/Fastmail.php (3)
src/Emails/Normalizer/Provider.php (8)
Provider(10-72)supports(15-15)normalize(24-24)toLowerCase(68-71)removePlusAddressing(39-47)removeDots(52-55)getCanonicalDomain(29-29)getSupportedDomains(34-34)src/Emails/Normalizer/Providers/Icloud.php (4)
supports(21-24)normalize(26-46)getCanonicalDomain(48-51)getSupportedDomains(53-56)src/Emails/Email.php (2)
normalize(281-284)getCanonicalDomain(310-320)
src/Emails/Normalizer/Providers/Yahoo.php (2)
src/Emails/Normalizer/Provider.php (9)
Provider(10-72)supports(15-15)normalize(24-24)toLowerCase(68-71)removePlusAddressing(39-47)removeDots(52-55)removeHyphens(60-63)getCanonicalDomain(29-29)getSupportedDomains(34-34)src/Emails/Email.php (2)
normalize(281-284)getCanonicalDomain(310-320)
tests/Normalizer/Providers/OutlookTest.php (2)
src/Emails/Normalizer/Providers/Outlook.php (5)
Outlook(15-52)supports(24-27)normalize(29-41)getCanonicalDomain(43-46)getSupportedDomains(48-51)src/Emails/Email.php (2)
normalize(281-284)getCanonicalDomain(310-320)
src/Emails/Normalizer/Providers/Gmail.php (2)
src/Emails/Normalizer/Provider.php (8)
Provider(10-72)supports(15-15)normalize(24-24)toLowerCase(68-71)removeDots(52-55)removePlusAddressing(39-47)getCanonicalDomain(29-29)getSupportedDomains(34-34)src/Emails/Email.php (2)
normalize(281-284)getCanonicalDomain(310-320)
src/Emails/Normalizer/Providers/Generic.php (2)
src/Emails/Normalizer/Provider.php (9)
Provider(10-72)supports(15-15)normalize(24-24)toLowerCase(68-71)removePlusAddressing(39-47)removeDots(52-55)removeHyphens(60-63)getCanonicalDomain(29-29)getSupportedDomains(34-34)src/Emails/Email.php (2)
normalize(281-284)getCanonicalDomain(310-320)
tests/EmailTest.php (9)
src/Emails/Email.php (20)
get(104-107)getLocal(112-115)getDomain(120-123)getDomainOnly(128-131)getLocalOnly(136-139)isValid(144-147)hasValidLocal(152-175)hasValidDomain(180-193)isDisposable(198-205)isFree(210-222)isCorporate(227-235)getProvider(240-254)getSubdomain(259-268)hasSubdomain(273-276)normalize(281-284)getFormatted(376-391)getUnique(291-297)isNormalizationSupported(302-305)getCanonicalDomain(310-320)src/Emails/Normalizer/Provider.php (2)
normalize(24-24)getCanonicalDomain(29-29)src/Emails/Normalizer/Providers/Fastmail.php (2)
normalize(26-46)getCanonicalDomain(48-51)src/Emails/Normalizer/Providers/Generic.php (2)
normalize(22-43)getCanonicalDomain(45-49)src/Emails/Normalizer/Providers/Gmail.php (2)
normalize(26-41)getCanonicalDomain(43-46)src/Emails/Normalizer/Providers/Icloud.php (2)
normalize(26-46)getCanonicalDomain(48-51)src/Emails/Normalizer/Providers/Outlook.php (2)
normalize(29-41)getCanonicalDomain(43-46)src/Emails/Normalizer/Providers/Protonmail.php (2)
normalize(26-46)getCanonicalDomain(48-51)src/Emails/Normalizer/Providers/Yahoo.php (2)
normalize(30-53)getCanonicalDomain(55-58)
src/Emails/Email.php (8)
src/Emails/Normalizer/Provider.php (4)
Provider(10-72)normalize(24-24)getCanonicalDomain(29-29)supports(15-15)src/Emails/Normalizer/Providers/Fastmail.php (4)
Fastmail(15-57)normalize(26-46)getCanonicalDomain(48-51)supports(21-24)src/Emails/Normalizer/Providers/Generic.php (4)
Generic(14-56)normalize(22-43)getCanonicalDomain(45-49)supports(16-20)src/Emails/Normalizer/Providers/Gmail.php (4)
Gmail(15-52)normalize(26-41)getCanonicalDomain(43-46)supports(21-24)src/Emails/Normalizer/Providers/Icloud.php (4)
Icloud(15-57)normalize(26-46)getCanonicalDomain(48-51)supports(21-24)src/Emails/Normalizer/Providers/Outlook.php (4)
Outlook(15-52)normalize(29-41)getCanonicalDomain(43-46)supports(24-27)src/Emails/Normalizer/Providers/Protonmail.php (4)
Protonmail(15-57)normalize(26-46)getCanonicalDomain(48-51)supports(21-24)src/Emails/Normalizer/Providers/Yahoo.php (4)
Yahoo(16-64)normalize(30-53)getCanonicalDomain(55-58)supports(25-28)
tests/Normalizer/Providers/YahooTest.php (2)
src/Emails/Normalizer/Providers/Yahoo.php (5)
Yahoo(16-64)supports(25-28)normalize(30-53)getCanonicalDomain(55-58)getSupportedDomains(60-63)src/Emails/Normalizer/Provider.php (4)
supports(15-15)normalize(24-24)getCanonicalDomain(29-29)getSupportedDomains(34-34)
tests/Normalizer/Providers/GmailTest.php (2)
src/Emails/Normalizer/Providers/Gmail.php (5)
Gmail(15-52)supports(21-24)normalize(26-41)getCanonicalDomain(43-46)getSupportedDomains(48-51)src/Emails/Normalizer/Provider.php (4)
supports(15-15)normalize(24-24)getCanonicalDomain(29-29)getSupportedDomains(34-34)
src/Emails/Normalizer/Providers/Outlook.php (1)
src/Emails/Normalizer/Provider.php (7)
Provider(10-72)supports(15-15)normalize(24-24)toLowerCase(68-71)removePlusAddressing(39-47)getCanonicalDomain(29-29)getSupportedDomains(34-34)
tests/Normalizer/Providers/ProtonmailTest.php (2)
src/Emails/Normalizer/Providers/Protonmail.php (5)
Protonmail(15-57)supports(21-24)normalize(26-46)getCanonicalDomain(48-51)getSupportedDomains(53-56)src/Emails/Email.php (2)
normalize(281-284)getCanonicalDomain(310-320)
tests/Normalizer/Providers/GenericTest.php (3)
src/Emails/Normalizer/Providers/Generic.php (5)
Generic(14-56)supports(16-20)normalize(22-43)getCanonicalDomain(45-49)getSupportedDomains(51-55)src/Emails/Normalizer/Provider.php (4)
supports(15-15)normalize(24-24)getCanonicalDomain(29-29)getSupportedDomains(34-34)src/Emails/Email.php (2)
normalize(281-284)getCanonicalDomain(310-320)
🪛 markdownlint-cli2 (0.18.1)
README.md
221-221: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
230-230: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
242-242: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
251-251: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
332-332: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
337-337: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 PHPMD (2.15.0)
src/Emails/Normalizer/Providers/Protonmail.php
26-26: Avoid unused parameters such as '$domain'. (undefined)
(UnusedFormalParameter)
src/Emails/Normalizer/Providers/Icloud.php
26-26: Avoid unused parameters such as '$domain'. (undefined)
(UnusedFormalParameter)
src/Emails/Normalizer/Providers/Fastmail.php
26-26: Avoid unused parameters such as '$domain'. (undefined)
(UnusedFormalParameter)
src/Emails/Normalizer/Providers/Yahoo.php
30-30: Avoid unused parameters such as '$domain'. (undefined)
(UnusedFormalParameter)
src/Emails/Normalizer/Providers/Gmail.php
26-26: Avoid unused parameters such as '$domain'. (undefined)
(UnusedFormalParameter)
src/Emails/Normalizer/Providers/Generic.php
16-16: Avoid unused parameters such as '$domain'. (undefined)
(UnusedFormalParameter)
src/Emails/Normalizer/Providers/Outlook.php
29-29: Avoid unused parameters such as '$domain'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (5)
src/Emails/Normalizer/Providers/Icloud.php (1)
37-40: Verify iCloud's dot-handling behavior when plus-addressing is present.The implementation removes dots only when plus-addressing is detected, which is an unusual normalization rule. Most providers either always ignore dots (like Gmail) or always preserve them.
Can you confirm this is the correct behavior for iCloud? If this is based on official documentation or testing, consider adding a reference comment to explain this non-standard approach.
src/Emails/Normalizer/Providers/Fastmail.php (1)
37-40: Verify Fastmail's dot-handling behavior when plus-addressing is present.The implementation removes dots only when plus-addressing is detected, matching the Icloud provider's approach. This is an unusual normalization rule that should be verified.
Can you confirm this is the correct behavior for Fastmail? If both Fastmail and iCloud use this same rule, consider documenting the rationale or providing a reference to official provider documentation.
src/Emails/Normalizer/Providers/Yahoo.php (1)
41-44: Verify Yahoo's dot-handling behavior when plus-addressing is present.The implementation removes dots only when plus-addressing is detected, following the same pattern as Icloud and Fastmail providers. This conditional behavior should be verified.
Can you confirm this is the correct behavior for Yahoo? The comment at Line 13 states "Preserves dots in local part" but the implementation conditionally removes them when plus-addressing is present.
tests/EmailTest.php (1)
342-362: Dataset expectation for 'test.com' is correctVerification confirms 'test.com' is present in
data/disposable-domains.php(line 61808), aligning with the test expectation that$email->isDisposable()returns true for this domain.src/Emails/Email.php (1)
169-172: PHP 8 functions (str_starts_with/str_ends_with) are properly supportedThe project requires PHP >= 8.0, and the CI matrix tests against PHP 8.1, 8.2, 8.3, and nightly—no 7.x versions. The code is safe to use these PHP 8.0+ functions without compatibility concerns.
| /** | ||
| * Generic | ||
| * | ||
| * Handles generic email normalization for unsupported providers | ||
| * - Only removes plus addressing | ||
| * - Preserves all other characters | ||
| */ |
There was a problem hiding this comment.
Docblock doesn’t match implementation.
Code lowercases local and removes dots/hyphens when plus exists; doc says otherwise. Align comments to avoid confusion.
/**
* Generic
*
* Handles generic email normalization for unsupported providers
- * - Only removes plus addressing
- * - Preserves all other characters
+ * - Lowercases local part
+ * - Removes plus addressing
+ * - Removes dots and hyphens only when plus addressing was present
*/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Generic | |
| * | |
| * Handles generic email normalization for unsupported providers | |
| * - Only removes plus addressing | |
| * - Preserves all other characters | |
| */ | |
| /** | |
| * Generic | |
| * | |
| * Handles generic email normalization for unsupported providers | |
| * - Lowercases local part | |
| * - Removes plus addressing | |
| * - Removes dots and hyphens only when plus addressing was present | |
| */ |
🤖 Prompt for AI Agents
In src/Emails/Normalizer/Providers/Generic.php around lines 7 to 13, the
docblock claims this provider "only removes plus addressing" and "preserves all
other characters" but the implementation actually lowercases the local part and
strips dots and hyphens when a plus-addressing segment exists; update the
docblock to accurately describe the implemented behavior (e.g., state that it
lowercases the local part and removes dots and hyphens when plus addressing is
present, and that plus addressing is removed), or alternatively change the
implementation to match the original docblock—pick one and make the comment and
code consistent.
| /** | ||
| * ProtonMail | ||
| * | ||
| * Handles ProtonMail email normalization | ||
| * - Removes plus addressing | ||
| * - Preserves dots and hyphens in local part | ||
| * - Normalizes to protonmail.com domain | ||
| */ |
There was a problem hiding this comment.
Docblock contradicts code.
Document the actual rules (lowercase, remove plus, remove dots only when plus is present, preserve hyphens, canonicalize to protonmail.com).
/**
* ProtonMail
*
* Handles ProtonMail email normalization
- * - Removes plus addressing
- * - Preserves dots and hyphens in local part
- * - Normalizes to protonmail.com domain
+ * - Lowercases local part
+ * - Removes plus addressing
+ * - Removes dots only when plus addressing was present
+ * - Preserves hyphens
+ * - Normalizes domain to protonmail.com
*/📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * ProtonMail | |
| * | |
| * Handles ProtonMail email normalization | |
| * - Removes plus addressing | |
| * - Preserves dots and hyphens in local part | |
| * - Normalizes to protonmail.com domain | |
| */ | |
| /** | |
| * ProtonMail | |
| * | |
| * Handles ProtonMail email normalization | |
| * - Lowercases local part | |
| * - Removes plus addressing | |
| * - Removes dots only when plus addressing was present | |
| * - Preserves hyphens | |
| * - Normalizes domain to protonmail.com | |
| */ |
🤖 Prompt for AI Agents
In src/Emails/Normalizer/Providers/Protonmail.php around lines 7 to 14, the
docblock currently states rules that contradict the implementation; update the
docblock to precisely describe the actual normalization: convert to lowercase,
remove any plus addressing (strip the plus and everything after), remove dots
from the local part only when a plus addressing was present, preserve hyphens in
the local part, and canonicalize the domain to protonmail.com; make the wording
concise and reflect these exact rules so the comments match the code.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
README.md (1)
99-100: Non-existent validator classes referenced; use correctThe documentation references
EmailBasicandEmailAddressvalidators which do not exist in the codebase. The actual validator class isRun this script to confirm the available validator classes:
#!/bin/bash # Verify available Email validator classes echo "=== Validator class files ===" find src/Emails/Validator -name "*.php" -type f | xargs basename -a | sort echo "" echo "=== Check if EmailBasic or EmailAddress classes exist ===" grep -r "class EmailBasic\|class EmailAddress" src/ echo "" echo "=== Confirm Email class exists ===" grep "class Email" src/Emails/Validator/Email.php | head -1Also applies to: 107-114, 117-125, 127-134, 140-141
🧹 Nitpick comments (1)
README.md (1)
221-221: Markdown linting: Replace bold emphasis with proper headings.The static analysis flagged multiple lines using bold emphasis (
**text**) where markdown headings should be used instead (MD036 violations). Lines 221, 230, 242, 251, and 332 should use###or####heading syntax rather than bold emphasis for section titles.Apply this diff to fix the markdown linting issues:
-**Update All Domains** +### Update All Domains-**Update Disposable Domains Only** +### Update Disposable Domains Only-**Update Free Domains Only** +### Update Free Domains Only-**Show Statistics** +### Show Statistics-**Eldad Fux** +## Eldad FuxAlso applies to: 230-230, 242-242, 251-251, 332-332
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/codeql-analysis.yml(1 hunks)README.md(1 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
README.md
221-221: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
230-230: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
242-242: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
251-251: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
332-332: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (3)
.github/workflows/codeql-analysis.yml (2)
1-6: Workflow naming corrected; critical issues resolved.The previous review flagged misleading "CodeQL" naming and an outdated checkout action. Both issues have been successfully addressed:
- Workflow and job names correctly reflect "Composer Checks" instead of "CodeQL"
- Checkout action updated to v4 (no longer v3)
- Problematic
git checkout HEAD^2command removed
13-16: Docker-based Composer checks are reasonable; verify error propagation.The workflow runs Composer checks inside a Docker container. Ensure that if
composer installorcomposer checkfails, the workflow step properly exits with a non-zero status code to fail the CI run. The shell-cinvocation should propagate failures correctly, but verify the Docker exit behavior if check failures are not currently blocking merges.README.md (1)
3-5: GitHub Actions badges correctly replace outdated Travis CI.The CI badge configuration has been updated from Travis CI to GitHub Actions workflows (Tests, Linter, CodeQL), correctly reflecting the current CI infrastructure.
lohanidamodar
left a comment
lohanidamodar
left a comment
There was a problem hiding this comment.
few small comments.
…README for clarity on usage; refactor getFormatted method to utilize constants instead of strings.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
README.md (1)
111-112: Update validator class names to match actual implementation.The past review comment about non-existent
EmailBasicandEmailAddressvalidators is still valid. These classes don't exist in the codebase. Replace all references with the actual validator classUtopia\Emails\Validator\Email).Apply this diff to fix the import statements:
-use Utopia\Emails\Validator\EmailBasic; -use Utopia\Emails\Validator\EmailAddress; +use Utopia\Emails\Validator\Email as EmailValidator; use Utopia\Emails\Validator\EmailDomain;And update the usage examples:
-// Basic email validation (using PHP's filter_var) -$basicValidator = new EmailBasic(); -$basicValidator->isValid('user@example.com'); // true -$basicValidator->isValid('invalid-email'); // false - -// Advanced email validation -$validator = new EmailAddress(); +// Email validation +$validator = new EmailValidator(); $validator->isValid('user@example.com'); // trueAnd fix the API documentation:
-* **EmailBasic** - Basic email validation using PHP's filter_var function. -* **EmailAddress** - Advanced email validation with custom rules. +* **Email** - Validates complete email addresses with custom rules.Also applies to: 119-126, 152-153
src/Emails/Email.php (1)
99-99: Use normalized email in exception messages.The past review comment is still valid. Use
$this->emailinstead of$this->emailcontains the normalized (lowercased and trimmed) value that's actually stored.Apply this diff:
- throw new Exception("'{$email}' must be a valid email address"); + throw new Exception("'{$this->email}' must be a valid email address");Apply at both lines 99 and 106.
Also applies to: 106-106
🧹 Nitpick comments (3)
README.md (2)
233-233: Use proper markdown headings instead of bold emphasis.These subsections use bold text instead of proper markdown headings, causing linter warnings (MD036). For better document structure and accessibility, use
###or####headings.Based on static analysis hints.
Apply this diff:
-**Update All Domains** +#### Update All Domains -**Update Disposable Domains Only** +#### Update Disposable Domains Only -**Update Free Domains Only** +#### Update Free Domains Only -**Show Statistics** +#### Show StatisticsAlso applies to: 242-242, 254-254, 263-263
344-344: Consider using heading format for author name.The author name uses bold emphasis instead of a proper markdown heading, causing a linter warning (MD036).
Based on static analysis hints.
Apply this diff if you want to maintain consistency:
-**Eldad Fux** +### Eldad FuxAlternatively, you can leave the name as plain text without bold formatting.
src/Emails/Email.php (1)
287-293: Clarify normalize() method documentation.The
normalize()method simply returns$this->email, which is already normalized in the constructor (line 90). The method documentation suggests it performs normalization, but it's actually just returning the pre-normalized value. Consider updating the documentation to clarify this.Apply this diff to clarify:
/** - * Normalize email address (remove extra spaces, convert to lowercase) + * Get normalized email address (already lowercased and trimmed in constructor) */ public function normalize(): stringAlternatively, if this method is meant to be consistent with a broader API pattern, consider adding a note that normalization happens automatically on construction.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.md(1 hunks)src/Emails/Email.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Emails/Email.php (11)
src/Emails/Normalizer/Provider.php (4)
Provider(10-72)normalize(24-24)getCanonicalDomain(29-29)supports(15-15)src/Emails/Normalizer/Providers/Fastmail.php (4)
Fastmail(15-57)normalize(26-46)getCanonicalDomain(48-51)supports(21-24)src/Emails/Normalizer/Providers/Gmail.php (4)
Gmail(15-52)normalize(26-41)getCanonicalDomain(43-46)supports(21-24)src/Emails/Normalizer/Providers/Icloud.php (4)
Icloud(15-57)normalize(26-46)getCanonicalDomain(48-51)supports(21-24)src/Emails/Normalizer/Providers/Outlook.php (4)
Outlook(15-52)normalize(29-41)getCanonicalDomain(43-46)supports(24-27)src/Emails/Normalizer/Providers/Protonmail.php (4)
Protonmail(15-57)normalize(26-46)getCanonicalDomain(48-51)supports(21-24)src/Emails/Normalizer/Providers/Yahoo.php (4)
Yahoo(16-64)normalize(30-53)getCanonicalDomain(55-58)supports(25-28)src/Emails/Validator/EmailCorporate.php (1)
isValid(32-45)src/Emails/Validator/EmailDomain.php (1)
isValid(32-45)src/Emails/Validator/EmailLocal.php (1)
isValid(32-45)src/Emails/Validator/EmailNotDisposable.php (1)
isValid(32-45)
🪛 GitHub Actions: Linter
src/Emails/Email.php
[error] 1-1: Pint linting issue detected: class_attributes_separation, unary_operator_spaces, … (lint rules reported by Laravel Pint).
🪛 markdownlint-cli2 (0.18.1)
README.md
233-233: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
242-242: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
254-254: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
263-263: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
344-344: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (2)
README.md (1)
307-329: Well-documented manual domain management.The manual domain management section clearly explains how users can customize free and disposable domain lists with practical PHP examples.
src/Emails/Email.php (1)
150-202: Solid validation logic.The email validation methods are well-implemented:
isValid()uses PHP's built-infilter_varfor overall format validationhasValidLocal()performs thorough checks: length limits, character validation, consecutive dot prevention, and leading/trailing dot checkshasValidDomain()leveragesfilter_varwith a test local part to validate domain formatThese align well with the validation rules documented in the README.
…xibility in email handling
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/Emails/Email.php (4)
242-247: Redundant conditional check (already flagged in previous review).The check at lines 242-244 is redundant because
isFree()already returnsfalsewhen a domain is both free and disposable. This condition can never betrue, so the explicit check and early return serve no purpose.Simplify to:
public function isCorporate(): bool { - // If domain is both free and disposable, prioritize free classification - if ($this->isFree() && $this->isDisposable()) { - return false; // It's free, not corporate - } - return ! $this->isFree() && ! $this->isDisposable(); }
250-267: Multi-level TLD handling bug (already flagged in previous review).The
getProvider()method incorrectly handles multi-level TLDs such as.co.uk. Foruser@example.co.uk, it returnsco.ukinstead ofexample.co.uk. This is a known issue from previous reviews.
338-359: Duplicate provider initialization (already flagged in previous review).The provider initialization code (lines 340-348) is duplicated in
isDomainSupported()at lines 366-374. This should be extracted to a separate initialization method as previously suggested.
1-405: Pint linting issues (already flagged in previous review).The Laravel Pint linter reported code style violations. Run
./vendor/bin/pintto auto-fix these formatting issues.Based on pipeline failures.
🧹 Nitpick comments (1)
src/Emails/Email.php (1)
291-297: Misleading method name for already-normalized data.The
normalize()method simply returns$this->email, which is already normalized in the constructor (lowercased and trimmed at line 94). This method name suggests it performs normalization, but it doesn't. Consider renaming togetNormalized()or documenting that normalization happens in the constructor.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Emails/Email.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/Emails/Email.php (8)
src/Emails/Normalizer/Provider.php (4)
Provider(10-72)normalize(24-24)getCanonicalDomain(29-29)supports(15-15)src/Emails/Normalizer/Providers/Fastmail.php (4)
Fastmail(15-57)normalize(26-46)getCanonicalDomain(48-51)supports(21-24)src/Emails/Normalizer/Providers/Generic.php (4)
Generic(14-56)normalize(22-43)getCanonicalDomain(45-49)supports(16-20)src/Emails/Normalizer/Providers/Gmail.php (4)
Gmail(15-52)normalize(26-41)getCanonicalDomain(43-46)supports(21-24)src/Emails/Normalizer/Providers/Icloud.php (4)
Icloud(15-57)normalize(26-46)getCanonicalDomain(48-51)supports(21-24)src/Emails/Normalizer/Providers/Outlook.php (4)
Outlook(15-52)normalize(29-41)getCanonicalDomain(43-46)supports(24-27)src/Emails/Normalizer/Providers/Protonmail.php (4)
Protonmail(15-57)normalize(26-46)getCanonicalDomain(48-51)supports(21-24)src/Emails/Normalizer/Providers/Yahoo.php (4)
Yahoo(16-64)normalize(30-53)getCanonicalDomain(55-58)supports(25-28)
…roviders until manual confirmation is obtained for their support across Fastmail, Generic, iCloud, Outlook, ProtonMail, and Yahoo. Update test cases accordingly to reflect these changes.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/Emails/Normalizer/Providers/Yahoo.php (1)
7-14: Update docblock to reflect current implementation.The docblock states rules that don't match what the code actually does. Currently, the normalize method only lowercases the local part. Update the docblock to describe the actual behavior:
/** * Yahoo * * Handles Yahoo email normalization - * - TODO: Plus addressing, dots, and hyphens removal commented out until manual confirmation - * - Preserves dots and hyphens in local part + * - Lowercases local part + * - TODO: Plus addressing, dots, and hyphens removal require manual confirmation before implementation * - Normalizes to yahoo.com domain */src/Emails/Normalizer/Providers/Outlook.php (1)
7-14: Clarify docblock to describe current behavior.The docblock should describe what the code currently does. Update to reflect the actual implementation:
/** * Outlook * * Handles Outlook, Hotmail, and Live email normalization - * - TODO: Plus addressing removal commented out until manual confirmation - * - Preserves dots in local part + * - Lowercases local part + * - TODO: Plus addressing removal requires manual confirmation before implementation * - Normalizes to outlook.com domain */src/Emails/Normalizer/Providers/Fastmail.php (1)
7-14: Update docblock to accurately describe current implementation.The docblock should reflect what the code actually does now rather than future plans:
/** * Fastmail * * Handles Fastmail email normalization - * - TODO: Plus addressing and dots removal commented out until manual confirmation - * - Preserves dots and hyphens in local part + * - Lowercases local part + * - TODO: Plus addressing and dots removal require manual confirmation before implementation * - Normalizes to fastmail.com domain */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/Emails/Normalizer/Providers/Fastmail.php(1 hunks)src/Emails/Normalizer/Providers/Generic.php(1 hunks)src/Emails/Normalizer/Providers/Icloud.php(1 hunks)src/Emails/Normalizer/Providers/Outlook.php(1 hunks)src/Emails/Normalizer/Providers/Protonmail.php(1 hunks)src/Emails/Normalizer/Providers/Yahoo.php(1 hunks)tests/EmailTest.php(1 hunks)tests/Normalizer/Providers/FastmailTest.php(1 hunks)tests/Normalizer/Providers/GenericTest.php(1 hunks)tests/Normalizer/Providers/IcloudTest.php(1 hunks)tests/Normalizer/Providers/OutlookTest.php(1 hunks)tests/Normalizer/Providers/ProtonmailTest.php(1 hunks)tests/Normalizer/Providers/YahooTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/Normalizer/Providers/GenericTest.php
- tests/Normalizer/Providers/YahooTest.php
- tests/Normalizer/Providers/OutlookTest.php
🧰 Additional context used
🧬 Code graph analysis (10)
src/Emails/Normalizer/Providers/Yahoo.php (2)
src/Emails/Normalizer/Provider.php (6)
Provider(10-72)supports(15-15)normalize(24-24)toLowerCase(68-71)getCanonicalDomain(29-29)getSupportedDomains(34-34)src/Emails/Email.php (2)
normalize(294-297)getCanonicalDomain(323-333)
src/Emails/Normalizer/Providers/Protonmail.php (3)
src/Emails/Normalizer/Provider.php (6)
Provider(10-72)supports(15-15)normalize(24-24)toLowerCase(68-71)getCanonicalDomain(29-29)getSupportedDomains(34-34)src/Emails/Normalizer/Providers/Gmail.php (4)
supports(21-24)normalize(26-41)getCanonicalDomain(43-46)getSupportedDomains(48-51)src/Emails/Email.php (2)
normalize(294-297)getCanonicalDomain(323-333)
tests/Normalizer/Providers/ProtonmailTest.php (3)
src/Emails/Normalizer/Providers/Protonmail.php (5)
Protonmail(15-58)supports(21-24)normalize(26-47)getCanonicalDomain(49-52)getSupportedDomains(54-57)src/Emails/Email.php (2)
normalize(294-297)getCanonicalDomain(323-333)tests/EmailTest.php (1)
test_get_canonical_domain(849-883)
src/Emails/Normalizer/Providers/Generic.php (2)
src/Emails/Normalizer/Provider.php (6)
Provider(10-72)supports(15-15)normalize(24-24)toLowerCase(68-71)getCanonicalDomain(29-29)getSupportedDomains(34-34)src/Emails/Email.php (2)
normalize(294-297)getCanonicalDomain(323-333)
tests/Normalizer/Providers/IcloudTest.php (3)
src/Emails/Normalizer/Providers/Icloud.php (5)
Icloud(15-58)supports(21-24)normalize(26-47)getCanonicalDomain(49-52)getSupportedDomains(54-57)src/Emails/Normalizer/Provider.php (4)
supports(15-15)normalize(24-24)getCanonicalDomain(29-29)getSupportedDomains(34-34)src/Emails/Email.php (2)
normalize(294-297)getCanonicalDomain(323-333)
src/Emails/Normalizer/Providers/Icloud.php (1)
src/Emails/Normalizer/Provider.php (6)
Provider(10-72)supports(15-15)normalize(24-24)toLowerCase(68-71)getCanonicalDomain(29-29)getSupportedDomains(34-34)
tests/EmailTest.php (9)
src/Emails/Email.php (20)
get(117-120)getLocal(125-128)getDomain(133-136)getDomainOnly(141-144)getLocalOnly(149-152)isValid(157-160)hasValidLocal(165-188)hasValidDomain(193-206)isDisposable(211-218)isFree(223-235)isCorporate(240-248)getProvider(253-267)getSubdomain(272-281)hasSubdomain(286-289)normalize(294-297)getFormatted(389-404)getUnique(304-310)isNormalizationSupported(315-318)getCanonicalDomain(323-333)src/Emails/Normalizer/Providers/Fastmail.php (2)
normalize(26-47)getCanonicalDomain(49-52)src/Emails/Normalizer/Providers/Generic.php (2)
normalize(22-44)getCanonicalDomain(46-50)src/Emails/Normalizer/Providers/Icloud.php (2)
normalize(26-47)getCanonicalDomain(49-52)src/Emails/Normalizer/Providers/Outlook.php (2)
normalize(29-42)getCanonicalDomain(44-47)src/Emails/Normalizer/Providers/Protonmail.php (2)
normalize(26-47)getCanonicalDomain(49-52)src/Emails/Normalizer/Providers/Yahoo.php (2)
normalize(29-53)getCanonicalDomain(55-58)src/Emails/Normalizer/Provider.php (2)
normalize(24-24)getCanonicalDomain(29-29)src/Emails/Normalizer/Providers/Gmail.php (2)
normalize(26-41)getCanonicalDomain(43-46)
tests/Normalizer/Providers/FastmailTest.php (3)
src/Emails/Normalizer/Providers/Fastmail.php (5)
Fastmail(15-58)supports(21-24)normalize(26-47)getCanonicalDomain(49-52)getSupportedDomains(54-57)src/Emails/Normalizer/Provider.php (4)
supports(15-15)normalize(24-24)getCanonicalDomain(29-29)getSupportedDomains(34-34)tests/EmailTest.php (1)
test_get_canonical_domain(849-883)
src/Emails/Normalizer/Providers/Fastmail.php (4)
src/Emails/Normalizer/Provider.php (6)
Provider(10-72)supports(15-15)normalize(24-24)toLowerCase(68-71)getCanonicalDomain(29-29)getSupportedDomains(34-34)src/Emails/Normalizer/Providers/Generic.php (4)
supports(16-20)normalize(22-44)getCanonicalDomain(46-50)getSupportedDomains(52-56)src/Emails/Normalizer/Providers/Icloud.php (4)
supports(21-24)normalize(26-47)getCanonicalDomain(49-52)getSupportedDomains(54-57)src/Emails/Normalizer/Providers/Gmail.php (4)
supports(21-24)normalize(26-41)getCanonicalDomain(43-46)getSupportedDomains(48-51)
src/Emails/Normalizer/Providers/Outlook.php (3)
src/Emails/Normalizer/Provider.php (6)
Provider(10-72)supports(15-15)normalize(24-24)toLowerCase(68-71)getCanonicalDomain(29-29)getSupportedDomains(34-34)src/Emails/Normalizer/Providers/Gmail.php (4)
supports(21-24)normalize(26-41)getCanonicalDomain(43-46)getSupportedDomains(48-51)src/Emails/Email.php (2)
normalize(294-297)getCanonicalDomain(323-333)
🪛 PHPMD (2.15.0)
src/Emails/Normalizer/Providers/Yahoo.php
29-29: Avoid unused parameters such as '$domain'. (undefined)
(UnusedFormalParameter)
src/Emails/Normalizer/Providers/Protonmail.php
26-26: Avoid unused parameters such as '$domain'. (undefined)
(UnusedFormalParameter)
src/Emails/Normalizer/Providers/Generic.php
16-16: Avoid unused parameters such as '$domain'. (undefined)
(UnusedFormalParameter)
src/Emails/Normalizer/Providers/Icloud.php
26-26: Avoid unused parameters such as '$domain'. (undefined)
(UnusedFormalParameter)
src/Emails/Normalizer/Providers/Fastmail.php
26-26: Avoid unused parameters such as '$domain'. (undefined)
(UnusedFormalParameter)
src/Emails/Normalizer/Providers/Outlook.php
29-29: Avoid unused parameters such as '$domain'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (2)
tests/Normalizer/Providers/FastmailTest.php (1)
30-37: LGTM!The test comprehensively validates both supported and unsupported domains with appropriate assertions.
src/Emails/Normalizer/Providers/Icloud.php (1)
17-24: LGTM!The domain configuration and supports method are correctly implemented with proper strict comparison.
…acing deprecated methods and introducing a new Canonicals namespace. Update README and tests to reflect changes in method names and functionality.
…ain parsing and validation. Refactor getProvider, getSubdomain, and hasSubdomain methods to utilize the new Domain functionality, ensuring better handling of email domains.
…comments and whitespace. Update domain validation logic for improved clarity and consistency.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
src/Emails/Email.php (6)
110-112: Code correctly interpolates exception messages.The exception messages properly use double quotes and interpolate the
250-264: Verify Domain::getRegisterable() handles multi-level TLDs correctly.The implementation delegates to
Domain::getRegisterable()from the utopia-php/domains package. If that method uses a Public Suffix List, multi-level TLDs should be handled correctly. Otherwise, the concern raised in past reviews aboutbusiness.co.uk→co.ukremains valid.Run this script to test the actual behavior:
#!/bin/bash # Test getProvider() with multi-level TLD domains cat > test_provider.php << 'EOF' <?php require_once 'vendor/autoload.php'; use Utopia\Emails\Email; $testCases = [ 'user@business.co.uk', 'user@example.com', 'user@mail.example.co.uk', ]; foreach ($testCases as $emailAddr) { try { $email = new Email($emailAddr); echo "$emailAddr -> getProvider(): " . $email->getProvider() . "\n"; } catch (Exception $e) { echo "$emailAddr -> ERROR: " . $e->getMessage() . "\n"; } } EOF php test_provider.php rm test_provider.php
330-376: Extract duplicate provider initialization logic.Both
getProviderForDomain()(lines 332-341) andisDomainSupported()(lines 358-367) contain identical provider initialization code. Extract this to a single helper method to improve maintainability.Apply this refactor:
+ /** + * Initialize the providers array if needed + */ + protected static function initializeProviders(): void + { + if (self::$providers === null) { + self::$providers = [ + new Gmail, + new Outlook, + new Yahoo, + new Icloud, + new Protonmail, + new Fastmail, + ]; + } + } + /** * Get the appropriate provider for a given domain */ protected function getProviderForDomain(string $domain): Provider { - if (self::$providers === null) { - self::$providers = [ - new Gmail, - new Outlook, - new Yahoo, - new Icloud, - new Protonmail, - new Fastmail, - ]; - } + self::initializeProviders(); foreach (self::$providers as $provider) { if ($provider->supports($domain)) { return $provider; } } return new Generic; } /** * Check if a domain is supported by any provider */ protected function isDomainSupported(string $domain): bool { - if (self::$providers === null) { - self::$providers = [ - new Gmail, - new Outlook, - new Yahoo, - new Icloud, - new Protonmail, - new Fastmail, - ]; - } + self::initializeProviders(); foreach (self::$providers as $provider) { if ($provider->supports($domain)) { return true; } } return false; }
208-218: Consider adding validation for data file loading.If
disposable-domains.phpis missing or returns a non-array, this will cause a runtime error. While these files should always exist in the package, adding defensive validation improves robustness.Apply this defensive pattern:
public function isDisposable(): bool { if (self::$disposableDomains === null) { - self::$disposableDomains = include __DIR__.'/../../data/disposable-domains.php'; + $data = @include __DIR__.'/../../data/disposable-domains.php'; + self::$disposableDomains = is_array($data) ? $data : []; } return in_array($this->domain, self::$disposableDomains); }
220-235: Consider adding validation for data file loading.Similar to
isDisposable(), add defensive validation when loading the free domains data file.Apply this defensive pattern:
public function isFree(): bool { if (self::$freeDomains === null) { - self::$freeDomains = include __DIR__.'/../../data/free-domains.php'; + $data = @include __DIR__.'/../../data/free-domains.php'; + self::$freeDomains = is_array($data) ? $data : []; } // If domain is both free and disposable, prioritize disposable classification if (in_array($this->domain, self::$freeDomains) && $this->isDisposable()) { return false; // It's disposable, not free } return in_array($this->domain, self::$freeDomains); }
240-248: Simplify isCorporate() logic—redundant check.The explicit check for
isFree() && isDisposable()at lines 243-245 is redundant becauseisFree()(line 230-231) already returnsfalsewhen a domain is both free and disposable. The logic can be simplified.Apply this simplification:
public function isCorporate(): bool { - // If domain is both free and disposable, prioritize free classification - if ($this->isFree() && $this->isDisposable()) { - return false; // It's free, not corporate - } - return ! $this->isFree() && ! $this->isDisposable(); }The comment was misleading—disposable is actually prioritized in
isFree(), not free.
🧹 Nitpick comments (2)
README.md (2)
232-232: Use proper markdown headings instead of bold text for section titles.Lines 232, 241, 253, 262 use bold text (
**text**) for subsection titles, which reduces document structure clarity. Replace with proper markdown headings (e.g.,#### Update All Domains) to improve semantic structure and navigation.Apply these changes:
-**Update All Domains** +#### Update All Domains-**Update Disposable Domains Only** +#### Update Disposable Domains Only-**Update Free Domains Only** +#### Update Free Domains Only-**Show Statistics** +#### Show Statistics
343-343: Use proper markdown heading for the Authors section.Line 343 uses bold text for "Authors" instead of a proper heading. Replace with
## Authorsfor consistency with other top-level sections.Apply this change:
-**Eldad Fux** +## Authors + +Eldad Fux
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
README.md(1 hunks)src/Emails/Canonicals/Provider.php(1 hunks)src/Emails/Canonicals/Providers/Fastmail.php(1 hunks)src/Emails/Canonicals/Providers/Generic.php(1 hunks)src/Emails/Canonicals/Providers/Gmail.php(1 hunks)src/Emails/Canonicals/Providers/Icloud.php(1 hunks)src/Emails/Canonicals/Providers/Outlook.php(1 hunks)src/Emails/Canonicals/Providers/Protonmail.php(1 hunks)src/Emails/Canonicals/Providers/Yahoo.php(1 hunks)src/Emails/Email.php(1 hunks)tests/Canonicals/Providers/FastmailTest.php(1 hunks)tests/Canonicals/Providers/GenericTest.php(1 hunks)tests/Canonicals/Providers/GmailTest.php(1 hunks)tests/Canonicals/Providers/IcloudTest.php(1 hunks)tests/Canonicals/Providers/OutlookTest.php(1 hunks)tests/Canonicals/Providers/ProtonmailTest.php(1 hunks)tests/Canonicals/Providers/YahooTest.php(1 hunks)tests/EmailTest.php(1 hunks)tests/Validator/EmailCorporateTest.php(1 hunks)tests/Validator/EmailDomainTest.php(1 hunks)tests/Validator/EmailLocalTest.php(1 hunks)tests/Validator/EmailNotDisposableTest.php(1 hunks)tests/Validator/EmailTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/Validator/EmailDomainTest.php
- tests/Validator/EmailTest.php
- tests/Validator/EmailCorporateTest.php
- tests/Validator/EmailLocalTest.php
🧰 Additional context used
🧬 Code graph analysis (18)
src/Emails/Canonicals/Providers/Gmail.php (2)
src/Emails/Canonicals/Provider.php (8)
Provider(10-72)supports(15-15)getCanonical(24-24)toLowerCase(68-71)removeDots(52-55)removePlusAddressing(39-47)getCanonicalDomain(29-29)getSupportedDomains(34-34)src/Emails/Email.php (2)
getCanonical(296-302)getCanonicalDomain(315-325)
tests/Validator/EmailNotDisposableTest.php (2)
src/Emails/Validator/EmailNotDisposable.php (5)
EmailNotDisposable(13-66)isValid(32-45)getDescription(20-23)getType(62-65)isArray(52-55)src/Emails/Email.php (1)
isValid(152-155)
src/Emails/Canonicals/Providers/Fastmail.php (3)
src/Emails/Canonicals/Provider.php (6)
Provider(10-72)supports(15-15)getCanonical(24-24)toLowerCase(68-71)getCanonicalDomain(29-29)getSupportedDomains(34-34)src/Emails/Canonicals/Providers/Generic.php (4)
supports(16-20)getCanonical(22-44)getCanonicalDomain(46-50)getSupportedDomains(52-56)src/Emails/Email.php (2)
getCanonical(296-302)getCanonicalDomain(315-325)
tests/Canonicals/Providers/GmailTest.php (2)
src/Emails/Canonicals/Providers/Gmail.php (5)
Gmail(15-52)supports(21-24)getCanonical(26-41)getCanonicalDomain(43-46)getSupportedDomains(48-51)src/Emails/Email.php (2)
getCanonical(296-302)getCanonicalDomain(315-325)
src/Emails/Canonicals/Providers/Icloud.php (2)
src/Emails/Canonicals/Provider.php (6)
Provider(10-72)supports(15-15)getCanonical(24-24)toLowerCase(68-71)getCanonicalDomain(29-29)getSupportedDomains(34-34)src/Emails/Email.php (2)
getCanonical(296-302)getCanonicalDomain(315-325)
tests/Canonicals/Providers/FastmailTest.php (3)
src/Emails/Canonicals/Providers/Fastmail.php (5)
Fastmail(15-58)supports(21-24)getCanonical(26-47)getCanonicalDomain(49-52)getSupportedDomains(54-57)src/Emails/Email.php (2)
getCanonical(296-302)getCanonicalDomain(315-325)tests/EmailTest.php (1)
test_get_canonical_domain(849-883)
src/Emails/Canonicals/Providers/Generic.php (2)
src/Emails/Canonicals/Provider.php (6)
Provider(10-72)supports(15-15)getCanonical(24-24)toLowerCase(68-71)getCanonicalDomain(29-29)getSupportedDomains(34-34)src/Emails/Email.php (2)
getCanonical(296-302)getCanonicalDomain(315-325)
src/Emails/Canonicals/Providers/Protonmail.php (2)
src/Emails/Canonicals/Provider.php (6)
Provider(10-72)supports(15-15)getCanonical(24-24)toLowerCase(68-71)getCanonicalDomain(29-29)getSupportedDomains(34-34)src/Emails/Email.php (2)
getCanonical(296-302)getCanonicalDomain(315-325)
src/Emails/Canonicals/Providers/Yahoo.php (2)
src/Emails/Canonicals/Provider.php (6)
Provider(10-72)supports(15-15)getCanonical(24-24)toLowerCase(68-71)getCanonicalDomain(29-29)getSupportedDomains(34-34)src/Emails/Email.php (2)
getCanonical(296-302)getCanonicalDomain(315-325)
src/Emails/Email.php (12)
src/Emails/Canonicals/Provider.php (4)
Provider(10-72)getCanonical(24-24)getCanonicalDomain(29-29)supports(15-15)src/Emails/Canonicals/Providers/Fastmail.php (4)
Fastmail(15-58)getCanonical(26-47)getCanonicalDomain(49-52)supports(21-24)src/Emails/Canonicals/Providers/Generic.php (4)
Generic(14-57)getCanonical(22-44)getCanonicalDomain(46-50)supports(16-20)src/Emails/Canonicals/Providers/Gmail.php (4)
Gmail(15-52)getCanonical(26-41)getCanonicalDomain(43-46)supports(21-24)src/Emails/Canonicals/Providers/Icloud.php (4)
Icloud(15-58)getCanonical(26-47)getCanonicalDomain(49-52)supports(21-24)src/Emails/Canonicals/Providers/Outlook.php (4)
Outlook(15-53)getCanonical(29-42)getCanonicalDomain(44-47)supports(24-27)src/Emails/Canonicals/Providers/Protonmail.php (4)
Protonmail(15-58)getCanonical(26-47)getCanonicalDomain(49-52)supports(21-24)src/Emails/Canonicals/Providers/Yahoo.php (4)
Yahoo(15-64)getCanonical(29-53)getCanonicalDomain(55-58)supports(24-27)src/Emails/Validator/EmailCorporate.php (1)
isValid(32-45)src/Emails/Validator/EmailDomain.php (1)
isValid(32-45)src/Emails/Validator/EmailLocal.php (1)
isValid(32-45)src/Emails/Validator/EmailNotDisposable.php (1)
isValid(32-45)
tests/Canonicals/Providers/YahooTest.php (3)
src/Emails/Canonicals/Providers/Yahoo.php (5)
Yahoo(15-64)supports(24-27)getCanonical(29-53)getCanonicalDomain(55-58)getSupportedDomains(60-63)src/Emails/Canonicals/Provider.php (4)
supports(15-15)getCanonical(24-24)getCanonicalDomain(29-29)getSupportedDomains(34-34)src/Emails/Email.php (2)
getCanonical(296-302)getCanonicalDomain(315-325)
src/Emails/Canonicals/Providers/Outlook.php (2)
src/Emails/Canonicals/Provider.php (6)
Provider(10-72)supports(15-15)getCanonical(24-24)toLowerCase(68-71)getCanonicalDomain(29-29)getSupportedDomains(34-34)src/Emails/Email.php (2)
getCanonical(296-302)getCanonicalDomain(315-325)
tests/Canonicals/Providers/OutlookTest.php (2)
src/Emails/Canonicals/Providers/Outlook.php (5)
Outlook(15-53)supports(24-27)getCanonical(29-42)getCanonicalDomain(44-47)getSupportedDomains(49-52)src/Emails/Canonicals/Provider.php (4)
supports(15-15)getCanonical(24-24)getCanonicalDomain(29-29)getSupportedDomains(34-34)
tests/Canonicals/Providers/GenericTest.php (2)
src/Emails/Canonicals/Providers/Generic.php (5)
Generic(14-57)supports(16-20)getCanonical(22-44)getCanonicalDomain(46-50)getSupportedDomains(52-56)src/Emails/Email.php (2)
getCanonical(296-302)getCanonicalDomain(315-325)
tests/EmailTest.php (10)
src/Emails/Email.php (18)
get(128-131)getLocal(136-139)getDomain(144-147)isValid(152-155)hasValidLocal(160-183)hasValidDomain(188-206)isDisposable(211-218)isFree(223-235)isCorporate(240-248)getProvider(253-264)getSubdomain(269-273)hasSubdomain(278-281)getAddress(286-289)getFormatted(381-396)getCanonical(296-302)isCanonicalSupported(307-310)getCanonicalDomain(315-325)tests/Validator/EmailTest.php (1)
EmailTest(21-94)src/Emails/Canonicals/Provider.php (2)
getCanonical(24-24)getCanonicalDomain(29-29)src/Emails/Canonicals/Providers/Fastmail.php (2)
getCanonical(26-47)getCanonicalDomain(49-52)src/Emails/Canonicals/Providers/Generic.php (2)
getCanonical(22-44)getCanonicalDomain(46-50)src/Emails/Canonicals/Providers/Gmail.php (2)
getCanonical(26-41)getCanonicalDomain(43-46)src/Emails/Canonicals/Providers/Icloud.php (2)
getCanonical(26-47)getCanonicalDomain(49-52)src/Emails/Canonicals/Providers/Outlook.php (2)
getCanonical(29-42)getCanonicalDomain(44-47)src/Emails/Canonicals/Providers/Protonmail.php (2)
getCanonical(26-47)getCanonicalDomain(49-52)src/Emails/Canonicals/Providers/Yahoo.php (2)
getCanonical(29-53)getCanonicalDomain(55-58)
tests/Canonicals/Providers/ProtonmailTest.php (3)
src/Emails/Canonicals/Providers/Protonmail.php (5)
Protonmail(15-58)supports(21-24)getCanonical(26-47)getCanonicalDomain(49-52)getSupportedDomains(54-57)src/Emails/Email.php (2)
getCanonical(296-302)getCanonicalDomain(315-325)tests/EmailTest.php (1)
test_get_canonical_domain(849-883)
src/Emails/Canonicals/Provider.php (3)
src/Emails/Canonicals/Providers/Generic.php (2)
getCanonical(22-44)getCanonicalDomain(46-50)src/Emails/Canonicals/Providers/Gmail.php (2)
getCanonical(26-41)getCanonicalDomain(43-46)src/Emails/Email.php (2)
getCanonical(296-302)getCanonicalDomain(315-325)
tests/Canonicals/Providers/IcloudTest.php (3)
src/Emails/Canonicals/Providers/Icloud.php (5)
Icloud(15-58)supports(21-24)getCanonical(26-47)getCanonicalDomain(49-52)getSupportedDomains(54-57)src/Emails/Email.php (2)
getCanonical(296-302)getCanonicalDomain(315-325)tests/EmailTest.php (1)
test_get_canonical_domain(849-883)
🪛 GitHub Actions: Linter
src/Emails/Email.php
[error] 1-1: Laravel Pint lint failed: 1 style issue reported - unary_operator_spaces, not_operator_with_successor_s… (see above in logs).
🪛 markdownlint-cli2 (0.18.1)
README.md
232-232: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
241-241: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
253-253: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
262-262: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
343-343: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🪛 PHPMD (2.15.0)
src/Emails/Canonicals/Providers/Gmail.php
26-26: Avoid unused parameters such as '$domain'. (undefined)
(UnusedFormalParameter)
src/Emails/Canonicals/Providers/Fastmail.php
26-26: Avoid unused parameters such as '$domain'. (undefined)
(UnusedFormalParameter)
src/Emails/Canonicals/Providers/Icloud.php
26-26: Avoid unused parameters such as '$domain'. (undefined)
(UnusedFormalParameter)
src/Emails/Canonicals/Providers/Generic.php
16-16: Avoid unused parameters such as '$domain'. (undefined)
(UnusedFormalParameter)
src/Emails/Canonicals/Providers/Protonmail.php
26-26: Avoid unused parameters such as '$domain'. (undefined)
(UnusedFormalParameter)
src/Emails/Canonicals/Providers/Yahoo.php
29-29: Avoid unused parameters such as '$domain'. (undefined)
(UnusedFormalParameter)
src/Emails/Canonicals/Providers/Outlook.php
29-29: Avoid unused parameters such as '$domain'. (undefined)
(UnusedFormalParameter)
🔇 Additional comments (17)
tests/Validator/EmailNotDisposableTest.php (1)
23-97: LGTM! Comprehensive test coverage for EmailNotDisposable validator.The test suite thoroughly covers:
- Valid non-disposable emails (corporate and free email providers)
- Invalid disposable emails from various temporary email services
- Invalid email formats and edge cases
- Non-string input validation
- Validator metadata (description, type, isArray)
The test structure is clear and well-organized with good assertions.
src/Emails/Canonicals/Providers/Gmail.php (1)
26-41: LGTM! The unused$domainparameter is required by the interface.The PHPMD warning about the unused
$domainparameter is a false positive. The parameter is required by the abstractProvider::getCanonical()signature and is intentionally unused here because Gmail always normalizes to'gmail.com'regardless of the input domain ('gmail.com'or'googlemail.com').The canonicalization logic correctly implements Gmail's documented behavior:
- Lowercases the local part
- Removes all dots
- Removes plus-addressing
tests/Canonicals/Providers/GenericTest.php (1)
30-96: LGTM! Generic provider tests correctly verify fallback behavior.The test suite appropriately validates the Generic provider's minimal canonicalization behavior:
- Supports all domains (universal fallback)
- Only lowercases the local part (preserves dots, hyphens, and other characters)
- Returns empty canonical domain and empty supported domains array
The commented-out tests (lines 45-58) for plus-addressing are appropriately deferred until generic provider behavior can be confirmed across different email services.
tests/Canonicals/Providers/GmailTest.php (1)
30-84: LGTM! Excellent comprehensive test coverage for Gmail canonicalization.The test suite thoroughly validates Gmail's well-documented canonicalization behavior:
- Dot removal from local parts
- Plus-addressing removal
- Domain normalization (googlemail.com → gmail.com)
- Edge cases (trailing dots, leading dots, consecutive dots, empty tags)
- Multiple dots and complex patterns
All test cases align with Gmail's official aliasing behavior. The coverage is comprehensive and assertions are clear.
tests/Canonicals/Providers/YahooTest.php (1)
42-102: No changes required; Yahoo canonicalization tests are correct.Yahoo Mail does not reliably support plus-addressing, making the commented-out plus-address tests appropriately disabled. Dots are significant for Yahoo addresses and hyphens are supported as disposable/alias addresses when explicitly created in Settings, both of which the active test cases correctly preserve. The domain normalization to yahoo.com (lines 95–98) aligns with Yahoo's behavior for its owned domains (ymail.com, rocketmail.com).
tests/Canonicals/Providers/ProtonmailTest.php (1)
40-76: Plus addressing and dot handling are documented in ProtonMail's official support — several test expectations appear inconsistent with actual behavior.The web search reveals:
- ProtonMail supports plus-addressing (address+tag format), with aliases created automatically and delivering to the inbox
- Dots in the local-part are treated as the same address — first.last and firstlast deliver to the same account
This creates two issues with the current test:
Commented tests (lines 43–58): Plus-addressing is officially supported, so these tests should likely be uncommented or the canonicalization logic revisited, rather than left pending.
Dot-preservation tests (lines 60–61): The test expects
user.name→user.name, but official docs indicate dots are insignificant. If ProtonMail strips or ignores dots during canonicalization, these test expectations are incorrect.Edge cases (lines 64–65): Preserving
user.and.userviolates email format standards and contradicts the stated dot-handling behavior.Recommend: confirm whether the canonicalization implementation actually strips dots (matching ProtonMail's behavior), and update test cases accordingly.
tests/Canonicals/Providers/IcloudTest.php (1)
40-76: Plus addressing tests are correctly commented out; dot preservation is correctly implemented per iCloud's documented behavior.iCloud Mail does not reliably accept plus-addressed mail and Apple's official documentation does not document support for ad‑hoc "plus addressing" (user+tag@icloud.com). The commented-out tests (lines 43–58) are intentionally excluded, not pending verification—they reflect iCloud's actual lack of support.
Dots (periods) in iCloud addresses are significant and not normalized away the way Gmail handles them. The dot-preservation tests (lines 60–61) correctly reflect this behavior, and the edge cases with leading/trailing dots appropriately preserve them.
The edge case concern about RFC 5321 validity is separate from canonicalization; canonicalization typically normalizes valid input rather than validates it.
tests/Canonicals/Providers/FastmailTest.php (1)
21-84: LGTM! Test coverage aligns with current implementation.The test suite correctly validates the current Fastmail provider behavior (lowercasing and domain canonicalization) while appropriately commenting out test cases for normalization rules pending manual confirmation. The active edge case tests (leading/trailing dots) provide useful coverage.
src/Emails/Canonicals/Providers/Generic.php (1)
14-57: LGTM! Generic provider serves as appropriate fallback.The implementation correctly provides minimal normalization (lowercasing only) for unsupported providers. The docblock accurately describes the current behavior with TODO notes for future enhancements.
Note: The PHPMD warning about unused
$domainparameter is a false positive—the parameter is required by the abstractProviderinterface.src/Emails/Canonicals/Providers/Yahoo.php (1)
15-64: LGTM! Yahoo provider follows consistent pattern.The implementation correctly handles Yahoo's domain variants and applies minimal normalization pending confirmation of Yahoo-specific rules. The docblock accurately describes the current behavior.
Note: The PHPMD warning about unused
$domainparameter is a false positive—the parameter is required by the abstractProviderinterface.src/Emails/Canonicals/Providers/Icloud.php (1)
15-58: LGTM! iCloud provider correctly handles Apple domain variants.The implementation properly supports iCloud's multiple domain aliases (icloud.com, me.com, mac.com) and applies consistent normalization. The docblock accurately describes the current behavior.
Note: The PHPMD warning about unused
$domainparameter is a false positive—the parameter is required by the abstractProviderinterface.src/Emails/Canonicals/Provider.php (1)
10-72: LGTM! Well-designed abstract base for canonicalization providers.The abstract interface clearly defines the contract for provider implementations, and the protected helper methods provide reusable normalization utilities. The
removePlusAddressingmethod correctly handles the edge case where a plus sign appears at the start of the local part.src/Emails/Canonicals/Providers/Protonmail.php (1)
15-58: LGTM! ProtonMail provider handles multiple domain aliases correctly.The implementation properly supports ProtonMail's domain variants (protonmail.com, proton.me, pm.me) and applies consistent normalization. The docblock accurately describes the current behavior.
Note: The PHPMD warning about unused
$domainparameter is a false positive—the parameter is required by the abstractProviderinterface.src/Emails/Canonicals/Providers/Outlook.php (1)
15-53: LGTM! Outlook provider comprehensively covers Microsoft email domains.The implementation correctly handles Outlook, Hotmail, and Live domains including UK variants, and applies consistent normalization. The docblock accurately describes the current behavior.
Note: The PHPMD warning about unused
$domainparameter is a false positive—the parameter is required by the abstractProviderinterface.src/Emails/Canonicals/Providers/Fastmail.php (1)
15-58: LGTM! Fastmail provider implementation is consistent and correct.The implementation properly supports Fastmail's domain variants and applies minimal normalization pending confirmation of Fastmail-specific rules. The docblock accurately describes the current behavior and aligns with the test expectations.
Note: The PHPMD warning about unused
$domainparameter is a false positive—the parameter is required by the abstractProviderinterface.tests/Canonicals/Providers/OutlookTest.php (1)
21-99: LGTM! Test coverage is appropriate for current Outlook provider implementation.The test suite properly exercises the Outlook provider's current behavior:
- Domain support verification for all Outlook variants
- Canonicalization with dot preservation (matches implementation)
- Proper TODO markers for commented-out plus addressing tests
The commented-out test cases are appropriately marked and align with the implementation's TODO comments.
tests/EmailTest.php (1)
22-912: LGTM! Comprehensive test coverage for Email class.The test suite provides excellent coverage across:
- Email parsing and validation (constructor, getters, validation methods)
- Provider classification (free, disposable, corporate)
- Subdomain and provider extraction
- Email formatting and canonicalization
- Edge cases and error handling
Past issues flagged in previous reviews (10minutemail.com, tutanota.com, tempmail.org, constructor error messages) have been addressed.
…ndency management. Refactor Email class to enhance domain validation and initialization of providers, ensuring better error handling and clarity in logic.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/Canonicals/Providers/GenericTest.php (1)
29-70: Track the deferred plus-addressing functionality.The extensive commented-out test cases indicate that plus-addressing, dot-removal, and hyphen-removal behavior is intentionally deferred pending manual confirmation. This is a reasonable approach, but ensure this work is tracked.
Note: The active tests include edge cases like
'user.'and'.user'which are technically invalid email local parts but are currently preserved as-is. Verify this is the intended behavior for the Generic provider.Do you want me to open a tracking issue for implementing and testing the plus-addressing canonicalization behavior across all providers?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/Emails/Email.php(1 hunks)tests/Canonicals/Providers/FastmailTest.php(1 hunks)tests/Canonicals/Providers/GenericTest.php(1 hunks)tests/Canonicals/Providers/GmailTest.php(1 hunks)tests/Canonicals/Providers/IcloudTest.php(1 hunks)tests/Canonicals/Providers/OutlookTest.php(1 hunks)tests/Canonicals/Providers/ProtonmailTest.php(1 hunks)tests/Canonicals/Providers/YahooTest.php(1 hunks)tests/EmailTest.php(1 hunks)tests/Validator/EmailCorporateTest.php(1 hunks)tests/Validator/EmailDomainTest.php(1 hunks)tests/Validator/EmailLocalTest.php(1 hunks)tests/Validator/EmailNotDisposableTest.php(1 hunks)tests/Validator/EmailTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/Canonicals/Providers/OutlookTest.php
- tests/Validator/EmailLocalTest.php
- tests/Validator/EmailNotDisposableTest.php
- tests/Canonicals/Providers/GmailTest.php
- tests/Canonicals/Providers/ProtonmailTest.php
- tests/Canonicals/Providers/YahooTest.php
🧰 Additional context used
🧬 Code graph analysis (8)
tests/EmailTest.php (8)
src/Emails/Email.php (18)
get(128-131)getLocal(136-139)getDomain(144-147)isValid(152-155)hasValidLocal(160-183)hasValidDomain(188-206)isDisposable(211-218)isFree(223-235)isCorporate(240-248)getProvider(253-264)getSubdomain(269-273)hasSubdomain(278-281)getAddress(286-289)getFormatted(381-396)getCanonical(296-302)isCanonicalSupported(307-310)getCanonicalDomain(315-325)src/Emails/Canonicals/Providers/Generic.php (2)
getCanonical(22-44)getCanonicalDomain(46-50)src/Emails/Canonicals/Providers/Fastmail.php (2)
getCanonical(26-47)getCanonicalDomain(49-52)src/Emails/Canonicals/Providers/Gmail.php (2)
getCanonical(26-41)getCanonicalDomain(43-46)src/Emails/Canonicals/Providers/Icloud.php (2)
getCanonical(26-47)getCanonicalDomain(49-52)src/Emails/Canonicals/Providers/Protonmail.php (2)
getCanonical(26-47)getCanonicalDomain(49-52)src/Emails/Canonicals/Providers/Outlook.php (2)
getCanonical(29-42)getCanonicalDomain(44-47)src/Emails/Canonicals/Providers/Yahoo.php (2)
getCanonical(29-53)getCanonicalDomain(55-58)
tests/Canonicals/Providers/IcloudTest.php (1)
src/Emails/Canonicals/Providers/Icloud.php (5)
Icloud(15-58)supports(21-24)getCanonical(26-47)getCanonicalDomain(49-52)getSupportedDomains(54-57)
src/Emails/Email.php (12)
src/Emails/Canonicals/Provider.php (4)
Provider(10-72)getCanonical(24-24)getCanonicalDomain(29-29)supports(15-15)src/Emails/Canonicals/Providers/Fastmail.php (4)
Fastmail(15-58)getCanonical(26-47)getCanonicalDomain(49-52)supports(21-24)src/Emails/Canonicals/Providers/Generic.php (4)
Generic(14-57)getCanonical(22-44)getCanonicalDomain(46-50)supports(16-20)src/Emails/Canonicals/Providers/Gmail.php (4)
Gmail(15-52)getCanonical(26-41)getCanonicalDomain(43-46)supports(21-24)src/Emails/Canonicals/Providers/Icloud.php (4)
Icloud(15-58)getCanonical(26-47)getCanonicalDomain(49-52)supports(21-24)src/Emails/Canonicals/Providers/Outlook.php (4)
Outlook(15-53)getCanonical(29-42)getCanonicalDomain(44-47)supports(24-27)src/Emails/Canonicals/Providers/Protonmail.php (4)
Protonmail(15-58)getCanonical(26-47)getCanonicalDomain(49-52)supports(21-24)src/Emails/Canonicals/Providers/Yahoo.php (4)
Yahoo(15-64)getCanonical(29-53)getCanonicalDomain(55-58)supports(24-27)src/Emails/Validator/EmailCorporate.php (1)
isValid(32-45)src/Emails/Validator/EmailDomain.php (1)
isValid(32-45)src/Emails/Validator/EmailLocal.php (1)
isValid(32-45)src/Emails/Validator/EmailNotDisposable.php (1)
isValid(32-45)
tests/Canonicals/Providers/FastmailTest.php (3)
src/Emails/Canonicals/Providers/Fastmail.php (5)
Fastmail(15-58)supports(21-24)getCanonical(26-47)getCanonicalDomain(49-52)getSupportedDomains(54-57)src/Emails/Canonicals/Provider.php (4)
supports(15-15)getCanonical(24-24)getCanonicalDomain(29-29)getSupportedDomains(34-34)src/Emails/Email.php (2)
getCanonical(296-302)getCanonicalDomain(315-325)
tests/Canonicals/Providers/GenericTest.php (4)
src/Emails/Canonicals/Providers/Generic.php (5)
Generic(14-57)supports(16-20)getCanonical(22-44)getCanonicalDomain(46-50)getSupportedDomains(52-56)src/Emails/Canonicals/Provider.php (4)
supports(15-15)getCanonical(24-24)getCanonicalDomain(29-29)getSupportedDomains(34-34)src/Emails/Email.php (2)
getCanonical(296-302)getCanonicalDomain(315-325)tests/EmailTest.php (1)
test_get_canonical_domain(836-870)
tests/Validator/EmailDomainTest.php (2)
src/Emails/Validator/EmailDomain.php (5)
EmailDomain(13-66)isValid(32-45)getDescription(20-23)getType(62-65)isArray(52-55)src/Emails/Email.php (1)
isValid(152-155)
tests/Validator/EmailCorporateTest.php (2)
src/Emails/Validator/EmailCorporate.php (5)
EmailCorporate(13-66)isValid(32-45)getDescription(20-23)getType(62-65)isArray(52-55)src/Emails/Email.php (1)
isValid(152-155)
tests/Validator/EmailTest.php (2)
src/Emails/Email.php (2)
isValid(152-155)src/Emails/Validator/Email.php (5)
isValid(32-45)getDescription(20-23)getType(62-65)isArray(52-55)
🔇 Additional comments (16)
tests/Canonicals/Providers/GenericTest.php (3)
1-15: LGTM!The test class structure follows PHPUnit best practices with proper typing and initialization in setUp().
17-27: LGTM!The test correctly validates that the Generic provider supports all domains, with good coverage of various TLDs and known provider domains.
72-83: LGTM!Both tests correctly validate that the Generic provider has no canonical domain and returns an empty array for supported domains, aligning with its role as a fallback provider.
tests/Canonicals/Providers/FastmailTest.php (4)
1-15: LGTM!The test class structure follows the same solid pattern as the other provider tests.
17-24: LGTM!The test correctly validates Fastmail's domain support with appropriate positive and negative cases.
26-58: Consistent with the Generic provider tests.This test follows the same pattern of deferring plus-addressing behavior. The active test cases appropriately verify lowercase conversion and dot preservation for Fastmail.
60-70: LGTM!Both tests correctly validate Fastmail's canonical domain and supported domains list.
tests/Canonicals/Providers/IcloudTest.php (4)
1-15: LGTM!The test class follows the established pattern with proper initialization.
17-25: LGTM!The test correctly validates iCloud's domain support including all Apple email domains (icloud.com, me.com, mac.com).
27-63: LGTM!The test follows the consistent pattern and additionally validates domain canonicalization (me.com and mac.com normalize to icloud.com), which is an important feature for the iCloud provider.
65-75: LGTM!Both tests correctly validate iCloud's canonical domain and the complete list of Apple email domains.
tests/Validator/EmailDomainTest.php (1)
1-70: LGTM! Comprehensive test coverage for EmailDomain validator.The test suite provides excellent coverage of valid domains, invalid formats, non-string inputs, and validator metadata. Edge cases like consecutive dots, leading/trailing characters, and filter_var quirks are well-documented.
tests/Validator/EmailCorporateTest.php (1)
1-110: LGTM! Well-structured test suite for EmailCorporate validator.The test suite comprehensively validates corporate domain classification, correctly rejecting free and disposable providers while accepting legitimate corporate domains. The edge cases and metadata checks are thorough.
tests/EmailTest.php (1)
1-900: LGTM! Comprehensive and well-organized test suite.This test file provides excellent coverage of the Email class functionality:
- Basic operations and validation
- Domain classification (free, disposable, corporate)
- Subdomain and provider extraction
- Formatting and normalization
- Exception handling for invalid inputs
- Extensive canonicalization testing for multiple providers
The conservative approach using TODO comments for unverified provider features (plus addressing, dot handling) is prudent and prevents false assumptions about provider behavior.
tests/Validator/EmailTest.php (1)
1-81: LGTM! Clean and focused validator test suite.The test suite correctly validates the Email validator's behavior across valid inputs, invalid formats, non-string inputs, and metadata. The inline comments explaining filter_var quirks are helpful for maintainability.
src/Emails/Email.php (1)
1-397: Implementation is sound; past review comments adequately cover improvement opportunities.The Email class provides a comprehensive email handling API with validation, classification, and canonicalization features. The implementation correctly:
- Normalizes and parses email inputs
- Validates local and domain parts with appropriate edge case handling
- Classifies domains as free, disposable, or corporate with proper prioritization
- Provides provider-aware canonicalization
Past review comments have already identified areas for improvement (error handling for data files, provider initialization duplication, linting). The current implementation is functional and well-tested.
…ata file, ensuring it returns an array before assignment. This improves error handling and robustness in the isDisposable method.
… around the 'is_array' checks for disposable and free domains validation. This change enhances consistency in the code style.
…tan dependency to version 1.10 for improved static analysis capabilities.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores