Add http.route tag to SymfonyIntegration.php #2992

cataphract · 2024-12-12T02:26:55Z

Description

Adds http.route to symfony. Supersedes #2710 and #2477

Reviewer checklist

Test coverage seems ok.
Appropriate labels assigned.

codecov-commenter · 2024-12-12T02:58:57Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 61.93%. Comparing base (38ce35a) to head (6df661a).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2992      +/-   ##
==========================================
- Coverage   61.96%   61.93%   -0.03%     
==========================================
  Files         140      140              
  Lines       12388    12388              
  Branches     1622     1622              
==========================================
- Hits         7676     7673       -3     
- Misses       3999     4002       +3     
  Partials      713      713

see 2 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 38ce35a...6df661a. Read the comment docs.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

pr-commenter · 2024-12-12T03:00:01Z

Benchmarks [ tracer ]

Benchmark execution time: 2025-08-05 16:10:46

Comparing candidate commit 6df661a in PR branch glopes/symfony-route with baseline commit 38ce35a in branch master.

Found 1 performance improvements and 2 performance regressions! Performance is the same for 175 metrics, 0 unstable metrics.

scenario:PDOBench/benchPDOBaseline

🟥 execution_time [+4.595µs; +5.449µs] or [+7.355%; +8.723%]

scenario:PDOBench/benchPDOBaseline-opcache

🟩 execution_time [-4.522µs; -3.613µs] or [-6.729%; -5.376%]

scenario:SymfonyBench/benchSymfonyOverhead-opcache

🟥 execution_time [+162.887µs; +271.453µs] or [+2.059%; +3.431%]

github-actions · 2024-12-12T14:42:11Z

Snapshots difference summary

The following differences have been observed in committed snapshots. It is meant to help the reviewer.
The diff is simplistic, so please check some files anyway while we improve it.

If you need to update snapshots, please refer to CONTRIBUTING.md

PROFeNoM · 2024-12-17T13:08:40Z

Makefile

@@ -913,7 +913,6 @@ TEST_WEB_81 := \
 	test_web_nette_30 \
 	test_web_slim_312 \
 	test_web_slim_4 \
-	test_web_symfony_52 \


Why are we removing those tests from PHP 8.1 up to PHP 8.3? 🤔

Symfony 5.2 doesn't actually work with those PHP versions. The tests fail with:

[17-Dec-2024 19:40:57 -03] [ddtrace] [warning] Error raised in ddtrace's closure defined at /Users/gustavo.lopes/repos/dd-trace-php/src/DDTrace/Integrations/Symfony/SymfonyIntegration.php:394 for Symfony\Component\HttpKernel\HttpKernel::handle(): Return type of Symfony\Component\Routing\RouteCollection::count() should either be compatible with Countable::count(): int, or the #[\ReturnTypeWillChange] attribute should be used to temporarily suppress the notice in /Users/gustavo.lopes/repos/dd-trace-php/tests/Frameworks/Symfony/Version_5_2/vendor/symfony/routing/RouteCollection.php on line 69 /Users/gustavo.lopes/repos/dd-trace-php/tests/Common/WebFrameworkTestCase.php:74 /Users/gustavo.lopes/repos/dd-trace-php/tests/Common/MultiPHPUnitVersionAdapter_typed.php:33

I can see that version of symfony being executed on the pipeline: https://app.circleci.com/pipelines/github/DataDog/dd-trace-php/19819/workflows/69eaa9d7-067a-40e8-9a0c-5bd35b27e5a1/jobs/6415988
P.S: I didn't check any more of the combinations deleted below

PROFeNoM

FYI, I can see the following warning/error from our CI: failure.

On a similar note, the Symfony 3.0 (:skull:) tests seems not to have the http.route tag: failure

estringana · 2024-12-17T14:20:34Z

I am missing tests with optional parameters https://symfony.com/doc/current/routing.html#optional-parameters, more than one locale https://symfony.com/doc/current/routing.html#localized-routes-i18n

estringana · 2024-12-20T14:39:07Z

src/DDTrace/Integrations/Symfony/SymfonyIntegration.php

+            }
+            $rootSpan->meta['symfony.route.name'] = $route_name;
+
+            // the rest is for determining http.route


I'd rather extract then all the code below this comment to its own function. Comments sooner or later lie. Just my opinion, not blocking the pr or anything

I've refactored it

estringana · 2024-12-20T14:40:42Z

appsec/tests/integration/src/test/www/symfony62/src/Controller/HomeController.php

-     */
-    public function dynamicAction(Request $request)
+    #[Route("/dynamic-path/{param01}", locale: "en")]
+    #[Route("/caminho-dinamico/{param01}", locale: "pt")]


How is the PT route reported if locale is pt. I can't see any test

We don't report the routes and I don't hit this route, only then en one. The point is that there being several locales forces the route name not be a key in the route collection; instead we get route_name.locale.

