ImageItem presave() fails when pointing to a non-existing file entity

I have encountered this issue, but my error is not exactly like main error.

Fatal error: Call to a member function getFileUri() on null in E:\drupal\core\modules\image\src\Plugin\Field\FieldType\ImageItem.php on line 322
Drush command terminated abnormally due to an unrecoverable error.

Sightly different in that the function is called on null, not a non-object.

I believe I have encountered this error, but on d6 migrations. It happens during the fid lookup in core/modules/file/src/Plugin/migrate/process/d6/CckFile.php. See #2640842: Make related file migration ID configurable in d6_cck_file

I also am experiencing this bug. With 8.0.x it worked properly. The problem have started after update to 8.1.x.

I am also finding this same error on a custom d6 to d8 migration.

My destination is a node bundle (article) and the corresponding field should have been skiped as some fid are empty and the field is not required. This is my field migration config:

field_image:
    -
      plugin: skip_on_empty
      method: process
    -
      plugin: migration
      migration: opcions_files
      source: fid

I have hacked the ImageItem adding a check for $this->entity in the conditional. It however is trying to insert a empty string in as the fid so I guess the migration is trying to save a field_image field that should be considered empty on the entity destination.

Not sure how to debug further. I think this is more relevant to the migration system, though.

Having the same issue, migrating d > 8. Drupal 8.16 core.
Fatal error: Call to a member function getFileUri() on a non-object in core/modules/image/src/Plugin/Field/FieldType/ImageItem.php on line 317.

I have deleted the images from the content type that i wanted to migrate and rest of fields could be migrated. Make sure that content types field format are set to default. eg. if you have plain text it will not be migrated.

As @sclapes suggested, changing the line to read:

