Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
To be more consistent with the rest of core it seems to make sense to rename the removing of aggregation items to "delete".
Comment | File | Size | Author |
---|---|---|---|
#23 | interdiff-20-23.txt | 1.56 KB | martin107 |
#23 | drupal_aggregator_use_delete_over_remove-1878302-23.patch | 20.78 KB | martin107 |
#20 | drupal_aggregator_use_delete_over_remove-1878302-20.patch | 19.22 KB | martin107 |
#20 | interdiff-17-20.txt | 1.54 KB | martin107 |
#17 | interdiff-14-17.txt | 1.03 KB | martin107 |
Comments
Comment #1
chrisroane CreditAttribution: chrisroane commentedUpdated the remove text
Comment #2
chrisroane CreditAttribution: chrisroane commentedComment #4
ParisLiakos CreditAttribution: ParisLiakos commented#1: aggregator-renameremove-707484.patch queued for re-testing.
Comment #6
ParisLiakos CreditAttribution: ParisLiakos commented#1: aggregator-renameremove-707484.patch queued for re-testing.
Comment #7
ParisLiakos CreditAttribution: ParisLiakos commentedmakes sense
Comment #8
alexpottI 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
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.
Comment #9
Anonymous (not verified) CreditAttribution: Anonymous commentedReroll the patch as per the points in the #8.
Comment #10
areke CreditAttribution: areke commentedThe patch no longer applies; it needs to be re-rolled.
Comment #11
InternetDevels CreditAttribution: InternetDevels commentedComment #12
sandipmkhairnar CreditAttribution: sandipmkhairnar commentedThanks InternetDevels. I have checked the patch its working fine for me.
Comment #13
awochna CreditAttribution: awochna commentedThis patch no longer applies. Needs reroll.
Comment #14
awochna CreditAttribution: awochna commentedComment #15
martin107 CreditAttribution: martin107 commentedReroll.
Comment #17
martin107 CreditAttribution: martin107 commentedCorrections to newly added tests.
Comment #18
Sutharsan CreditAttribution: Sutharsan commentedComment #19
ParisLiakos CreditAttribution: ParisLiakos commentedoops, typo:P
But since we are there change it to inheritdoc
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
Comment #20
martin107 CreditAttribution: martin107 commentedThanks for the diff -M tip
I will add it as an optional parameter to step 13 of https://drupal.org/patch/reroll
Comment #21
ParisLiakos CreditAttribution: ParisLiakos commentedThank you:)
we need to update existing change notices about this change here (which i guess should happen after commit?)
Comment #22
alexpottWhat about
Drupal\aggregator\Tests\RemoveFeedTest
?Comment #23
martin107 CreditAttribution: martin107 commentedRenamed file, fixed class name, getInfo() references etc.
Comment #24
martin107 CreditAttribution: martin107 commentedComment #25
alexpottCommitted 81d8c4a and pushed to 8.x. Thanks!
We need to ensure that the change notices for aggregator are up to date.
Comment #26
ParisLiakos CreditAttribution: ParisLiakos commentedThanks all
change notices updated
https://drupal.org/node/1957310
https://drupal.org/node/2191221