Deleting a term, that don't have any image associated, caused some errors.
I'll investigate further..

CommentFileSizeAuthor
#10 rewritten_deleting.patch7.56 KBthepanz
#1 rewritten_deleting.patch4.5 KBthepanz

Comments

thepanz’s picture

thepanz commented
Assigned: Unassigned » thepanz
Priority: Normal » Critical
Status: Active » Reviewed & tested by the community
StatusFileSize
new rewritten_deleting.patch4.5 KB

My patch fixes this issue, and the missing deletion of IMG.

I also cleanup the deletion function.

nancydru’s picture

nancydru
she/her/hers
Primary language English
Location Boston
commented
Status: Reviewed & tested by the community » Needs review

@thePanz: Some module maintainers get very upset when someone marks their own patch as RTBC. It's not a big deal for me.

I will get to this shortly.

thepanz’s picture

thepanz commented

Sorry NancyDru, I saw some maintainers following that way.. so I followed them :)

What I mean setting my patch to RTBC is that I used it on a production site without bugs or errors... sorry for my misunderstanding..

Regards

ps: do you think that migrating from custom path to Drupal {files} could improve TI? I mean: we store the imagename in DB, why can't we rely on {files} and save only (TID, FID) in our table? :)

nancydru’s picture

nancydru
she/her/hers
Primary language English
Location Boston
commented

I have at least twice started on converting to tid/fid and have gotten thrown off each time. It is not a trivial change (think about the update code), but I think it is the right thing to do. In the past it did make the external image impossible, but I think now that I just copy it to the site, it may work now.

If you are interested in issue status meanings, take a look at http://drupal.org/node/156119. You can set your own issue to RTBC if it is trivial, but this one is not.

nancydru’s picture

nancydru
she/her/hers
Primary language English
Location Boston
commented
Status: Needs review » Needs work

I get Error deleting term image; it seems that this image doesn't exists. when there is an image.

nancydru’s picture

nancydru
she/her/hers
Primary language English
Location Boston
commented
Priority: Critical » Normal

$full_path = $taxonomy_image_path . $old_path; but there is no "$old_path".

thepanz’s picture

thepanz commented

Sorry, I've got some other working project right now.. I'll give you feedback (or an updated patch) in next days..

[EDIT] I've put some edits on _delete() function. My idea is to delete Drupal messages from it and let the code that calls _delete() to act on true/false results. So external modules can handle TaxonomyImage better. What do you think?

Cheers,
thePanz

nancydru’s picture

nancydru
she/her/hers
Primary language English
Location Boston
commented
Status: Needs work » Fixed

I nearly rewrote the function to be more straightforward. I also added a setting to show/not-show all the messages.

thepanz’s picture

thepanz commented
Status: Fixed » Active

Hi NancyDru, I ported my patch with your latest commit in -dev snapshot.
I essentially removed the _verbose_ setting, and cleaned the _delete() function: it returns TRUE or FALSE as a result so caller function can (if needed) provide an error message.

My intention is to provide _delete($tid) and _add($tid, $filename) like an API (for the letter I noticed a patch that goes in that direction).

What do you think? Sould we port all this "features" into a 2.x version cause it's a major module change (I also think about moving from custom table with filenames to drupal {files} FID)?

thepanz’s picture

thepanz commented
StatusFileSize
new rewritten_deleting.patch7.56 KB
nancydru’s picture

nancydru
she/her/hers
Primary language English
Location Boston
commented
Status: Active » Needs work

I don't have a problem with returning TRUE or FALSE.

I like the verbose setting, and here's why: The most important part of the function is to break the term-image association and clear the cache. If, for some reason, the files table or the disk cannot be deleted, it is not the end of the world; there would be junk left, but the image no longer displays. I can live with that, but it would be nice if there was a way to find out where the function failed. But I suspect most people probably don't care as long as the image goes away.

Also, I cleaned up some of the comments and your new patch puts them back.

Yes, I would like to get this to only saving the fid, which is normalizing the database. I just need to get back to it. The hardest part of doing this will be the hook_update_N code. That would justify a version change.

nancydru’s picture

nancydru
she/her/hers
Primary language English
Location Boston
commented

More justification for the fid use idea: #437814: Images won't load and site "crashes" after upgrade from 1.5 to 1.6

xurizaemon’s picture

xurizaemon
he/him
Location Ōtepoti, Aotearoa 🏝
commented

Is this ticket the only one blocking a new stable release? IMO some of the other issues already fixed in -dev would justify a new stable release already (depending on expected completion of this issue, of course).

Is there a separate ticket for the FID conversion?

agileware’s picture

agileware commented

subscribing.
A new stable release would be awesome.