title says all

CommentFileSizeAuthor
#14 interdiff-2154781.txt2.1 KBParisLiakos
#14 drupal-aggregator_sources_views-2154781-14.patch23.75 KBParisLiakos
#11 drupal-aggregator_sources_views-2154781-11.patch23.8 KBParisLiakos
#11 interdiff-2154781.txt1.85 KBParisLiakos
#9 drupal-aggregator_sources_views-2154781-9.patch22.42 KBParisLiakos
#9 interdiff-2154781.txt8.02 KBParisLiakos
#7 drupal-aggregator_sources_views-2154781-7.patch16.45 KBParisLiakos
#1 drupal-aggregator_sources_views-2154781-1.patch33.77 KBParisLiakos
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Status: Active » Needs review
FileSize
drupal-aggregator_sources_views-2154781-1.patch33.77 KB

This was an awesome cleanup..got rid of 2 custom theme functions, and moved their custom logic to the EntityViewBuilders and views modes.
Also exposed feed's field UI..we need the same for items, but that can be done in another issue

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Issue tags: +Needs tests

i need to add a test for #2114699: Regression: Aggregator item template expects an item to have an ftitle field but it can never have one so tagging accordingly

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Parent issue: » #1955760: [Meta] Convert aggregator pages/blocks to views

Status: Needs review » Needs work

The last submitted patch, 1: drupal-aggregator_sources_views-2154781-1.patch, failed testing.

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Title: Convert aggregator/sources to views » Convert aggregator/sources and aggregator/opml to views
Status: Needs work » Postponed

So the plan goes like that:

  1. Add the OPML plugins to views on #2154957: Add OPML functionality to views
  2. We cant convert aggregator/opml on the issue linked above, because feeds need to be attached to pages
  3. So, we convert both aggregator/sources and aggregator/opml to views here, so we have the feed icons also working
ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented

i 've splitted most of the cleanup done here in a separate issue #2261425: Streamline aggregator's entities rendering with rest of core would be nice to get this in first

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Status: Postponed » Needs review
FileSize
drupal-aggregator_sources_views-2154781-7.patch16.45 KB

probably that easy after the issue above is in

Status: Needs review » Needs work

The last submitted patch, 7: drupal-aggregator_sources_views-2154781-7.patch, failed testing.

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Status: Needs work » Needs review
FileSize
interdiff-2154781.txt8.02 KB
drupal-aggregator_sources_views-2154781-9.patch22.42 KB
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
  1. +++ b/core/modules/aggregator/config/install/views.view.aggregator_sources.yml
@@ -0,0 +1,417 @@
+    display_plugin: page
+    id: page_1
+    display_title: Page
+    position: 1
+    provider: views
+    display_options:
+      field_langcode: '***LANGUAGE_language_content***'
+      field_langcode_add_to_query: null
+      path: aggregator/sources
+      menu:
+        type: normal
+        title: Sources
+        description: ''
+        weight: 0
+        context: '0'
+        menu_name: tools

    Note: We can actually force a given route name, if needed. 

  2. +++ b/core/modules/aggregator/config/schema/aggregator.views.schema.yml
@@ -1,5 +1,13 @@
 
+"views.row.entity:aggregator_feed":
+  type: views_entity_row
+  label: 'Entity options'
+
+"views.row.entity:aggregator_item":
+  type: views_entity_row
+  label: 'Entity options'

    Are you sure this is actually still needed? I don't think so. 

  3. +++ b/core/modules/views/config/schema/views.row.schema.yml
@@ -49,3 +49,32 @@ views.row.rss_fields:
+views.row.opml_fields:
+  type: mapping
+  label: 'OPML field options'

    We fix them now, but shouldn't this be of type views.row.base or something like that? I guess we want to represent the views class hierarchy in the schema as well.

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
FileSize
interdiff-2154781.txt1.85 KB
drupal-aggregator_sources_views-2154781-11.patch23.8 KB

Are you sure this is actually still needed? I don't think so.

Well it was the only way i could find to stop Drupal\config\Tests\DefaultConfigTest complain about display.default.display_options.row.options, if there is another way, happy to hear it!

3. yes, good point

Also added some tests

Status: Needs review » Needs work

The last submitted patch, 11: drupal-aggregator_sources_views-2154781-11.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Meh, there was an issue adding views.row.entity:*which works for all of them. I will not block the issue for that.

@ParisLiakos
Awesome work!!!!!!!!!!!!!!!!!

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Status: Needs work » Needs review
FileSize
drupal-aggregator_sources_views-2154781-14.patch23.75 KB
interdiff-2154781.txt2.1 KB

Thank you for the quick reviews!!

Meh, there was an issue adding views.row.entity:*which works for all of them

Yeah, that would be very nice! :) I tried to find it, but no success though:(

I put the new tests in a bad spot:P just some reordering and this should be back to green

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Awesome work!!

@Paris
It would be nice if you could write some change record.

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented

thank you!

i updated https://www.drupal.org/node/2084727 with the functions & paths removed

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.0.x, thanks!

  • catch committed 993a860 on 8.0.x 
    Issue #2154781 by ParisLiakos: Convert aggregator/sources and aggregator...

Status: Fixed » Closed (fixed)

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