?
Referenced as Related to: # 8770
avatar roland-d
roland-d
13 Jan 2017

Pull Request for Issue #8770 .

Summary of Changes

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.

Testing Instructions

  1. Create an article and make sure to assign a tag to the article
  2. Check the ucm_content table to see that the article is there as well
  3. Trash the article
  4. Delete the article from trash
  5. Check the ucm_content table and see that the article is still there
  6. Apply patch
  7. Create an article and make sure to assign a tag to the article
  8. Check the ucm_content table to see that the article is there as well
  9. Trash the article
  10. Delete the article from trash
  11. Check the ucm_content table and see that the article has now been removed

Documentation Changes Required

None

Votes

# of Users Experiencing Issue
1/1
Average Importance Score
3.00

avatar roland-d roland-d - open - 13 Jan 2017
avatar roland-d
roland-d - comment - 13 Jan 2017

@wilsonge that is because an array is passed to this function. That is because you can select multiple items to be deleted at once.

avatar alikon
alikon - comment - 14 Jan 2017

tested successfully on postgresql

avatar alikon
alikon - comment - 14 Jan 2017

unable to mark successfull test on issue tracker


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13584.

avatar sanderpotjer
sanderpotjer - comment - 14 Jan 2017

Successfully tested. Related #__ucm_content deleted after emptying the article trash.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13584.

avatar jeckodevelopment jeckodevelopment - change - 14 Jan 2017
Labels Added: ?
avatar jeckodevelopment jeckodevelopment - change - 14 Jan 2017
Status New Pending
avatar jeckodevelopment jeckodevelopment - change - 14 Jan 2017
Status Pending Ready to Commit
avatar jeckodevelopment
jeckodevelopment - comment - 14 Jan 2017

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13584.

avatar wilsonge
wilsonge - comment - 14 Jan 2017

I still don't understand how this is getting an array. The call stack should look something like:

  1. JTable::delete for the single ID (note doesn't support an array of data)
  2. JTable::delete calls JTableObserverTags::onBeforeDelete via the observer's for JTable
  3. JTableObserverTags::onBeforeDelete calls JHelperTags::deleteTagData

I don't understand why we have an array for a single item?

avatar mbabker
mbabker - comment - 14 Jan 2017

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™.

avatar mbabker mbabker - change - 14 Jan 2017
Status Ready to Commit Needs Review
avatar wilsonge
wilsonge - comment - 14 Jan 2017

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

avatar roland-d
roland-d - comment - 14 Jan 2017

@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.

avatar mbabker
mbabker - comment - 14 Jan 2017

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.

avatar wilsonge
wilsonge - comment - 14 Jan 2017

Try this #13592

avatar roland-d
roland-d - comment - 15 Jan 2017

Closing in favor of #13592

avatar roland-d roland-d - change - 15 Jan 2017
Status Needs Review Closed
Closed_Date 0000-00-00 00:00:00 2017-01-15 07:59:40
Closed_By roland-d
avatar roland-d roland-d - close - 15 Jan 2017
avatar joomla-cms-bot joomla-cms-bot - change - 15 Jan 2017
Category Libraries

Add a Comment

Login with GitHub to post a comment