Pull Request for Issue #8770 .
Deleting an article doesn't remove the article from the ucm_content table because an array is sent while an integer is expected. This pull request fixes this.
None
tested successfully on postgresql
unable to mark successfull test on issue tracker
Successfully tested. Related #__ucm_content deleted after emptying the article trash.
Labels |
Added:
?
|
Status | New | ⇒ | Pending |
Status | Pending | ⇒ | Ready to Commit |
RTC
I still don't understand how this is getting an array. The call stack should look something like:
JTable::delete
for the single ID (note doesn't support an array of data)JTable::delete
calls JTableObserverTags::onBeforeDelete
via the observer's for JTable
JTableObserverTags::onBeforeDelete
calls JHelperTags::deleteTagData
I don't understand why we have an array for a single item?
deleteTagData
isn't receiving an array, it's casting the $contentItemId
parameter to an array. You're adding undocumented functionality.
The method is documented to receive an integer. Anything calling it with an array is Doing It Wrong™.
Status | Ready to Commit | ⇒ | Needs Review |
OK So our problem is this https://github.com/joomla/joomla-cms/blob/staging/libraries/joomla/table/table.php#L942 - So this is always an array - but so we support multi-key tables. I don't think ucm content supports the concept of a multikey special table. So what we need to do is grab the array here. Check it only has one value. If it does use that. If it doesn't, throw and InvalidArgumentException
@mbabker I should change it further upstream then because Joomla is sending an array to this function.
@wilsonge since there is always an array are you saying we should throw that exception? As we can delete more than one item at once we should just loop through this array.
Whatever we do it should be fixed, it's a real mess now.
It works now because $contentItemId
doesn't even get used in the unTagItem()
method (it gets passed but the method does other stuff to figure out what ID to remove, :facepalm:). So if this param is really an array I'd love to know how JTableCorecontent::deleteByContentId()
works correctly because it has no provisioning to convert the value to a single scalar type, so perhaps the bug is with that method specifically and the changes here just happen to make it work while also introducing new undocumented API behavior to account for what is apparently already incorrect use of the API.
Either way here we have a massive documentation and behavior inconsistency and taking B/C into consideration since the behavior is documented now to expect an integer that needs to continue working until 4.0 at a minimum. If this patch is going to go through, the API MUST document the array behavior too; we already suck at documenting our API or how to use the code, let's not add more magical boxes please.
Status | Needs Review | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2017-01-15 07:59:40 |
Closed_By | ⇒ | roland-d |
Category | ⇒ | Libraries |
@wilsonge that is because an array is passed to this function. That is because you can select multiple items to be deleted at once.