Problem/Motivation

Aggregator's rendering is still stuck to pre-entity conversion

Proposed resolution

Instead of using custom logic, templates etc, move everything to hook_entity_extra_field_info() and field formatters

Remaining tasks

Review
Commit

User interface changes

  • Field UI is now enabled for feed entities under admin/config/services/aggregator
  • Two view modes are available under the "Manage display" tab: "Default" and "Summary"

API changes

  • aggregator_feed_source theming function is renamed to aggregator_feed to match what usually happens with other entities in core..eg node
  • aggregator_summary_items and aggregator_summary theming functions removed
Files: 
CommentFileSizeAuthor
#41 interdiff-2261425.txt3.13 KBParisLiakos
#41 drupal-aggregator_render-2261425-40.patch38.96 KBParisLiakos
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,626 pass(es). View
#37 interdiff-2261425.txt1.1 KBParisLiakos
#37 drupal-aggregator_render-2261425-37.patch38.07 KBParisLiakos
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-aggregator_render-2261425-37.patch. Unable to apply patch. See the log in the details link for more information. View
#34 interdiff-2261425.txt12.58 KBParisLiakos
#34 drupal-aggregator_render-2261425-34.patch38.32 KBParisLiakos
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,317 pass(es), 4 fail(s), and 0 exception(s). View
#31 interdiff-2261425.txt986 bytesParisLiakos
#31 drupal-aggregator_render-2261425-29.patch36.53 KBParisLiakos
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,319 pass(es). View
#27 drupal-aggregator_render-2261425-27.patch36.53 KBParisLiakos
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,315 pass(es), 2 fail(s), and 0 exception(s). View
#27 interdiff-2261425.txt17.61 KBParisLiakos
#22 interdiff.txt1.68 KBParisLiakos
#22 drupal-aggregator_render-2261425-22.patch36.13 KBParisLiakos
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,386 pass(es). View
#20 interdiff.txt1.59 KBParisLiakos
#20 drupal-aggregator_render-2261425-20.patch36.13 KBParisLiakos
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,504 pass(es), 43 fail(s), and 27 exception(s). View
#16 drupal-aggregator_render-2261425-15.patch36 KBParisLiakos
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,235 pass(es). View
#10 2261425.10.patch32.56 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,515 pass(es). View
#10 5-10-interdiff.txt726 bytesalexpott
#5 interdiff-2261425.txt1.19 KBParisLiakos
#5 drupal-aggregator_render-2261425-5.patch32.56 KBParisLiakos
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,554 pass(es), 1 fail(s), and 0 exception(s). View
#2 interdiff-2261425.txt2.35 KBParisLiakos
#2 drupal-aggregator_render-2261425-2.patch32.54 KBParisLiakos
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,504 pass(es), 11 fail(s), and 0 exception(s). View
#1 drupal-aggregator_render-2261425-1.patch31.78 KBParisLiakos
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,593 pass(es), 12 fail(s), and 0 exception(s). View

Comments

ParisLiakos’s picture

Issue summary: View changes
Status: Active » Needs review
FileSize
31.78 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,593 pass(es), 12 fail(s), and 0 exception(s). View

I am not entirely sure about hook_entity_extra_field_info() tbh..but this is a quite old patch, i just splitted it out from #2154781: Convert aggregator/sources and aggregator/opml to views
I am now studying #2010930: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title), it seems that there might be no need to use it

ParisLiakos’s picture

Issue summary: View changes
FileSize
32.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,504 pass(es), 11 fail(s), and 0 exception(s). View
2.35 KB

Ok, i got it:) great stuff happening in D8 :)
Anyway, i fixed the form ones, i would prefer to switch to formatters in a followup, preferebaly after the Node conversion, because many susefull stuff aggregator needs are added there..eg timestamp formatter

Status: Needs review » Needs work

The last submitted patch, 2: drupal-aggregator_render-2261425-2.patch, failed testing.

The last submitted patch, 1: drupal-aggregator_render-2261425-1.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
32.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,554 pass(es), 1 fail(s), and 0 exception(s). View
1.19 KB

Well i have no clue why ConfigImportAllTest fails and running it locally it just takes forever and ends with Maximum execution time of 240 seconds exceeded
Geez, why we keep creating tests like that..any help appreciated

