Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Comment | File | Size | Author |
---|---|---|---|
#24 | drush-1186480.patch | 21.69 KB | jonhattan |
#19 | drush-1186480.patch | 19.25 KB | jonhattan |
#17 | drush-1186480.patch | 18.46 KB | jonhattan |
#16 | drush-batch-progress-16.patch | 19.61 KB | greg.1.anderson |
#12 | drush-1186480.patch | 11.55 KB | jonhattan |
Comments
Comment #1
jonhattanThat 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.
Comment #2
jonhattanhere's a fully working solution :)
Comment #3
greg.1.anderson CreditAttribution: greg.1.anderson commentedI 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.Comment #4
jonhattanyes it works this way. offsetSet() is called each time a value is assigned.
Comment #5
greg.1.anderson CreditAttribution: greg.1.anderson commentedHow about some comments on how this all plugs together?
Comment #6
jonhattanI'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.
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.
Comment #7
jonhattanbatchTest.php is missing in above patch.
Comment #8
moshe weitzman CreditAttribution: moshe weitzman commentedComment #9
Cyberwolf CreditAttribution: Cyberwolf commentedSubscribing.
Comment #10
pcambrasuscribe
Comment #11
jonhattanComment #12
jonhattanSame patch over current master.
Comment #13
moshe weitzman CreditAttribution: moshe weitzman commentedComment #14
greg.1.anderson CreditAttribution: greg.1.anderson commentedThe code looks really good, but the batch test case fails for some reason:
Comment #15
moshe weitzman CreditAttribution: moshe weitzman commentedPerhaps run phpunit with --verbose and see if you get more info.
Would be nice to get this in.
Comment #16
greg.1.anderson CreditAttribution: greg.1.anderson commentedProblem 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?
Comment #17
jonhattanmaster changed. rerolled
Comment #18
greg.1.anderson CreditAttribution: greg.1.anderson commentedThere was a mistake in my patch:
That "TRUE ||" should not be there.
Comment #19
jonhattanEfectively 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.
Comment #20
greg.1.anderson CreditAttribution: greg.1.anderson commentedThe 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.
Comment #21
greg.1.anderson CreditAttribution: greg.1.anderson commented... 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.
Comment #22
jonhattanI forget to say that siteUpgradeTest passes clean without this patch.
#finished is a valid key. Per d7's
include/update.inc
:sadly the problem dissapear by simply commenting out this line
$batch_context = new DrushBatchContext($batch_context);
Comment #23
greg.1.anderson CreditAttribution: greg.1.anderson commentedYes, your experience in #22 now matches what I was seeing yesterday. Hope this technique can be made viable.
Comment #24
jonhattansomehow fixed...
in drush_batch_worker():
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.
Comment #25
greg.1.anderson CreditAttribution: greg.1.anderson commentedThat is eerie. I would not have guessed that the object would have survived the array_merge like that -- but it works flawlessly. Good job.
Comment #26
moshe weitzman CreditAttribution: moshe weitzman commentedLooks very nice. Feel free to commit after reviewing items below ...
Undefined variable: return command.inc:170
. Perhaps not introduced by this patchChecked available update data for <em class="placeholder">Field</em>.
Are there other parts of drush that should better use 'backend show progress'?
Comment #27
jonhattanAs 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.