if ((empty($width) || empty($height)) && $this->entity) {

appears to work and allows the upgrade to proceed.

Although triggered during migration, the root problem is in the image module's ImageItem class. I'm guessing the reason it's not been reported outside of migration is that existing cases are always setting the dimensions explicitly - next step should be a fail test leaving the dimensions empty.

I'm seeing this triggered when $is_file = is_a($this->entity, \Drupal\file\FileInterface::class); returns FALSE. $this->entity isn't empty, it is just a string and not an File class.

Patch on #17 solves the issue. Now the migration runs normally. In my case, now I can continue to debug my migration ;)

I applied the patch directly on 8.2.0. Thanks @heddn

Based on #18, marking as RTBC

When does $this->entity get set to a string and not an entity? Should we be enforcing it gets set further up the stack instead?

re #20: it comes from a migration from D6/D7. And right now migrations do not validate any data, they just stuff it into the destination. After #2745797: Add option to content entity destinations for validation, we could turn on that validation, but that has its own set of problems.

Confirming this is necessary and works for the significant migration from Drupal 7 use case! Seems a small and reasonable validation in ImageItem. Setting back to RTBC.

If it happens with migrations, does that mean we're missing test coverage for migrations? I'd rather see that than the test hunk added here, but in general still feels like the wrong approach.

A few thoughts about #23.

• The life of D8 is such that there are a lot of live sites already, so bad data already exists. Ergo a fix here is appropriate for those sites.
• There is a validation patch at #2745797: Add option to content entity destinations for validation that would pick up some validation, however, I'm going to guess that most folks aren't going to enable it. It catches too many problems and an easier solution in a lot of cases is to migrate the data then fix it on the new site.
• There's an awful lot of cruft on old sites. To assume that it is all going to be fixed during a migration is a false hope.
• Migrations of files take place in two steps. First is to migrate files. Second step is to migrate the file entity references. If *anything* goes wrong in the files migration (which is notoriously difficult), then you could encounter this problem.

TLDR; Yes, it would be nice to catch this in migrate. No, it isn't very likely to happen very soon. In the mean time, this has good test coverage and fixes a know problem faced by many sites today.

The life of D8 is such that there are a lot of live sites already, so bad data already exists. Ergo a fix here is appropriate for those sites.

That's not just a migration issue then.

Also this hides bad data with no error though on those existing sites - so for example if you had a bug where files go missing (which we do have several of due to file_usage being broke), they could continue to go missing and nothing's going to tell you unless you actually spot the specific file. Maybe switching from an exception to E_USER_WARNING might be appropriate.

I really like that idea. Right now it isn't an exception, it is a fatal so improving that to a E_USER_WARNING is much nicer. Then the migration won't fail half-way through and all those warnings can be reviewed in the logs and fixed on a per file basis.

I can confirm that the patch submitted on #17 works for a D7 -> D8 migration (v8.2.0).

I agree, a E_USER_WARNING for this case would be a good thing to have.

Assigning to myself to work on this.

adding E_USER_WARNING for preSave()

+++ b/core/modules/image/src/Plugin/Field/FieldType/ImageItem.php
@@ -313,13 +314,16 @@ public function preSave() {
+        trigger_error(sprintf("Expected value to be a entity image, %s given", gettype($this->entity)),E_USER_WARNING);

1. We typically get this error when $this->entity is null. And gettype() doesn't provide very helpful info. Possibly better to word: "A malformed file was provided. Cannot determine its dimensions."
2. Nit: Need space between comma in gettype($this->entity)),E_USER_WARNING

thanks! @heddn

I can confirm that the patch submitted on #17 works.

Many thanks!

Restesting since the failed test passes locally.

Having this same problem on my upgrade. Tried the latest patch, but it failed with errors. (I am using PHP 5.6 if that matters.)

re #38: what errors?

I got this error message:

An AJAX HTTP error occurred.
HTTP Result Code: 200
Debugging information follows.
Path: /my_drupal_directory/batch?id=14&op=do_nojs&op=do
StatusText: OK
ResponseText:
Fatal error: Call to a member function getFileUri() on null in .../core/modules/image/src/Plugin/Field/FieldType/ImageItem.php line 317

re #40, the error you are getting shows the patch is *not* applied. That error should go away once the patch here is added. It should get converted into a E_USER_WARNING error.

Waiting on more feedback than just #36, but I think this is close to RTBC.

The patch works for me in a D6 -> D8 migration.

#36 & #42 confirm this fixes things. For #38, there seems to be a question if the patch was properly applied. Based on that, I think this can proceed to RTBC, yes?

Which patch is being reviewed?

re #45: the patch in #35

Patch #35 removed the error for me. In our particular case, we were saving images via inline-entity-form from the panelizer IPE interface.

The patch in #35 is failing tests?

To be more exact, that test is wrong, a trigger error is not an exception, and it can't be tested like that.

I'm not sure if it even makes sense to trigger an error, I'd just check it and move on if it's not valid. Testing that it does *not* fail is enough.

Assigning to myself to work on tests.

So, the test in #35 is kinda doing the right thing. PHP Unit causes errors to be thrown as exceptions. See https://phpunit.de/manual/4.8/en/appendixes.configuration.html. So the warning gets converted to an exception in the presave and gets converted to a EntityStorageException. The failure is coming from a fixture in the node migration that doesn't have a valid File migration already run.

It seems like #2488860: Bring phpunit bridge into drupal and use it for unit tests and simpletest to handle Deprecation might be able to help with some of this, except its focus right now is really user deprecated. Not a general solution for handling these warnings.

Going to put this to the side for now as I've run out of time. I tried adding d7_file to the list of executed migrations in d7/MigrateNodeTest. But that didn't fix things. I wonder if we have a bad set of data in the fixtures for D7 files.

+++ b/core/modules/image/tests/src/Kernel/ImageItemTest.php
@@ -108,6 +109,15 @@ public function testImageItem() {
+    $this->setExpectedException(EntityStorageException::class, "A malformed file was provided. Cannot determine its dimensions.");
+    $entity->save();
+
+    $this->assertEmpty($entity->image_test->width);
+    $this->assertEmpty($entity->image_test->height);

The problem here is that setExpectedException() means 'when you get this exception, the test is over.'

You could change the following assertEmpty() calls to assertNotEmpty() and the test will still pass, because they're after the exception. Basically, this patch swallows all the assertions after the exception.

So: Verifying the error-throwing behavior should be a separate test method. You'll have to use a try/catch block in order to check the values of height and width after the exception is thrown.

You don't really need the error handler fancy stuff from #2488860: Bring phpunit bridge into drupal and use it for unit tests and simpletest to handle Deprecation, just a try/catch.

I'd much rather see an exception thrown here to signal that there's no image entity when you expect one, but yah, what a pain for migrations.

Thanks for the hints on the exception catching Miles. Now to track down the failure in d7 node. It seems like a missing fixture for the file migration. But we'll have to dig into the migration to prove that first.

I ran into the same problem on D8 and fixed it like this.

All we now need a failing test.

@jibran, we've got basically the same patch earlier in #35. We've just been working on tests since then. I'm just now finishing what I think we need.

Interdiff is against #35. Tests pass locally. Let's see what the bot says.

+++ b/core/modules/image/src/Plugin/Field/FieldType/ImageItem.php
@@ -313,13 +314,18 @@ public function preSave() {
+    if (($this->entity instanceof FileInterface)) {
...
+    else {
+      trigger_error(sprintf("A malformed file was provided. Cannot determine its dimensions."), E_USER_WARNING);
+    }

No offence but I think my apporoch is much simpler we don't need instanceof check and else is not necessary at all IMHO. In my patch checking isset($this->entity) will also set the entity if it is not set already.

If we don't have the else with E_USER_WARNING, then a migration will run and save with a silent error condition. Or that was the idea after introducing it in #25. I'm on the fence. It seems a couple folks want to fail more silently. I just want to get something committed.

Failing silently is not new in migration world AFAICS.

And un-assigning myself for now.

While it's not new, I would think having a message logged at minimum would be helpful, especially if you are doing custom migrations.

Moving forward on these types of issues, helps to bring Drupal into a more resonant environment and style with other platforms.

Thus 'migrating from x platform' to 'drupal' seems less of a risk, or at minimum, supportive.

1. +++ b/core/modules/image/src/Plugin/Field/FieldType/ImageItem.php
   @@ -313,13 +314,18 @@ public function preSave() {
   +    else {
   +      trigger_error(sprintf("A malformed file was provided. Cannot determine its dimensions."), E_USER_WARNING);
   +    }
   

   I think we can be a bit more specific here. We know there is a target_id or it would have been filtered out by the isEmpty() check but that target_id does point to anything.

   I'd go with: Invalid file ID ($this->target_id) provided. Cannot determine the file dimensions.

2. +++ b/core/modules/image/tests/src/Kernel/ImageItemTest.php
   @@ -129,4 +130,23 @@ public function testImageItem() {
   +    $entity->image_test->target_id = 0;
   

   I'd go with a more realistic scenario of a target_id 9999 or so. I think ther's an issue to filter out target_id 0 in some cases, then this wouldn't test anything useful anymore.

   Becaus of that, I would also add an explicit fail if the save() goes through without throwing that exception.

3. +++ b/core/modules/image/tests/src/Kernel/ImageItemTest.php
   @@ -129,4 +130,23 @@ public function testImageItem() {
   +    catch (EntityStorageException $exception) {
   +      $this->assertEquals($exception->getMessage(), 'A malformed file was provided. Cannot determine its dimensions.');
   +      $this->assertEmpty($entity->image_test->width);
   +      $this->assertEmpty($entity->image_test->height);
   +    }
   

   Explicitly Looking for an EntityStorageException is a bit strange.

   The actual exception is also in ->getPrevious() or so, then we can also assert what kind of exception is.

   I think it would be useful to explain in a comment that PHPUnit re-throws php warnings as exceptions and that's why we are testing it here as that.

Add org credit.

Thanks for the quick update!

Lets see what the core committers have to say about it, looks ok to me now.

Changing the issue title to something that IMHO makes more sense, the property (as in field api property) does exist, but it could be empty.

+++ b/core/modules/image/tests/src/Kernel/ImageItemTest.php
@@ -129,4 +130,26 @@ public function testImageItem() {
 
+  /**
+   * Tests a malformed image.
+   */
+  public function testImageItemMalformed() {
+    // Validate entity is a file and don't gather dimensions if it is not.
+    $entity = EntityTest::create();
+    $entity->image_test = NULL;
+    $entity->image_test->target_id = 9999;
+    // PHPUnit re-throws E_USER_WARNING as an exception.

Is the problem that there's no file on disk, or that the file on disk isn't an image? It looks like this is all handling the latter case, and if so 'is a file' seems like the wrong comment.

Same with 'Invalid file id 9999' - should it be "File ID 9999 points to a missing or malformed image"? Is it exactly the same code path for missing and malformed?

Good points. This might help with that.

No, isEmpty() is not the same and doesn't work. The field isn't empty, it has an ID, that ID just doesn't point to an existing entity, so ->entity can't load it.

That's confusing then. Why would instanceof EntityInterface pass checks in isEmpty() but instanceof FileInterface not pass in the ImageItem (see interdiff)? Running tests locally.

Ahh, because:

    // Avoid loading the entity by first checking the 'target_id'.
    if ($this->target_id !== NULL) {
      return FALSE;
    }

Is this fine? It seems like maybe we should load the entity just to be safe. But that adds overhead too.

Yes, we do need to keep that logic in isEmpty() as-is. If we don't, it doesn't let us stub out records to non existent entities during a migration, then later on add the referenced item. And there are other use cases too I'm sure. Although I could see argument for the contrary too. But that's scope creep me thinks and a potential different issue.

So, back to adding the instanceof check in the presave.

Does this help D6 any longer @heddn?

It helps anywhere where there is garbage data. D6 tends to have more of it than D7, but its valid to use in a D7 site as well.

This bug remains an issue, at least for me in D8.4.0 beta 1. Would the patch written in the D8.3 environment work with D8.4.0-beta1?

Update: patch for D8.3 works with D8.4. Thanks! I did, however, have to split the patch into 5 separate patches and applied on each of the files listed in the patch. That part wasn't clear for a rookie like myself going in. But maybe someone else will benefit from my mistake.

@heddn, the triggered error should be one condition deeper, because it's response to the lack of width/height.

How this for an updated wording on the error?

Thanks that error message is better suited for the condition that fails.

@heddn #81 It seem a wrong patch.

Interdiff seems right, let's see if this is correct now?

Ah, only read the interdiff, glad the testbot was doing it's job;)

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

Not working on Drupal 8.5.3. $this->entity is always NULL.

 // Determine the dimensions if necessary.
    if ($this->entity && $this->entity instanceof EntityInterface) {
      if (empty($width) || empty($height)) {
        $image = \Drupal::service('image.factory')->get($this->entity->getFileUri());
        if ($image->isValid()) {
          $this->width = $image->getWidth();
          $this->height = $image->getHeight();
        }
      }
    }
    else {
      trigger_error(sprintf("Missing file with ID %s.", $this->target_id), E_USER_WARNING);
    }

This does not fix $this->enitty being null, it just fixes having a fatal error *when* it is.

If you get those errors then it is working exactly as designed and you need to look into why it happens, could be faulty migrations or something deleting files that are still in use. Create a new issue with more details about your specific case if you can't figure it out. A support channel like Drupal Answers, Forum or slack might be better for that than an issue.

Is this fix being rolled into Drupal core? We shouldn't have this error be a show-stopper, right? It seems like my team is experiencing this issue due to simply different parts of our site being migrated at different times. We have a live Drupal 7 site and are slowly migrating it to Drupal 8. The files portion is one of the bigger portions so we don't re-run that every time we migrate another content type, which means a migrated content type has references to files that do not exist (has nodes that are newer than our file migration) so that this discrepancy seems to be causing these WSOD's/errors. It seems like it's less than ideal behavior to show end users error messages simply because a .jpg is missing here or there. Granted in our case we're mid-migration on a development site so there's no end users to have their sensitivities offended *grin*.