Use ADR pattern #457

core23 · 2018-06-16T17:00:35Z

I am targeting this branch, because this is BC.

Changelog

### Changed
- `Controller\PostController` is now deprecated in favor of `Action\*Action`

Todo

add NEXT_MAJOR comments
move to Action namespace
test this on an actual project

Subject

Using invokable CaaSes vs one big controller achieves several things:

it discourages piling on more actions in controllers with vague names (SRP)
it leads to easier to unit test actions and visible deps (DIP)
it will allow us to make more services private

Refs sonata-project/SonataAdminBundle#5120

core23 · 2018-06-16T17:02:57Z

Not sure how to handle getCommentForm. This method is used accross several actions. Should I create an abstract action or should we duplicate this piece of code.

kunicmarko20

Great job, love this change to Actions 👍

kunicmarko20 · 2018-06-16T17:02:23Z

src/Action/CollectionPostArchiveAction.php

+     */
+    private $collectionManager;
+
+    public function __construct(BlogInterface $blog, PostManagerInterface $postManager, CollectionManagerInterface $collectionManager)


This is over 120 chars, can you make it multiline.

kunicmarko20 · 2018-06-16T17:04:10Z

src/Action/ModerateCommentAction.php

+use Symfony\Component\Routing\RouterInterface;
+use Symfony\Component\Security\Core\Exception\AccessDeniedException;
+
+class ModerateCommentAction


kunicmarko20 · 2018-06-16T17:04:33Z

src/Action/MonthlyPostArchiveAction.php

