[#42709] - [5.1] Fix Form::renderField() can cause duplicate database queries
- Fixed in Code Base
- 28 Feb 2024
- Medium
- Build: 5.1-dev
- # 42709
- Diff
- Fedik:field-collect-data
User tests: Successful: Unsuccessful:
Pull Request for Issue
Summary of Changes
Method FormField::getLayoutData() called multiple times while field rendering, which can cause a multiple SQL queries.
The PR introduces a new method for data collection with in memory caching collectLayoutData().
Thing to consider:
I marked getLayoutData() as deprecated, hovewer both methods can live side by side, if people like.
Undeprecated back :)
Testing Instructions
Create a custom field under libraries/src/Form/Field/TestField.php
With following code:
<?php
namespace Joomla\CMS\Form\Field;
use Joomla\CMS\Factory;
class TestField extends CheckboxesField
{
protected $type = 'Test';
protected function getOptions()
{
$db = Factory::getDbo();
$db->setQuery(
$db->getQuery(true)
->select($db->quoteName(['title', 'id'], ['text', 'value']))
->from('#__content')
->setLimit(5)
);
$list = array_map(function($r) {$r->checked = null;return $r; }, $db->loadObjectList());
return array_merge(parent::getOptions(), $list);
}
}Then add this field in to mod_custom parameters XML: modules/mod_custom/mod_custom.xml
<field type="test" name="test" label="test"/>
<field type="test" name="test2" label="test2"/>
<field type="test" name="test3" label="test3"/>Enable Debug, and Debug Query.
Go to edit the "Modules: Custom" parameters, and watch amount of queries in debug bar.
Actual result BEFORE applying this Pull Request
let say will be 52 queries
Expected result AFTER applying this Pull Request
Will be 6 queries less, so 46.
In my test I got 3 queries per 1 testing field.
Link to documentations
Please select:
- Documentation link for docs.joomla.org:
- No documentation changes for docs.joomla.org needed
- Pull Request link for manual.joomla.org: joomla/Manual#217
- No documentation changes for manual.joomla.org needed
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries Front End Plugins |
On comment on boolean parameters, based on who believe it's not the a good practice to use them
It just 1, not many of them (as in linked article) :)
Hovewer, I can do without this parameter, and developers can reset the data property manualy.
I'm not sure about the side effects on all fields, but it's possible that it breaks formfields which expect to be called several times?
I can't think any reason why it should be valid. The getData() should return consistent data, in any amount of call.
One side effect, that many fields will work much faster, in a couple ns ?
| Labels |
Added:
bug
PR-5.1-dev
|
||
I have removed reset parameter
I have tested this item ✅ successfully on 8d3650c
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42709.
I have tested this item ✅ successfully on 8d3650c
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42709.
| Status | Pending | ⇒ | Ready to Commit |
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42709.
| Labels |
Added:
RTC
|
||
| Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2024-02-28 00:31:56 |
| Closed_By | ⇒ | LadySolveig |
Thank you @Fedik and for testing @alikon @martin-zw
I just stumbled upon a user's message, he can't submit a form. So I spend some time digging into it.
This change causes issues at least with the calendar field in case one tries to render the label and then the input field. Since the layoutdata is now cached and the calendar field is changing the value-data in the getInput()-method, you get the wrong value/date in the frontend.
Steps to repeat:
- have a calendar field definition in a form with
default="NOW"andtranslateformat=true - render the formelement by calling the
getLabel-method before thegetInputmethod - use a language like German as the backend language to get a different time format (it works with English, since there is no translation needed)
- Result: in the HTML source, a date like 2024-03-07 is written, instead of 7.3.2024. This makes the calendar-JS a bit crazy and also causes an exception when submitting the form.
Within Joomla, I can't reproduce this. But I can with my custom component.
@Fedik does this makes sense to you? Maybe I'm missing something here.
That sounds like a bug in the calendar field, I will make a fix later.
@svenbluege please test the fix #43234
Hi,
I got an issue with subforms, caused by this feature. In subforms, fields will be called multiple times; in this case, joomla 5.1 uses extra data (collected from getLayoutData()) from first field for all fields of same type, such as field value. This is a big issue and crashes all subforms!
It is a good thing to reduce database queries and use collected data for multiple calls just one time, but in this case, is causes a lot of issues.
Regards, Daniel
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42709.
Please provide an example, how to reproduce that. Thanks.
And open a new issue for that.
Okay, @Fedik, I created a new issue #43303
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/42709.
If I read it correctly you introduced caching for all formfields right?
On comment on
booleanparameters, based on who believe it's not the a good practice to use them (ex. https://understandlegacycode.com/blog/what-is-wrong-with-boolean-parameters/). I would suggest to keep getLayoutData and add the caching functionality, beside that add aresetLayoutData()(or similar name) function instead of adding abooleanparameter.I'm not sure about the side effects on all fields, but it's possible that it breaks formfields which expect to be called several times? I'm not sure didn't thought about this long enough.