Make CoreController actions CaaSes #5120

greg0ire · 2018-06-09T22:03:35Z

I am targeting this branch, because this is BC.

Changelog

### Changed
- `Controller\CoreController` is now deprecated in favor of `Action\{Search,Dasbhoard}Action`

Todo

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

kunicmarko20 · 2018-06-10T06:10:25Z

src/Controller/SearchAction.php

+     */
+    public function __invoke(Request $request)
+    {
+        if ($request->get('admin') && $request->isXmlHttpRequest()) {


What about inversing if and putting the default return instead of this.

I just did this, good call :)

kunicmarko20 · 2018-06-10T06:11:32Z

src/DependencyInjection/Compiler/AddDependencyCallsCompilerPass.php

@@ -378,7 +378,7 @@ public function fixTemplates(

            // set template for simple pager if it is not already overwritten
            if ('setPagerType' === $method[0]
-                && $method[1][0] === Pager::TYPE_SIMPLE
+                && Pager::TYPE_SIMPLE === $method[1][0]


Is this in scope of this PR?

The build will not pass otherwise.

kunicmarko20

Great idea, I love it!

core23 · 2018-06-10T06:33:09Z

src/Controller/DashboardAction.php

+use Symfony\Bundle\FrameworkBundle\Controller\Controller;
+use Symfony\Component\HttpFoundation\Request;
+
+final class DashboardAction extends Controller


Why do you extend the symfony Controller?

For one thing and one thing only: the render method. I looked at it, it is fairly complicated because it looks for the templating component, and for twig. Not sure if I could safely get rid of it.

core23 · 2018-06-10T06:34:30Z

src/Controller/DashboardAction.php

+ * file that was distributed with this source code.
+ */
+
+namespace Sonata\AdminBundle\Controller;


Not a blocker, but I would prefer the ..\Action namespace for actions.

Then maybe also change the name of service file to actions?

greg0ire · 2018-06-10T09:13:19Z

@core23 why did you relabel it to minor?

I'm going to make another PR for cs.

greg0ire · 2018-06-10T09:16:12Z

@core23 why did you relabel it to minor?

Ah I know, that's because of the deprecations, forgot that rule 👍

core23 · 2018-06-10T13:05:24Z

Ah I know, that's because of the deprecations, forgot that rule 👍

That's one reason. You also introduce new stuff (the action classes), so it's a minor.

greg0ire · 2018-06-10T13:40:27Z

I introduce new classes, but I introduce no new features I think :P

greg0ire · 2018-06-10T21:28:24Z

Made Codacy happy :)

core23 · 2018-06-11T04:05:11Z

src/DependencyInjection/SonataAdminExtension.php

@@ -141,6 +141,7 @@ public function load(array $configs, ContainerBuilder $container)
        $loader->load('block.xml');
        $loader->load('menu.xml');
        $loader->load('commands.xml');
+        $loader->load('actions.xml');


Why use singular? We use commands and services

What about block and menu?

Maybe those are faulty, or maybe there is an unspoken rule about when to use singular and when to use plural, all I know is that in Symfony, you have services.yaml. Either way, fixing this is out of scope of this PR.

Symfony is refering to a services.yaml:
https://symfony.com/doc/current/service_container.html#service-container-services-load-example

So we should use plurals 👍

greg0ire · 2018-06-11T18:18:22Z

Finally managed to test it on a very old project, it doesn't work :P

greg0ire · 2018-06-11T19:33:21Z

Ready for review :)

greg0ire · 2018-06-13T13:38:24Z

greg0ire requested a review (and also, a merge would be nice) from sonata-project/contributors 2 minutes ago

covex-nn · 2018-06-13T13:46:15Z

Thank you, @greg0ire =)

greg0ire · 2018-06-13T14:28:12Z

@greg0ire is happy :)

greg0ire added the pedantic label Jun 9, 2018

greg0ire force-pushed the invokable_caas branch from 9c7fa4e to 1d064fb Compare June 9, 2018 22:05

kunicmarko20 reviewed Jun 10, 2018

View reviewed changes

core23 reviewed Jun 10, 2018

View reviewed changes

core23 added minor and removed pedantic labels Jun 10, 2018

greg0ire force-pushed the invokable_caas branch 8 times, most recently from cfe7999 to 09bc8db Compare June 10, 2018 12:34

