[DX] ADR usage #8153

Guikingone · 2017-07-14T14:15:00Z

With the new approach used by the container, the old controller can be easily replaced by the ADR pattern, as Symfony grow, I think it should be the role of the documentation to show how to use the pattern and how to adapt the framework structure.

PS : I'm not pretty sure of the 'note' syntax.

With the new approach used by the container, the old controller can be replaced easily by the ADR pattern, as Symfony grow, I think it should be the role of the documentation to show how to use the pattern and how to adapt the framework structure. PS : I'm not pretty sure of the 'note' syntax.

Remove the PHP 7.1 syntax.

Removed the ..note block.

yceruto

This one documents part of symfony/symfony#21723 so we may add a note for this feature @Route("/hello", name="hello") explaining when this only work? and close #7549 then ;) Thanks!

yceruto · 2017-07-14T14:36:22Z

controller/service.rst

+     */
+    final class HelloAction
+    {
+        /** @var EngineInterface */


docblock can be removed

yceruto · 2017-07-14T14:37:28Z

controller/service.rst

+         * HelloAction constructor.
+         *
+         * @param EngineInterface $renderEngine
+         */


docblock can be removed

yceruto · 2017-07-14T14:37:42Z

controller/service.rst

+         * @param EngineInterface $renderEngine
+         */
+        public function __construct(
+            EngineInterface $renderEngine


should be one line?

Maybe just engine? this do a bit more than only render ;)

yceruto · 2017-07-14T14:38:12Z

controller/service.rst

+         * @throws \RuntimeException
+         *
+         * @return Response
+         */


docblock can be removed

yceruto · 2017-07-14T14:41:09Z

controller/service.rst

+in order to work, you must update the services.yml file ::
+
+    parameters:
+        #parameter_name: value


can be removed parameters section?

yceruto · 2017-07-14T14:41:31Z

controller/service.rst

+
+    /**
+     * Class HelloAction
+     */


can be removed

yceruto · 2017-07-14T14:43:49Z

controller/service.rst

+            methods: ['GET']
+            defaults: { _controller: AppBundle\Action\HelloAction }
+
+    .. code-block:: php-annotations


php annotations should be the first (as recommended way)?

http://symfony.com/doc/current/contributing/documentation/standards.html#formats:

Routing: Annotations, YAML, XML, PHP

yceruto

A few more... ;)

yceruto · 2017-07-14T15:37:38Z

controller/service.rst

+        # app/config/routing.yml
+        hello:
+            path:     /hello
+            methods: ['GET']


This requirement has not been configured in the rest of formats so I guess can be removed?

yceruto · 2017-07-14T15:38:44Z

controller/service.rst

