Updated: Comment #12

Problem/Motivation

The remove items logic is in the aggregator feed entity but, the refresh one is in a procedural function. This just hurts DX by making things more confusing to discover.

Proposed resolution

Introduce a service and move all related logic there.
Keep the $feed->removeItems() method there as a shortcut and also add a $feed->refreshItems() for consistency.
I dont see a reason why not and i think it boosts DX

Remaining tasks

Patch is there, review rtbc

User interface changes

None

API changes

aggregator_refresh_items() becomes deprecated

Original report by @ParisLiakos

The remove items logic is already in the Feed entity, but not the refresh items, which still is a procedural function.
Lets move it too in the Feed entity to make things cosistent.
In the process get rid of the @todo in AggregatorController::feedRefresh()

CommentFileSizeAuthor
#26 drupal-aggregator_refresh-2159369-26.patch17.91 KBParisLiakos
#24 interdiff.txt738 bytesParisLiakos
#24 drupal-aggregator_refresh-2159369-24.patch17.91 KBParisLiakos
#22 drupal-aggregator_refresh-2159369-22.patch17.91 KBParisLiakos
#21 drupal-aggregator_refresh-2159369-21.patch17.89 KBParisLiakos
#21 interdiff.txt7.35 KBParisLiakos
#20 drupal-aggregator_refresh-2159369-20.patch14.34 KBParisLiakos
#14 interdiff.txt978 bytesParisLiakos
#14 drupal-aggregator_refresh-2159369-14.patch14.26 KBParisLiakos
#12 drupal-aggregator_refresh-2159369-12.patch14.26 KBParisLiakos
#4 drupal-aggregator_refresh-2159369-4.patch8.83 KBParisLiakos
#2 drupal-aggregator_refresh-2159369-2.patch8.83 KBParisLiakos
#1 drupal-aggregator_refresh-2159369-1.patch8.91 KBParisLiakos
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Status: Active » Needs review
FileSize
drupal-aggregator_refresh-2159369-1.patch8.91 KB
ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Assigned: ParisLiakos » Unassigned
Issue tags: +API clean-up
FileSize
drupal-aggregator_refresh-2159369-2.patch8.83 KB

reroll

Status: Needs review » Needs work

The last submitted patch, 2: drupal-aggregator_refresh-2159369-2.patch, failed testing.

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Status: Needs work » Needs review
FileSize
drupal-aggregator_refresh-2159369-4.patch8.83 KB

oops

Status: Needs review » Needs work

The last submitted patch, 4: drupal-aggregator_refresh-2159369-4.patch, failed testing.

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Status: Needs work » Needs review

4: drupal-aggregator_refresh-2159369-4.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 4: drupal-aggregator_refresh-2159369-4.patch, failed testing.

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Status: Needs work » Needs review 
Fatal error: Allowed memory size of 268435456 bytes exhausted (tried to allocate 268435456 bytes) in /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Database/Connection.php on line 336
FATAL Drupal\book\Tests\BookTest: test runner returned a non-zero error code (255).

4: drupal-aggregator_refresh-2159369-4.patch queued for re-testing.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

that could be a loop, but I have no idea how a book fail is related to aggregator :)

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented

it now passed:) but yes, i dont see the relation here, i really doubt those failures are this patch's effects

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Is it just be that it feels wrong to put soo much logic into an entity instead of a nice decoupled service?

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Title: Move aggregator_refresh() logic to Feed entity » Move aggregator_refresh() and most of $feed->removeItems() logic to a service
Issue summary: View changes
FileSize
drupal-aggregator_refresh-2159369-12.patch14.26 KB

It certainly does. I just tried to do smaller steps, but yes, probably too small.
so, i created aggregator.items.importer service and moved the remove and refresh items there, which now makes the feed entity look a lot better.

sorry, i messed up interrdiff, but i doubt it would be much of a help.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/modules/aggregator/lib/Drupal/aggregator/Controller/AggregatorController.php
@@ -123,8 +123,10 @@ protected function buildPageList(array $items, $feed_source = '') {
-    // @todo after https://drupal.org/node/1972246 find a new place for it.
-    aggregator_refresh($aggregator_feed);
+    $message = $aggregator_feed->refreshItems()
+      ? $this->t('There is new syndicated content from %site.', array('%site' => $aggregator_feed->label()))
+      : $this->t('There is no new syndicated content from %site.', array('%site' => $aggregator_feed->label()));
+    drupal_set_message($message);

Nice!

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
FileSize
drupal-aggregator_refresh-2159369-14.patch14.26 KB
interdiff.txt978 bytes

thank you!
just removing an unrelated newline addition and a forgotten use statement

The last submitted patch, 12: drupal-aggregator_refresh-2159369-12.patch, failed testing.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Issue tags: +Needs change record

Reminder: you can now create a draft change record for API changes.
https://groups.drupal.org/node/402688
A change record will be required for an API change to be committed starting Feb. 14, but you're encouraged to create a draft for this issue now as well. :)

Also, rather than deprecating the function, should we consider whether simply removing it is appropriate, given the efforts elsewhere to get rid of @deprecated cruft?

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented

Also, rather than deprecating the function, should we consider whether simply removing it is appropriate, given the efforts elsewhere to get rid of @deprecated cruft?

aggregator_queue_info() still uses it as worker callback, so i dont think it should be removed
before those are converted to plugins

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Issue tags: -Needs change record

added https://drupal.org/node/2191221 as draft
also included the aggregator_remove() which didnt have a change notice

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

drupal-aggregator_refresh-2159369-14.patch no longer applies.

error: patch failed: core/modules/aggregator/aggregator.services.yml:16
error: core/modules/aggregator/aggregator.services.yml: patch does not apply
ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
drupal-aggregator_refresh-2159369-20.patch14.34 KB

Reroll

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
FileSize
interdiff.txt7.35 KB
drupal-aggregator_refresh-2159369-21.patch17.89 KB

actually, i was wrong in #17
We can use an anonymous function for that and remove the global aggregator_refresh() one (Which is really cool:))

 function aggregator_queue_info() {
   $queues['aggregator_feeds'] = array(
     'title' => t('Aggregator refresh'),
-    'worker callback' => 'aggregator_refresh',
+    'worker callback' => function (FeedInterface $feed) {
+      $feed->refreshItems();
+    },
     'cron' => array(
       'time' => 60,
     ),

I also removed the save() call from the ItemsImporter service to the Feed::refreshItems(), to make it even more consistent with how removeItems() works

Change notice updated, tests are green locally

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
FileSize
drupal-aggregator_refresh-2159369-22.patch17.91 KB

reroll

Status: Needs review » Needs work

The last submitted patch, 22: drupal-aggregator_refresh-2159369-22.patch, failed testing.

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
Status: Needs work » Needs review
FileSize
drupal-aggregator_refresh-2159369-24.patch17.91 KB
interdiff.txt738 bytes

ugh

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

This just looks perfect!

ParisLiakos’s picture

ParisLiakos CreditAttribution: ParisLiakos commented
FileSize
drupal-aggregator_refresh-2159369-26.patch17.91 KB

reroll after #1878302: Use 'delete' over 'remove' in the aggregator module went in

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

This looks like really nice clean-up.

Committed and pushed to 8.x. Thanks!

  • Commit c1c71a6 on 8.x by webchick: 
    Issue #2159369 by ParisLiakos: Move aggregator_refresh() and most of ->...
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

?

Status: Fixed » Closed (fixed)

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