Expand FeedInterface with methods

Yes, see the instructions that I just posted to in #2016679-12: Expand Entity Type interfaces to provide methods, protect the properties.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedInterface.phpundefined
@@ -14,6 +14,255 @@
+   * @return \Drupal\comment\CommentInterface|null
+   *   The class instance that this method is called on.
+   */
+  public function setTitle($title);
...
+   * @return \Drupal\aggregator\Plugin\Core\Entity\FeedInterface|null
+   *   The class instance that this method is called on.
+   */
+  public function setLangcode($langcode);

All setters should return \Drupal\aggregator\FeedInterface.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Core/Entity/Feed.phpundefined
@@ -41,133 +41,17 @@
    * Overrides Drupal\Core\Entity\EntityNG::init().
    */
   public function init() {
     parent::init();

You can remove this method completely.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/Core/Entity/Feed.phpundefined
@@ -264,4 +148,194 @@ protected function clearBlockCacheDefinitions() {
   }
 
+
+

Too any empty lines here.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.phpundefined
@@ -145,33 +145,32 @@ public function feedRefresh(FeedInterface $aggregator_feed, Request $request) {
-    $result = $this->database->query('SELECT f.fid, f.title, f.url, f.refresh, f.checked, f.link, f.description, f.hash, f.etag, f.modified, f.image, f.block, COUNT(i.iid) AS items FROM {aggregator_feed} f LEFT JOIN {aggregator_item} i ON f.fid = i.fid GROUP BY f.fid, f.title, f.url, f.refresh, f.checked, f.link, f.description, f.hash, f.etag, f.modified, f.image, f.block ORDER BY f.title');
-
+    $feeds = $this->entityManager->getStorageController('aggregator_feed')->loadMultiple();

Yes, not sure about changing this, missing some parts like ->items (which is a count()). I'd just leave this for now.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedFormController.phpundefined
@@ -35,27 +35,27 @@ public function form(array $form, array &$form_state) {
-      '#default_value' => $feed->language()->id,
+      '#default_value' => $feed->getLangcode(),

This is not necessary.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedInterface.phpundefined
@@ -14,6 +14,255 @@
+  /**
+   * Returns the language code of the feed..
+   *
+   * @return string
+   *   The languagecode of the feed.
+   */
+  public function getLangcode();

Yeah, we didn't this for other interfaces. language() unfortunately returns the language entity but that's fine.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedInterface.phpundefined
@@ -14,6 +14,255 @@
+  public function setLangcode($langcode);

This one is useful, so keep that.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedInterface.phpundefined
@@ -14,6 +14,255 @@
+   * Sets the url to the feed.
+   *
+   *
+   * @param string $url
+   *   A string containing the url of the feed.
...
+   * Sets the refresh rate of the feed.
+   *
+   *
+   * @param int $refresh

Should only have a single empty line above @param.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedInterface.phpundefined
@@ -14,6 +14,255 @@
+   * @return int
+   *   The refresh rate of the feed.
...
+   *   The refresh rate of the feed.

Let's document that it's in seconds.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedInterface.phpundefined
@@ -14,6 +14,255 @@
+  public function getRefresh();

getRefreshRate()? Method names should be a good compromise between self-explaining and not too long :)

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedInterface.phpundefined
@@ -14,6 +14,255 @@
+   *  Returns the last time where the feed was checked for new items, as Unix timestamp.

Two spaces in front of the text. let's reove the as unix timestamp from here, to keep it under 80 characters. We still have that in the @return.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedInterface.phpundefined
@@ -14,6 +14,255 @@
+   */
+  public function getChecked();

Let's name this (get/set)LastCheckedTime(), that's also used as variable for the feed source template.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedInterface.phpundefined
@@ -14,6 +14,255 @@
+   * Sets the Time when this feed was queued for refresh, 0 if not queued.
+   *
+   *
+   * @param int $checked

Again two empty lines.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedInterface.phpundefined
@@ -14,6 +14,255 @@
+   */
+  public function getQueued();

Let's also try to find a better method name for this... getQuuedTime()?

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedInterface.phpundefined
@@ -14,6 +14,255 @@
+   * @param int $queued
+   *    A int with the time of the last refresh.

No need to repeat in, just timestamp of the last refresh should be enough.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedInterface.phpundefined
@@ -14,6 +14,255 @@
+   * Returns the parent website of the feed.
+   *
+   * @return int
+   *   The node ID of the node where the comment is attached.
+   */
+  public function getLink();

What about getWebsiteLink() then?

the @return is copied from somewhere.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedInterface.phpundefined
@@ -14,6 +14,255 @@
+   * Returns the Calculated hash of the feed data, used for validating cache.
+   *
+   * @return string
+   *   The Calculated hash of the feed data.
+   */
+  public function getHash();

Not sure what to name this one.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedInterface.phpundefined
@@ -14,6 +14,255 @@
+   * Return when the feed was last modified, as a Unix timestamp.
+   *
+   * @return int
+   *   The timestamp of the last modified.

This could use a little bit more documentation, the @return also isn't really a sentence.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedInterface.phpundefined
@@ -14,6 +14,255 @@
+  public function getModified();

getLastModified() I'd say.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedInterface.phpundefined
@@ -14,6 +14,255 @@
+  /**
+   * Return the Number of items to display in the feed’s block.
+   *
+   * @return int
+   *   The Number of items to display in the feed’s block.
+   */
+  public function getBlock();

Tricky one. getBlockItemCount()? Sad that this is still in here, wanted to move it to instance configuration· Too late now I guess.

Thanks, I think we're getting close here :)

1. +++ b/core/modules/aggregator/aggregator.module
   @@ -358,21 +358,21 @@ function aggregator_refresh(Feed $feed) {
   -        if (empty($feed->link->value)) {
   -          $feed->link->value = $feed->url->value;
   +        if (is_null($feed->getLink())) {
   +          $feed->setLink($feed->getUrl());
            }
   

   I don't think there is a need for an explicit is_null() check, just if (!$feed->getLink()) should be fine. The empty was when those objects weren't typed and might or might not have those properties.

2. +++ b/core/modules/aggregator/aggregator.pages.inc
   @@ -232,27 +232,27 @@ function template_preprocess_aggregator_feed_source(&$variables) {
   -  if (!empty($feed->image->value) && $feed->label() && !empty($feed->link->value)) {
   +  if (!is_null($feed->getImage()) && $feed->label() && !is_null($feed->getLink())) {
   

   Same here.

3. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.php
   @@ -185,19 +186,19 @@ public function adminOverview() {
   -        'title' => $this->t('Edit'),
   +        'title' => t('Edit'),
            'href' => "admin/config/services/aggregator/edit/feed/$feed->fid",
          );
          $links['delete'] = array(
   -        'title' => $this->t('Delete'),
   +        'title' => t('Delete'),
            'href' => "admin/config/services/aggregator/delete/feed/$feed->fid",
          );
          $links['remove'] = array(
   -        'title' => $this->t('Remove items'),
   +        'title' => t('Remove items'),
            'href' => "admin/config/services/aggregator/remove/$feed->fid",
          );
          $links['update'] = array(
   -        'title' => $this->t('Update items'),
   +        'title' => t('Update items'),
   

   This seems like an accidental revert, those changes should be removed from the patch.

4. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Entity/Feed.php
   @@ -41,121 +41,6 @@
   -   * @var \Drupal\Core\Entity\Field\FieldInterface
   

   The class has been renamed, so this will conflict here, just re-remove it.

5. +++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedInterface.php
   @@ -14,6 +14,239 @@
   +   * Sets the Langcode of the feed.
   

   lowercase, also not sure if we should use "language code".

6. +++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedInterface.php
   @@ -14,6 +14,239 @@
   +   * Returns the url to the feed.
   

   Should be URL I think (only in the docblock, the method name has to be camel case.

7. +++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedInterface.php
   @@ -14,6 +14,239 @@
   +   * Returns the last time where the feed was checked for new items, as Unix timestamp.
   

   Over 80 characters. Maybe remove the as unix timestamp part, as we have that in the @return anyway.

8. +++ b/core/modules/aggregator/lib/Drupal/aggregator/FeedInterface.php
   @@ -14,6 +14,239 @@
   +   * Sets the Time when this feed was queued for refresh, 0 if not queued.
   

   Time here and in the other docbocks below should be lowercase.

#28: drupal8.aggregator-module.2028037-28.patch queued for re-testing.

I think this is good to go.

42: drupal8.aggregator-module.2028037-42.patch queued for re-testing.

Agreed that there is probably no overlap, there is already a uuid() method provided by Entityinterface.