Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alarcombe’s picture

Assigned: Unassigned » alarcombe
YesCT’s picture

Assigned: alarcombe » Unassigned

@alarcombe if you had any work here, it's ok to post it, even it it's only partial. Or ask any questions you have.

Otherwise I think this is available. :)

alarcombe’s picture

@YesCT - sure, was eyeing this up wrt https://drupal.org/node/2016701 but won't have time over the next couple of weeks to look at it.

undertext’s picture

Status: Active » Needs review
FileSize
14.92 KB
Berdir’s picture

Status: Needs review » Needs work

Thanks for working on this, looks good, just a few minor things and one method rename.

  1. +++ b/core/modules/aggregator/aggregator.pages.inc
    @@ -151,21 +151,21 @@ function theme_aggregator_summary_item($variables) {
     
    ...
    +  if (isset($item->ftitle) && $item->getFid() != null) {
    +    $variables['source_url'] = url("aggregator/sources/$item->getFid()");
    

    method calls in code don't work, you could add {} around it, but I prefer '...' . $item->getFid().

  2. +++ b/core/modules/aggregator/lib/Drupal/aggregator/ItemInterface.php
    @@ -14,4 +14,135 @@
    +   * Returns the feed  id of aggregator item.
    

    Additional space in here.

  3. +++ b/core/modules/aggregator/lib/Drupal/aggregator/ItemInterface.php
    @@ -14,4 +14,135 @@
    +  public function getFid();
    ...
    +  public function setFid($fid);
    

    Let's do getFeedId() and setFeedId() instead, which is much more self-explaining.

  4. +++ b/core/modules/aggregator/lib/Drupal/aggregator/ItemInterface.php
    @@ -14,4 +14,135 @@
    +   * @param int $fid
    +   *   The feed id
    +   * @return \Drupal\aggregator\ItemInterface
    +   *   The called feed item entity.
    

    there needs to be an empty line between @param and @return.

  5. +++ b/core/modules/aggregator/lib/Drupal/aggregator/ItemInterface.php
    @@ -14,4 +14,135 @@
    +  public function  getLink();
    

    Another double space.

jsbalsera’s picture

Assigned: Unassigned » jsbalsera
jsbalsera’s picture

Assigned: jsbalsera » Unassigned
Status: Needs work » Needs review
FileSize
14.95 KB

It needed a re-roll, also made the changes.

Berdir’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/aggregator/aggregator.pages.inc
    @@ -78,21 +78,21 @@ function theme_aggregator_summary_item($variables) {
    +  if (isset($item->ftitle) && $item->getFid() != null) {
    +    $variables['source_url'] = url('aggregator/sources/' . $item->getFid());
    
    +++ b/core/modules/aggregator/lib/Drupal/aggregator/ItemStorageController.php
    @@ -50,7 +50,7 @@ public function deleteCategories(array $entities) {
    +    $result = $this->database->query('SELECT cid FROM {aggregator_category_feed} WHERE fid = :fid', array(':fid' => $item->getFid()));
    
    +++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/processor/DefaultProcessor.php
    @@ -171,17 +171,17 @@ public function process(Feed $feed) {
    +      $entry->setFid($feed->id());
    +      $entry->setLink($item['link']);
    

    Those calls should now use FeedId() instead of Fid()

  2. +++ b/core/modules/aggregator/lib/Drupal/aggregator/ItemInterface.php
    @@ -14,4 +14,136 @@
    +  public function setTitle($title);
    +
    +
    +  /**
    

    two empty lines here.

  3. +++ b/core/modules/aggregator/lib/Drupal/aggregator/ItemInterface.php
    @@ -14,4 +14,136 @@
    +  public function setAuthor($author);
    +
    +
    +  /**
    

    same here.

jsbalsera’s picture

Assigned: Unassigned » jsbalsera
jsbalsera’s picture

Assigned: jsbalsera » Unassigned
Status: Needs work » Needs review
FileSize
14.96 KB
Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, looks good to me!

ParisLiakos’s picture

Agreed it looks good, +1 for (set|get)FeedId()

+++ b/core/modules/aggregator/aggregator.pages.inc
@@ -78,21 +78,21 @@ function theme_aggregator_summary_item($variables) {
-  if (isset($item->ftitle) && isset($item->fid->value)) {
-    $variables['source_url'] = url("aggregator/sources/$item->fid->value");
+  if (isset($item->ftitle) && $item->getFid() != null) {
+    $variables['source_url'] = url('aggregator/sources/' . $item->getFeedId());

getFid() should be getFeedId()

null should be NULL (caps)

and !== instead of !=

But lets store it to a temporary variable to avoid same function call

otherwise looks good to me too, so leaving as RTBC.

Thanks!

johnennew’s picture

This follows best practice on the conversion to methods.

+1 for RTBC

alexpott’s picture

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

Patch no longer applies.

ParisLiakos’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
15.03 KB

FieldInterface => FieldItemListInterface

Berdir’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Novice, +Entity Field API

The last submitted patch, core-aggregator_item_properties-2028039-15.patch, failed testing.

johnennew’s picture

Issue tags: +Needs reroll
mcrittenden’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
15.01 KB
ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

thanks for reroll

tstoeckler’s picture

Looks good. For extra credit Item and ItemInterface could use an empty newline for the closing bracket of the class, but not setting to needs work for that. Could also be fixed pre-commit.

alexpott’s picture

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

Patch no longer applies.

xeniak’s picture

Status: Needs work » Needs review
FileSize
14.5 KB

Reroll.

tim.plunkett’s picture

Issue tags: -Needs reroll

Removing tags

ParisLiakos’s picture

reroll missed a spot. also removing an irrelevant change

johnennew’s picture

Status: Needs review » Reviewed & tested by the community

This minor change looks good to go.

Xano’s picture

alexpott’s picture

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

Patch no longer applies.

amateescu’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
14.92 KB

Rerolled.

alexpott’s picture

Title: Expand ItemInterface (aggregator.module) with methods » Changoe notice: Expand ItemInterface (aggregator.module) with methods
Priority: Normal » Major
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs change record

Committed bb54afd and pushed to 8.x. Thanks!

Berdir’s picture

Title: Changoe notice: Expand ItemInterface (aggregator.module) with methods » Expand ItemInterface (aggregator.module) with methods
Priority: Major » Normal
Status: Needs work » Fixed
Issue tags: -Needs change record

We don't enforce that the methods are used and we already have a general change notice that base fields now have methods that should be used: https://drupal.org/node/1806650. I don't think we need to reference every single issue that adds them, added the meta issue..

alexpott’s picture

Title: Expand ItemInterface (aggregator.module) with methods » Change notice: Expand ItemInterface (aggregator.module) with methods
Priority: Normal » Major
Status: Fixed » Needs work
Issue tags: +Needs change record
alexpott’s picture

Title: Change notice: Expand ItemInterface (aggregator.module) with methods » Expand ItemInterface (aggregator.module) with methods
Priority: Major » Normal
Status: Needs work » Fixed

xpost :)

Status: Fixed » Closed (fixed)

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

xjm’s picture

Issue summary: View changes
Issue tags: -Needs change record