+        {
+            return new Response(
+                $this->renderEngine->render('default/index.html.twig')
+            );


should be in one line

yceruto · 2017-07-14T15:41:32Z

controller/service.rst

+         * @param EngineInterface $renderEngine
+         */
+        public function __construct(
+            EngineInterface $renderEngine


Maybe just engine? this do a bit more than only render ;)

Guikingone · 2017-07-14T16:56:31Z

@yceruto For the docblocks, yes, for sure, I've updated the commits.

For the code, I'm not in the same position, if you look at the PSR (especially the 2 http://www.php-fig.org/psr/psr-2/#overview), they say clearly that a line should be under 80:

There MUST NOT be a hard limit on line length; the soft limit MUST be 120 characters; lines SHOULD be 80 characters or less.

If you pass the __construct in one line, yes, that works but for the Response return, that's over 85 characters long, way too much in my opinion.

For the config files, that's a personal opinion but the requirements key MUST be mandatory and in fact, Insight asks for this key if she's not defined, in my opinion, the documentation should show the right way of using the files and classes.

For the parameters key, I think she can stay for simple visibility and easier understanding of the whole file but in the "simplest" way of configuring the service, yes, she can be removed.

For the PR, I'm not sure of understanding your vision :/

yceruto · 2017-07-14T16:59:03Z

controller/service.rst

+        public function __construct(
+            EngineInterface $engine
+        ) {
+            $this->renderEngine = $render;


$this->engine = $engine ? and fix related use

yceruto · 2017-07-14T16:59:59Z

controller/service.rst

+
+        public function __construct(
+            EngineInterface $engine
+        ) {


public function __construct(EngineInterface $engine) one line?

Guikingone · 2017-07-14T17:05:13Z

@yceruto Should be better.

yceruto · 2017-07-14T17:37:59Z

Please, I'm only address you to Coding Standards for Symfony project (i.e these aren't my personal preferences).

For the code, I'm not in the same position, if you look at the PSR (especially the 2 http://www.php-fig.org/psr/psr-2/#overview), they say clearly that a line should be under 80

Following http://symfony.com/doc/current/contributing/code/standards.html#structure:

Declare all the arguments on the same line as the method/function name, no matter how many arguments there are;

That's applicable to:

Guikingone · 2017-07-14T17:42:23Z

Should be better, I understand your vision, I continue to use the PSR rules into my projects, that's why I recommend the multi-lines approach :).

For the annotations panels order, I've just followed the order used in the first part of the document, no more :)

Change the order of the routing files and deleted the methods key in yaml routing.

Guikingone · 2017-07-15T06:20:35Z

@yceruto Should be better, I've removed the methods key and change the order of the recommended routing configuration.

For the parameters key, I think about using '//' in order add a reminder that some keys are missing from the example but that's just for the example, what do you think of this approach?

// ...

    services:
         _defaults:
             autowire: true
             autoconfigure: true
             public: false
             
         # Allow to load every actions
         AppBundle\Action\:
             resource: '../../src/AppBundle/Action/'
             public: true

Guikingone · 2017-07-17T06:24:29Z

@yceruto Services.yml corrected, I've use the # ... syntax, Yaml doesn't support // ..., I've thought about this after my comments :(

EDIT : Corrected the indentation bug.

Guikingone · 2017-07-22T07:32:41Z

@yceruto Hi, small question about this PR, should I update the usage after the release of Flex (in Alpha mode, I know but that's a major release) via the blog ?

Flex change some configuration added via this PR and it's probably a good idea to show via the doc how to inject the ADR logic using Flex.

Probably @javiereguiluz can help us to decide ?

HeahDude

Thank you for proposing this! Not sure this is the best place to do it though.

HeahDude · 2017-07-29T14:31:39Z

controller/service.rst

@@ -80,6 +80,98 @@ If your controller implements the ``__invoke()`` method - popular with the
 Action-Domain-Response (ADR) pattern, you can simply refer to the service id
 (``AppBundle\Controller\HelloController`` or ``app.hello_controller`` for example).

+If you want to use the ADR pattern rather than the default controller approach, you can use


I think this deserves its own article to be referenced here.

HeahDude · 2017-07-29T14:33:00Z

controller/service.rst

+            resource: '../../src/AppBundle/Action/'
+            public: true
+
+Once the file is updated, delete your Controller folder and create an Action one then 


"Once the file is updated, delete your Controller folder and create an Action class using the ADR principles, i.e ::"

HeahDude · 2017-07-29T14:33:47Z

controller/service.rst

+    namespace AppBundle\Action;
+
+    use Symfony\Component\HttpFoundation\Response;
+    use Symfony\Component\Templating\EngineInterface;


This component is to be deprecated, a Twig Environment should be used instead.

HeahDude · 2017-07-29T14:34:13Z

controller/service.rst

+
+    final class HelloAction
+    {
+        private $render;


Should be named $twig or $renderer.

HeahDude · 2017-07-29T14:35:48Z

controller/service.rst

+
+        // app/config/routing.php
+        $collection->add('hello', new Route('/hello', array(
+            '_controller' => 'AppBundle\Action\HelloAction',


HelloAction::class

Guikingone · 2017-07-30T09:36:41Z

@HeahDude Hi, thanks for the review, in fact, after reading your messages, I was thinking about a different approach, we all know that the doc should reflect the current usage of the framework, in this approach, I was thinking of proposing a new chapter in the "Guides" part, I don't have a specific name in mind but something like "Logic" or "Pattern" could be useful, in this chapter, I was thinking about linking to specific chapter about ADR, API or even MVC (using the Sf approach and actual code), we can build a specific API chapter like @javiereguiluz was talking about in the dedicated Slack then adding the ADR chapter, this way, we can allow the visitor to find and easily see how the pattern/logic can be adapted into Sf via example and dedicated tips, any thought about this ?

For the corrections, I correct all your recommendations as soon as I'm back on my computer.

Updating the classes call and prepare for dedicated article.

Guikingone · 2017-08-03T07:32:01Z

@HeahDude Just corrected the classes call and prepare for the dedicated article, gonna clone the repo on my computer and add the new file as soon as possible (probably before Sunday).

Guikingone · 2017-08-03T15:46:08Z

@HeahDude Hi, I've just added the dedicated file and the link from service.rst.

controller/adr.rst

yceruto · 2017-08-03T16:03:04Z

controller/adr.rst

+        }
+    }
+
+Final though


Guikingone · 2017-09-12T07:43:25Z

Updated the Request call then the Twig render() call.

yceruto · 2017-09-12T13:04:49Z

controller/adr.rst

+        public function __construct(Environment $twig)
+        {
+            $this->twig = $twig;
+        }


Thinking again about this approach, if we have just one method/controller, shouldn't be simpler inject the required dependencies into __invoke() directly? Thus, we are avoiding define one property per service argument, if so the action service needs to be tagged with controller.service_arguments.

After

final class HelloAction { public function __invoke(Environment $twig): Response { return new Response($twig->render('default/index.html.twig')); } }

Guikingone · 2017-09-13T13:00:34Z

@yceruto It can work but this make appear the "controller" word inside a "pattern" who don't use it, for me, it can be strange for newcomer (and even for experimented developer) to use the word controller when your class became an Action.

This says, @dunglas can probably say more than me about this pattern.

Corrected the service injection and services configuration.

Guikingone · 2017-10-16T12:33:26Z

@yceruto Just made the corrections on service injection via the tags, sorry for the delay.

yceruto · 2017-10-16T12:52:00Z

controller/adr.rst

+        AppBundle\Action\:
+            resource: '../../src/AppBundle/Action/'
+            tags: ['controller.service_arguments']
+            public: true


This behavior has been modified for 3.3, now all the services tagged with controller.service_arguments are public by definition and according to #8415 we can remove it so.

yceruto · 2017-10-16T12:53:07Z

controller/adr.rst

+As the framework evolve, you must update the services.yml file in order to
+use the latest features of the DependencyInjection component, this way, here's the updates::
+
+    # ...


can be removed?

Guikingone · 2017-10-16T13:01:32Z

@yceruto Should be better.

Guikingone · 2017-10-21T13:06:56Z

@HeahDude Your requested changes stay blocking while they were accepted and corrected, bug?

Added a Responder call and the configuration linked.

Guikingone · 2017-11-24T13:45:29Z

Just added a new block for Responder injection and configuration.

Fix the yaml call and the extra line in the responder configuration block.

dunglas · 2017-11-24T14:42:27Z

controller/adr.rst

+
+In Symfony, you're used to implement the MVC pattern and extending the default :class:`Symfony\\Bundle\\FrameworkBundle\\Controller\\Controller`
+class.
+Since the 3.3 update, Symfony is capable of using natively the ADR approach.


Maybe it is worth it to add a link to https://github.com/pmjones/adr

Just added the link to the official ADR repository.

Guikingone · 2017-11-24T15:10:48Z

@dunglas Should be better.

fabpot · 2018-01-24T15:00:57Z

controller/adr.rst

+How to implement the ADR pattern
+================================
+
+In Symfony, you're used to implement the MVC pattern and extending the default :class:`Symfony\\Bundle\\FrameworkBundle\\Controller\\Controller`


Symfony has never promoted the MVC pattern (at least, not since version 2). Instead, we are presenting Symfony as being a Request/Response framework where the controller converts a Request to a Response via a Controller.

Yes, my apologies for this words, the meaning of my "text" was to clearly say that a lot of developers use Symfony with MVC and that using this pattern is way more used than just transforming a Request into Response (which is handled by the framework in a certain way, the actual transformation occurs in the controller).

fabpot · 2018-01-24T15:01:14Z

controller/adr.rst

+            tags: 
+                - 'controller.service_arguments'
+
+Now that the container knows about our actions, time to build a simple Action !


extra whitespace before the !

fabpot · 2018-01-24T15:01:26Z

controller/adr.rst

+Updating your classes
+---------------------
+
+As the framework evolve, you must update your classes, first, delete your Controller folder and create an Actions one then a new class using the ADR principles, for this example, call it ``HelloAction.php``: 


typo: evolves

fabpot · 2018-01-24T15:04:46Z

controller/adr.rst

+Keep in mind that this approach can be completely different from what you're used to use, in order to
+keep your code clean and easy to maintain, we recommend to use this approach only if your code is
+decoupled from the internal framework logic (like with Clean Architecture approach) or if you start a new
+project and need to keep the logic linked to your business rules.


I don't understand this last paragraph and the supposed benefits. I don't see how this approach or what you refer to the old one is cleaner or easier. I don't understand why this approach is more decoupled than the other one. I'm not saying this is good or bad, just that from a beginner's perspective, the benefits are not very clear.

fabpot · 2018-01-24T15:05:13Z

controller/service.rst

@@ -80,6 +80,11 @@ If your controller implements the ``__invoke()`` method - popular with the
 Action-Domain-Response (ADR) pattern, you can simply refer to the service id
 (``AppBundle\Controller\HelloController`` or ``app.hello_controller`` for example).

+As this approach is evolving faster and can be easily transposed into Symfony, we recommend


How is it evolving?

At this stage, not so much, like before, there's was an error in the real sense of my words, ADR still young and the "logic" keep evolving with time and usage by the developers, the true meaning was to say that using ADR still a "WIP" in Symfony and as the "logic" of ADR still evolving by the feedbacks of the developers, his implementation inside Symfony can be modified with future evolution of the framework or pattern.

In fact, it was possible since Symfony 3.1 when recompiling the container (or like @dunglas do it, by injecting the classes as controllers and mapping the return of each one of them) but using it in production is way easier since 3.3 and 3.4 (and even easier in 4.0)

This PR was merged into the 4.0 branch. Discussion ---------- Controller naming habits As Symfony isn't an `MVC` framework but a `Request/Response` one (see #8153 (comment)), the name of the class which handles the Request isn't forced to contain `Controller`, it's probably more a common usage than a need to make Symfony works. Commits ------- 5b29f5b Minor reword 2b61e3b suggest(Controller): suggestion about naming habits

javiereguiluz · 2019-09-27T13:47:47Z

What should we do with this proposal? I don't see people talking or promoting ADR on Symfony Slack or blogs or web sites or aggregators like Hacker News. Should we close without merging?

Guikingone · 2019-09-27T14:04:55Z

For me, as ADR is only an evolution of MVC (which is mainly used in today's applications), I think that we can close this PR, it's not the role of Symfony to promote this type of architecture as everyone can have a different point of view when it comes to how to implement it 🙂

fabpot · 2019-09-27T14:08:48Z

I agree that this is opinionated and should not be part of the official docs.

javiereguiluz · 2019-09-27T14:14:58Z

OK, let's close then. Guillaume thanks for being so understanding. Thanks.

carsonbot added the Status: Needs Review label Jul 14, 2017

Guikingone added 2 commits July 14, 2017 16:19

Update service.rst

0d0d4cc

Remove the PHP 7.1 syntax.

Update service.rst

7359837

Removed the ..note block.

yceruto suggested changes Jul 14, 2017

View reviewed changes

Update service.rst

885e696

yceruto suggested changes Jul 14, 2017

View reviewed changes

Guikingone added 2 commits July 14, 2017 19:04

Update service.rst

220021e

Update service.rst

1fda263

Update service.rst

11c77c7

Update service.rst

d21feb2

Change the order of the routing files and deleted the methods key in yaml routing.

Update service.rst

a449220

Update service.rst

6d1ed62

HeahDude reviewed Jul 29, 2017

View reviewed changes

HeahDude added this to the 3.3 milestone Jul 29, 2017

Update service.rst

a81b625

Updating the classes call and prepare for dedicated article.

Added the dedicated file and link from service.rst

8444e98

yceruto reviewed Aug 3, 2017

View reviewed changes

controller/adr.rst Outdated Show resolved Hide resolved

yceruto reviewed Aug 3, 2017

View reviewed changes

controller/adr.rst Outdated

}

}

Final though

Copy link

Member

yceruto Aug 3, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thought?

yceruto reviewed Sep 12, 2017

View reviewed changes

Update adr.rst

dc6ddc6

Corrected the service injection and services configuration.

yceruto suggested changes Oct 16, 2017

View reviewed changes

Guikingone added 2 commits October 16, 2017 14:59

Update adr.rst

f1aad20

Update adr.rst

16c8472

[ADD](Responder classe)[!P2]

eca9a0a

Added a Responder call and the configuration linked.

[FIX](Syntax)[!P2]

aa0a207

Fix the yaml call and the extra line in the responder configuration block.

dunglas reviewed Nov 24, 2017

View reviewed changes

[FIX](ADR Repository)[!P2]

551aed6

Just added the link to the official ADR repository.

javiereguiluz mentioned this pull request Jan 24, 2018

[RFC] Add a make:action command symfony/maker-bundle#58

Closed

fabpot requested changes Jan 24, 2018

View reviewed changes

fix(typo): fix on typo and space.

3dacc68

Guikingone mentioned this pull request Feb 16, 2018

Controller naming habits #9282

Merged

Guikingone changed the title ~~Adding the ADR approach.~~ [DX] ADR usage Feb 11, 2019

wouterj modified the milestones: 3.3, 3.4 Apr 14, 2019

javiereguiluz closed this Sep 27, 2019

fritzmg mentioned this pull request Jan 16, 2020

Fix back end route controller example contao/docs#225

Merged

Uh oh!

[DX] ADR usage #8153

[DX] ADR usage #8153

Uh oh!

Conversation

Guikingone commented Jul 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yceruto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

yceruto Jul 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yceruto left a comment

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!

Guikingone commented Jul 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Guikingone commented Jul 14, 2017

Uh oh!

yceruto commented Jul 14, 2017

Uh oh!

Guikingone commented Jul 14, 2017

Uh oh!

Guikingone commented Jul 15, 2017

Uh oh!

Guikingone commented Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Guikingone commented Jul 22, 2017

Uh oh!

HeahDude left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Guikingone commented Jul 30, 2017

Uh oh!

Guikingone commented Aug 3, 2017

Uh oh!

Guikingone commented Aug 3, 2017

Uh oh!

Uh oh!

Guikingone commented Jul 14, 2017 •

edited

Loading

yceruto left a comment •

edited

Loading

yceruto Jul 14, 2017 •

edited

Loading

Guikingone commented Jul 17, 2017 •

edited

Loading

yceruto Sep 12, 2017 •

edited

Loading