Streamline aggregator's entities rendering with rest of core

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 :)

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...

Looking into it...

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

... to catch stuff like this.

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.

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.

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

+++ 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.

@#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..

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?

interdiff looks great :)

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?

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.

Looking great! :)

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.

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

Yes, looks great I think.

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

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.

This also fixed #397962: Aggregator: link breaks if feed not refreshed yet.