Clear feed hashes if the expire setting changes

Postponing this on the resolution of 📌 Convert the Feed hash to the State API Fixed . If that one makes it in, then this will be easier. Really, all we need to do is delete all of the hashes if the expire setting is updated. The only reason we have the hashes in the first place is for optimization, preventing feeds from being processed if they haven't changed.

I'm not sure if this is a bug either, but I can definitely agree that it's undesirable behavior and we need to make a change.

Nice 👌

The new state service is in, so this issue is unblocked.

I think we should add a description to the expire setting's form element that explains if it's changed, then it will cause all feeds to be completely reprocessed, potentially resulting in long wait times for imports on sites with large numbers of feeds.

@larowlan I need your opinion on this. While I was creating the new state service I just assumed that eventually I'd add a deleteAllHashes() function in the ItemsImporter service to fix this issue. So I started writing it and quickly realized that I was adding a function for other classes to depend on, but it's not included in the interface. In fact, the service's hash functions should probably be protected except that the deprecated Feed functions say to use those instead. I haven't decided on a way to work around this, but I've come up with several options.

1. Add the hash functions to ItemsImporterInterface - This would be a BC break. And isn't necessarily a good idea anyway because the hash is a function of our default importer, not something overriding services should have to implement.
2. Add a HashStorage service that the importer and the settings form can both use. Then put the hash functions in it.
   The deprecated Feed functions would be updated to point at these instead.
3. Add a more general state storage service that can be reused for the other Feed properties that we've talked about converting to state.
4. To not overthink it and just let the settings form delete all the hashes.

I think all of the options have potential problems. I'd like to know what you think and if you have other ideas.

2 or 3 work for me, or a simple option is a variant of 1 where we add a new interface that ItemsImporter implements so we retain BC

eg SomeNewInterfaceThatSupportsDeletingHashes

class ItemImporter implements SomeNewInterfaceThatSupportsDeletingHashes, ItemImporterInterface

I've been thinking about this for days analyzing the options. I just can't see a way to do this by changing or adding an interface without potentially causing a problem. And once I realized that adding a service meant that we'd be adding a service to wrap our own service to fix its shortcomings I realized how ridiculous it sounded.

So I wrote a patch that implements option 4. The function to clear the hashes is short and sweet. I don't think it has any dependency issue and it's an OK solution. Please let me know what you think.

The inline @var comment in the test referenced the wrong class.

+++ b/src/Plugin/aggregator/processor/DefaultProcessor.php
@@ -173,12 +186,27 @@ class DefaultProcessor extends AggregatorPluginSettingsBase implements Processor
+      $this->keyValue->get($id)->delete(ItemsImporter::AGGREGATOR_HASH_KEY);

was going to suggest we use deleteMultiple but then realised they're segmented by feed ID

Turned out fairly clean in the end

was going to suggest we use deleteMultiple but then realised they're segmented by feed ID

Right?! I was disappointed by that too.

I didn't mean to mark it as fixed until I'd patched 1.x, but #20 didn't apply.

Thanks for the review, @larowlan!

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