[com_fields] Fetch all custom field values in one query #14558
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -417,15 +417,14 @@ public function getForm($data = array(), $loadData = true) | |
| * Setting the value for the gven field id, context and item id. | ||
| * | ||
| * @param string $fieldId The field ID. | ||
| * @param string $itemId The ID of the item. | ||
| * @param string $value The value. | ||
| * | ||
| * @return boolean | ||
| * | ||
| * @since 3.7.0 | ||
| */ | ||
| public function setFieldValue($fieldId, $itemId, $value) | ||
| { | ||
| $field = $this->getItem($fieldId); | ||
| $params = $field->params; | ||
|
|
@@ -452,7 +451,7 @@ public function setFieldValue($fieldId, $context, $itemId, $value) | |
| } | ||
| else | ||
| { | ||
| $oldValue = $this->getFieldValue($fieldId, $itemId); | ||
| $value = (array) $value; | ||
|
|
||
| if ($oldValue === null) | ||
|
|
@@ -481,7 +480,6 @@ public function setFieldValue($fieldId, $context, $itemId, $value) | |
|
|
||
| $query->delete($query->qn('#__fields_values')) | ||
| ->where($query->qn('field_id') . ' = ' . (int) $fieldId) | ||
| ->where($query->qn('item_id') . ' = ' . $query->q($itemId)); | ||
|
|
||
| $this->getDbo()->setQuery($query)->execute(); | ||
|
|
@@ -492,7 +490,6 @@ public function setFieldValue($fieldId, $context, $itemId, $value) | |
| $newObj = new stdClass; | ||
|
|
||
| $newObj->field_id = (int) $fieldId; | ||
| $newObj->item_id = $itemId; | ||
|
|
||
| foreach ($value as $v) | ||
|
|
@@ -508,11 +505,10 @@ public function setFieldValue($fieldId, $context, $itemId, $value) | |
| $updateObj = new stdClass; | ||
|
|
||
| $updateObj->field_id = (int) $fieldId; | ||
| $updateObj->item_id = $itemId; | ||
| $updateObj->value = reset($value); | ||
|
|
||
| $this->getDbo()->updateObject('#__fields_values', $updateObj, array('field_id', 'item_id')); | ||
| } | ||
|
|
||
| $this->valueCache = array(); | ||
|
|
@@ -524,16 +520,15 @@ public function setFieldValue($fieldId, $context, $itemId, $value) | |
| * Returning the value for the given field id, context and item id. | ||
| * | ||
| * @param string $fieldId The field ID. | ||
| * @param string $itemId The ID of the item. | ||
| * | ||
| * @return NULL|string | ||
| * | ||
| * @since 3.7.0 | ||
| */ | ||
| public function getFieldValue($fieldId, $itemId) | ||
| { | ||
| $key = md5($fieldId . $itemId); | ||
|
|
||
| if (!key_exists($key, $this->valueCache)) | ||
| { | ||
|
|
@@ -544,7 +539,6 @@ public function getFieldValue($fieldId, $context, $itemId) | |
| $query->select($query->qn('value')) | ||
| ->from($query->qn('#__fields_values')) | ||
| ->where($query->qn('field_id') . ' = ' . (int) $fieldId) | ||
| ->where($query->qn('item_id') . ' = ' . $query->q($itemId)); | ||
|
|
||
| $rows = $this->getDbo()->setQuery($query)->loadObjectList(); | ||
|
|
@@ -569,6 +563,46 @@ public function getFieldValue($fieldId, $context, $itemId) | |
| return $this->valueCache[$key]; | ||
| } | ||
|
|
||
| /** | ||
| * Returning the values for the given field ids, context and item id. | ||
| * | ||
| * @param array $fieldIds The field Ids. | ||
| * @param string $itemId The ID of the item. | ||
| * | ||
| * @return NULL|array | ||
| * | ||
| * @since 3.7.0 | ||
| */ | ||
| public function getFieldValues(array $fieldIds, $itemId) | ||
| { | ||
| $key = md5(serialize($fieldIds) . $itemId); | ||
|
|
||
| if (!key_exists($key, $this->valueCache)) | ||
| { | ||
| $this->valueCache[$key] = null; | ||
|
There was a problem hiding this comment. If I read the code correctly, this line is not needed. You set $this->valueCache[$key] = $data; later already |
||
|
|
||
| $query = $this->getDbo()->getQuery(true); | ||
|
|
||
| $query->select(array($query->qn('field_id'), $query->qn('value'))) | ||
| ->from($query->qn('#__fields_values')) | ||
| ->where($query->qn('field_id') . ' in (' . implode(',', ArrayHelper::toInteger($fieldIds)) . ')') | ||
|
There was a problem hiding this comment. Should be IN instead of in. Am I correct that we always use upper case string for SQL keywords? |
||
| ->where($query->qn('item_id') . ' = ' . $query->q($itemId)); | ||
|
|
||
| $rows = $this->getDbo()->setQuery($query)->loadObjectList(); | ||
|
|
||
| $data = array(); | ||
|
|
||
| foreach ($rows as $row) | ||
| { | ||
| $data[$row->field_id] = $row->value; | ||
| } | ||
|
|
||
| $this->valueCache[$key] = $data; | ||
| } | ||
|
|
||
| return $this->valueCache[$key]; | ||
| } | ||
|
|
||
| /** | ||
| * Cleaning up the values for the given item on the context. | ||
| * | ||
|
|
@@ -581,10 +615,16 @@ public function getFieldValue($fieldId, $context, $itemId) | |
| */ | ||
| public function cleanupValues($context, $itemId) | ||
| { | ||
| // Delete with inner join is not possible so we need to do a subquery | ||
| $fieldsQuery = $this->getDbo()->getQuery(true); | ||
| $fieldsQuery->select($fieldsQuery->qn('id')) | ||
| ->from($fieldsQuery->qn('#__fields')) | ||
| ->where($fieldsQuery->qn('context') . ' = ' . $fieldsQuery->q($context)); | ||
|
There was a problem hiding this comment. This looks wrong to me as you have removed context from the db table ;) There was a problem hiding this comment. That query is against #__fields and not #__fields_values. It is correct. There was a problem hiding this comment. Ah thanks i got confused. 👍 There was a problem hiding this comment. Do you know if we use in the core a delete with an inner join as subselects are slower? I couldn't find a way to do it. There was a problem hiding this comment. I don't know that we use it. But that does not mean that we don't use it. 😄 There was a problem hiding this comment. I'm not that concerned about performance for deleting fields. It's not done multiple times per second usually. Much more important that it works reliably 😄 There was a problem hiding this comment. @laoneo in com_finder there is something similar https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_finder/helpers/indexer/taxonomy.php#L345 There was a problem hiding this comment. I saw that, but also doesn't use an inner join. |
||
|
|
||
| $query = $this->getDbo()->getQuery(true); | ||
|
|
||
| $query->delete($query->qn('#__fields_values')) | ||
| ->where($query->qn('field_id') . ' IN (' . $fieldsQuery . ')') | ||
| ->where($query->qn('item_id') . ' = ' . $query->q($itemId)); | ||
|
|
||
| $this->getDbo()->setQuery($query)->execute(); | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
You are not changed it now, but database field with type TEXT NOT NULL can not have default value on mysql.
https://dev.mysql.com/doc/refman/5.7/en/blob.html
There is a lot of such mistakes in joomla but please do not repeat it.
I see 2 solutions. Allow to NULL or always set default value in php code.
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.
That sort of stuff should be done in a separate pr, basically on thing in one pr, otherwise it gets harder to review.
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.
Yes, I want only mention about it.
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.
No problem.