Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RTM] Draft of Contao controllers #1376

Merged
merged 14 commits into from Jun 11, 2018

Conversation

@aschempp
Copy link
Contributor

commented Feb 18, 2018

Abstract controllers that implement the same features as the abstract ContentElement and Module classes did.

@leofeyer leofeyer requested a review from Toflar Feb 18, 2018

@leofeyer leofeyer changed the base branch from 4.5 to master Feb 20, 2018

@leofeyer leofeyer changed the base branch from master to 4.5 Feb 20, 2018

@leofeyer leofeyer added the feature label Feb 20, 2018

@leofeyer leofeyer added this to the 4.6.0 milestone Feb 20, 2018

@leofeyer

This comment has been minimized.

Copy link
Member

commented Feb 20, 2018

The PR should be targeted against the master branch.

@aschempp aschempp force-pushed the aschempp:feature/abstract-controller branch from 4312316 to 0a6dc71 Feb 20, 2018

@leofeyer leofeyer changed the base branch from 4.5 to master Feb 21, 2018

@leofeyer leofeyer changed the title Draft of Contao controllers [WIP] Draft of Contao controllers Feb 21, 2018

@leofeyer leofeyer removed this from the 4.6.0 milestone Feb 21, 2018

@aschempp aschempp force-pushed the aschempp:feature/abstract-controller branch from 6bf2b11 to f3d6796 Apr 9, 2018

@aschempp

This comment has been minimized.

Copy link
Contributor Author

commented Apr 9, 2018

I have just updated this PR to make our fragments actually useful. The problem of inline fragment renderer is that it creates a subrequest without any of the current request information. It create a GET request even if the master request is POST, respectively drops POST parameters and much more.

Our fragments work different than the Symfony core, that's why it makes sense to inherit the master request in our case. It is still very much possible to keep the existing behavior by simply overriding the renderer in the tag configuration.

@Toflar

This comment has been minimized.

Copy link
Member

commented Apr 9, 2018

Our fragments work different than the Symfony core, that's why it makes sense to inherit the master request in our case. It is still very much possible to keep the existing behavior by simply overriding the renderer in the tag configuration.

That's correct. Reviewed and concept approved. But tests need to be fixed 😄

@aschempp aschempp changed the title [WIP] Draft of Contao controllers [RFC] Draft of Contao controllers Apr 10, 2018

@aschempp

This comment has been minimized.

Copy link
Contributor Author

commented Apr 10, 2018

@contao/developers any other feedback? Can we agree that we want this implementation before I spend more time on fixing tests?

@Toflar

This comment has been minimized.

Copy link
Member

commented Apr 10, 2018

I've got it on my list, will review today :)

@Toflar
Copy link
Member

left a comment

I've reviewed. Excellent work and definitely must be merged. I've added a few comments that I think should be addressed first though 😄

