Use 3.3 DI features in CodeExplorerBundle #576
Conversation
| tags: | ||
| - { name: twig.extension } | ||
| class: CodeExplorerBundle\Twig\SourceCodeExtension | ||
| tags: [twig.extension] |
There was a problem hiding this comment.
use autoconfiguration to simplify the file too. almost everything can be removed then
There was a problem hiding this comment.
Sure, if there is a consensus on this, I'd be pleased to add autoconfigure and classnamed services here, as explained in the description (I just want to gather opinions here first) :)
There was a problem hiding this comment.
Well, AppBundle was migrated to use all new features. I think CodeExplorerBundle should be updated too (and the listener should become a subscriber to avoid tagging manually)
There was a problem hiding this comment.
At first I thought as Maxime: let's show the new Symfony in AppBundle and the old Symfony in CodeExplorerBundle. But now I'm starting to think as Christophe. If this is the new and recommended Symfony ... why show the old Symfony too?
Besides, before November we'll delete the CodeExplorerBundle (and Appbundle too!) because we'll use Symfony 4 and there are no bundles in that version!
ogizanagi
changed the title
|
Following #576 (review), I've updated the PR to use most of the new 3.3 DI features. But I still avoided to use PSR-4 autoloading & autowiring on the contrary of what we did for the AppBundle.
Because we could consider this bundle as a more advanced entrypoint for intermediate developers, while the AppBundle is the main show case for newcomers. I still think it's important to show how to wire things manually and demonstrate the Symfony DI & its configuration capabilities. Developers can learn a lot by going deeper on how things work behind the scene (actually, it's not even that far, its only basic configuration we wrote since years. Still, it feels important to me). But if my concerns aren't shared, I can still update the PR to use PSR-4 autoloading & autowiring on the whole |
| * examples see https://symfony.com/doc/current/cookbook/service_container/event_listener.html | ||
| * Tip: listeners and subscribers are common in Symfony applications, but this particular | ||
| * one is too advanced and too specific for the demo application needs. | ||
| * For more common examples see https://symfony.com/doc/current/event_dispatcher.html |
There was a problem hiding this comment.
I would remove this extended doc. We already have it on the subscribers in the AppBundle. People digging into the internals of CodeExplorerBundle should already know what a subscriber is. And it will avoid having to maintain this message multiple time.
|
Thanks @ogizanagi. |
This PR was merged into the master branch. Discussion ---------- Use 3.3 DI features in CodeExplorerBundle Since 3.3, [listeners do not have to be public anymore](https://github.com/symfony/symfony/pull/20953/files#diff-8604f72b4ae78c6c364c1af8c1dee17cL63). I also believe declaring services private by default by adding `_defaults: { public: false }` on top will become a best practice, so I think we should use it in the CodeExplorerBundle. Actually, we could also use classnamed services, autoconfigure and autowiring, but at least for the last one, I think we should keep a place in this application where services are declared entirely manually (autoconfigure is debatable this way too, that's true). WDYT? Commits ------- c391498 Use 3.3 DI features in CodeExplorerBundle
Since 3.3, listeners do not have to be public anymore.
I also believe declaring services private by default by adding
_defaults: { public: false }on top will become a best practice, so I think we should use it in the CodeExplorerBundle.Actually, we could also use classnamed services, autoconfigure and autowiring, but at least for the last one, I think we should keep a place in this application where services are declared entirely manually (autoconfigure is debatable this way too, that's true).
WDYT?