Pluggable, injected batch storage.

Files: 
CommentFileSizeAuthor
#18 1998250-followup.patch493 bytesbleen18
FAILED: [[SimpleTest]]: [MySQL] 35,938 pass(es), 875 fail(s), and 443 exception(s).
[ View ]
#11 1998250-10.pluggable-batch-storage.interdiff.txt968 bytesdww
#10 1998250-10.pluggable-batch-storage.patch7.15 KBdww
PASSED: [[SimpleTest]]: [MySQL] 56,241 pass(es).
[ View ]
#9 1998250-9.patch7.34 KBpatrickd
PASSED: [[SimpleTest]]: [MySQL] 55,883 pass(es).
[ View ]
#9 1998250-9-interdiff.txt2.17 KBpatrickd
#7 1998250-7.patch7.42 KBpatrickd
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1998250-7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#7 1998250-7-interdiff.txt1.41 KBpatrickd
#5 1998250-5.patch7.5 KBpatrickd
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#5 1998250-5-interdiff.patch1.81 KBpatrickd
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#3 1998250_3.patch7.08 KBchx
PASSED: [[SimpleTest]]: [MySQL] 57,361 pass(es).
[ View ]
batch.storage.patch7.03 KBchx
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Comments

Status:Active» Needs review

Status:Needs review» Needs work

The last submitted patch, batch.storage.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new7.08 KB
PASSED: [[SimpleTest]]: [MySQL] 57,361 pass(es).
[ View ]

Status:Needs review» Needs work

A)

+      file_put_contents('/tmp/log', print_r($batch, TRUE));

B) Sorry if I'm out of the loop, but:

+    arguments: ['@database', '@request']

vs.

+  public function __construct(Connection $connection, Request $request) {
+    $this->connection = $connection;
+    $this->request = $request;
+  }

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

Status:Needs work» Needs review
StatusFileSize
new1.81 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
new7.5 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

The "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! -.-"

Status:Needs review» Needs work

Excellent, 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. ;)

Status:Needs work» Needs review
StatusFileSize
new1.41 KB
new7.42 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1998250-7.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Oh, Good point!

Status:Needs review» Needs work

Sorry, I suck. I missed this one, too:

D)

+++ b/core/lib/Drupal/Core/Utility/BatchStorageInterface.php
@@ -0,0 +1,53 @@
+<?php
+/**
+ * @file
+ * Contains
+ */

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 for isset($_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';

Status:Needs work» Needs review
StatusFileSize
new2.17 KB
new7.34 KB
PASSED: [[SimpleTest]]: [MySQL] 55,883 pass(es).
[ View ]

You don't! :) Good points!

Category:bug» task
StatusFileSize
new7.15 KB
PASSED: [[SimpleTest]]: [MySQL] 56,241 pass(es).
[ View ]

Now 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...

Forgot to attach this one...

Status:Needs review» Reviewed & tested by the community

dww almost RTBC'd this so I am doing it now that his concerns are addressed.

p.s. In IRC, chx complained about still looking at $_REQUEST instead of Drupal::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.

Title:Move batch to pluggable storageChange notice: Move batch to pluggable storage
Priority:Normal» Critical
Status:Reviewed & tested by the community» Active

Nice... tested the interactive installer... all good.

Committed a2f15f2 and pushed to 8.x. Thanks!

I think this needs a change notice.

Sweet !

system_cron():
-  // Cleanup the batch table and the queue for failed batches.
-  db_delete('batch')
-    ->condition('timestamp', REQUEST_TIME - 864000, '<')
-    ->execute();
+  // Cleanup the queue for failed batches.

Looks like this misses a call to Drupal::service('batch.storage')->cleanup();
(nothing currently calls that method anywhere)

I know this was committed already, but shouldn't this be "Deletes a batch" ?

+++ b/core/lib/Drupal/Core/Utility/BatchStorageInterface.phpundefined
@@ -0,0 +1,53 @@
+  /**
+   * Loads a batch.
+   *
+   * @param int $id
+   *   The ID of the batch to delete.
+   */

Assigned:Unassigned» chx
Priority:Critical» Normal

The 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.

Assigned:chx» Unassigned
Priority:Normal» Critical
Status:Active» Needs review
StatusFileSize
new493 bytes
FAILED: [[SimpleTest]]: [MySQL] 35,938 pass(es), 875 fail(s), and 443 exception(s).
[ View ]

quick patch ....

Assigned:Unassigned» chx
Priority:Critical» Normal
Status:Needs review» Active

A 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.

Status:Active» Needs review

#18: 1998250-followup.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 1998250-followup.patch, failed testing.

Title:Change notice: Move batch to pluggable storageMove batch to pluggable storage
Assigned:chx» Unassigned
Issue summary:View changes
Status:Needs work» Fixed

Change notice has been provided, so marking this fixed.

Documentation followup can be in a new issue.

Status:Fixed» Closed (fixed)

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