Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

Status: Active » Needs review

Status: Needs review » Needs work

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

chx’s picture

Status: Needs work » Needs review
FileSize
7.08 KB
dww’s picture

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

patrickd’s picture

Status: Needs work » Needs review
FileSize
1.81 KB
7.5 KB

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

dww’s picture

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

patrickd’s picture

Status: Needs work » Needs review
FileSize
1.41 KB
7.42 KB

Oh, Good point!

dww’s picture

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';

patrickd’s picture

Status: Needs work » Needs review
FileSize
2.17 KB
7.34 KB

You don't! :) Good points!

dww’s picture

Category: bug » task
FileSize
7.15 KB

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

dww’s picture

Forgot to attach this one...

chx’s picture

Status: Needs review » Reviewed & tested by the community

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

dww’s picture

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.

alexpott’s picture

Title: Move batch to pluggable storage » Change 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.

yched’s picture

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)

bleen’s picture

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.
+   */
chx’s picture

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.

bleen’s picture

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

quick patch ....

chx’s picture

Assigned: Unassigned » chx
Priority: Critical » Normal
Status: Needs review » Active
jibran’s picture

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.

andyceo’s picture

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.

kim.pepper’s picture

Title: Change notice: Move batch to pluggable storage » Move 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.