Routing: don't require to install doctrine/annotations to use attributes #916
Conversation
|
I think @derrabus had a plan about this, let's ask him if there are any missing parts? |
We need to make this PR happen, but this is problem. What exactly would happen (what error) if someone uses PHP 7 on 5.3 after this recipe? Could we only error (and with a clear error) if they started trying to use annotations without doctrine/annotations installed? Thanks! |
|
According to our past policies, I'm 👎 to break the experience for PHP 7 users. This is 6 months too early. Let's wait for Symfony 6 to do this. |
|
I don't know if that's going to be acceptable, but technically, we can achieve something conditional by using the kernel instead here. |
|
The current experience is not acceptable either. You need to install a totally useless library just to generate some config to use a native feature. With this PR, users wanting to use PHP 7 will just have to comment two blocks of configuration if they really want to do so. We're just talking about new projects here. Will users start a new project with Symfony 5.3 (to be released only in May) with a version of PHP that will not be supported in November of the same year? We should make the experience good for the most common setup for new projects (latest PHP version and latest Symfony version), not degrade it for everyone to support legacy configurations. An alternative (less good, but that can allow reaching consensus) could be to just comment the lines added by this PR. So it will work out of the box for PHP 7, and PHP 8 users will be able to discover how to make the app working for them. In my opinion it's less good than making the experience good for PHP 8 users, but it's still better than the current situation (and this could be done even for 5.2). |
|
Most ppl will have to install doctrine/annotations for entities anyway, so I wouldn't call it useless (yet). What about my proposal to use the Kernel instead? We can wait 6 months to do this in Symfony 6 IMHO. Doing this in 5.4 LTS is too "aggressive" IMHO. |
|
Only those using Doctrine ORM, and hopefully attributes support will land in the ORM before the release of Symfony 5.3. Regarding adding code in the kernel, I'm not a fond of "polluting"/making more complex the generated code for supporting things like this. Adding commented configs is less intrusive IMHO. Edit: actually Doctrine ORM 2.9-dev already supports attributes: doctrine/orm#8266 |
|
I just ran: SYMFONY_ENDPOINT=https://flex.symfony.com/r/github.com/symfony/recipes/916 symfony new --version=dev test-annotand here is the output. This is a blocker on my side. 👎 as is.
|
|
Are you using PHP 8? Here is what I get:
To prevent this error message with PHP 7 we can either... comment the config I added in this PR or make PHP 8 mandatory in the Also, I opened a PR improving the error message: symfony/symfony#40586 |
|
I agree with Nicolas that we should not add a recipe that results in an instant KO on PHP 7. An alternative would be to make Another problem I see is that the |
(trying to block any accidental merge, see discussion)
|
+1 regarding the filename, I'll update the PR. Regarding the no-op, I fear that it will be very hard to debug. Isn't a proper error message better than a no-op? |
I share that concern.
I agree, but what would be the proper channel for that error message? The exception breaks |
|
Sure. I don't think that we have many choices here. Either we display an error message for PHP 7 users telling them to upgrade their PHP version or to run I'm in favor of the first solution (because I think that our main target should be PHP 8 users). The second solution is less good but acceptable too IMHO (it's still improvement compared to the current situation). |
…ll Annotations for using attributes/annots (dunglas) This PR was merged into the 5.2 branch. Discussion ---------- [Config][FrameworkBundle] Hint to use PHP 8+ or to install Annotations for using attributes/annots …s to use attributes/annots | Q | A | ------------- | --- | Branch? | 5.2 | Bug fix? | no | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tickets | See symfony/recipes#916 | License | MIT | Doc PR | n/a Improve the error message to hint that you can upgrade to PHP 8 instead of using `doctrine/annotations`. Commits ------- af6f3c2 [Config][FrameworkBundle] Hint to use PHP 8+ or to install Annotations to use attributes/annots
…ll Annotations for using attributes/annots (dunglas) This PR was merged into the 5.2 branch. Discussion ---------- [Config][FrameworkBundle] Hint to use PHP 8+ or to install Annotations for using attributes/annots …s to use attributes/annots | Q | A | ------------- | --- | Branch? | 5.2 | Bug fix? | no | New feature? | no <!-- please update src/**/CHANGELOG.md files --> | Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files --> | Tickets | See symfony/recipes#916 | License | MIT | Doc PR | n/a Improve the error message to hint that you can upgrade to PHP 8 instead of using `doctrine/annotations`. Commits ------- af6f3c2c08 [Config][FrameworkBundle] Hint to use PHP 8+ or to install Annotations to use attributes/annots
|
I would propose to not throw an annotation loader exception when there are no php files in the directory yet. |
|
We now have another related problem: DoctrineBundle supports attributes, but unlike Symfony components, it isn't able to load attributes and annotations at the same time. |
|
@dunglas We can do so safely for 6.0 at least. |
this is a terrible bug that confuse me two hours. OMG I think this bug should be fixed as quickly as possible or marked in the documention since it will make developers suspecting that it is their own fault |
|
Should we change this to a 6.0 recipe and merge now? |
|
Let's do it for 6.0. |
9c4c154 to
c208077
Compare
|
PR updated to target 6.0. |
|
Thanks for the PR 😍 How to test these changes in your application
Diff between recipe versionsIn order to help with the review stage, I'm in charge of computing the diff between the various versions of patched recipes. symfony/routing3.3 vs 4.0diff --git a/symfony/routing/3.3/config/routes.yaml b/symfony/routing/4.0/config/routes.yaml
index 59d1945..c3283aa 100644
--- a/symfony/routing/3.3/config/routes.yaml
+++ b/symfony/routing/4.0/config/routes.yaml
@@ -1,6 +1,3 @@
-# This file is the entry point to configure your own HTTP routes.
-# Files in the routes/ subdirectory configure the routes for your dependencies.
-
#index:
# path: /
-# defaults: { _controller: 'App\Controller\DefaultController::index' }
+# controller: App\Controller\DefaultController::index4.0 vs 4.2diff --git a/symfony/routing/4.2/config/packages/routing.yaml b/symfony/routing/4.2/config/packages/routing.yaml
new file mode 100644
index 0000000..7e97762
--- /dev/null
+++ b/symfony/routing/4.2/config/packages/routing.yaml
@@ -0,0 +1,3 @@
+framework:
+ router:
+ utf8: true4.2 vs 5.1diff --git a/symfony/routing/4.2/config/packages/routing.yaml b/symfony/routing/5.1/config/packages/routing.yaml
index 7e97762..b45c1ce 100644
--- a/symfony/routing/4.2/config/packages/routing.yaml
+++ b/symfony/routing/5.1/config/packages/routing.yaml
@@ -1,3 +1,7 @@
framework:
router:
utf8: true
+
+ # Configure how to generate URLs in non-HTTP contexts, such as CLI commands.
+ # See https://symfony.com/doc/current/routing.html#generating-urls-in-commands
+ #default_uri: http://localhost5.1 vs 5.3diff --git a/symfony/routing/5.1/config/packages/prod/routing.yaml b/symfony/routing/5.1/config/packages/prod/routing.yaml
deleted file mode 100644
index b3e6a0a..0000000
--- a/symfony/routing/5.1/config/packages/prod/routing.yaml
+++ /dev/null
@@ -1,3 +0,0 @@
-framework:
- router:
- strict_requirements: null
diff --git a/symfony/routing/5.1/config/packages/routing.yaml b/symfony/routing/5.3/config/packages/routing.yaml
index b45c1ce..4b766ce 100644
--- a/symfony/routing/5.1/config/packages/routing.yaml
+++ b/symfony/routing/5.3/config/packages/routing.yaml
@@ -5,3 +5,8 @@ framework:
# Configure how to generate URLs in non-HTTP contexts, such as CLI commands.
# See https://symfony.com/doc/current/routing.html#generating-urls-in-commands
#default_uri: http://localhost
+
+when@prod:
+ framework:
+ router:
+ strict_requirements: null5.3 vs 6.0diff --git a/symfony/routing/5.3/config/routes.yaml b/symfony/routing/6.0/config/routes.yaml
index c3283aa..5b102f6 100644
--- a/symfony/routing/5.3/config/routes.yaml
+++ b/symfony/routing/6.0/config/routes.yaml
@@ -1,3 +1,7 @@
-#index:
-# path: /
-# controller: App\Controller\DefaultController::index
+controllers:
+ resource: ../src/Controller/
+ type: annotation
+
+kernel:
+ resource: ../src/Kernel.php
+ type: annotation |
|
Looks like the CI cannot handle 6.0 recipes yet. |
Currently, installing
doctrine/annotationsis needed to be able to use the#[Route()]PHP 8 attribute. It'unfortunate because attributes can work out of the box and this dependency is not needed with PHP 8.This PR enables "annotation" support (actually attribute support) by default, even if
doctrine/annotationsisn't installed.This PR targets Symfony 5.3 as it's a new feature. A side effect will be that the default skeleton will not work by default with PHP 7, but IMHO it's not a big issue because most new projects using Symfony 5.3 should use PHP 8. Also, this moves the ecosystem forward.