Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

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.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
3.12 KB
4.19 KB
dawehner’s picture

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.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
2.44 KB
4.35 KB

This should fix that.

dawehner’s picture

+++ 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?

ParisLiakos’s picture

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

ParisLiakos’s picture

damiankloip’s picture

Status: Needs work » Needs review
FileSize
2 KB
4.6 KB

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?

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

I am fine with doing that on a followup.

alexpott’s picture

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.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
740 bytes
4.71 KB

There we go.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Nice!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed f7a4182 and pushed to 8.x. Thanks!

kim.pepper’s picture

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?

ParisLiakos’s picture

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

kim.pepper’s picture

damiankloip’s picture

Seems like a good idea to me!

Status: Fixed » Closed (fixed)

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

Anonymous’s picture

Issue summary: View changes

added followup