Aggregator views integration

FYI, #293318: Convert Aggregator feeds into entities converts feed items to EntityNG, doesn't use a property table yet, however. so this shouldn't conflict much...

Another issue which is blocked on #1783196: Get the views plugin manager from the DIC when possible (because you simple can't add a new view)

The diff in #4 got in somewhere else.

Rerolled, updated all the codestyles, fixed a lot of the code.

Fix these errors: Init method has been wrong.

#14: drupal-1821844-14.patch queued for re-testing.

i think something is missing, that could allow us to replace admin/config/services/aggregator in the future

That's a good idea, did you reviewed the patch?

Nope, i didnt review it, i was just playing with views:P
I took a closer look now

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/views/argument/Fid.phpundefined
@@ -0,0 +1,42 @@
+    $result = db_query("SELECT f.title FROM {aggregator_feed} f WHERE f.fid IN (:fids)", array(':fids' => $this->value));
+    $query = db_select('aggregator_feed', 'f');
+    $query->addField('f', 'title');
+    $query->condition('f.fid', $this->value);
+    $result = $query->execute();

I guess the first line needs to be removed..we could use entity_load_multiple but dunno, if it is worth it.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/views/argument/Iid.phpundefined
@@ -0,0 +1,42 @@
+    $placeholders = implode(', ', array_fill(0, sizeof($this->value), '%d'));

I dont see placeholders variable used after this..i guess one more leftover

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/views/row/Rss.phpundefined
@@ -0,0 +1,112 @@
+    $query->addExpression('ai.link', 'feed_LINK');

i dont get why link is uppercase

I don't know about the feed_LINK part but this has been part of the aggregator integration since views2.

All i can complain about is

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/views/argument/Fid.phpundefined
@@ -0,0 +1,38 @@
+      $titles[] = check_plain($feed->title);

should be ->label()

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/views/argument/Iid.phpundefined
@@ -0,0 +1,38 @@
+      $titles[] = check_plain($feed->title);

same

There we go.

do we need tests here?

#22: drupal-1821844-22.patch queued for re-testing.

Working on writing tests and fixing some minor issues.

Started to write some tests and fix some of the handlers.

Would be great to get #1956220: Allow to use xpath on drupal unit tests/unit tests so we can test the feed output, on the DUTB.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/ItemStorageController.phpundefined
@@ -25,7 +25,9 @@ public function create(array $values) {
     // Set an initial timestamp, this will be overwritten if known.
-    $entity->timestamp->value = REQUEST_TIME;
+    if (!isset($entity->timestamp->value)) {
+      $entity->timestamp->value = REQUEST_TIME;
+    }

Maybe remove the comment too?

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/views/row/Rss.phpundefined
@@ -0,0 +1,113 @@
+    $query->addExpression('ai.link', 'feed_LINK');

Lets make this lowercase here, it is ugly as hell and i am pretty sure it wont break anything

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/Views/IntegrationTest.phpundefined
@@ -0,0 +1,131 @@
+    debug($view->preview());

oops

Thanks for the review, here is an interdiff.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/views/row/Rss.phpundefined
@@ -0,0 +1,110 @@
+    $query = db_select('aggregator_item', 'ai');
+    $query->fields('ai');
+    $query->condition('iid', $iid);
+    $item = $query->execute()->fetchObject();

i see you removed the extra fields, so i guess if they are not needed, lets load the item using the entity storage?items are entities:)

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/Views/IntegrationTest.phpundefined
@@ -0,0 +1,131 @@
+  /**
+   * Tests basic aggregator_item view.
+   */
+  public function testAggregatorFeedView() {
+
+  }

Fill it or remove it:) also wrong docblock

There we go.

/me sighs about myself.

So this time I converted the rss.php to load the entity.

sorry, i just noticed them

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/views/row/Rss.phpundefined
@@ -0,0 +1,111 @@
+        'attributes' => array('isPermaLink' => 'false')

missing comma

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/views/row/Rss.phpundefined
@@ -0,0 +1,111 @@
+      'row' => $item

comma as well

Thanks for the review. Fixed the exception as well.

Oh wow, I would have not exception that this patch is so big.

this looks awesome now:) thanks
RTBC!

