Hi

While I brower the code of flippy, for the code in flippy_uninstall, I think we can improve it a little bit.

ref: http://drupalcode.org/project/boost.git/blob/refs/heads/7.x-1.x:/boost.i...

This looks like more efficient.

 function boost_uninstall() {
     // Clear variables
     $name = 'boost_';
     db_delete('variable')
       ->condition('name', db_like($name) . '%', 'LIKE')
       ->execute();
     cache_clear_all('variables', 'cache_bootstrap');
   }

I make a patch here, and hope it can help.

Thanks for your attention.

Any questions, feel free to let me know. thank you.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

zterry95’s picture

Status: Active » Needs review
zterry95’s picture

small problem with last patch, pls use this one.

Already test it and works well.

rli’s picture

Good point zterry95, will review and commit it soon.

thanks.

rli’s picture

Status: Needs review » Needs work

Hi zterry95,

Just had one question about your patch. By comparing your patch and the original code, I see you did not include unset($conf[$name]); in your patch. But I thought it was necessary to unset this variable key as this variable will loaded in each page request.

I am going to change the status to need work now, but please feel free to post any ideas here.

Thanks.

zterry95’s picture

Hi rli

Thanks for your reminder.

You are right, we need to clean cache of bootstrap.

This has been emplemented in variable_del
http://api.drupal.org/api/drupal/includes%21bootstrap.inc/function/varia...

So I improve my patch.

Hope this time, it can be passed:)

zterry95’s picture

Status: Needs work » Needs review
rli’s picture

Hi zterry95,

I have been thinking about your patch.

If you have compared your patch and variable_del function, you would find out variable_del is unsetting the $conf['name'] which can not be done in this patch if you don't have the variable name.

To clean everything up in the uninstall, I think we should clean $conf['variable name'] too. Any ideas?

zterry95’s picture

considering your comments, I do 2 things just now.

1: tracking the code

2: test myself

The results it that it not necessary to unset $conf['variable name'].

the reason is that, after we implement

db_delete('variable')....;
cache_clear_all('variables', 'cache_bootstrap');

the variable is still on the module uninstall page. but if we redirect to other page or refresh page, the variable(flippy_*) will be unset from $conf automaticly.

and after we uninstall a module, then redirect to another page or refresh, it is a naturally operation. then the variable(flippy_*) doesn't stay in $conf any more.

So I think unset($conf[variable name]) is not necessary and my last patch will work pretty well.

Feel free if you have any questions on my answers. I will be glad to explain.

rli’s picture

yeah, understood and agree with you.

just considered variable_del does include the unset for a reason. but cannot think of any examples now.

will commit to dev for now.

rli’s picture

yeah, understood and agree with you.

just considered variable_del does include the unset for a reason. but cannot think of any examples now.

will commit to dev for now.

zterry95’s picture

nice.
cheers!

rli’s picture

Status: Needs review » Closed (fixed)
zterry95’s picture

glad to see this patch has been applied.
but a little dispointed that, this patch is not authored by me:(

You should add below parameter when you commit this.
git commit --author="zterry95 <zterry95@1952394.no-reply.drupal.org>"

Then this should be my commit.

Thanks for your attention.