[#17895] - [com_fields] Fix fields display HTML prepared 4 or 5 times per article, make it be prepared only twice
- Fixed in Code Base
- 29 Mar 2018
- Medium
- Build: staging
- # 17895
- Diff
- ggppdk:patch-45
User tests: Successful: Unsuccessful:
Pull Request for Issue #17889
Fields display is prepared multiple times
-- 5 times per record / article in single record view
-- 4 times per record / article in category view
Now it should be created only twice
- once for manual display
- and once if automatic display is 1 - After Title or 2 - Before Display or 3 - After Display
Summary of Changes
Reduces field's value property preparation (plugin events + layout loading) to 2 times
Explanation:
field's value property is the field's HTML display which includes plugin triggering and loading of layouts, and (rawvalue property is the raw DB value)
Testing Instructions
- Fields can be displayed automatically using field configuration TAB "options" automatic display
- Fields continue to be possible to be displayed manually inside the template of the view, using
$item->jcfields[_field_id_num_]->value
In both cases the plugins events should have been called and the layout of the field should have been loaded and the result should have been set to value property of the field
Expected result
Fields plugin event and layouts should be called only once
(This PR reduces to 2)
Actual result
They are called 5 times in single record (e.g. article) view
They are called 4 times (per article ) in category view
Documentation Changes Required
None
joomla-cms-bot
- | Category | ⇒ | Administration com_fields Front End Plugins |
| Status | New | ⇒ | Pending |
| Title |
|
||||||
| Labels |
Added:
?
|
||
| Title |
|
||||||
Just wondering how much you gain trough this patch? I mean if automatic display is activated on the field (which is the default), then it has no effect. The check if the jcfields property exists, makes absolutely sense, but the parameter addition in the getFields function does make it more complex then it has to be and does fulfill only one use case. If the display function makes such a big performance impact, then there should be a better way to improve.
On line 191 in administrator/components/com_fields/helpers/fields.php should is_boolean($prepareValue) maybe be is_bool($prepareValue)? is_boolean() gives me an error, is_bool() is a built-in PHP method.
maybe be is_bool(...)
Yes , i have it is_bool in my local copy, i missed pushing changes to this PR
Just wondering how much you gain through this patch? I mean if automatic display is activated on the field (which is the default), then it has no effect
it has a good performance benefit
- the fields are prepared by getFields() for all 3 events (1 - After Title or 2 - Before Display or 3 - After Display) , and after the preparation the caller of getFields, checks which event is used for automatic display and disgards all the preparation work for fields not using the current event !
really considerable waste of cpu cycles
I will calculate with PR for filesystem access applied , the speedup factor that this PR gives
@ggppdk can you please describe in Test Instructions how to Test and where to see how often a Field is called?
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17895.
Without spending time today (to remember all details of this PR again)
I would say,
after applying patch (and also before applying pathc) You could try to add here (temporarily):
https://github.com/joomla/joomla-cms/pull/17895/files#diff-4d276fa5ac7a065127d4620092ed38fdR194
echo $field->id . '<br/>';
or
echo $field->label . '<br/>';
and see how many times it gets displayed
in administrator/components/com_fields/helpers/fields.php after Line 192 JPluginHelper::importPlugin('fields'); append echo $field->id . '<br/>'; or echo $field->label . '<br/>';?
yes
I have tested this item
Using echo $field->id . '<br/>'; as described above; "Automatic Display" is on Before Display:
Without PR
Field is called 4 Times + 1 in Text.
With PR
Field is called 2 Times + 1 in Text.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17895.
| Title |
|
||||||
I have tested this item
Label is echoed only twice times instead of four.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17895.
| Status | Pending | ⇒ | Ready to Commit |
Ready to Commit after two successful tests.
Please resolve conflicting Files.
Please resolve conflicting Files
will get some time to do it, on quick look, it seems easy to resolve
just i would like to have some time to retest too
| Labels |
Added:
Conflicting Files
?
|
||
Conflicts resolved
It was only about
$fields = FieldsHelper::getFields($parts[0] . '.' . $parts[1], $item, true);becoming
$context = $parts[0] . '.' . $parts[1];
....
$fields = FieldsHelper::getFields($context, $item, true);
mbabker
- | Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2018-03-29 11:55:29 |
| Closed_By | ⇒ | mbabker | |
| Labels |
Removed:
Conflicting Files
|
||
Ready for testing