Status: Needs review » Needs work

The last submitted patch, 5: drupal-aggregator_render-2261425-5.patch, failed testing.

Berdir’s picture

Yeah, we only need extra fields for stuff that we can't handle with formatters/widgets. Thanks for working on this, on my review list :)

Berdir’s picture

I think that test fail means it's verifying that there no config changes left to sync at the end, looks like aggregator view display configuration has updated itself during the import, we should make sure that the provide the correct file as an import...

Will ask @alexpott to verify that...

alexpott’s picture

Looking into it...

Geez, why we keep creating tests like that..any help appreciated

... to catch stuff like this.

alexpott’s picture

Status: Needs work » Needs review
FileSize
726 bytes
32.56 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,515 pass(es). View

So the issue is with entity.view_display.aggregator_feed.aggregator_feed.default the order is being swapped around for some reason.

  bundle: aggregator_feed
  mode: default
  content:
-   items:
-     weight: 0
    checked:
      weight: 0
    image:
      weight: 1
    description:
--- 4,13 ----
  bundle: aggregator_feed
  mode: default
  content:
    checked:
      weight: 0
+   items:
+     weight: 0
    image:
      weight: 1
    description:

And this is because checked and items have the same weight.

Berdir’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/aggregator/aggregator.module
    @@ -106,6 +92,84 @@ function aggregator_permission() {
     /**
    + * Implements hook_entity_bundle_info().
    + */
    +function aggregator_entity_bundle_info() {
    +  $bundles['aggregator_feed']['aggregator_feed']['label'] = t('Aggregator feed');
    +  $bundles['aggregator_item']['aggregator_item']['label'] = t('Aggregator item');
    +  return $bundles;
    +}
    +
    

    This is added by default in EntityManager::getAllBundleInfo(). Wondering if we should make the feed the bundle of items, but we don't need to do this here.

  2. +++ b/core/modules/aggregator/aggregator.theme.inc
    --- /dev/null
    +++ b/core/modules/aggregator/config/install/entity.view_display.aggregator_feed.aggregator_feed.default.yml
    
    +++ b/core/modules/aggregator/config/install/entity.view_display.aggregator_feed.aggregator_feed.default.yml
    --- /dev/null
    +++ b/core/modules/aggregator/config/install/entity.view_display.aggregator_feed.aggregator_feed.summary.yml
    

    As mentioned above, it shouldn't be necessary to provide those files, as this information is build automatically based on base and extra fields.

  3. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.php
    @@ -42,14 +42,9 @@ public function feedAdd() {
        */
       public function viewFeed(FeedInterface $aggregator_feed) {
    -    $entity_manager = $this->entityManager();
    -    $feed_source = $entity_manager->getViewBuilder('aggregator_feed')
    +    return $this->entityManager()
    +      ->getViewBuilder('aggregator_feed')
           ->view($aggregator_feed, 'default');
    -    // Load aggregator feed item for the particular feed id.
    -    $items = $entity_manager->getStorage('aggregator_item')->loadByFeed($aggregator_feed->id(), 20);
    -    // Print the feed items.
    -    $build = $this->buildPageList($items, $feed_source);
    -    return $build;
       }
    

    Can we use _entity_view in the route?

  4. +++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedViewBuilder.php
    @@ -16,12 +21,123 @@
    +      if ($display->getComponent('checked')) {
    +        // Render the checked timestamp as time ago.
    +        $last_checked = $entity->getLastCheckedTime();
    +        if ($last_checked) {
    +          $updated = t('@time ago', array('@time' => format_interval(REQUEST_TIME - $last_checked)));
    +        }
    +        else {
    +          $updated = t('never');
    +        }
    +        $build[$id]['checked'] = array(
    +          '#markup' => t('<em>Updated:</em> @updated', array('@updated' => $updated)),
    +          '#prefix' => '<div class="feed-updated">',
    +          '#suffix' => '</div>',
    +        );
    +      }
    

    Not sure if we want to go there here, but what we could do is make this is a formatter instead and set it up in the base field definition, similar for others.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
13.24 KB
34.46 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,391 pass(es). View

thanks for the review!
All valid points :)

I converted link and author fields to formatters..I added @todos for the rest fields..and for image, well its a special case, will need an issue on its own..(to be filed)

ParisLiakos’s picture

  1. +++ b/core/modules/aggregator/aggregator.theme.inc
    @@ -15,30 +15,18 @@
    +  $variables['title'] = check_plain($item->label());
    

    uhm String::checkPlain

  2. +++ b/core/modules/aggregator/config/install/entity.view_display.aggregator_feed.aggregator_feed.summary.yml
    @@ -0,0 +1,16 @@
    +dependencies:
    +  entity:
    +    - entity.view_mode.aggregator_feed.summary
    
    +++ b/core/modules/aggregator/config/install/entity.view_display.aggregator_item.aggregator_item.summary.yml
    @@ -0,0 +1,16 @@
    +dependencies:
    +  entity:
    +    - entity.view_mode.aggregator_item.summary
    

    I need to re-export those, now also the module dependency is added

ParisLiakos’s picture

FileSize
34.52 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,623 pass(es). View
1.71 KB

fixed #13

ParisLiakos’s picture

FileSize
34.24 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,964 pass(es). View

reroll for PSR4

ParisLiakos’s picture

FileSize
36 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,235 pass(es). View

reroll, also injecting the date service into the view builders

joelpittet’s picture

Issue tags: +Twig, +Template consolidation

Drive by review and tagging for http://drupaltwig.org/.

  1. +++ b/core/modules/aggregator/templates/aggregator-feed.html.twig
    @@ -0,0 +1,34 @@
    + *   {% hide(content.field_example) %} to temporarily suppress the printing
    
    +++ b/core/modules/aggregator/templates/aggregator-item.html.twig
    @@ -4,12 +4,12 @@
    + *   {% hide(content.field_example) %} to temporarily suppress the printing
    

    There is no hide() in twig. https://www.drupal.org/node/2212845

  2. +++ b/core/modules/aggregator/templates/aggregator-feed.html.twig
    @@ -0,0 +1,34 @@
    +<div{{ attributes}}>
    

    Needs a space after attributes per standards. @see https://drupal.org/node/1823416

joelpittet’s picture

+++ b/core/modules/aggregator/src/FeedViewBuilder.php
@@ -16,12 +22,113 @@
+          '#markup' => t('<em>Updated:</em> @updated', array('@updated' => $updated)),
+          '#prefix' => '<div class="feed-updated">',
+          '#suffix' => '</div>',
...
+          '#prefix' => '<div class="feed-description">',
+          '#suffix' => '</div>',

+++ b/core/modules/aggregator/src/ItemViewBuilder.php
@@ -16,16 +22,102 @@
+          '#prefix' => '<div class="feed-source-meta">',
+          '#suffix' => '</div>',
...
+            '#prefix' => '<span class="feed-item-date">',
+            '#suffix' => '</span>',
...
+          '#prefix' => '<div class="item-description">',
+          '#suffix' => '</div>',

Is there any way we can get these tags in template?

The goal is to let these deal with the data and let the templates deal with marking that data up.

I can't think of a way so I thought I'd ask.

Berdir’s picture

@#18: A lot of that markup will go away when we can switch to formatters for things like the updated and description fields, then those will get the default field template classes/markup. I wouldn't worry too much about that now..

ParisLiakos’s picture

FileSize
36.13 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,504 pass(es), 43 fail(s), and 27 exception(s). View
1.59 KB

thanks @joelpittet for the review! those hide() were old cp's :) updated them to reflect the new way of doing this.
The prefix/suffix tags..yeah, i only put them there to simulate the old markup and to actually have some wrappers..When those are converted to formatters, i will then be able to use the regular field wrappers and we can get rid of them

Status: Needs review » Needs work

The last submitted patch, 20: drupal-aggregator_render-2261425-20.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
36.13 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,386 pass(es). View
1.68 KB

ah, date service was renamed

Status: Needs review » Needs work

The last submitted patch, 22: drupal-aggregator_render-2261425-22.patch, failed testing.

joelpittet’s picture

Status: Needs work » Needs review

Thank you @ParisLiakos for fixing those changes. @Berdir it's good to know the plan, is there an issue showing this change to formatters? Maybe it should be a child of this issue or related or something?

ParisLiakos’s picture

The node issue is the one is the one that will provide the timestamp formatters i need to build upon..and for the description field there is already an issue

ParisLiakos’s picture

FileSize
17.61 KB
36.53 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,315 pass(es), 2 fail(s), and 0 exception(s). View

got rid many todos. i hope this is ready to go now. Only leftover is the description field which will be resolved in the issue linked above

ParisLiakos’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 27: drupal-aggregator_render-2261425-27.patch, failed testing.

Berdir’s picture

interdiff looks great :)

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
36.53 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,319 pass(es). View
986 bytes

