Files: 
CommentFileSizeAuthor
#12 2049085-12.patch4.71 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,474 pass(es).
[ View ]
#12 interdiff-2049085-12.txt740 bytesdamiankloip
#9 2049085-7.patch4.6 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,316 pass(es).
[ View ]
#9 interdiff-2049085-7.txt2 KBdamiankloip
#5 2049085-5.patch4.35 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 58,066 pass(es).
[ View ]
#5 interdiff-2049085-5.txt2.44 KBdamiankloip
#2 2049085-2.patch4.19 KBdamiankloip
FAILED: [[SimpleTest]]: [MySQL] 57,517 pass(es), 2 fail(s), and 7 exception(s).
[ View ]
#2 interdiff-2049085-2.txt3.12 KBdamiankloip
AggregatorFeedBlock-injection.patch3.33 KBdamiankloip
PASSED: [[SimpleTest]]: [MySQL] 57,477 pass(es).
[ View ]

Comments

Status:Needs review» Needs work

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Derivative/AggregatorFeedBlock.phpundefined
@@ -24,13 +26,43 @@ class AggregatorFeedBlock implements DerivativeInterface {
     $this->derivatives[$derivative_id]['admin_label'] = t('@title feed latest items', array('@title' => $result->title));

Let's also inject the translator.

Status:Needs work» Needs review
StatusFileSize
new3.12 KB
new4.19 KB
FAILED: [[SimpleTest]]: [MySQL] 57,517 pass(es), 2 fail(s), and 7 exception(s).
[ View ]

Status:Needs review» Reviewed & tested by the community

Thank you!

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 2049085-2.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.44 KB
new4.35 KB
PASSED: [[SimpleTest]]: [MySQL] 58,066 pass(es).
[ View ]

This should fix that.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Derivative/AggregatorFeedBlock.php
@@ -24,29 +27,70 @@ class AggregatorFeedBlock implements DerivativeInterface {
+  /**
+   * {@inheritdoc}
    */
   public function getDerivativeDefinition($derivative_id, array $base_plugin_definition) {
     if (!empty($this->derivatives) && !empty($this->derivatives[$derivative_id])) {
       return $this->derivatives[$derivative_id];
     }
-    $result = db_query('SELECT fid, title, block FROM {aggregator_feed} WHERE block <> 0 AND fid = :fid', array(':fid' => $derivative_id))->fetchObject();
+    $result = $this->connection->query('SELECT fid, title, block FROM {aggregator_feed} WHERE block <> 0 AND fid = :fid', array(':fid' => $derivative_id))->fetchObject();
     $this->derivatives[$derivative_id] = $base_plugin_definition;
     $this->derivatives[$derivative_id]['delta'] = $result->fid;
-    $this->derivatives[$derivative_id]['admin_label'] = t('@title feed latest items', array('@title' => $result->title));
+    $this->derivatives[$derivative_id]['admin_label'] = $this->translationManager->translate('@title feed latest items', array('@title' => $result->title));
     return $this->derivatives[$derivative_id];
   }
   /**
-   * Implements \Drupal\Component\Plugin\Derivative\DerivativeInterface::getDerivativeDefinitions().
+   * {@inheritdoc}
    */
   public function getDerivativeDefinitions(array $base_plugin_definition) {
     // Add a block plugin definition for each feed.
-    $result = db_query('SELECT fid, title FROM {aggregator_feed} WHERE block <> 0 ORDER BY fid');
+    $result = $this->connection->query('SELECT fid, title FROM {aggregator_feed} WHERE block <> 0 ORDER BY fid');
     foreach ($result as $feed) {
       $this->derivatives[$feed->fid] = $base_plugin_definition;
       $this->derivatives[$feed->fid]['delta'] = $feed->fid;
-      $this->derivatives[$feed->fid]['admin_label'] = t('@title feed latest items', array('@title' => $feed->title));
+      $this->derivatives[$feed->fid]['admin_label'] = $this->translationManager->translate('@title feed latest items', array('@title' => $feed->title));
     }
     return $this->derivatives;
   }

Is there a good reason why we don't use the normal single calls multiple thing as in most other derivative classes?

Status:Needs review» Needs work

$this->translationManager->translate() should be $this->t() or the string wont be picked up for parsing.
see #2019679: Support for Drupal 8 $this->t()

Maybe we should add a DerivativeBase class for this, similar to FormBase and ControllerBase, but here at least, we should just add a protected t() method and use this instead

Status:Needs work» Needs review
StatusFileSize
new2 KB
new4.6 KB
PASSED: [[SimpleTest]]: [MySQL] 58,316 pass(es).
[ View ]

I guess we will have to at some point. This whole base class thing is getting crazy.

@dawehner: It's a good question, and I guess it would pretty much load all of them at one point anyway, then cache the definitions? I think we should make a follow up to change that though?

Status:Needs review» Reviewed & tested by the community

I am fine with doing that on a followup.

Status:Reviewed & tested by the community» Needs work

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Derivative/AggregatorFeedBlock.php
@@ -24,31 +27,81 @@ class AggregatorFeedBlock implements DerivativeInterface {
+    $this->basePluginId = $base_plugin_id;

basePluginId should be declared as a protected property on the class.

Status:Needs work» Needs review
StatusFileSize
new740 bytes
new4.71 KB
PASSED: [[SimpleTest]]: [MySQL] 58,474 pass(es).
[ View ]

There we go.

Status:Needs review» Reviewed & tested by the community

Nice!

Status:Reviewed & tested by the community» Fixed

Committed f7a4182 and pushed to 8.x. Thanks!

As a followup, would it be worthwhile moving db calls to the FeedStorageController, and removing the db queries and dependency on the database from here?

sure, moving everything to entity storage is always better than raw sql queries

Seems like a good idea to me!

Status:Fixed» Closed (fixed)

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

Issue summary:View changes

added followup