estringana · 2025-03-17T15:15:13Z

src/DDTrace/Integrations/Symfony/SymfonyIntegration.php

+                } else {
+                    $routeCollection = $router->getRouteCollection();
+                    $route = $routeCollection->get($route_name);
+                    if ($route == null && ($locale = $request->get('_locale')) !== null) {


You have $locale already above

cataphract requested review from a team as code owners December 12, 2024 02:26

cataphract mentioned this pull request Dec 12, 2024

Add http.route tag to SymfonyIntegration.php #2710

Closed

2 tasks

cataphract force-pushed the glopes/symfony-route branch 4 times, most recently from 6a1adeb to b32dca7 Compare December 12, 2024 02:52

cataphract force-pushed the glopes/symfony-route branch 2 times, most recently from 1c181cf to 387a651 Compare December 12, 2024 14:41

cataphract force-pushed the glopes/symfony-route branch 6 times, most recently from 644c737 to ddbe994 Compare December 16, 2024 21:27

PROFeNoM reviewed Dec 17, 2024

View reviewed changes

cataphract force-pushed the glopes/symfony-route branch from ddbe994 to 7233cec Compare December 17, 2024 13:52

cataphract force-pushed the glopes/symfony-route branch from 7233cec to 0ee8ad9 Compare December 18, 2024 15:14

estringana reviewed Dec 20, 2024

View reviewed changes

cataphract force-pushed the glopes/symfony-route branch from ee8946d to 04884ce Compare January 15, 2025 20:20

cataphract force-pushed the glopes/symfony-route branch from 04884ce to 1a756da Compare March 7, 2025 10:30

Julian-Louis and others added 7 commits March 7, 2025 10:30

Add http.route tag to SymfonyIntegration.php

4df216d

symfony http.route: use existing hook instead

79bdeb2

Cache routes

d846fb7

Support localized routes

2d99410

Fix symfony 3.0 test

fd40352

Small refactoring

02b0bbb

Add option to disable http.route in symfony

9dcb398

cataphract force-pushed the glopes/symfony-route branch from 1a756da to 9dcb398 Compare March 7, 2025 10:30

estringana reviewed Mar 17, 2025

View reviewed changes

cataphract added 2 commits April 9, 2025 18:37

Merge remote-tracking branch 'origin/master' into glopes/symfony-route

6c9155a

Eliminate redundant store

ec2d9b2

cataphract force-pushed the glopes/symfony-route branch from ee457d5 to ec2d9b2 Compare April 10, 2025 10:06

cataphract requested a review from estringana April 10, 2025 14:51

estringana approved these changes Jul 28, 2025

View reviewed changes

Merge remote-tracking branch 'origin/master' into glopes/symfony-route

6df661a

cataphract force-pushed the glopes/symfony-route branch from b11f5e6 to 6df661a Compare August 5, 2025 15:35

cataphract merged commit 6ec272b into master Aug 6, 2025
1838 of 1857 checks passed

cataphract deleted the glopes/symfony-route branch August 6, 2025 16:10

github-actions bot added this to the 1.12.0 milestone Aug 6, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add http.route tag to SymfonyIntegration.php #2992

Add http.route tag to SymfonyIntegration.php #2992

Uh oh!

cataphract commented Dec 12, 2024

Uh oh!

codecov-commenter commented Dec 12, 2024 •

edited

Loading

Uh oh!

pr-commenter bot commented Dec 12, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Dec 12, 2024

Uh oh!

PROFeNoM Dec 17, 2024

Uh oh!

cataphract Dec 17, 2024

Uh oh!

estringana Mar 17, 2025

Uh oh!

PROFeNoM left a comment

Uh oh!

estringana commented Dec 17, 2024

Uh oh!

estringana Dec 20, 2024

Uh oh!

cataphract Dec 20, 2024

Uh oh!

estringana Dec 20, 2024

Uh oh!

cataphract Dec 20, 2024

Uh oh!

estringana Mar 17, 2025

Uh oh!

Uh oh!

Uh oh!

Add http.route tag to SymfonyIntegration.php #2992

Add http.route tag to SymfonyIntegration.php #2992

Uh oh!

Conversation

cataphract commented Dec 12, 2024

Description

Reviewer checklist

Uh oh!

codecov-commenter commented Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pr-commenter bot commented Dec 12, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks [ tracer ]

scenario:PDOBench/benchPDOBaseline

scenario:PDOBench/benchPDOBaseline-opcache

scenario:SymfonyBench/benchSymfonyOverhead-opcache

Uh oh!

github-actions bot commented Dec 12, 2024

Snapshots difference summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PROFeNoM left a comment

Choose a reason for hiding this comment

Uh oh!

estringana commented Dec 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented Dec 12, 2024 •

edited

Loading

pr-commenter bot commented Dec 12, 2024 •

edited

Loading