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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support multiple fragments on the same controller #2120

Merged
merged 4 commits into from
Aug 13, 2020

Conversation

aschempp
Copy link
Member

@aschempp aschempp commented Aug 7, 2020

If the same fragment controller implements multiple content elements or frontend modules (example: https://github.com/terminal42/contao-oembed/blob/master/src/Controller/OEmbedController.php#L14-L18), the options will overwrite each other.

This is the best solution I came up with, but it's still kinda ugly 馃槥

@aschempp aschempp added the bug label Aug 7, 2020
@aschempp aschempp added this to the 4.9 milestone Aug 7, 2020
@aschempp aschempp requested a review from a team August 7, 2020 09:31
@aschempp aschempp self-assigned this Aug 7, 2020
@leofeyer
Copy link
Member

Is implementing multiple elements/modules in the same fragment controller a supported use-case?

but it's still kinda ugly 馃槥

What exactly bothers you?

@Toflar
Copy link
Member

Toflar commented Aug 11, 2020

I think @aschempp is concerned because of things having state. But I don't think that's true here. They don't have state. The fragment options do not change after container build. Just applying the correct ones is different.
I actually think that looks good here. Or what's ugly, @aschempp?

Copy link
Member

@Toflar Toflar left a comment

Choose a reason for hiding this comment

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

Ah I see. Yes, this actually is state so the concept here seems wrong. What about actually dynamically creating 3 different services with different options during container build time? So the actual service one registers is not registered in the container but 3 child definitions per type, each having their own options? That's only required in case of multiple annotations on the same service of course but that would be the proper way to go imho :)

@aschempp aschempp requested a review from Toflar August 11, 2020 11:39
Copy link
Member

@Toflar Toflar left a comment

Choose a reason for hiding this comment

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

馃憤 馃憤 馃憤

@leofeyer leofeyer merged commit cebea38 into contao:4.9 Aug 13, 2020
@aschempp aschempp deleted the bugfix/fragment-options branch August 13, 2020 13:45
@philvdb
Copy link

philvdb commented Sep 24, 2020

This actually broke my setup because I was getting subcontrollers inside controllers (i. e. sub-content elements inside content elements) by referring to the service by class name.

$objSubController = System::getContainer()->get(FooController::class);

This approach basically still works now, unfortunately none of the options in the annotations of that service are available anymore. I believe that is because in RegisterFragmentsPass only the $childDefinition now has the call to setFragmentOptions added. The original service loses those fragment options, which means I cannot override the type or template settings and they are inferred from the class name. This is not always correct in my case leading to fatal errors on the site as templates are not found.

My code works again if I explicitly use the child definition by getting the child service like this:

$objSubController = System::getContainer()->get('contao.fragment._contao.content_element.foo');

Is that best practice now? Or could we add a call to setFragmentOptions for the original service as well, maybe at least with the first set of annotation options?

@aschempp
Copy link
Member Author

You should never call the fragment controller directly. Use the Symfony fragment handler if you need to do that, similar to https://github.com/contao/contao/blob/master/core-bundle/src/Resources/contao/modules/ModuleProxy.php#L25

@philvdb
Copy link

philvdb commented Sep 28, 2020

Okay thanks for your help, I will look into it.

@philvdb
Copy link

philvdb commented Oct 6, 2020

Thank you again, @aschempp . Every little bit helps in my journey to understanding The Symfony Way:tm: - I am now using the FragmentHandler (no dependency injection for that intended, it seems? As Contao gets it from the container as well?) and request attributes to inform the controller of models and objects. Makes a lot of sense. So this message is just another thank you to you for steering me in the right direction.

@aschempp
Copy link
Member Author

aschempp commented Oct 9, 2020

ALWAYS use dependency injection if you can! Contao only does it this way because the legacy classes cannot use dependency injection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants