Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ParisLiakos’s picture

Status: Active » Postponed
Issue tags: +VDC
drupalninja99’s picture

This is good since we don't have a clean way (that I am aware of) to cleaning output XML in a page hook anyway.

ParisLiakos’s picture

Status: Postponed » Active

integration is in

ParisLiakos’s picture

Title: Convert aggregator/rss and aggregator/opml to views » Convert aggregator/rss to views
Status: Active » Needs review
FileSize
9.4 KB

Lets keep OPML seperate for now, cause it needs an additional display plugin

dawehner’s picture

Issue tags: +Needs manual testing
+++ b/core/modules/aggregator/aggregator.pages.incundefined
@@ -354,80 +354,6 @@ function aggregator_page_categories() {
-  $feed_length = config('system.rss')->get('items.view_mode');
...
-        $summary = text_summary($feed->description, NULL, config('aggregator.settings')->get('items.teaser_length'));

So we accept that these settings are dropped?

+++ b/core/modules/aggregator/aggregator.pages.incundefined
@@ -354,80 +354,6 @@ function aggregator_page_categories() {
-  $output .= format_rss_channel(t('@site_name aggregator', array('@site_name' => $site_name)), $url, $description, $items);

This seems to be a valid information as well.

ParisLiakos’s picture

i closed #2004622: aggregator_test output includes drupal page content as duplicate, aggregator/rss is broken atm:) this fixes it

Status: Needs review » Needs work
Issue tags: -Needs manual testing, -VDC

The last submitted patch, drupal-aggregator_rss_view-1955760-5.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
Issue tags: +Needs manual testing, +VDC
pguillard’s picture

Assigned: Unassigned » pguillard
pguillard’s picture

Apparently something has changed at the same time. This is a new patch that should be ok.

pguillard’s picture

Assigned: pguillard » Unassigned
lslinnet’s picture

Assigned: Unassigned » lslinnet

Working on a reroll of the patch:

patching file core/modules/aggregator/aggregator.module
Reversed (or previously applied) patch detected!  Assume -R? [n] y
Hunk #1 succeeded at 152 with fuzz 1 (offset 5 lines).
patching file core/modules/aggregator/aggregator.pages.inc
Hunk #1 succeeded at 319 (offset -35 lines).
patching file core/modules/aggregator/config/views.view.aggregator_rss_feed.yml
lslinnet’s picture

This should be the reroll, including a nice little interdiff

lslinnet’s picture

Assigned: lslinnet » Unassigned
Crell’s picture

I checked this out manually and it seems to be fine. The only comment I'd have is that for core views we should probably have meaningful display machine names. I don't know if we've been doing that in other Views conversions, though.

Can someone reroll with a useful machine name for the displays?

lslinnet’s picture

Assigned: Unassigned » lslinnet

Will change the name to something meaning full as suggested.

lslinnet’s picture

Have updated the machine names so they now reflect what they are actually a list of (feed_items and feed_items_per_category)

Crell’s picture

Status: Needs review » Reviewed & tested by the community

Yay!

tstoeckler’s picture

Status: Reviewed & tested by the community » Needs work

I've seen among others @dawehner specifically request for the auto-generated display names (i.e. feed_1, etc.) to stay. I don't know why that is, and I also find meaningful names, well..., more meaningul!, but I give him the benefit of the doubt that this has a reason.

dawehner’s picture

Status: Needs work » Reviewed & tested by the community

To be honest this is just personal preference (and the feeling that people overuse this feature), so I will not block that.

YesCT’s picture

Issue tags: +RTBC July 1

This issue was RTBC and passing tests on July 1, the beginning of API freeze.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

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

Anonymous’s picture

Issue summary: View changes

Issue summary update

ParisLiakos’s picture