Skip to content

[IT9PE1-32062] Keep accepting request URLs without a TLD - #45

Merged
fguimier merged 2 commits into
mainfrom
florian_IT9PE1-32062_url-require-tld
Sep 29, 2026
Merged

fguimier merged 2 commits into
mainfrom
florian_IT9PE1-32062_url-require-tld

Conversation

@fguimier

@fguimier fguimier commented Sep 29, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Refs IT9PE1-32062

  • Pass requireTld: false explicitly on the Url constraint of Log::$requestUrl. Omitting it is deprecated since Symfony 7.1, and Symfony 8 flips the default to true: request URLs without a TLD (http://localhost/...) would then fail validation.
  • false is the current default, so nothing changes on Symfony 7. On Symfony 8 it keeps today's behaviour.
  • Adds conflict: symfony/validator <7.4: validator is only a dev requirement and symfony/framework-bundle 7.4 still accepts validator 6.4, so a consumer on validator 7.0 would otherwise hit an unknown named argument error.

Test plan

  • Fully covered by automated tests: LogTest::testRequestUrlWithoutTldIsValid validates a localhost URL. Without the fix it fails on the highest dependency set (symfony/validator 8.1.8, checked locally).

Shipping

Merging this PR ships nothing: it needs a semver tag / GitHub release, then a
composer require assoconnect/log-bundle:^x.y bump PR in the backend. Two reviews total. A minor release (v2.7.0) covers #44 (merged) and this PR.

🤖 Generated with Claude Code

@fguimier
fguimier force-pushed the florian_IT9PE1-32062_url-require-tld branch 2 times, most recently from 487e21b to 17bc936 Compare September 29, 2026 19:00
@sonarqubecloud

Copy link
Copy Markdown

❌ The last analysis has failed.

See analysis details on SonarQube Cloud

@sonarqubecloud

Copy link
Copy Markdown

@fguimier

Copy link
Copy Markdown
Contributor Author

🤖 Review gate — 0 error / 0 warning / 2 info

Human review needed: yes — stacked on #44, so it cannot be merged before it. On its own the agent found nothing to flag: requireTld: false keeps the current 7.x validation of Log::$requestUrl, LogTest::testRequestUrlWithoutTldIsValid fails on symfony/validator 8.x if the fix is reverted, and the symfony/validator <7.4 conflict protects consumers who install validator separately.

🧪 Coverage: 1/1 behavior changes covered — discriminates on the highest job only (validator 8.x); on lowest (7.4) the default is already false
🎯 Hotspot thresholds: none (fewer than 100 files touched in 12 months)

⚠️ Self-review by the author is mandatory before merging, regardless of this verdict.

Florian Guimier and others added 2 commits September 29, 2026 21:12
Symfony 7.1 deprecates omitting the requireTld option of the Url
constraint, and Symfony 8 flips its default to true. Log::$requestUrl
would then reject URLs such as http://localhost/..., so the current
behaviour is now spelled out.

Refs IT9PE1-32062

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
symfony/validator is only a dev requirement, and symfony/framework-bundle
7.4 still accepts validator 6.4. A consumer on validator 7.0 would get an
unknown named argument error from #[Assert\Url(requireTld: false)].

Refs IT9PE1-32062

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@fguimier
fguimier force-pushed the florian_IT9PE1-32062_url-require-tld branch from bf9fd15 to 2813b23 Compare September 29, 2026 19:14
@fguimier

Copy link
Copy Markdown
Contributor Author

🤖 Review gate — re-run after #44 was merged, supersedes the previous verdict — 0 error / 0 warning / 2 info

Human review needed: no — no longer stacked: rebased on main, the diff is exactly the two commits already reviewed (requireTld: false on Log::$requestUrl, and the symfony/validator <7.4 conflict). The previous verdict was forced to human review only because of the stack. Can be merged without a peer review.

🧪 Coverage: 1/1 behavior changes covered — LogTest::testRequestUrlWithoutTldIsValid discriminates on the highest job (validator 8.x)
🎯 Hotspot thresholds: none (fewer than 100 files touched in 12 months)

⚠️ Self-review by the author is mandatory before merging, regardless of this verdict.

@sonarqubecloud

Copy link
Copy Markdown

@fguimier
fguimier merged commit ccfc528 into main Sep 29, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant