Hi !

I implemented a specific feed parser, extending CSV parser, to parse IDX protocol based files (same format, without header, separating by #). I selected "delete" on the processor to remove items that disapears from the file. When I import the first time the file, parsing is successful and items are created. When I remove some lines in my files, the processor remove the item accordingly.

Now when I give an empty file to the parser, it bypass the "clean" event and old items remains in the system... It seems the clean method is called ONLY if there is one or more items in the file...

I don't know if it's possible, but maybe is there a possibility to "force" the cleaning of old items when the file is empty ? If anyone see a quick solution to implement it would be helpful.

Thanks in advance for any suggestion.

Issue fork feeds-3444986

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

titouille created an issue. See original summary.

titouille’s picture

titouille
Primary language French
commented

Ok, so if anyone search a quick answer to this problem, I solved it like this.

Because I have a IDXFetcherResult extending FetcherResult, I can override the "cleanUp" method.

I have a IDXFileFetcher too, with fetch() method. I used this fetch method to pass to the IDXFetcherResult constructor the $feed and the "update_non_existent" state : 

return new IDXFetcherResult(
  $path,
  $feed,
  $this->feedType->getProcessor()->getConfiguration('update_non_existent')
);

Now, on my IDXFetcherResult, I implemented the cleanUp method like this : 

  public function cleanUp() {

    // Get file content as string.
    $content = file_get_contents($this->filePath);

    // Check if file is empty.
    // If it's the case, and only if it's the case, we must
    // clean ALL previously imported items.
    // We must add this process because the standard feeds system
    // doesn't clean items when it encount an empty list.
    if (empty(trim($content))) {

      $storage = \Drupal::entityTypeManager()
        ->getStorage('my-entity-type');

      // Get list of entities to clean.
      $ids = $storage
        ->getQuery()
        ->accessCheck(FALSE)
        ->condition('feeds_item.target_id', $this->feed->id())
        ->condition('feeds_item.hash', $this->updateNonExistent, '<>')
        ->execute();

      if (!empty($ids)) {
        /** @var \Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher $dispatcher */
        $dispatcher = \Drupal::service('event_dispatcher');

        $entities = $storage->loadMultiple($ids);

        // Iterate over each entity and mimic the clean process.
        // We use the same process to avoid potential errors.
        foreach ($entities as $entity) {

          $dispatcher->dispatch(new InitEvent($this->feed, 'clean'), FeedsEvents::INIT_IMPORT);
          $dispatcher->dispatch(new CleanEvent($this->feed, $entity), FeedsEvents::CLEAN);
        }
      }
    }
  }

Hope this can help.

nigel cunningham made their first commit to this issue’s fork.

nigel cunningham opened merge request !192

nigelcunningham’s picture

nigelcunningham commented
Status: Active » Needs review

Attaching a patch I've prepared that ensures the cleanup is done when there is no data to import.

megachriz’s picture

megachriz
Primary language Dutch
commented
Status: Needs review » Needs work
Issue tags: +Needs tests

This looks like a good approach.

It does require fixing to tests and spelling:

And I think that this requires test coverage:

  • Add a test case to \Drupal\Tests\feeds\Kernel\UpdateNonExistentTest that demonstrates that content is cleaned when the source is empty.
  • Add a test case to \Drupal\Tests\feeds\Kernel\UpdateNonExistentTest that ensures that content is not cleaned when there is a fetch error. For example when the HTTP Fetcher receives a 403. Perhaps this case can be simplified with a custom fetcher that throws an exception of some kind. I think that a \Drupal\feeds\Exception\FetchException gets thrown on a 403.
  • Add a test case to \Drupal\Tests\feeds\Kernel\UpdateNonExistentTest that ensures that content is not cleaned when a fetch resulted into a case where no data changed. The HTTP Fetcher receives a 304 in that case and throws a \Drupal\feeds\Exception\EmptyFeedException. Perhaps this case can be simplified with a custom fetcher that just throws a EmptyFeedException.

The two latter test cases ensure that data does not get wiped out unintentionally.

nigelcunningham’s picture

nigelcunningham commented

Ah, the spelling isn't a mistake, it's because I'm not an American!

I'll see if I can find time to write the tests but I'm quite likely not to get around to it due to all the other things on my plate, sorry.

megachriz’s picture

megachriz
Primary language Dutch
commented

Ah, the spelling isn't a mistake, it's because I'm not an American!

Ah okay. I'm not American either. But I'm also not English. I just happen to see that the MR failed cspell tests. I think that the standards on drupal.org are American English. For example, elsewhere I see that the word "color" is used instead of the British "colour". So I think we should follow the American English spelling here.

I'll see if I can find time to write the tests but I'm quite likely not to get around to it due to all the other things on my plate, sorry.

I understand. Then this issue will likely hang here for a while. My plate is also quite big and I do my best to prioritize Feeds issues. Currently I think there are many issues with a higher priority than this one, so this one then will have to wait.

ruslan piskarov made their first commit to this issue’s fork.

codebymikey made their first commit to this issue’s fork.

codebymikey’s picture

codebymikey commented
Status: Needs work » Needs review

Updated the merge request with the relevant tests highlighted in #6 as well as addressed most of the deprecation and cspell warnings with the exception of the ZfExtensionManagerSfContainer.

megachriz’s picture

megachriz
Primary language Dutch
commented
Issue tags: -Needs tests

Wow! This looks great. In the code I could only spot that some of the code comments and names of methods could be improved. And that there are a few unrelated fixes. Ideally these should be handled in a separate issue, but I do appreciate getting these fixed too. 😀

I'm going to apply the changes on a live site that uses Feeds just to see if that results into any regressions.

Thanks for the work so far!

megachriz’s picture

megachriz
Primary language Dutch
commented
Status: Needs review » Fixed

I've tested the MR this way:

  • Tried to import a CSV file with nothing in it: no items were cleaned. This is good.
  • Tried to import a CSV file that did not exist (404): no items were cleaned. This is good.
  • Tried to import a CSV file that did not change (304): no items were cleaned. This is good.
  • Tried to import a CSV file that has only a header row: all items were cleaned. This is good.
  • Tried to import a XML file with zero items in it: all items were cleaned. This is good.

I went through the code one more time and only made some small changes to code comments.

So I think all is good. Merged! Thanks all for contributing.

Status: Fixed » Closed (fixed)

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