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.
Looking at the variables table and function nodeblock_node_insert(), I gather that nodeblock sets a variable for every single node that is created. Is that really necessary? I'd say that you only need those variables for node types that are nodeblock-enabled. I have not run performance tests, but for large sites with many nodes, I guess this could cause problems with speed and memory usage.
Comments
Comment #1
nevets CreditAttribution: nevets commentedI would suggest custom table over using variable_set() in that case.
Comment #2
michaellander CreditAttribution: michaellander commentedIt's not supposed to be doing that, this is a bug. It is supposed to be creating one for each node where nodeblock IS enabled. I'll look into moving it into a database after I fix the bug.
Comment #3
marcvangendmikegfx, thanks for the quick reply. I wasn't sure this is a bug, but now I'm marking the issue as such.
When you fix the bug, please include a db update function that purges all unnecessary variables. If you need my help testing of reviewing, please let me know.
Comment #4
michaellander CreditAttribution: michaellander commentedThere is a fix for now in dev. Currently it will clean it's self up as nodes of content types that don't have nodeblock enabled but do have variables incorrectly set will remove the variables when the node is updated. I also tweaked the variable removal code to clean itself a bit more thoroughly.
I'll do a db_update function to purge the variables when I make an official release.
Just a note, I'll look into moving the settings into a DB table shortly.
Comment #5
michaellander CreditAttribution: michaellander commentedComment #6
marcvangendThanks, you're fast! I'll test this next monday, when I'm back at work.
Comment #7
marcvangendThe fix works as advertised:
- When re-saving a node that has a nodeblock_block_[nid] variable, the variable is cleaned up if the content type is not nodeblock-enabled.
- When creating a new node, no variable is added if the content type is not nodeblock-enabled.
I think this is RTBC.
Comment #8
zambrey CreditAttribution: zambrey commentedWow, I was just looking at variable table and some big WTF came into my mind :)
Now it works but some cleaning is still required here.
Comment #9
michaellander CreditAttribution: michaellander commentedYeah, sorry about that zambrey. It was a dumb bug on my part. I just committed another build on the dev branch that includes an nodeblock.install file that should go through and clean up the crap when you run updates. Please test in a dev environement so we can mark this RTBC and push live. I'll worry about moving it all into a table on a later version as I think it's more important that this bug be fixed first.
Comment #10
michaellander CreditAttribution: michaellander commentedComment #11
michaellander CreditAttribution: michaellander commentedI've tested this on a site with 5000 nodes, 2/3'rds of which were bad entries and it was fine. Would like at least one more person to test and confirm. If it checks out I'll make a release.
Comment #12
zambrey CreditAttribution: zambrey commentedThanks for fixing this. After update all false variables are gone.
I looked into update code a bit and I have one small question.
I don't see any view_mode_node_block pattern on my variables and my $content_types var during update is as follows:
Is this if statement valid? Shouldn't I have only baner & intro entries in $content_types variable?
Comment #13
michaellander CreditAttribution: michaellander commentedYou're correct. I had some old vars in my dev environment that caused confusion. Just commited with those switched to:
else if(strpos($name, 'nodeblock_comment_link_') === false && strpos($name, 'nodeblock_node_link_') === false && strpos($name, 'nodeblock_view_mode_') === false) {
Thank you!
Comment #14
marcvangendI tested the dev version with the change from #13 applied manually. Looks good to me! Existing nodeblocks still work, I didn't see any errors or notices and most importantly the number of 'nodeblock_block_xxx' variables in my database went from 810 to 5. Marking as RTBC.
Comment #15
zambrey CreditAttribution: zambrey commentedAlright, it's looking good now. Thanks.
+1 to publish new release with this fix.
Comment #16
michaellander CreditAttribution: michaellander commented