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

Comments

ParisLiakos’s picture

ParisLiakos commented
Status: Active » Needs review
StatusFileSize
new 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 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 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 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 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 commented
Status: Postponed » Needs review
StatusFileSize
new 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 commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2154781.txt8.02 KB
new drupal-aggregator_sources_views-2154781-9.patch22.42 KB
dawehner’s picture

dawehner
Primary language German
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 commented
StatusFileSize
new interdiff-2154781.txt1.85 KB
new 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
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 commented
Status: Needs work » Needs review
StatusFileSize
new drupal-aggregator_sources_views-2154781-14.patch23.75 KB
new 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
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 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
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.