#37: drupal-1821844-37.patch queued for re-testing.

+++ b/core/modules/aggregator/aggregator.views.inc
@@ -0,0 +1,343 @@
+  $data['aggregator_item']['table']['group']  = t('Aggregator');

One extra space before =.

+++ b/core/modules/aggregator/aggregator.views.inc
@@ -0,0 +1,343 @@
+      'click sortable' => TRUE,
+     ),
+    'argument' => array(

Another extra space here and in a dozen similar places.

+++ b/core/modules/aggregator/aggregator.views.inc
@@ -0,0 +1,343 @@
+    'argument' => array(
+      'id' => 'string',
+    ),
+    'sort' => array(
+      'id' => 'standard',
+    ),
+    'filter' => array(
+      'id' => 'string',
+    ),
+    'argument' => array(
+      'id' => 'string',
+    ),

Extra argument.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/views/argument/CategoryCid.php
@@ -0,0 +1,38 @@
+    $result = db_query("SELECT title FROM {aggregator_category} where cid IN (:cid)", array(':cid' => $this->value))->fetchAssoc();
+    foreach ($result as $category) {
+      $titles[] = check_plain($category->title);

I think ->fetchAssoc() does not work here.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/views/field/Category.php
@@ -0,0 +1,81 @@
+      $this->options['alter']['path'] = "aggregator/category/$cid";

Path is aggregator/categories/$cid.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/views/filter/CategoryCid.php
@@ -0,0 +1,40 @@
+    $result = db_query('SELECT * FROM {aggregator_category} ORDER BY title');
+    foreach ($result as $category) {
+      $this->value_options[$category->cid] = $category->title;

SELECT * -> SELECT cid, title

+++ b/core/modules/aggregator/tests/modules/aggregator_test_views/test_views/views.view.test_aggregator_items.yml
@@ -0,0 +1,114 @@
+  feed_1:

This is not used in tests. Should it use webtest if it is impossible to test feed until #1956220: Allow to use xpath on drupal unit tests/unit tests?

One extra space before =.

Wow you are an eagle!

Couldn't we use fetchAllKeyed(0, 1) instead?

Instead of fetchAllKeyed() or instead of fetchCol?

Heads up, #15266: Replace aggregator category system with taxonomy is green.

Is the other one commitable in a reasonable timeframe?

it is no problem, whichever gets committed first, since views integration files are not touched from the other patch..this patch here just introduce new files...i can easily git rm the categories one...just please dont give any extra attention to the categories integration..

it will die soonish:D
so if olli's concerns are resolved this should go back to rtbc

I'd like to set this back to rtbc, but I found these:
Warning: Invalid argument supplied for foreach() in Drupal\aggregator\Plugin\views\row\Rss->render() (line 98)
Notice: Trying to get property of non-object in Drupal\aggregator\Plugin\views\argument\CategoryCid->title_query() (line 33)

Good catches, the first warning I'm not sure about, because I don't know when elements would be there? It's not declared as a property or anything? I've wrapped it for now, but it could be removed?

I have also changed the argument title_query method as fetchCol will return a flat array of values, not an object. So $row->title will never exist. We can just use that array and return a mapped version of that.

I fixed all consers beside the tests, as yeah the category integration will be dropped soon.

+++ b/core/modules/aggregator/tests/modules/aggregator_test_views/test_views/views.view.test_aggregator_items.yml
@@ -0,0 +1,114 @@
+human_name: test_aggregator_items

label

I don't know when elements would be there? It's not declared as a property or anything? I've wrapped it for now, but it could be removed?

I think we can remove that. In the original patch (or in d7) that is $item->elements which is set right before the foreach so there is no way elements could be there.

Yep, that's what I thought, and it seems there is no plan to actually ever need it. Let's rip it out!

Just fixing the label as well.

Great!

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/Views/IntegrationTest.phpundefined
@@ -0,0 +1,117 @@
+      $expected_author = filter_xss($items[$iid]->author->value, array());

Should be using aggregator_filter_xss()

And I think we a test to ensure aggregator_filter_xss() is being used as expected on the aggregrator item author and description.

Here's the requested updates, assertions are failing now. I'd like some oversight on this.

@wamilton, I think there has been some changes in the annotations:

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/views/argument/CategoryCid.php
@@ -0,0 +1,36 @@
+ * @Plugin(
+ *   id = "aggregator_category_cid",
+ *   module = "aggregator"
+ * )

These have changed to @PluginID("aggregator_category_cid")

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/views/row/Rss.php
@@ -0,0 +1,105 @@
+ *   type = "feed"

This is using display_types = {"feed"}

In addition to those changes:

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/Views/IntegrationTest.php
@@ -0,0 +1,114 @@
+      $expected_author = filter_xss($items[$iid]->author->value, array());

This should also use aggregator_filter_xss() as pointed out by @alexpott.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/Views/IntegrationTest.php
@@ -0,0 +1,114 @@
+      $expected_description = aggregator_filter_xss($items[$iid]->description->value, array());

That aggregator_filter_xss() takes only one parameter.

Fixed #58 but #54 is still pending.

Thank you!

+++ b/core/modules/aggregator/aggregator.views.incundefined
@@ -0,0 +1,337 @@
+      'click sortable' => TRUE,
...
+      'click sortable' => TRUE,
...
+      'click sortable' => TRUE,
...
+      'click sortable' => TRUE,
...
+      'click sortable' => TRUE,
...
+      'click sortable' => FALSE,
...
+      'click sortable' => TRUE,
...
+      'click sortable' => TRUE,
...
+      'click sortable' => TRUE,
...
+      'click sortable' => TRUE,
...
+      'click sortable' => TRUE,
...
+      'click sortable' => FALSE,
...
+      'click sortable' => TRUE,
...
+      'click sortable' => TRUE,
...
+      'click sortable' => TRUE,

As mentioned in the other issue, you can drop it.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/views/argument/CategoryCid.phpundefined
@@ -0,0 +1,33 @@
+    $titles = db_query("SELECT title FROM {aggregator_category} where cid IN (:cid)", array(':cid' => $this->value))->fetchCol();

What about injecting the db connection as well?

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/views/argument/CategoryCid.phpundefined
@@ -0,0 +1,33 @@
+      return check_plain($title);

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/views/argument/Fid.phpundefined
@@ -0,0 +1,35 @@
+      $titles[] = check_plain($feed->label());

check_plain should be replaced by String::checkPlain now

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/views/argument/Fid.phpundefined
@@ -0,0 +1,35 @@
+    $feeds = \Drupal::service('plugin.manager.entity')->getStorageController('aggregator_feed')->load($this->value);

Even more injections.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/views/argument/Iid.phpundefined
@@ -0,0 +1,35 @@
+    $items = \Drupal::service('plugin.manager.entity')->getStorageController('aggregator_item')->load($this->value);
...
+      $titles[] = check_plain($feed->label());

Same here.

Thanks @dawehner and @olli for the review. Fixed everything in #60.

Looking pretty good!

+++ b/core/modules/aggregator/aggregator.views.incundefined
@@ -0,0 +1,322 @@
+      'id' => 'aggregator_xss',
...
+      'id' => 'xss',

I guess aggregator_xss is the correct id? or is the item description intentionally not using the Aggregator xss implementation on purpose?

Before, when we have the 'click sortable' keys in the data, that would disable click sorting for both description fields. The aggregator specific Xss field handler can just implement its own click_sortable method and return FALSE. If using views' xss handler for item description is correct then 'click sortable' => FALSE will need to be added back to the data.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/views/field/Xss.phpundefined
@@ -0,0 +1,34 @@
+  public function sanitizeValue($value, $type = NULL) {
+    if ($type == 'xss') {
+      return aggregator_filter_xss($value);
+    }
+    else {
+      return parent::sanitizeValue($value, $type);
+    }

If this is extending Drupal\views\Plugin\views\field\Xss then sanitize value will always be passed 'xss' the type. So why have the fallback? I guess the actual type is pretty much irrelevant here, as we always want it run though this.

Converted 'id' => 'xss', for guid to 'id' => 'standard', as per @dawehner suggestion.
Added back 'click sortable' => FALSE, that change was unnecessary. Good catch @damiankloip.
Simplified sanitizeValue.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/views/argument/CategoryCid.phpundefined
@@ -0,0 +1,79 @@
+   * Creates an instance of the plugin.
+   *
+   * @param \Symfony\Component\DependencyInjection\ContainerInterface $container
+   *    The DI Container.
+   * @param array $configuration
+   *   A configuration array containing information about the plugin instance.
+   * @param string $plugin_id
+   *   The plugin ID for the plugin instance.
+   * @param array $plugin_definition
+   *   The plugin implementation definition.
+   *
+   * @return static
+   *   Returns an instance of this plugin.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/views/argument/Fid.phpundefined
@@ -0,0 +1,81 @@
+  /**
+   * Creates an instance of the plugin.
+   *
+   * @param \Symfony\Component\DependencyInjection\ContainerInterface $container
+   *    The DI Container.
+   * @param array $configuration
+   *   A configuration array containing information about the plugin instance.
+   * @param string $plugin_id
+   *   The plugin ID for the plugin instance.
+   * @param array $plugin_definition
+   *   The plugin implementation definition.
+   *
+   * @return static
+   *   Returns an instance of this plugin.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/views/argument/Iid.phpundefined
@@ -0,0 +1,81 @@
+  /**
+   * Creates an instance of the plugin.
+   *
+   * @param \Symfony\Component\DependencyInjection\ContainerInterface $container
+   *    The DI Container.
+   * @param array $configuration
+   *   A configuration array containing information about the plugin instance.
+   * @param string $plugin_id
+   *   The plugin ID for the plugin instance.
+   * @param array $plugin_definition
+   *   The plugin implementation definition.
+   *
+   * @return static
+   *   Returns an instance of this plugin.

Should be {@inheritdoc}. While you are here, __construct should be better above, as that's THE important function here.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/views/field/Category.phpundefined
@@ -0,0 +1,78 @@
+   * Data should be made XSS safe prior to calling this function.
+   */
+  protected function render_link($data, $values) {

Needs docs, sorry.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/views/field/TitleLink.phpundefined
@@ -0,0 +1,82 @@
+   * Renders aggregator item's title as link.
...
+   * Data should be made XSS safe prior to calling this function.
...
+  protected function render_link($data, $values) {

Needs docs.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/views/filter/CategoryCid.phpundefined
@@ -0,0 +1,34 @@
+class CategoryCid extends InOperator {
...
+    $this->value_options = db_query('SELECT cid, title FROM {aggregator_category} ORDER BY title')->fetchAllKeyed();

This one also could get a injection?

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/views/row/Rss.phpundefined
@@ -0,0 +1,105 @@
+      $item->{$name} = $value->value;
...
+        'value' => gmdate('r', $entity->timestamp->value),
...
+        'value' => $entity->author->value,
...
+        'value' => $entity->guid->value,
...
+    return theme($this->themeFunctions(), array(

so views_view_row_css takes care about the escaping here.

Thanks you @dawehner for the review and help. Fixed everything in #64.

We have at least some basic testing, so that's good to go! Thank you very much for bring this home.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/ItemStorageController.phpundefined
@@ -24,8 +24,9 @@ class ItemStorageController extends DatabaseStorageControllerNG {
+    if (!isset($entity->timestamp->value)) {
+      $entity->timestamp->value = REQUEST_TIME;
+    }

Howcome we have added this to this patch? Doesn't seem like views integration :)

Howcome we have added this to this patch? Doesn't seem like views integration :)

In order to test the timestamp rendering we have to be able to force a certain value, but the ItemStorageController just forced you to use the request all the time, so this seems to be a totally fine adapation.

OK, makes sense. We should consider moving (follow up) this for all created properties as there have been similar problems in tests recently.

curl https://drupal.org/files/1821844-65.patch | git a
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 36565  100 36565    0     0  19731      0  0:00:01  0:00:01 --:--:-- 28700
error: patch failed: core/modules/aggregator/lib/Drupal/aggregator/ItemStorageController.php:24
error: core/modules/aggregator/lib/Drupal/aggregator/ItemStorageController.php: patch does not apply

Rerolled.

#73: 1821844-73.patch queued for re-testing.

#1893772: Move entity-type specific storage logic into entity classes

Thank you for the rerole!

Committed to 8.x. Thanks.