This is something I ran across when documenting drush_set_error for dgd7. This patch makes the following changes:

  1. If a function calls drush_set_error, it should also return FALSE as its result code
  2. Calls to drush_set_error should have a first parameter that is an error code constant, such as 'DRUSH_ARBITRARY_ERROR' that other scripts could check for
  3. drush_log(..., 'warning') is better than drush_set_error in cases where the error is not fatal (e.g. pm-enable can still enable other modules in the argument list if one of the specified modules does not exist)

There are more of these to do, but I thought I'd seek feedback before going through them. There is some question as to how valuable these changes are, since things essentially work without them.

#1 used to be important, and would cause inconsistent behavior with rollbacks if not followed, or so I seem to remember -- it does not seem to be an issue any longer.

#2 does not really hurt anything, as drush will log the first param as the default behavior (so this is wrong yet innocuous behavior)

#3 may affect some callers -- I'm taking away error codes that someone might be testing for. It seems unlikely that anyone would be testing for these 'partial error' conditions, so I think that the warning is better than the set error. set error will trigger a rollback, which isn't supported for the commands where this is being done, but it still seems undesirable.

Let me know which of these changes are desired, and I'll do the rest (or not, as applicable).

CommentFileSizeAuthor
drush-set-error.patch9.15 KBgreg.1.anderson

Comments

moshe weitzman’s picture

Status: Needs review » Reviewed & tested by the community

These all look good and proper to me. I'm not too bothered when we get it wrong, but if someone takes the time to fix them up, I'll gratefully accept.

greg.1.anderson’s picture

Committed these; perhaps I'll do more later.

greg.1.anderson’s picture

Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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