While writing hooks for my module to use mail_edit, I spelt a key incorrectly. When I noticed, and corrected the error, the old entry remained in the list of mails.
On inspection of the code, it looks like what's going on is that _mail_edit_key_registry_rebuild() will go and check for new keys, but it just adds them to the db without clearing out the old keys for modules it gets a response from. It might even make sense to delete all from the mail_edit_registry at the start of this function, as it'll get all the data it needs back anyway.
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | mail_edit-633982-4.patch | 1.18 KB | hinikato |
| #3 | mail_edit-633982-3.patch | 1.13 KB | hinikato |
| #2 | mail_edit-633982-2.patch | 532 bytes | hinikato |
Comments
Comment #1
litwol commentedThanks for finding that bug! Please provide a patch, i'll review and commit it. i've not worked on mail_edit in a while and it would take too much out of my time to figure things out again. thanks!
Comment #2
hinikato commentedPlease see the attached patch for fixing this bug.
This patch for 6.x-1.x-dev version of the mail_edit module.
Comment #3
hinikato commentedI provide a new patch that fixes updating descriptions for keys.
Comment #4
hinikato commentedI have been fixed the deletion of the relation tables for the previous patch and provide the new one (needs works).
Comment #5
salvisI'm not sure that deleting data for non-responding modules is the way to go. Disabling a module is not uninstalling it, and trashing its data because it doesn't answer at the moment seems to be too drastic. Maybe just drop keys for modules that do answer but don't provide that key anymore?
While the registry implementation is nicely done, I'm not real sure whether it's worth the trouble at all. Is it used for anything besides building the admin/build/mail-edit page? That page is rarely accessed, and it would be simpler to just retrieve the keys when that page is built.
@hinikato: Please don't drop the commented out tracing line — this comes in handy at times.
Comment #6
YK85 commentedsubscribing - it would be nice for mail templates of uninstalled modules to be removed. Great module! Thanks
Comment #7
YK85 commentedThe below warning shows when clicking into a mail template that no longer is being used (module is uninstalled).
Comment #8
salvis@yaz085: Is that with or without the patch in #4?
Comment #9
YK85 commentedHi salvis,
That warning was without the patch in #4.
I was afraid to test the patch without a newer version of the patch as the status was set to 'needs work'.
Comment #10
salvisThanks, yaz085.
@litwol: Does this caching of the mailkeys really make sense? Conceptually, it's nice, but why don't we just gather the keys when we need them to display the overview page? I don't think this would cause a strain on the server, do you?
Comment #11
litwol commentedI haven't used the module in a while so i cannot adequately evaluate it now. i'm leaving that decision up to you.
Comment #12
chuckbar77 commentedsubscribing
Comment #13
robby.smith commented+1 subscribing
Comment #14
chuckbar77 commentedI was wondering if there may have been any development in this feature?
Thank you
Comment #15
salvisNo. First priority is the port to D7, which will probably remove the caching. This could then be back-ported to D6.
The patch in #4 is too drastic (see #5).
Comment #16
chuckbar77 commentedThank you for the quick reply!
Great module =) I look forward to following your work.
Comment #17
YK85 commentedHi salvis,
I was wondering if you may have had time to work on D7 and any backports to D6 that I could help with testing?
Appreciate all your hard work and hope to contribute to further development of this very useful module.
Best
Comment #18
salvisI've been working on Subscriptions for D7, which needs Mail Editor, so Mail Editor is certainly in the queue, but we're not quite there yet.
I'll certainly appreciate your testing as soon as I have something to show.