oops, too many dots :P

Wim Leers’s picture

Status: Needs review » Needs work

Great work here! :)

Mostly nitpicks, but also some questions.

  1. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
    @@ -0,0 +1,95 @@
    +   * @param \Drupal\Core\Datetime\DateFormatter $date_formatter
    +   *    The date formatter service.
    

    Wrong indentation.

  2. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
    @@ -0,0 +1,95 @@
    +    $this->dateFormatter = $date_formatter;
    

    I don't see a protected $dateFormatter property on this class yet?

  3. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/TimestampAgoFormatter.php
    @@ -0,0 +1,95 @@
    +      $elements[$delta] = array('#markup' => $updated);
    

    Doesn't this need to be wrapped in SafeMarkup::set()?

  4. +++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/UriLinkFormatter.php
    @@ -0,0 +1,44 @@
    + * Contains \Drupal\Core\Field\Plugin\Field\FieldFormatter\UriFormatter.
    ...
    +class UriLinkFormatter extends FormatterBase {
    

    Mismatch.

  5. +++ b/core/modules/aggregator/aggregator.module
    @@ -107,6 +93,52 @@ function aggregator_permission() {
    +function aggregator_entity_extra_field_info() {
    

    I'm no fan of this. But it's definitely a step forward from hardcodedness in templates!

    It'd be great if this could document *why* these have to be "extra" fields and can't be "proper" fields.

  6. +++ b/core/modules/aggregator/aggregator.theme.inc
    @@ -74,104 +61,46 @@ function theme_aggregator_page_opml($variables) {
    +  $variables['full'] = $variables['elements']['#view_mode'] == 'full';
    ...
    +  if ($variables['full']) {
    +    $variables['link'] = array(
    +      '#theme' => 'feed_icon',
    +      '#url' => $feed->getUrl(),
    +      '#title' => t('!title feed', array('!title' => $feed->label())),
    +    );
    

    Shouldn't this be configurable in the Feed's view mode?
    And it looks like it is… so … is this if-test a leftover that can be removed? Or what's going on here?

  7. +++ b/core/modules/aggregator/aggregator.theme.inc
    @@ -74,104 +61,46 @@ function theme_aggregator_page_opml($variables) {
    +      '#title' => t('More<span class="visually-hidden"> posts about @title</span>', array(
    

    There's no space before the span and a space after, should be vice versa :)

  8. +++ b/core/modules/aggregator/config/install/core.entity_view_display.aggregator_item.aggregator_item.summary.yml
    @@ -0,0 +1,18 @@
    +  entity:
    +    - core.entity_view_mode.aggregator_item.summary
    

    A dependency on itself (here and elsewhere)? Or am I misreading that?

  9. +++ b/core/modules/aggregator/src/FeedViewBuilder.php
    @@ -16,12 +21,95 @@
    +   * @param \Drupal\Core\Config\Config $config
    +   *    The 'aggregator.settings' config.
    

    Wrong indentation.

  10. +++ b/core/modules/aggregator/src/FeedViewBuilder.php
    @@ -16,12 +21,95 @@
    +        // When in summary view mode respect the list_max setting.
    

    Comma after "mode".

  11. +++ b/core/modules/aggregator/src/ItemViewBuilder.php
    @@ -18,14 +18,40 @@ class ItemViewBuilder extends EntityViewBuilder {
    +          '#href' => 'aggregator/sources/' . $feed->id(),
    

    Can't this use #route_name and #route_parameters instead?

Berdir’s picture

5. Some could be formatters and some of those already have @todo's, but extra fields in general is still a valid concept, one example is items. That's a list of something and doesn't exist as a field, so it is "extra".
6. Maybe should be moved from a view mode check to a $display->getComponent() check, which respects the current view mode setting?
7. he visually hidden thing actually makes it tricky, because you don't want a space if it is hidden, so it needs to be inside? Weird but correct I think.
8. You did. Dependency on the view mode, this is the display.

ParisLiakos’s picture

Status: Needs work » Needs review
Related issues: +#2339917: Move feed image field rendering to a formatter
FileSize
38.32 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,317 pass(es), 4 fail(s), and 0 exception(s). View
12.58 KB

Thanks for the review!

1.Fixed
2.Thanks!!i obviously forgot that
3. No, because it comes from t()
4. Fixed
5. While i was there..i opened an issue to move feed image to formatter..its possible, but i dont want to do it here because its not that simple and straightforward like the other formatters. Also! i realized i could move the "feed" for items into a formatter really easily :))
6. I did what Berdir suggested, which required me to also provide install config for the "default" view mode, so i can get rid of More link.
7. Yes, Berdir is right, its like that in the node module too.
9, 10 Fixed
11. Very good point!! fixed

Status: Needs review » Needs work

The last submitted patch, 34: drupal-aggregator_render-2261425-34.patch, failed testing.

Wim Leers’s picture

Looking great! :)

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
38.07 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch drupal-aggregator_render-2261425-37.patch. Unable to apply patch. See the log in the details link for more information. View
1.1 KB

lets remove some junk. hopefully back to green

Berdir’s picture

That's not the right fix. Instead, you need to define the config schema for those types to core.entity.schema.yml, see entity_view_display.field.number_unformatted as an example.

ParisLiakos’s picture

aah, i see now:) thanks for the tip!
will try to add the necessary fields there

Status: Needs review » Needs work

The last submitted patch, 37: drupal-aggregator_render-2261425-37.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
38.96 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,626 pass(es). View
3.13 KB

that simple?

Wim Leers’s picture

I think this is RTBC. I'll leave it to Berdir to take one last look and RTBC it :)

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Yes, looks great I think.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

We either need a new change record here - or we need to attach this to [#1905910] and do a massive update for themers.

ParisLiakos’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs change record

Thanks Wim and Berdir!
@alexpott: good idea! i updated the existing..i think this will be enough
back to RTBC

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed c554974 and pushed to 8.0.x. Thanks!

Yep I contributed to this patch but only in a very minor way to fix some default configuration.

diff --git a/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/UriLinkFormatter.php b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/UriLinkFormatter.php
index 8209e1e..762d5f3 100644
--- a/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/UriLinkFormatter.php
+++ b/core/lib/Drupal/Core/Field/Plugin/Field/FieldFormatter/UriLinkFormatter.php
@@ -7,7 +7,6 @@
 
 namespace Drupal\Core\Field\Plugin\Field\FieldFormatter;
 
-use Drupal\Component\Utility\String;
 use Drupal\Core\Field\FormatterBase;
 use Drupal\Core\Field\FieldItemListInterface;
 
diff --git a/core/modules/aggregator/src/FeedViewBuilder.php b/core/modules/aggregator/src/FeedViewBuilder.php
index d42641a..26523ab 100644
--- a/core/modules/aggregator/src/FeedViewBuilder.php
+++ b/core/modules/aggregator/src/FeedViewBuilder.php
@@ -7,7 +7,6 @@
 
 namespace Drupal\aggregator;
 
-use Drupal\Core\Entity\EntityInterface;
 use Drupal\Core\Entity\EntityManagerInterface;
 use Drupal\Core\Entity\EntityTypeInterface;
 use Drupal\Core\Entity\EntityViewBuilder;
diff --git a/core/modules/aggregator/src/ItemViewBuilder.php b/core/modules/aggregator/src/ItemViewBuilder.php
index a6488bb..9207b71 100644
--- a/core/modules/aggregator/src/ItemViewBuilder.php
+++ b/core/modules/aggregator/src/ItemViewBuilder.php
@@ -7,7 +7,6 @@
 
 namespace Drupal\aggregator;
 
-use Drupal\Component\Utility\String;
 use Drupal\Core\Entity\EntityViewBuilder;
 
 /**

Removed unused uses.

  • alexpott committed c554974 on 8.0.x
    Issue #2261425 by ParisLiakos, alexpott: Streamline aggregator's...
ParisLiakos’s picture

yay!
thanks!

Status: Fixed » Closed (fixed)

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

znerol’s picture