Deleted and edited aggregator feeds are restored from queue

This is still a problem. Old settings are restored to edited feeds. Deleting a queued feed breaks queue runs, although I'm not sure it's broken in the same way. Here's a reroll.

This looks great to me, the only thing I think we're missing is a hook_post_update to truncate the existing queue items that would have the entity stored, which is no longer compatible with the queue processor.
Thanks for working on this!

@larowlan I'm sorry for all the test spam above. I'm not sure why, but it kept being weird. It queued up two identical tests at the same time for one branch. It tested the wrong branch repeatedly (and just did it again). I feel bad for taking up resources on the test server, but it wasn't something I did intentionally.

Thanks for working on this!

No problem. The other day I looked through my old unclosed tickets and saw these. It's disappointing that this had tests and and a fix, but never got a proper review. I guess it just shows how little attention Aggregator got in Core.

No worries, if you're interested I'd be keen for a co-maintainer here - let me know

Here's an attempt at a post update function and a test for it. I'm positive the test isn't going to work, but I can't test it locally. I've never written a post update function before, let alone a test for one.

And yes, @larowlan, if you would like some help with the module, then I'm willing to co-maintain it. Thank you for the offer!

The last submitted patch, 75: 1067532-75.patch, failed testing. View results →
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The test just looks like the path to the dump file isn't right

Other than that it looks good to me

Added 📌 Add dcam as a co-maintainer Fixed

I've added you as a maintainer - thanks!

We don't actually need a fixture anyway. It's sufficient to create a queue item and then let it be deleted by the update. The update system just has to be tricked into running the update right after installing the module in the test setup(). This one is passing for me locally.

1. +++ b/tests/src/Functional/AggregatorCronTest.php
   @@ -18,6 +19,13 @@ class AggregatorCronTest extends AggregatorTestBase {
   +  protected static $modules = array('editor');
   

   What's the significance of editor here?

2. +++ b/tests/src/Functional/AggregatorCronTest.php
   @@ -45,6 +53,32 @@ class AggregatorCronTest extends AggregatorTestBase {
   +    $this->resetAll();
   

   is \Drupal::entityTypeManager()->getStorage('aggregator_feed')->loadUnchanged($feed->id()) enough here? ::resetAll is a bit of a sledgehammer approach

3. +++ b/tests/src/Functional/AggregatorCronTest.php
   @@ -45,6 +53,32 @@ class AggregatorCronTest extends AggregatorTestBase {
   +    $this->assertEquals(0, $database->query('SELECT COUNT(*) FROM {aggregator_feed}')->fetchField(), 'There are no feeds in the database.');
   

   Can we use assertCount(0, Feed::loadMultiple()) instead of a DB query?

4. +++ b/tests/src/Functional/AggregatorCronTest.php
   @@ -45,6 +53,32 @@ class AggregatorCronTest extends AggregatorTestBase {
   +    $this->assertEquals(1, $database->query("SELECT COUNT(*) FROM {queue} WHERE name = 'aggregator_feeds'")->fetchField(), 'There is one queued feed in the database.');
   ...
   +    $this->assertEquals(0, $database->query("SELECT COUNT(*) FROM {queue} WHERE name = 'aggregator_feeds'")->fetchField(), 'There are no queued feeds in the database.');
   

   Can we use $query->numberOfItems() instead?

5. +++ b/tests/src/Functional/AggregatorCronTest.php
   @@ -45,6 +53,32 @@ class AggregatorCronTest extends AggregatorTestBase {
   +    $this->assertEquals(0, $database->query('SELECT COUNT(*) FROM {aggregator_feed}')->fetchField(), 'There are still no feeds in the database.');
   

   here too

6. +++ b/tests/src/Functional/Update/AggregatorUpdateDeleteQueueTest.php
   @@ -0,0 +1,69 @@
   +    parent::setUp();
   +
   +    // Since Aggregator was just installed Drupal thinks that it doesn't need to
   +    // run the update. Remove it from the update list so it can be run by the
   +    // test.
   +    $key_value = \Drupal::service('keyvalue');
   +    $existing_updates = $key_value->get('post_update')->get('existing_updates', []);
   +    if ($key = array_search('aggregator_post_update_delete_queue_items', $existing_updates)) {
   +      unset($existing_updates[$key]);
   +    }
   

   Neat trick!

What's the significance of editor here?

See comment #31. Years ago the test simply failed unless the Editor module was installed. We weren't sure why. But I just re-ran the test without it and it passed. So maybe it's no longer a problem outside of Core or it was a symptom of a separate problem that got fixed. I'll upload a new patch without it.

is \Drupal::entityTypeManager()->getStorage('aggregator_feed')->loadUnchanged($feed->id()) enough here?

Works for me.

Can we use assertCount(0, Feed::loadMultiple()) instead of a DB query?

That's a good idea.

Can we use $query->numberOfItems() instead?

I meant to change that, but forgot. Thanks for catching it. The only reason that I can think of that I might have avoided it years ago was because the docblock for numberOfItems() says 'This is intended to provide a "best guess" count of the number of items in the queue' and it isn't clear why. So maybe I didn't trust it.

Neat trick!

Thanks!

This is fixed for contrib Aggregator. The patch in #82 needs to be rerolled for Core.

9.5 only gets security fixes now, moving back to fixed.

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