Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Pluggable, injected batch storage.
Comment | File | Size | Author |
---|---|---|---|
#18 | 1998250-followup.patch | 493 bytes | bleen |
#11 | 1998250-10.pluggable-batch-storage.interdiff.txt | 968 bytes | dww |
#10 | 1998250-10.pluggable-batch-storage.patch | 7.15 KB | dww |
#9 | 1998250-9.patch | 7.34 KB | patrickd |
#9 | 1998250-9-interdiff.txt | 2.17 KB | patrickd |
Comments
Comment #1
chx CreditAttribution: chx commentedComment #3
chx CreditAttribution: chx commentedComment #4
dwwA)
B) Sorry if I'm out of the loop, but:
vs.
Why is one thing called a '@database' and the other a Connection ?
C) Wouldn't we normally have PHPDoc to document all the public calls in BatchStorageInterface?
Otherwise, looks great to me.
Thanks!
-Derek
Comment #5
patrickd CreditAttribution: patrickd commentedThe "connection" <> "database" naming is used in some other places too, so we probably should move this discussion outside?
Removed debug code, added actual documentation to be inherited from the interface
Edit: oops, wrong interdiff extension, sorry! -.-"
Comment #6
dwwExcellent, thanks. That's better. Very minor final nit, then this is RTBC to my eyes:
The new PHPdoc on BatchStorageInterface always mentions "... (from|to) the database". I thought the whole point of this issue was not assuming batches are stored in the database. ;)
Comment #7
patrickd CreditAttribution: patrickd commentedOh, Good point!
Comment #8
dwwSorry, I suck. I missed this one, too:
D)
Contains ... ? ;)
And, since I'm trying to be careful this time, a few other questions/nits:
E) Isn't it a bit weird to have the Interface for our nice pluggable / re-usable class know about
$_REQUEST['id']
? There are only a handful of call-sites to load(), and they all seem to check forisset($_REQUEST['id'])
themselves, so it seems bizzarre to then punt that and invoke load() without an arg and rely on magic. Just seems cleaner to demand that load() always gets an id, and callers know where it comes from instead of having this weirdness filter all the way down into the pluggable class.F) Seems like we no longer need this in _system_batch_theme():
require_once __DIR__ . '/../../includes/batch.inc';
Comment #9
patrickd CreditAttribution: patrickd commentedYou don't! :) Good points!
Comment #10
dwwNow that BatchStorage::load() isn't tainted with
REQUEST['id']
magic we don't need a Request at all (neither as a protected member, registered as part of the service, nor passed in the constructor). Attached works fine in local testing (both the installer and checking for available updates). I probably should have had you reroll so I can RTBC, but I'm tired of marking this issue needs work. ;)Also, I fail to see how this issue is a bug, and chx ignored my requests in IRC for an explanation, so I'm moving to a task...
Comment #11
dwwForgot to attach this one...
Comment #12
chx CreditAttribution: chx commenteddww almost RTBC'd this so I am doing it now that his concerns are addressed.
Comment #13
dwwp.s. In IRC, chx complained about still looking at
$_REQUEST
instead ofDrupal::request()->request->get('id')
but I argued that's out of scope for this issue (which is just about pluggable storage for batches) and there's already #1998638: Replace almost all remaining superglobals ($_GET, $_POST, etc.) with Symfony Request object about a real conversion.Comment #14
alexpottNice... tested the interactive installer... all good.
Committed a2f15f2 and pushed to 8.x. Thanks!
I think this needs a change notice.
Comment #15
yched CreditAttribution: yched commentedSweet !
Looks like this misses a call to Drupal::service('batch.storage')->cleanup();
(nothing currently calls that method anywhere)
Comment #16
bleen CreditAttribution: bleen commentedI know this was committed already, but shouldn't this be "Deletes a batch" ?
Comment #17
chx CreditAttribution: chx commentedThe change notice http://drupal.org/node/2005460 but msonnabaum notes it is possible to use the expirable K-V store instead! That should indeed be possible.
Will do that.
Comment #18
bleen CreditAttribution: bleen commentedquick patch ....
Comment #19
chx CreditAttribution: chx commentedComment #20
jibranA before and after will be great on http://drupal.org/node/2005460 :). Please can we move K-V store implementation to new issue. Because then we can have separate change notice for that as well and a sample issue for swapping connection with K-V store.
Comment #21
andyceo CreditAttribution: andyceo commented#18: 1998250-followup.patch queued for re-testing.
Comment #23
kim.pepperChange notice has been provided, so marking this fixed.
Documentation followup can be in a new issue.