+    public function __invoke(Request $request, $year, $month)
+    {
+        return $this->renderArchive($request, [
+            'date' => $this->getPostManager()->getPublicationDateQueryParts(sprintf('%d-%d-%d', $year, $month, 1), 'month'),


over 120 chars

kunicmarko20 · 2018-06-16T17:05:10Z

src/Action/AbstractPostArchiveAction.php

+            'route_parameters' => $request->get('_route_params'),
+        ], $parameters);
+
+        $response = $this->render(sprintf('@SonataNews/Post/archive.%s.twig', $request->getRequestFormat()), $parameters);


I think this is over 120 chars

kunicmarko20 · 2018-06-16T17:06:32Z

src/Action/ViewPostAction.php

+    /**
+     * @param null|SeoPageInterface $seoPage
+     */
+    public function setSeoPage(SeoPageInterface $seoPage = null)


Is this needed? (It will be replaced with Trait?)

Yes, after merging sonata-project/SonataSeoBundle#274

kunicmarko20 · 2018-06-16T17:07:43Z

src/DependencyInjection/SonataNewsExtension.php

@@ -39,6 +39,7 @@ public function load(array $configs, ContainerBuilder $container)
        $bundles = $container->getParameter('kernel.bundles');

        $loader = new XmlFileLoader($container, new FileLocator(__DIR__.'/../Resources/config'));
+        $loader->load('actions.xml');


everything is singular, this one is plural 😄

We already had this discussion:
sonata-project/SonataAdminBundle#5120 (comment)

Would prefer singular too, but symfony is mainly using plural for this

so, we should rename others then

Feel free to do this

greg0ire · 2018-06-16T17:50:47Z

src/Action/AbstractPostArchiveAction.php

+    private $postManager;
+
+    /**
+     * AbstractPostArchiveAction constructor.


Is this really needed?

greg0ire · 2018-06-16T17:52:57Z

src/Action/AbstractPostArchiveAction.php

+     * AbstractPostArchiveAction constructor.
+     *
+     * @param BlogInterface        $blog
+     * @param PostManagerInterface $postManager


This can be removed too IMO, and I thought you were against removing it but now I see you did not put it on some methods below.

greg0ire · 2018-06-16T17:54:52Z

src/Action/HomeAction.php

+use Symfony\Component\HttpFoundation\RedirectResponse;
+use Symfony\Component\Routing\RouterInterface;
+
+final class HomeAction


I think we could reuse this and get entirely rid of that action: https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Controller/RedirectController.php#L57

greg0ire · 2018-06-16T18:02:41Z

This is so much cleaner! I was going to ask you to split the test , but I realise there is none :P

greg0ire · 2018-06-17T16:38:35Z

src/Action/AbstractPostArchiveAction.php

+            'route_parameters' => $request->get('_route_params'),
+        ], $parameters);
+
+        $response = $this->render(


@covex-nn , apparently you're doing a series of PR to decouple from the templating component, I didn't realise that when I made my PR. Maybe we could entirely skip the extends Controller and use $response->setContent($this->twig->render($view, $parameters)) ? Or is it wrong/to soon? See https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Controller/ControllerTrait.php#L224

Too soon. We still support symfony 2.8: https://github.com/symfony/symfony/blob/2.8/src/Symfony/Bundle/FrameworkBundle/Controller/Controller.php#L184-L190

@greg0ire PR about controller actions and PR about decoupling from templating are not related to each other (my PR was about using templating service inside blocks). So, i think, if you would inject twig into controller action, it will work. But, may be some other actions use other Controller/ControllerTrait features?

Maybe yeah, but there's probably not that many, and if it can spare us from injecting the whole container, then it might be good. I might try it on the new actions I created but technically it will be a BC-break.

core23 · 2018-06-19T06:54:53Z

Anything to do here? @greg0ire @covex-nn @kunicmarko20

greg0ire · 2018-06-19T06:57:24Z

Anything to do here? @greg0ire @covex-nn @kunicmarko20

Experimenting with the Twig injection if you feel like it. Should work, even for 2.8

covex-nn · 2018-06-19T15:07:23Z

src/Controller/PostController.php


-        return $response;
+        return $action->renderArchive($this->resolveRequest($request), $criteria, $parameters);


core23 · 2018-06-19T15:17:02Z

Experimenting with the Twig injection if you feel like it. Should work, even for 2.8

This is hard / impossible in a BC way. So I will do this on the master branch afterwards

greg0ire · 2018-06-19T17:33:56Z

Thanks @core23 !

core23 added minor in progress labels Jun 16, 2018

kunicmarko20 reviewed Jun 16, 2018

View reviewed changes

core23 force-pushed the feature/adr branch from 79ef13a to 524831b Compare June 16, 2018 17:18

greg0ire reviewed Jun 16, 2018

View reviewed changes

core23 force-pushed the feature/adr branch 2 times, most recently from 007f33f to 142d1b9 Compare June 17, 2018 06:04

greg0ire reviewed Jun 17, 2018

View reviewed changes

core23 force-pushed the feature/adr branch from 142d1b9 to f9433a1 Compare June 17, 2018 17:40

core23 removed the in progress label Jun 17, 2018

core23 force-pushed the feature/adr branch from f9433a1 to bc1ec57 Compare June 17, 2018 17:43

Use ADR pattern

f285934

core23 force-pushed the feature/adr branch from bc1ec57 to f285934 Compare June 18, 2018 17:02

greg0ire approved these changes Jun 19, 2018

View reviewed changes

covex-nn reviewed Jun 19, 2018

View reviewed changes

covex-nn approved these changes Jun 19, 2018

View reviewed changes

greg0ire merged commit 05c76d9 into sonata-project:3.x Jun 19, 2018

core23 deleted the feature/adr branch June 20, 2018 05:39

core23 mentioned this pull request Jun 21, 2018

Fixed wrong route parameter #462

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use ADR pattern #457

Use ADR pattern #457

core23 commented Jun 16, 2018 •

edited

Loading

core23 commented Jun 16, 2018

kunicmarko20 left a comment

kunicmarko20 Jun 16, 2018

kunicmarko20 Jun 16, 2018

kunicmarko20 Jun 16, 2018

kunicmarko20 Jun 16, 2018

kunicmarko20 Jun 16, 2018

core23 Jun 16, 2018

kunicmarko20 Jun 16, 2018

core23 Jun 16, 2018

kunicmarko20 Jun 16, 2018

core23 Jun 16, 2018

greg0ire Jun 16, 2018

greg0ire Jun 16, 2018

greg0ire Jun 16, 2018

greg0ire commented Jun 16, 2018

greg0ire Jun 17, 2018 •

edited

Loading

core23 Jun 17, 2018

covex-nn Jun 18, 2018

greg0ire Jun 18, 2018

core23 commented Jun 19, 2018

greg0ire commented Jun 19, 2018

covex-nn Jun 19, 2018 •

edited

Loading

core23 commented Jun 19, 2018

greg0ire commented Jun 19, 2018


		return $response;
		return $action->renderArchive($this->resolveRequest($request), $criteria, $parameters);

Use ADR pattern #457

Use ADR pattern #457

Conversation

core23 commented Jun 16, 2018 • edited Loading

Changelog

Todo

Subject

core23 commented Jun 16, 2018

kunicmarko20 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire commented Jun 16, 2018

greg0ire Jun 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

core23 commented Jun 19, 2018

greg0ire commented Jun 19, 2018

covex-nn Jun 19, 2018 • edited Loading

Choose a reason for hiding this comment

core23 commented Jun 19, 2018

greg0ire commented Jun 19, 2018

core23 commented Jun 16, 2018 •

edited

Loading

greg0ire Jun 17, 2018 •

edited

Loading

covex-nn Jun 19, 2018 •

edited

Loading