-
-
Notifications
You must be signed in to change notification settings - Fork 58
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
[WIP] Use the Symfony event dispatcher for hooks and callbacks #204
Conversation
Looks good to me!! My input: I would place all hook listeners in an extra subnamespace so it becomes |
Good point, will change it accordingly. |
I would suggest to use one class for all hook events. That will make the old stuff much cleaner. Also we only need one service with just a lot of tags. |
I don't think Insights and Scrutinizer would like that. They already don't like that we have only two controllers with too many methods. |
Well, Scruitinizer should not be the issue for code that only provides BC layers. https://scrutinizer-ci.com/docs/reviews/excluding_files_from_analysis |
I know. Still we should always follow the best practices, because people will look at how we've done things. |
Of course and I prefer your variant.
|
This will also solve our arguments order problem: // Call
$strTemp = $this->$callback[0]->$callback[1]($row, $blnWriteToFile, $vars, $parent);
// Usage
public function compileDefinition($row, $blnWriteToFile, $vars, $parent)
{
// do something
} But if we are using events, it will all be in the event object: // Call
$event = new CompileDefinitionEvent();
$event->setRow($row);
$event->setWriteToFile($blnWriteToFile);
$event->setVars($vars);
$event->setParent($parent);
$this->container->get('event_dispatcher')->dispatch('contao.initialize_system', $event);
// Usage
public function compileDefinition(CompileDefinitionEvent $event)
{
print_r($event->getRow());
} If we ever want to add or remove parameters, we don't have to stick to an argument order anymore :) |
Did you only realize this now? :D |
No, I just wanted to mention it :) |
9869e88
to
381547c
Compare
I'm still for one class, also using event subscribers (see http://symfony.com/doc/current/components/event_dispatcher/introduction.html#using-event-subscribers). Let's discuss that later today. |
Yes. I have still added another example to my POC to see how it works with parameters. |
You might want to have a look how we did it in dc-general. |
2e3654e
to
32ffaaf
Compare
e89893a
to
99bf80c
Compare
* | ||
* @return string The root directory | ||
*/ | ||
public function getRootDir() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a setter method is missing here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no setter method. The variable is only injected to include the initialize.php
file.
In addition to setting the event variables from hook references, you also need to set the variables where the event is fired. Otherwise the variables could still not be changed by reference. Example: // Trigger the getPageLayout hook (see #4736)
$event = new PageEvent($objPage, $objLayout, $this);
$kernel->getContainer()->get('event_dispatcher')->dispatch(ContaoEvents::GENERATE_PAGE, $event);
$objPage = $event->getPage();
$objLayout = $event->getLayout(); |
No, I found a better solution. Check my latest commit :) |
Not sure if that's not a problem. Now we are deciding wether the variables are passed by reference, instead of the hook developer… |
The developer will still have to put an ampersand before the argument, otherwise they will not receive the reference. Here's how you can test: // vendor/contao/calendar-bundle/src/Resources/config.php
$GLOBALS['TL_HOOKS']['modifyFrontendPage'][] = function ($page, $template, $handler) {
$template = 'fe_foo';
return $page;
}; The template name will not be changed, therefore // vendor/contao/calendar-bundle/src/Resources/config.php
$GLOBALS['TL_HOOKS']['modifyFrontendPage'][] = function ($page, &$template, $handler) {
$template = 'fe_foo';
return $page;
}; Then you get a |
I'm going to write the tests tonight. |
50d803c
to
0b4b9fb
Compare
👍 Is it still realistic for 4.1? :) |
Conflicts: src/EventListener/InitializeSystemListener.php src/Resources/contao/classes/FrontendTemplate.php src/Resources/contao/classes/RebuildIndex.php src/Resources/contao/drivers/DC_Table.php src/Resources/contao/library/Contao/Automator.php src/Resources/contao/library/Contao/Date.php src/Resources/contao/library/Contao/DcaLoader.php src/Resources/contao/library/Contao/Search.php src/Resources/contao/library/Contao/System.php src/Resources/contao/library/Contao/Template.php src/Resources/contao/library/Contao/User.php src/Resources/contao/library/Contao/Widget.php src/Resources/contao/modules/ModuleChangePassword.php src/Resources/contao/modules/ModulePassword.php tests/TestCase.php
ae77c88
to
42e9669
Compare
e710958
to
8365c10
Compare
a7ef637
to
3b12542
Compare
Here is the list of the currently available hooks in the core distribution:
The highlighted ones have been added already in the PR. |
As discussed on Mumble, @aschempp wants to try to set up a general service which allows to map events to hooks. If he succeeds, we do not need this PR at all. |
Closed in favor of #376. |
This is the PR for #97. Can you please confirm that this is the way to go, so I can adjust the other hooks as well?