Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.The first lines of imagecache_textactions.module and imagecache_textrender.module include some seriously crazy code...
imagecache_textactions.module:
cache_clear_all('imagecache_actions', 'cache');
require_once(dirname(__FILE__) .'/utility.inc'); // For simple color routines
imagecache_textrender.module:
cache_clear_all('imagecache_actions', 'cache');
// For simple color routines
module_load_include('inc', 'imagecache_textrender', 'utility');
This is actually close to being retarded!
| Comment | File | Size | Author |
|---|---|---|---|
| #6 | imagecache_actions-1266864-#6.patch | 408 bytes | jonhattan |
Comments
Comment #1
NancyDruFirst, there is no need to insult the maintainer. Second "critical" means "site down;" since this is all optional, it is not "critical."
Comment #2
dman CreditAttribution: dman commented:-/
In what way did the original code fail to work for you? Do you fail to see what it's doing?
In Drupal4 and Drupal5 - where this modules history lies - you'll find a hundred examples of similar working code. Module_load_include() became available as an alternative comparatively recently. In fact, many modules naively used to include(sites/default/modules/modulename/lib.inc) and this method was - at the time - a vast improvement.
It is not a bad idea to replace that line with updated code, and that can be done in the normal course of events ... but as it stands it's not actually broken in any way I can see.
Comment #3
dman CreditAttribution: dman commentedOK, I get it now, you are just trolling.
The files you mention haven't even been a part of 6.x-2.x-dev since the upgrade...
At best, you pulled up some old code from somewhere and found something that - though not wrong - looked outdated.
There is nothing to fix, as all this years code seems be using module_load_include where it should be.
Comment #4
erlendstromsvik CreditAttribution: erlendstromsvik commentedJust for reference, it is/was the cache_clear_all in global context which is/was embarrasing.
I just now grabbed the 6.x version and code is still there, so it is "is" and not "was".
As for the comment from NancyDru, at the moment of enabling that site went *BOOOM*.
Edit:
So no trolling, just exasperated because working on high performance sites makes downloading modules from Drupal a real painful lottery.
Comment #5
OneExtra CreditAttribution: OneExtra commentedYou're right - it's really not reasonable to do a cache_clear_all on each page load. I'll deactivate the module on my site.
Comment #6
jonhattanStill a major problem.
When imagecache_textactions is enabled, each page load performs a
cache_clear_all('imagecache_actions', 'cache');. See http://cgit.drupalcode.org/imagecache_actions/tree/imagecache_textaction...This seems to me a debug line, introduced in 8d32af57.
About performance: this line is directly consuming 12ms and an increase of 0.75MB in the memory usage peak. Indirectly it produces more harm because this cache is always empty (worst than not using a cache at all).
Comment #7
dman CreditAttribution: dman commentedimagecache textactions was experimental only and terribly unstable in 6.x .
Lots of work on it stopped around the time people started using CSS3 instead.
But yeah, that is just a debug, probably left in there from just before it was ripped out entirely because it just didn't work well enough
Seriously, texteffects were pretty crappy :=}
Comment #8
jonhattan@dman I'm doing a performance review to a big D6 site. Just found this bug and posted the patch. I won't discuss crappiness of texteffects :D
I've directed the dev team to apply the patch and moved onto the next task. Whatever you do is fine to me :)
Comment #9
dman CreditAttribution: dman commentedYeah we should fix it up - clearly an oversight.
I was just surprised to see someone using 6.x-1.x-dev (and texteffects) today
Comment #11
dman CreditAttribution: dman commented... hadn't touched that branch for three years.
Comment #12
dman CreditAttribution: dman commentedI also see that the reason the original issue was closed was because it had been reported against 6.x-2.x - where the problem no longer existed, and therefore could not be found or fixed.
The actual issue identified now was in the another branch - 6.x-1.x-dev - (and not even the 6.x-1.?-stable) so it could not be found back then.