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

nevets’s picture

I would suggest custom table over using variable_set() in that case.

michaellander’s picture

It'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.

marcvangend’s picture

Category: feature » bug

mikegfx, 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.

michaellander’s picture

Status: Needs review » Active

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

michaellander’s picture

Status: Active » Needs review
marcvangend’s picture

Status: Active » Needs review

Thanks, you're fast! I'll test this next monday, when I'm back at work.

marcvangend’s picture

Status: Needs review » Reviewed & tested by the community

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

zambrey’s picture

Status: Reviewed & tested by the community » Needs work

Wow, 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.

michaellander’s picture

Yeah, 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.

michaellander’s picture

Status: Needs work » Needs review
michaellander’s picture

I'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.

zambrey’s picture

Thanks for fixing this. After update all false variables are gone.

I looked into update code a bit and I have one small question.

else if(strpos($name, 'link_node_block') === false && strpos($name, 'view_mode_node_block') === false){

I don't see any view_mode_node_block pattern on my variables and my $content_types var during update is as follows:

Array
(
    [0] => baner
    [1] => intro
    [2] => view_mode_album
    [3] => view_mode_baner
    [4] => view_mode_frontpage_teaser
    [5] => view_mode_gallery
    [6] => view_mode_intro
    [7] => view_mode_news
    [8] => view_mode_page
    [9] => view_mode_video
    [10] => view_mode_webform
)

Is this if statement valid? Shouldn't I have only baner & intro entries in $content_types variable?

michaellander’s picture

You'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!

marcvangend’s picture

Status: Needs review » Reviewed & tested by the community

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

zambrey’s picture

Alright, it's looking good now. Thanks.
+1 to publish new release with this fix.

michaellander’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.