greg0ire changed the title ~~WIP: Make CoreController actions CaaSes~~ Make CoreController actions CaaSes Jun 10, 2018

greg0ire force-pushed the invokable_caas branch from 09bc8db to 13f7fbe Compare June 10, 2018 21:22

core23 reviewed Jun 11, 2018

View reviewed changes

core23 previously approved these changes Jun 11, 2018

View reviewed changes

greg0ire dismissed core23’s stale review via 1890f0e June 11, 2018 18:41

greg0ire force-pushed the invokable_caas branch 2 times, most recently from 1890f0e to 71cfddd Compare June 11, 2018 18:44

greg0ire force-pushed the invokable_caas branch from 71cfddd to df1dd6d Compare June 11, 2018 18:45

Make CoreController actions CaaSes

3be4685

greg0ire force-pushed the invokable_caas branch from df1dd6d to 3be4685 Compare June 11, 2018 18:48

greg0ire requested a review from a team June 11, 2018 19:33

OskarStark approved these changes Jun 12, 2018

View reviewed changes

greg0ire requested a review from a team June 13, 2018 06:08

core23 approved these changes Jun 13, 2018

View reviewed changes

greg0ire requested a review from a team June 13, 2018 12:03

covex-nn approved these changes Jun 13, 2018

View reviewed changes

greg0ire requested a review from a team June 13, 2018 13:38

covex-nn merged commit cd74a2d into sonata-project:3.x Jun 13, 2018

greg0ire deleted the invokable_caas branch June 13, 2018 14:27

core23 mentioned this pull request Jun 16, 2018

Use ADR pattern sonata-project/SonataNewsBundle#457

Merged

3 tasks

pestaa pushed a commit to Crosssec/SonataAdminBundle that referenced this pull request Aug 6, 2018

Make CoreController actions CaaSes (sonata-project#5120)

f0976a5

core23 mentioned this pull request Oct 6, 2018

More CRUDController actions CaaSec #5247

Closed

greg0ire mentioned this pull request Oct 23, 2018

Add configurable mailer sonata-project/SonataUserBundle#1052

Merged

Wharenn mentioned this pull request Oct 24, 2018

Use invokable actions sonata-project/SonataUserBundle#1053

Merged

2 tasks

jordisala1991 mentioned this pull request Aug 1, 2020

Remove deprecated code #6210

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make CoreController actions CaaSes #5120

Make CoreController actions CaaSes #5120

greg0ire commented Jun 9, 2018 •

edited

Loading

kunicmarko20 Jun 10, 2018

greg0ire Jun 10, 2018

kunicmarko20 Jun 10, 2018

greg0ire Jun 10, 2018 •

edited

Loading

kunicmarko20 left a comment

core23 Jun 10, 2018

greg0ire Jun 10, 2018

core23 Jun 10, 2018

kunicmarko20 Jun 10, 2018

greg0ire commented Jun 10, 2018

greg0ire commented Jun 10, 2018

core23 commented Jun 10, 2018

greg0ire commented Jun 10, 2018

greg0ire commented Jun 10, 2018

core23 Jun 11, 2018

greg0ire Jun 11, 2018 •

edited

Loading

core23 Jun 11, 2018

greg0ire Jun 11, 2018

core23 Jun 11, 2018

greg0ire commented Jun 11, 2018

greg0ire commented Jun 11, 2018

greg0ire commented Jun 13, 2018

covex-nn commented Jun 13, 2018

greg0ire commented Jun 13, 2018

Make CoreController actions CaaSes #5120

Make CoreController actions CaaSes #5120

Conversation

greg0ire commented Jun 9, 2018 • edited Loading

Changelog

Todo

Subject

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

greg0ire Jun 10, 2018 • edited Loading

Choose a reason for hiding this comment

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

greg0ire commented Jun 10, 2018

greg0ire commented Jun 10, 2018

core23 commented Jun 10, 2018

greg0ire commented Jun 10, 2018

greg0ire commented Jun 10, 2018

Choose a reason for hiding this comment

greg0ire Jun 11, 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

greg0ire commented Jun 11, 2018

greg0ire commented Jun 11, 2018

greg0ire commented Jun 13, 2018

covex-nn commented Jun 13, 2018

greg0ire commented Jun 13, 2018

greg0ire commented Jun 9, 2018 •

edited

Loading

greg0ire Jun 10, 2018 •

edited

Loading

greg0ire Jun 11, 2018 •

edited

Loading