Error: Call to a member function unlink() on null in Drupal\feeds\Result\HttpFetcherResult->cleanUp()

Relevant contrib code:

/**
   * {@inheritdoc}
   */
  public function cleanUp() {
    if ($this->filePath) {
      $this->fileSystem->unlink($this->filePath);
    }
  }

Offhand the error appears to be the sanity check is looking for if there is a filepath, and if so, call that method on filesystem. Welp, it's possible apparently to get to that point with a filesystem being established.

File system is set in the constructor:

  /**
   * Constructs an HttpFetcherResult object.
   *
   * @param string $file_path
   *   The path to the result file.
   * @param array $headers
   *   An array of HTTP headers.
   * @param \Drupal\Core\File\FileSystemInterface $file_system
   *   (optional) The file system service.
   */
  public function __construct($file_path, array $headers, FileSystemInterface $file_system = NULL) {
    parent::__construct($file_path);
    $this->headers = array_change_key_case($headers);
    if (is_null($file_system)) {
      $file_system = \Drupal::service('file_system');
    }
    $this->fileSystem = $file_system;
  }

In theory... we either get NULL for file system or an object that meets the FileSystemInterface requirement, which in turn has the method UnLink, so that's a dead end. So we have to be passing in NULL, which in turn calls \Drupal::service('file_system');... which may be returning NULL in some instances?

It's late and that's as far as I've puzzled.

Hi @jnicola,
I have fixed this issue Call to a member function unlink() on null in Drupal\feeds\Result\HttpFetcherResult->cleanUp() and also attached patch,
please review and verify.

Nice, thanks for the patch! A very easy fix indeed. We may utilize it. I guess I just wonder if this is a symptom of a larger issue or if this should have some logic in the constructor and throw an exception there about not having a file system instantiated yet, etc etc.

Thanks for the patch, but I think it is not the right solution. This solution essentially conditionally skips the cleanup. This could theoretically lead to a non-stop growing filesystem. I say theoretically, because a cleanup of the same file can happen at an other stage in the process as well.

I wonder how this issue can occur. If possible, it would be helpful if you could find the steps to reproduce the issue. Though I can understand that it could be difficult to figure out in this case.

For what's worth: in an earlier version of the code there was a procedural function call. This later had to be replaced with dependency injection because the procedural function got deprecated.

It looks like that somehow that dependency got emptied. Could it be that that happens when serializing and unserializing again?

Man I wish we could get you steps to reproduce! We've been trying to consistently get this and it just happens one time out of a thousand or so. One theory I have is it is happening when "poormans cron" is triggered on a Drupal request resulting from an AJAX request, and Drupal isn't fully bootstrapped. So NULL is passed in, and then it attempts to load the Drupal service and that in turn returns NULL but FEEDS isn't looking for that.

Another coworker says they can trigger this all from CLI drush cron calls though so... maybe I'm not right about that?

We encountered this while upgrading our site from Drupal 9 Drupal 10.

The issue in our case might be that we moved the production database to staging. And we did not move the files at the same time. And it seems like feeds is trying to cleanup a temporary file that is not present anymore.

I added some logging and noticed that it was trying to remove the following file:
/var/www/html/shared/temporary/feeds_http_fetcher15nWQr

When looking at the file system, I can see that the temporary file is there. The problem seems to be that other temporary files are getting more permissions:
-rwxrwxrwx.

But the feed temporary files are just
-rw-------.

And I think that causes the issue that the temporary file cannot be unlinked.

We noticed that we are using a patch from another issue that is very similar to this one in another project of ours, 🐛 Delete Empty Files Created If Empty/Error Exception is Returned Needs review . It's also related to unlinking files from the system. I also tested that changing file permissions alone did not fix the issue in our project.

A small update. I tested the patch from 🐛 Delete Empty Files Created If Empty/Error Exception is Returned Needs review . It did not solve the unlink() error. Testing this patch got rid of the error and now the feeds started to actually update and I get messages like this:

Feed Source: There are no new RSS items.

I think the unlink() error was fatal and it never was able to process the feeds completely.

Hm, I've got an idea about what could be the issue. I see that the property $fileSystem is new in Feeds 8.x-3.0-beta4 (I thought it was there for much longer, but I may got confused with an other class). So if there were any imports not completed yet before updating to Feeds 8.x-3.0-beta4, this error could occur.

So the fix would be to ensure that the service exists when unserializing a HttpFetcherResult object.

Let's see if I can come up with a fix.

I've provided a possible fix. :)

@HeikkiY
The issue of files not getting cleaned up or fail to get cleaned up sounds like an other issue. There are a few other issue open related to that issue, including the issue you already linked:
🐛 Delete Empty Files Created If Empty/Error Exception is Returned Needs review
🐛 in_progress filesystem grows indefinitely Active
🐛 FetcherResult::checkFile() throws \RuntimeException, which is never caught Needs review

Thank you @MegaChriz for merge request 147 in comment #11. Applying the patch fixed the error and allowed cron to finish! I work with @jnicola so this addresses the problem the issue was created for.

However after applying the patch, on the first cron run I saw this warning:

[warning] unlink(/mnt/tmp/projectname/feeds_http_fetcherw5KhhB): No such file or directory FileSystem.php:124

I have not seen this warning on subsequent cron runs.

I've merged the code!

The unlink warning tells us that the file that Feeds tried to delete was already deleted. I'm not sure if that issue is worth to address. Maybe when it would happen a lot. But since 8.x-3.0-beta4, Feeds now stores http downloaded files in a in_progress directory on either the private or the public filesystem. So things have changed that much, that the warning may only occur for data stored by Feeds before 8.x-3.0-beta4.

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