$template->inColumn = $section;
if (is_array($classes = $request->attributes->get('classes'))) {

This comment has been minimized.

Copy link
@Toflar

Toflar Apr 11, 2018

Member

\is_array()

This comment has been minimized.

Copy link
@qzminski

qzminski Apr 11, 2018

Contributor

Should be a task of php-cs-fixer, no?

This comment has been minimized.

Copy link
@aschempp

aschempp Apr 19, 2018

Author Contributor

Jup… see 5cdc39a

$template = new BackendTemplate('be_wildcard');
$template->wildcard = '### ' . strtoupper($GLOBALS['TL_LANG']['FMD'][$this->getType()][0]) . ' ###';

This comment has been minimized.

Copy link
@Toflar

Toflar Apr 11, 2018

Member

Can't we use our translator here?

This comment has been minimized.

Copy link
@aschempp

aschempp Apr 19, 2018

Author Contributor

Changed in 9059847

@@ -77,6 +78,10 @@ protected function registerFragments(ContainerBuilder $container, string $tag):
$preHandlers[$identifier] = $reference;
}
if (is_a($definition->getClass(), AbstractFragmentController::class, true)) {

This comment has been minimized.

Copy link
@Toflar

Toflar Apr 11, 2018

Member

That just kills the possibility to have a custom controller that needs to know the fragment options but does not want to extend the AbstractFragmentController. I mean, the purpose of the fragments is that they are fully independent controllers so I think we should work with a FragmentOptionsAwareInterface here, wdyt?

This comment has been minimized.

Copy link
@discordier

discordier Apr 18, 2018

Contributor

I second that. We should use small(!) interfaces for these tasks.
Brings also the benefit of being able to deprecate one way (old interface) and introduce a new way without changing the API. I guess we will have to alter the mechanics in the future which is easier by adding another interface than having version_compares() and the like.

This comment has been minimized.

Copy link
@aschempp

aschempp Apr 19, 2018

Author Contributor

Added in 43b042d

use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpKernel\Fragment\InlineFragmentRenderer;
class ForwardFragmentRenderer extends InlineFragmentRenderer

This comment has been minimized.

Copy link
@Toflar

Toflar Apr 11, 2018

Member

Would be great if you could add some docs here and explain why this is needed.

This comment has been minimized.

Copy link
@aschempp

aschempp Apr 19, 2018

Author Contributor

Added in 5523f84

@dmolineus

This comment has been minimized.

Copy link
Contributor

commented Apr 11, 2018

It's a great simplification of such attract controllers would be provided.

I'd prefer not to depend on the Contao template classes though. We could extend the symfony template engine wrapping the Contao templates. Then it's up to the developer to decide which template to use. Migrating later to twig templates would be really easy then.

For example following template syntax could be used:

  • contao:be:be_main.html5
  • contao:fe:fe_page.html5

I know it's off topic here but it would be nice to have this feature included when merging this pr.

If you like the idea I'd love to contribute the code. (I already do it in my toolkit library, https://github.com/netzmacht/contao-toolkit/blob/master/src/View/Template/ToolkitTemplateEngine.php)

*
* @return \Symfony\Component\HttpFoundation\Response
*/
protected function getBackendWildcard(ModuleModel $module, Request $request)

This comment has been minimized.

Copy link
@qzminski

qzminski Apr 11, 2018

Contributor

This method should actually be also available for content elements, as some of them may display dynamic data as well.

This comment has been minimized.

Copy link
@aschempp

aschempp Apr 18, 2018

Author Contributor

It should not be in the parent class for sure, because the generated URL is different for different types. Also, none of the core elements have wildcards. What's the reason not to show dynamic data?

This comment has been minimized.

Copy link
@qzminski

qzminski Apr 18, 2018

Contributor

And what's the reason to show backend wildcard for frontend modules?

This comment has been minimized.

Copy link
@aschempp

aschempp Apr 18, 2018

Author Contributor

¯\(ツ)

I guess it's also a quick link to edit the module, which does not make sense for an element. There are content elements that change their appearance in the backend, but never to a wildcard. The YouTube element does not generate an iframe but a link to the video for example.

This comment has been minimized.

Copy link
@qzminski

qzminski Apr 18, 2018

Contributor

Sometimes the content element (frontend module as well) relies on the frontend page settings, has some jQuery scripts in the template or simply a pagination that is useless in backend. There are also the cases were generated data may simply be not necessary to render in the backend.

The backend wildcard should also be available for content elements. It must not be enabled by default but I don't see a reason why not offer it for developer at all.

@aschempp aschempp force-pushed the aschempp:feature/abstract-controller branch from d920e2a to 5c68bf9 May 14, 2018

@aschempp

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2018

I have updated the PR. Most important change is to disable ignore_errors by default. If errors are ignores, it is not possible to handle ResponseException and fragments simply don't show up in the response. I noticed this because I had my own element that tried so use Controller::sentFileToBrowser.

@Toflar

This comment has been minimized.

Copy link
Member

commented May 14, 2018

Good find!

@leofeyer leofeyer added this to the 4.6.0 milestone May 31, 2018

use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;
abstract class AbstractFrontendModuleController extends AbstractFragmentController

This comment has been minimized.

Copy link
@leofeyer

leofeyer Jun 1, 2018

Member

I just realize that the classes are called ContentElement and Module in Contao, therefore this class should be renamed to AbstractModuleController, shouldn't it?

This comment has been minimized.

Copy link
@aschempp

aschempp Jun 1, 2018

Author Contributor

I dont think so, because there is also a backend module. Imho the old naming is „wrong“.

@aschempp aschempp changed the title [RFC] Draft of Contao controllers [RTM] Draft of Contao controllers Jun 9, 2018

@leofeyer leofeyer removed the test missing label Jun 11, 2018

leofeyer added some commits Jun 11, 2018

@leofeyer leofeyer merged commit 5b64c9e into contao:master Jun 11, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@leofeyer

This comment has been minimized.

Copy link
Member

commented Jun 11, 2018

Thank you @aschempp.

@christian-kolb

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2018

@leofeyer @aschempp @fritzmg @Toflar @qzminski @dmolineus Has someone of you already implemented a frontend module with this or the previous controller in a public repository? I think it would be great to have this linked from the 4.6.0 release blog post. This way more people would get a kickstart and we might see more bundles using it faster 🙂

@bytehead

This comment has been minimized.

Copy link
Member

commented Sep 21, 2018

Something like this?

# app/config/services.yml

services:
    _defaults:
        autowire: true
        autoconfigure: true
        public: false

    # Content Elements
    app.content_element.acme_element:
        class: AppBundle\ContentElement\AcmeElement
        tags:
            - { name: contao.content_element, category: myCategory }
        calls:
            - [ setContainer,[ '@service_container' ] ]

    # Modules
    app.frontend_module.acme_module:
        class: AppBundle\Module\AcmeModule
        tags:
            - { name: contao.frontend_module, category: myCategory }
        calls:
            - [ setContainer,[ '@service_container' ] ]
// src/AppBundle/ContentElement/AcmeElement.php

<?php

declare(strict_types=1);

namespace AppBundle\ContentElement;

use Contao\CoreBundle\Controller\ContentElement\AbstractContentElementController;
use Contao\ModuleModel;
use Contao\Template;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;

class AcmeElement extends AbstractContentElementController
{
    protected function getResponse(Template $template, ContentModel $model, Request $request): Response
    {
        $template->title = "AcmeElement";

        return new Response($template->parse());
    }
}
// src/AppBundle/Module/AcmeModule.php

<?php

declare(strict_types=1);

namespace AppBundle\Module;

use Contao\CoreBundle\Controller\FrontendModule\AbstractFrontendModuleController;
use Contao\ModuleModel;
use Contao\Template;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;

class AcmeModule extends AbstractFrontendModuleController
{
    protected function getResponse(Template $template, ModuleModel $model, Request $request): Response
    {
        $template->title = "AcmeModule";

        return new Response($template->parse());
    }
}
@qzminski

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2018

@bytehead looks good and similar to mine, I am just not sure if setContainer call in services.yml is really needed. I would rather use service injection here 🤔

@qzminski

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2018

You sure? The class extends Controller which should already have container in it https://github.com/contao/contao/blob/master/core-bundle/src/Controller/AbstractFragmentController.php#L23

@bytehead

This comment has been minimized.

Copy link
Member

commented Sep 21, 2018

Yes, just tested. Probably because AbstractFrontendModuleController is abstract?

@qzminski

This comment has been minimized.

Copy link
Contributor

commented Sep 21, 2018

Ok that's strange, I would rather expect it to automatically have container set just like described in Symfony docs https://symfony.com/doc/3.4/controller.html#the-base-controller-class-services

Ping @aschempp

@aschempp

This comment has been minimized.

Copy link
Contributor Author

commented Sep 25, 2018

¯\(ツ)
I don't know… I guess you should debug that. I'm not very familiar with autowiring and autoconfiguration …

@aschempp aschempp deleted the aschempp:feature/abstract-controller branch Sep 25, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.