After #1058874: drush_backend_invoke buffers output till end of command. Show progress. any batch process could show progress in real time but we lack the feature in batch api to raise messages as they come in. In fact, batch operations' progress message are based in:

$context['message'] = "Doing 1 of 3."

A proper change would be to improve drupal by changing this array based storage to something that allow drush (or watchdog, or ...) to hook and get the messages in real time.

It seems we have some oportunity to catch this in drush http://stackoverflow.com/questions/787692/operator-overloading-in-php

Comments

jonhattan’s picture

Status: Active » Needs review
FileSize
1.2 KB

That said, we need this to avoid patches like the one in #1002658-73: Drush does not correctly handle D7 project status queries that duplicates a lot of code to just insert a call to drush_log().

Attached patch seems to do the work but surely removing those references (&) is not good.

jonhattan’s picture

FileSize
1.79 KB

here's a fully working solution :)

greg.1.anderson’s picture

I have mixed feelings about overriding ArrayObject like this. Can you explain / comment where and how the offsetSet function is called? I'm guessing that the Drupal batch code inserts $batch_context['message'] = ..., and you catch that. I guess this is a good way to patch in, but it could use some additional comments.

jonhattan’s picture

yes it works this way. offsetSet() is called each time a value is assigned.

greg.1.anderson’s picture

Status: Needs review » Needs work

How about some comments on how this all plugs together?

jonhattan’s picture

Status: Needs work » Needs review
FileSize
6.97 KB

I've added some more documentation and the basis for testing it.

To allow testing it is needed to pass --include=/usr/src/drush/tests to the backend invokation.

I've done an ad hoc implementation of this for the batch api. It could be generalized by doing the same in _drush_backend_invoke(). Summary of changes:

1.

-function _drush_backend_batch_process($command = 'batch-process', $args) {
+function _drush_backend_batch_process($command = 'batch-process', $args, $options) {

2. Refactor code from _drush_backend_batch_process() to its caller drush_backend_batch_process(). It also fixes passing -u 1 correctly.

Back to batchTest.php: I've only written the skeleton. Now it needs to "do something" in the batch operations. Also I've duplicated code from backendTest.php to parse the output from backend.

Still needs work but feedback is very welcome.

jonhattan’s picture

FileSize
8.36 KB

batchTest.php is missing in above patch.

moshe weitzman’s picture

Assigned: Unassigned » greg.1.anderson
Cyberwolf’s picture

Subscribing.

pcambra’s picture

Title: Make the batch api benefit of backend show progress automagically. » Make the batch api benefit of backend show progress automatically.

suscribe

jonhattan’s picture

Assigned: greg.1.anderson » jonhattan
Status: Needs review » Needs work
jonhattan’s picture

FileSize
11.55 KB

Same patch over current master.

moshe weitzman’s picture

Assigned: jonhattan » greg.1.anderson
Status: Needs work » Needs review
greg.1.anderson’s picture

Status: Needs review » Needs work

The code looks really good, but the batch test case fails for some reason:

2) batchCase::testBatch
Unexpected exit code: /bin/drush archive-dump '@sites' --destination=/tmp/drush-cache/environments/dev-install-7.x-testing-mysql.tar.gz --root=/tmp/drush-sandbox/web --uri= --overwrite --nocolor
Failed asserting that <integer:1> matches expected <integer:0>
moshe weitzman’s picture

Perhaps run phpunit with --verbose and see if you get more info.

Would be nice to get this in.

greg.1.anderson’s picture

Problem with #14 was that the test was not written yet. Wrote a test that passes. :)

Also enhanced backend invoke so that --include is passed through per the drush backend invoke context propagation mechanism, rather than special-casing it.

Unfortunately, this code (both the patch here and the original patch in #12) is now failing with the site-upgrade test. I don't believe this was the case as recently as 12 August -- I suspect something changed in drush or drupal.

@jonhattan, could you look at this?

jonhattan’s picture

FileSize
18.46 KB

master changed. rerolled

greg.1.anderson’s picture

There was a mistake in my patch:

+      if (TRUE || !array_key_exists('local-context-only', $global_metadata) || !array_key_exists('remot-host', $site_record)) {

That "TRUE ||" should not be there.

jonhattan’s picture

FileSize
19.25 KB

Efectively there's a problem with updatedb. I haven't found the source of the problem.

Consider http://api.drupal.org/api/drupal/modules--user--user.install/function/us...

Drush enters the function and enlength password field to 128 but it doesn't re-enter again to perform password hashes. Hopefully we have an assert for that within siteUpgradeTest.

Attached #17 with #18 correction.

greg.1.anderson’s picture

The user_update_7000 function you reference above is using '#finished' to indicate that the batch process is or is not done, whereas _drush_unit_batch_operation sets 'finished'. Is this perhaps a difference between d6 and d7? I did not analyze to see if changing all 'finished' to '#finished' would fix this code for site-upgrade.

greg.1.anderson’s picture

... although I will add that it still does not make sense to me, as the handling of 'finished' should be invariant on the use of DrushBatchContext. However, in an earlier test I did, commenting out the use of DrushBatchContext did make the site-upgrade command work again. Did not re-test today to see if that's still the case, though.

jonhattan’s picture

I forget to say that siteUpgradeTest passes clean without this patch.

#finished is a valid key. Per d7's include/update.inc:

 * If an update function needs to be re-run as part of a batch process, it
 * should accept the $sandbox array by reference as its first parameter
 * and set the #finished property to the percentage completed that it is, as a
 * fraction of 1.

sadly the problem dissapear by simply commenting out this line
$batch_context = new DrushBatchContext($batch_context);

greg.1.anderson’s picture

Yes, your experience in #22 now matches what I was seeing yesterday. Hope this technique can be made viable.

jonhattan’s picture

Status: Needs work » Needs review
FileSize
21.69 KB

somehow fixed...

in drush_batch_worker():

      call_user_func_array($function, array_merge($args, array(&$batch_context)));
      $finished = $batch_context['finished'];
      if ($finished >= 1) {

added the 2nd line... problem is that ArrayObject doesn't play well with reference variables (see #1 and #2).
---
in the 3rd line I've changed == to >1 as in drupal7.

Also did this change, to get a better message during the update.

-  $context['message'] = 'Updating ' . check_plain($module) . ' module';
+  $context['message'] = 'Performing ' . $function;
greg.1.anderson’s picture

Status: Needs review » Reviewed & tested by the community

That is eerie. I would not have guessed that the object would have survived the array_merge like that -- but it works flawlessly. Good job.

moshe weitzman’s picture

Looks very nice. Feel free to commit after reviewing items below ...

  1. Running updatedb on D6 with no pending updates gives me Undefined variable: return command.inc:170 . Perhaps not introduced by this patch
  2. I think we prefer drush_set_error() over drush_log($value, 'error');
  3. I see some markup in the new log messages. We should strip_tags(), I think. From the site-upgrade test: Checked available update data for <em class="placeholder">Field</em>.

Are there other parts of drush that should better use 'backend show progress'?

jonhattan’s picture

Status: Reviewed & tested by the community » Fixed

As far as this patch allow drush to log progress of any batch set automatically. There's no need to adjust any batch.

Committed along with 1 and 2 from #25. Will open other issue for item 3.

Status: Fixed » Closed (fixed)

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