Drush rr completes by clearing the Drupal caches. This should not occur, IMO. During deployment you may need to customize the order in which certain tasks occur. Registry rebuild is typically the first task to run, but there are often times that you need drush updb to run prior to any caches being cleared, for instance if schema changes have occurred that would cause the cache clearing to fail with an Exception.

CommentFileSizeAuthor
#8 registry_rebuild-1298422-no-cache-clear-8.patch1.07 KBq0rban
#2 no_cc_all-1298422-1.patch734 bytesapotek
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

apotek’s picture

apotek CreditAttribution: apotek commented

This is INSANE! I was just thinking about this, and someone posted this analysis seconds before I was about to.

Thanks q0rban. A valid point. Functions should generally do one atomic thing--especially when there already are functions available to do the other parts.

ie, its better to write registry rebuild so it can be invoked

drush rr

with an *optional*

drush cc all

if the end-user wants it.

Man oh man.

apotek’s picture

apotek CreditAttribution: apotek commented
Category: feature » bug
Status: Active » Needs review
FileSize
no_cc_all-1298422-1.patch734 bytes

Here's a patch.

rfay’s picture

rfay
Primary language English
Location Palisade, CO, USA
CreditAttribution: rfay commented

drush rr is now a part of a deployment process? I thought it was just a rescue thing.

I'm OK with just telling them they ought to clear the cache instead of doing it for them.

q0rban’s picture

q0rban CreditAttribution: q0rban commented

It's becoming a part of our deployment process, because if any classes get removed or deleted, you can't run drush updb or drush cc all or drush fra. So immediately after a git pull, we run drush rr.

The other things we could do is make it an option on the drush command, or wrap the cache clear in a try/catch, or invoke the cache clear using drush_invoke. Not sure the latter two would work, but the first would be simple enough:

drush rr --no-cache-clear

  if (!drush_get_option('no-cache-clear')) {
    drupal_flush_all_caches();
    drush_log(dt('The caches have been cleared.'), 'success');
  }
  else {
    drush_log(dt('It is recommended you clear the Drupal cache as soon as possible.'), 'warning');
  }

I can work up a patch if you let me know which you prefer:

1.) Take it out completely
2.) Make clearing the caches a default operation, but add an option to disable it.
3.) Make NOT clearing the caches default, but add an option TO clear the cache.
4.) Investigate a drush_invoke('cc all') to see if that allows us to isolate the exceptions.
5.) Investigate try/catch.

q0rban’s picture

q0rban CreditAttribution: q0rban commented

Or:

6.) Using drush_confirm, provide the user an option to clear the caches during drush rr.

q0rban’s picture

q0rban CreditAttribution: q0rban commented

The patch in #1299116: Utilize drush API properly solves this issue. I can separate it out into two separate patches, but it is somewhat related, so I threw it in there. I went with option 6.

Also, I just saw apotek's patch in #2. Whoops. That patch looks good. If you prefer that, I will refactor the patch in 1299116.

q0rban’s picture

q0rban CreditAttribution: q0rban commented

apotek said:

I can't use this command in a command sequence if it's going to ask me for input.

What I would like to see is either:
1. Simply skip drush cache clearing and let the user simply run drush cc all. That can be chained together on one line: drush rr; drush cc all; boom!

or

2. Or, make it an option to *include* the cache clear. Again, trying to follow the idea that a function should do one thing, the focus on the function should be to rebuild the registry. Now, if the user really wants to also clear the caches, they can specify that with an option, like --with-cc=all <--that's too fancy.

Apotek and chatted on IRC about this, and while one could do a drush rr -n to use it in a command sequence, it's not very clear what you are saying "no" to. Instead, it would be clearer if there were an option to disable cache clearing. Working up a patch now.

q0rban’s picture

q0rban CreditAttribution: q0rban commented
FileSize
registry_rebuild-1298422-no-cache-clear-8.patch1.07 KB
apotek’s picture

apotek CreditAttribution: apotek commented

That looks good.

I would suggest not prompting for the user at all, ie, let it do it's thing…. especially since there's no option set for bypassing the prompt.

So, in my book, make it simpler, like 

if (!drush_get_option('no-cache-clear') {
   drush_drupal_cache_clear_all();
...

If there is an issue opened where someone needs a prompt, we can consider it then.

q0rban’s picture

q0rban CreditAttribution: q0rban commented
Status: Needs review » Fixed

Thanks for the feedback, apotek. Committed!

http://drupalcode.org/project/registry_rebuild.git/commit/aefe70d

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

(not verified) CreditAttribution: commented
Issue summary: View changes

typo fix