[do not merge] Change the default directory structure and use new features of 3.3 #1034
Conversation
c9b6ef3 to
62cf17c
Compare
app/config/services.yml
Outdated
| @@ -4,6 +4,5 @@ parameters: | |||
| # parameter_name: value | |||
|
|
|||
| services: | |||
| # service_name: | |||
| # class: AppBundle\Directory\ClassName | |||
| # 'My\ClassName': | |||
There was a problem hiding this comment.
One question: in your tests, how can you replace this service with another one (to use stubs for instance)?
There was a problem hiding this comment.
like any other service: by defining the class name explicitly:
services:
'My\ClassName':
class: 'My\TestClassName'There was a problem hiding this comment.
Yeah, this is what I'm afraid of: you're forced to mix both syntaxes :/
There was a problem hiding this comment.
what both syntaxes ? We don't have 2 syntaxes. We just made class default to the id when it is omitted. The id of your service is just My\ClassName in this case.
You are free to keep using other ids if you prefer btw. Services will not be forced to use a class name as id.
There was a problem hiding this comment.
Services will not be forced to use a class name as id.
Ok, I'm very afraid to be forced to use it, please keep it possible even for Sf4 please :)
There was a problem hiding this comment.
Forcing to use the class name does not make sense as you would then not be able to register more than one service for a class.
There was a problem hiding this comment.
Keep in mind that this new "feature" si optional, don't use it if you don't like it. Anyway, IMHO, if you need to change the class of a service for testing, you are doing something wrong.
There was a problem hiding this comment.
Thanks @fabpot.
Agree with you at some point, it could be useful to set new services in "test" environement when you want a "null" strategy like using in carsonbot for instance (https://github.com/carsonbot/carsonbot/blob/master/app/config/services_test.yml#L7). Not the topic, but if it's wrong I'd like to know why :)
app/config/routing.yml
Outdated
app:
resource: "@AppBundle/Controller/"
action:
resource: "../../src/Controller/"
There was a problem hiding this comment.
This works by chance :) see symfony/symfony#21231
| app: | ||
| resource: "@AppBundle/Controller/" | ||
| action: | ||
| resource: "../../src/Controller/" |
There was a problem hiding this comment.
This works by chance :) see symfony/symfony#21231
|
Now includes updated versions of:
It's ready to test. |
|
I really like the idea of Bundleless symfony applications but with this structure how we should register some features like :
thanks |
if you need a semantic configuration, you will need a bundle (well, technically, you need a DI extension, but adding one without a bundle becomes painful even though it is possible, as non-bundle extensions are called only when they have some config explicitly)
you can override |
|
I personally agree with all the goals of 3.3 (it took me some time to be ok with some of them though, but here we are). (It's just a question, do not see any offence, I do love bundle-less apps, but only after having used bundles for years since these ones given me a structured way to organize my code and, I fairly trust it, helped to make me a better developer) |
|
Hello folks, I love this PR but I also have a question related to my freelance position with lot of legacy-to-(symfony|microkernel|silex) projects. If there is no more AppBundle and the "best practices" encourage us to add our configuration, views, etc in There is (at least) three advantages to do this for DX and/or project :
And for @chalasr because his question is a true one, rename the actual And again, I love this PR :D |
|
The goal of this PR is to experiment with automatic service registration and bundle less apps. However the final target is Symfony Flex (it's why this PR has the |
|
I've updated this PR to use new features of 3.3 instead of ActionBundle:
I've also updated the directory (just a proposal):
Unchanged:
|
|
Nice! I have a question: Why not do |
|
@ruudk It's shorter and maybe more common, but I've nothing against |
|
@dunglas As you are breaking everything, could we broke also: |
|
@dunglas really interesting, this also need a MASSIVE update of docs and probably a new "Best practices" standard or all apps will become a mess ^^ Is this intended to replace the actual Symfony full edition? Or is Symfony flex a new "edition"? |
|
@mickaelandrieu it's just a personal experiment for now, nothing official (it's why the PR title contains [do not merge]. |
|
@lyrixx done |
|
Where does your ressources directory will go with this structure ? (translations / assets / ...) EDIT : Imo it should be a directory inside |
|
According to the new structure (purposed) the configuration path should be (i.e. https://github.com/symfony/symfony-standard/pull/1034/files#diff-aa0dbf36ede817d01f5be6f15d867bb4R65 |
| /** | ||
| * @Route("/", name="homepage") | ||
| */ | ||
| public function __invoke(Request $request): Response |
There was a problem hiding this comment.
Is mandatory to use __invoke() method here? if you are using route annotation, why not use a common method name instead? such as index for example :/
There was a problem hiding this comment.
It's to propose the concept that a controller represents a single action.
Using index() would encourage to create other methods, whereas using __invoke() will encourage you to create one single route bound to this controller.
Why not recommend (again?) the use of xml/yml for routing instead of using annotations? This would seriously decouple encourage decoupling from the SensioFrameworkExtraBundle, and also reinforce the "single-action controller" concept proposed in ADR pattern
There was a problem hiding this comment.
Why do you consider that an annotation is coupling? You can perfectly ignore it and do otherwise if you'd like to. It's plain descriptive only to me. "Use if you want". Thus no coupling at all.
There was a problem hiding this comment.
Using index() would encourage to create other methods, whereas using __invoke() will encourage you to create one single route bound to this controller.
It is confusing to me, Hello is a controller (src/Controller/Hello.php), a controller has actions (in most cases), if the concept is "A controller represents a single action" it mean that I need a controller per action? so the path shouldn't be src/Action/Hello.php then? We need create a php file per action then? :/
EDIT: I know it's optional, but IMHO default concepts should follow common cases.
There was a problem hiding this comment.
For Symfony, a controller is a callback. So in fact in the traditional Symfony structure the controllers are the methods not the classes, they are just badly named.
There was a problem hiding this comment.
@yceruto in HttpKernel, the controller is not the class. It is always the callable.
A controller cannot have actions. A single class can contain multiple controllers. Controllers are always methods in Symfony (unless you go back to symfony 1 where the term controller had a different meaning).
Btw, when using Silex, it is very common to avoid using classes entirely and use closures instead (which cannot be done in the fullstack framework as it does not play well with caching)
There was a problem hiding this comment.
@stof thanks for the explanation, to be clearer, we are defined two possible controllers here (at the same time):
'App\Controller\Hello''App\Controller\Hello::__invoke'
Currently we are using the second controller with the appearance to use the first one (which is confusing), so my suggestion here is to use a better method name if we use already @route() annotation, otherwise let's define the controller into some routing.yml to use the first one.
Now in Symfony it is not possible to declare this type of route as annotation but could be good to have something like this for these cases:
/**
* @Route("/", name="homepage")
*/
class Hello
{
public function __invoke()
{
return new Response('Hello World!');
}
}
Actually equal to:
homepage:
path: /
defaults: { _controller: src\Controller\Hello }
There was a problem hiding this comment.
@yceruto Your suggestion to put the route annotation on the class for invokable controller class is very interesting, and could be the object of an issue/PR in the SensioFrameworkExtraBundle repo 😄
There was a problem hiding this comment.
Pure MVC was never meant to be used on the web, ADR better describes how a web application works.
And for invocable Http-endpoints (which is technically what these are) I prefer Action instead of Controller as it better suites the intention.
There was a problem hiding this comment.
Now my proposal is in progress: [Routing][DX] Add full route definition for invokable controller/class WDYT?
|
I already use quite the same architecture in my projects (as far as the current Symfony release allow me to go). I will be pleased to see Symfony go this way. |
|
@mnapoli IMHO views and config are not resources. Views are almost sources and config, well it's config. It won't bother me to have a translations folder at the same level as |
|
@maximecolin, @mnapoli : IMHO views and configs are considered resources in a PHP project, as soon as it needs to be located and is consumed by your app. |
|
What's also to consider in this change of putting all resources together (and then I'll stop talking about it) is that it's a first step towards universal resource location (which is actually Puli's goal). Everything in |
|
And to add my brick in the wall, a |
|
@Pierstoval Puli is a great tool but we have to be careful. The project is not very active, it is still in beta, it is much more than a Resource Locator and, unless this has changed, Puli is not yet compatible with Twig 2.x. So two options, create a Resource Locator à la Puli or contribute to the project. BTW, I ❤️ Puli and this PR too. |
|
is there any way to get rid of |
|
@jrobeson Good point. My current directory structure for a single app : |
|
@ogizanagi Ok, I see what you mean. I'm just quite not confortable with the idea views are not code sources in the same way php files are. But it make more sens with this definition of resources. |
|
@dunglas what things like typescript, less, javascript etc, things that need pre-processing? Currently I use |
|
I have been using the following structure lately and quite like it: |
|
Whats the etc about? Docker and build-stuff? |
|
@sstok yeah, anything that doesn't really fit in the other dirs |
|
|
||
| public function getRootDir() | ||
| { | ||
| return __DIR__; |
There was a problem hiding this comment.
since templates and config are in the parent directory, why not setting the root dir at the root of the repository ?
There was a problem hiding this comment.
Because it's kernel's root dir, not project root dir
| @@ -1,4 +1,4 @@ | |||
| {% extends 'base.html.twig' %} | |||
| {% extends 'layout.html.twig' %} | |||
There was a problem hiding this comment.
I do prefer "base" over "layout". The layout refers to the style.
There was a problem hiding this comment.
Also, let's not change anything unnecessarily.
|
|
||
| Symfony\Component\Form\FormTypeInterface: | ||
| tags: ['form.type'] | ||
| public: true # Mandatory |
There was a problem hiding this comment.
Can this be removed now that symfony/symfony#21690 is merged and thus form types can be private?
@jrobeson see symfony/symfony#21837 which is working on it. |
|
Just an idea: can't the "action edition" become a new official Symfony edition instead of replacing the Standard Edition? Symfony CMF already makes updates to his full-stack application from time to time, all based on standard edition, and they seem to agree with it (or maybe it's just me who thinks this 😛 ). I mean, it has been a bit discussed that the installer could install different Symfony distributions than Demo and Standard Edition (see symfony/symfony-installer#39 ), but actually, the "action pattern" one could easily be one of the different editions. And it could engage the development process in the installer & the Symfony team on "How to define a Symfony edition", etc. Obviously, more editions = more maintenance, but as the "action" one will start with Symfony 3.3, this is less maintenance than the SE 😆 |
|
@Pierstoval having multiple official editions has a huge impact on the documentation. So I would not make it an official extension today, especially given that this PR is clearly experimental currently (and relies on unmerged Symfony PRs). |
|
I partially agree, especially about the fact that there is an impact on the documentation. But actually, a new Symfony edition is maintained by its maintainers, and so should be its documentation, what do you think? |
|
Symfony Distributions are dead. They won't exist in Symfony 4. We won't need them anymore. This will be replaced with Symfony Flex instead. I really have to blog about that as not everybody was at SymfonyCon in Berlin. Will try to do that ASAP to give more background. |
That's great news! 👍 |
would be great, especially as there is nothing about your keynote available AFAIK: no slides, no video (last time I checked). |
|
Closing this one. Flex is on its way now :) |
|
So you're confident that Flex will resolve all of these issues, and that everyone will embrace it? 😄 |
This PR leverages is part of the 0 config initiative. It leverages the numerous improvements done in 3.3 and ActionBundle:
srcdirectory, noAppBundleanymoreservices.ymlthanks to autowiringControllerTrait)