Calendar/events optimization

[qzminski]

I would like to propose a solution that will optimize the calendar/events module. This is also related to other modules like news and faq.

The problem: if you have dozens of events with multiple content elements each, the event list/menu module will generate the content event if it's not displayed. This will consume the resources and a lot of time, especially when you have both of the modules on the same page - the job will be done twice. I have measured the generation time with 50+ events and it took 10-20 seconds per each module (event list, event menu) to generate itself. In total it lasted about half a minute before the page was generated.

The solution: I think that the content elements can be generated on demand, i.e. in the template file. It should be as simple as adding the closure to the template. This way you can call it on demand, only when it needs to be used - like in the events reader module.

// module
$objTemplate->getContent = function() {
    // return the content
};

// template
echo $this->getContent();

[discordier]

We then should generalise the __get () of Template to execute the closure if the value is one.
This would be bc compatible then.

[Toflar]

The closure thing is what we chose in Isotope as well.

[leofeyer]

Changed in a4d2d9d.

[leofeyer]

Unfortunately, the change in the getter of the Template class causes problems:

<strong>Warning</strong>: next() expects exactly 1 parameter, 0 given in <strong>system/modules/core/library/Contao/Template.php</strong> on line <strong>98</strong>
<pre style="margin:11px 0 0">
#0 [internal function]: __error(2, 'next() expects ...', '/var/www...', 98, Array)
#1 system/modules/core/library/Contao/Template.php(98): next()
#2 system/modules/core/templates/elements/ce_slider_stop.html5(8): Contao\Template->__get('next')
#3 system/modules/core/library/Contao/BaseTemplate.php(88): include('/var/www...')
#4 system/modules/core/library/Contao/Template.php(250): Contao\BaseTemplate->parse()
#5 system/modules/core/classes/FrontendTemplate.php(46): Contao\Template->parse()
#6 system/modules/core/elements/ContentElement.php(284): Contao\FrontendTemplate->parse()
#7 system/modules/core/library/Contao/Controller.php(476): Contao\ContentElement->generate()
#8 system/modules/core/modules/ModuleArticle.php(213): Contao\Controller::getContentElement(Object(Contao\ContentModel), 'start')
#9 system/modules/core/modules/Module.php(282): Contao\ModuleArticle->compile()
#10 system/modules/core/modules/ModuleArticle.php(67): Contao\Module->generate()
#11 system/modules/core/library/Contao/Controller.php(409): Contao\ModuleArticle->generate(false)
#12 system/modules/core/library/Contao/Controller.php(269): Contao\Controller::getArticle(Object(Contao\ArticleModel), false, false, 'start')
#13 system/modules/core/pages/PageRegular.php(137): Contao\Controller::getFrontendModule('0', 'start')
#14 system/modules/core/controllers/FrontendIndex.php(267): Contao\PageRegular->generate(Object(Contao\PageModel), true)
#15 index.php(20): Contao\FrontendIndex->run()
#16 {main}
</pre></a>

[leofeyer]

Ok, so the problem here is the PHP function next(). If $strKey happens to be 'next' (or any other PHP function name), is_callable() will return true. I have now changed it to:

if ($this->arrData[$strKey] instanceof \Closure)

@contao/developers We are currently using is_callable() a lot for the DCA callbacks. Should we change all occurrences to instanceof \Closure as well?

[leofeyer]

Another option would be the following:

if (!is_string($this->arrData[$strKey]) && is_callable($this->arrData[$strKey]))

I found this variant in one of the Twig classes.

[aschempp]

We should use call_user_func anyway to support static calls etc.

[leofeyer]

We are talking about closures here. Anonymous functions.

[aschempp]

Well you said

We are currently using is_callable() a lot for the DCA callbacks. Should we change all occurrences to instanceof \Closure as well?

That would automatically be solved by using call_user_func or call_user_func_array. PHP will check and make sure to use the correct method of execution.

[leofeyer]

Our problem is not how to call the closure but how to detect whether it is a real closure or just a PHP function name.

[ausi]

IMO DCA callbacks and Template::__get() are two different contexts.

For Template::__get() the instanceof \Closure makes sense to me because we don’t want to execute strings or arrays in this context. The !is_string && is_callable check would still try to execute callable arrays, which would be bad IMO.

For DCA callbacks everything that is_callable should be executed. If I register the DCA callback "next", executing next() would be the right thing.

[aschempp]

For DCA callbacks everything that is_callable should be executed.

Everything else should also be executed. Because people registered a callback. If the callback is incorrect, the trying-to-execute will raise a message instead of silently ignore it.

I totally agree about the template part of what @ausi wrote. Maybe we should use is_scalar?

[ausi]

For DCA callbacks everything that is_callable should be executed.

Everything else should also be executed. Because people registered a callback. If the callback is incorrect, the trying-to-execute will raise a message instead of silently ignore it.

You are right, but maybe a custom error message would be nicer.

Maybe we should use is_scalar?

That would still execute arrays, which we don’t want.

[aschempp]

Maybe we should use is_scalar?

That would still execute arrays, which we don’t want.

I did mean if (!is_scalar($this->arrData[$strKey]) && $this->arrData[$strKey] instanceof \Closure) ;-)

[ausi]

Maybe we should use is_scalar?

That would still execute arrays, which we don’t want.

I did mean if (!is_scalar($this->arrData[$strKey]) && $this->arrData[$strKey] instanceof \Closure) ;-)

I don’t think this is necessary because instanceof just returns false for scalar types AFAIK.

[leofeyer]

Let's discuss this tomorrow. I don't think your suggestions are correct. I have checked the Symfony code and found no occurrences of is_scalar() or instanceof Closure. What I did find is !is_string() && is_callable() and is_object() && is_callable().

For Template::__get() the instanceof \Closure makes sense to me because we don’t want to execute strings or arrays in this context.

We do not want to execute strings aka PHP functions. Arrays, however, seem fine to me. They'll never map to a PHP function, so they are valid callbacks.

[ausi]

For Template::__get() the instanceof \Closure makes sense to me because we don’t want to execute strings or arrays in this context.

We do not want to execute strings aka PHP functions. Arrays, however, seem fine to me. They’ll never map to a PHP function, so they are valid callbacks.

Arrays can also map to PHP functions just like strings can. E.g. is_callable(['Closure', 'bind']) === true. What if I set $objTemplate->foo to an array of user input data and the user enters Config and preload as the two array values. !is_string() && is_callable() would return true, but executing this would be a security issue IMO.

[leofeyer]

You have a point. Then we should probably use is_object() && is_callable().

[ausi]

Then we should probably use is_object() && is_callable().

+1

[leofeyer]

Changed in 855fe68.