Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chrisroane’s picture

Updated the remove text

chrisroane’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, aggregator-renameremove-707484.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, aggregator-renameremove-707484.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

makes sense

alexpott’s picture

Title: Rename "remove items" to "delete items" » Use 'delete' over 'remove' in the aggregator module
Status: Reviewed & tested by the community » Needs work

I think we should go the whole way and rename the routines removeFeedItems(), updateAndRemove(),aggregator_remove(), ProcessorInterface::remove()

Also the route /admin/config/services/aggregator/remove/{aggregator_feed}

Also the confirm form question Are you sure you want to remove all items from the feed %feed?

The message The news items from %site have been removed.

Also update comments

   * Called by aggregator if either a feed is deleted or a user clicks on
   * "remove items".
   * Tests running "remove items" from 'admin/config/services/aggregator' page.
      // Remove all items that are older than flush item timer.

RemoveFeedItemTest which itself should be renamed...

Basically just search for remove in the aggregator code base... because just limiting the scope to "remove items" => "delete items" actually makes aggregator inconsistent.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
19.28 KB

Reroll the patch as per the points in the #8.

areke’s picture

Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs reroll

The patch no longer applies; it needs to be re-rolled.

InternetDevels’s picture

Status: Needs work » Needs review
FileSize
37.93 KB
22 KB
sandipmkhairnar’s picture

Thanks InternetDevels. I have checked the patch its working fine for me.

awochna’s picture

This patch no longer applies. Needs reroll.

awochna’s picture

Status: Needs review » Needs work
martin107’s picture

Status: Needs work » Needs review
FileSize
21.92 KB

Reroll.

Status: Needs review » Needs work

The last submitted patch, 15: drupal_aggregator_use_delete_over_remove-1878302-14.patch, failed testing.

martin107’s picture

Status: Needs work » Needs review
FileSize
21.92 KB
1.03 KB

Corrections to newly added tests.

Sutharsan’s picture

Issue tags: -Needs reroll
ParisLiakos’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/aggregator/processor/DefaultProcessor.php
    @@ -193,17 +193,17 @@ public function process(FeedInterface $feed) {
    -   * Implements \Drupal\aggregator\Plugin\ProcessorInterface::postProcess().
    +   * Itmplements \Drupal\aggregator\Plugin\ProcessorInterface::postProcess().
    

    oops, typo:P

    But since we are there change it to inheritdoc

  2. +++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/FeedParserTest.php
    @@ -23,7 +23,7 @@ public static function getInfo() {
    -    // Do not remove old aggregator items during these tests, since our sample
    +    // Do not deleted old aggregator items during these tests, since our sample
    

    should be "Do not delete "

Also would be nice to git diff with the -M option. Makes the patch easier to read when there are renamed/moved files

martin107’s picture

Status: Needs work » Needs review
FileSize
1.54 KB
19.22 KB

Thanks for the diff -M tip

I will add it as an optional parameter to step 13 of https://drupal.org/patch/reroll

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

Thank you:)

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Plugin/ProcessorInterface.php
@@ -46,14 +46,14 @@ public function process(FeedInterface $feed);
-  public function remove(FeedInterface $feed);
+  public function delete(FeedInterface $feed);

we need to update existing change notices about this change here (which i guess should happen after commit?)

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

What about Drupal\aggregator\Tests\RemoveFeedTest?

martin107’s picture

Status: Needs work » Needs review
FileSize
20.78 KB
1.56 KB

Renamed file, fixed class name, getInfo() references etc.

martin107’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 81d8c4a and pushed to 8.x. Thanks!

We need to ensure that the change notices for aggregator are up to date.

ParisLiakos’s picture

  • Commit 81d8c4a on 8.x by alexpott:
    Issue #1878302 by martin107, InternetDevels, nick_daffodil, chrisroane:...

Status: Fixed » Closed (fixed)

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