The oembed Resource value object should be more permissive for NULL dimensions

Glad you were able to figure this out!

Although I honestly do not understand how a faulty oembed URL can cause this. Could you please elaborate? I'd love to understand :)

Because entity validation was skipped when creating the entity, it would continously return an oembed validation error whenever loaded.

Ahhh! Now I understand :)

based on what I see, entity validation errors causes the cache layer to refuse caching the entity. It could obviously be something else, but after fixing the offending oembed resource, the entity caches correct.

No entity validation occurs during entity rendering. But it's possible the the Media module's oEmbed support does do some validation during rendering, to ensure it's actually possible to render correctly. This is what I suspect is going on. @phenaproxima was closely involved with Drupal 8's Media oEmbed support, so assigning to him for feedback.

I imagine that could absolutely be what's happening. When rendering media, it's very likely that the resource fetcher service will be invoked (it is used in order to get metadata which may well be used during rendering). The resource fetcher will load raw resource data from the persistent cache, then send that into one of the static factory methods of the Resource class. This is where the exception might happen, since the Resource class does a lot of validation on instantiation.

I think, looking at the code, Resource::setDimensions() is being a bit overzealous. For certain resource types, like photos or videos, requiring a width and height absolutely makes sense. But for things like rich content -- i.e., tweets -- or links, it doesn't. Resource::rich() is validating the dimensions of rich content (video, which is considered a subset of rich content, gets this as well), so your otherwise valid resource is failing.

My suggestion would probably be to modify the setDimensions() method so that it accepts a null value for $height, and doesn't bother validating $height if it's null. We could also separate the width and height setters into different methods, and have the ::rich() and ::video() factory methods call them separately as needed.

Per #8.

Un-assigning from myself, and tagging as an oEmbed issue.

Am I reading correctly that this could cause an exception to be thrown under normal site operation for affected media? (In addition to any performance implications.) If so then that probably qualifies as a major bug.

Didn't mean to remove a tag.

I've got a similar issue with embedded Facebook videos. The height is always NULL, and thus embeds can't be used anywhere.

By the sounds of it, there are two issues being discussed though. The overzealous exception and what should happen up the callstack when such exceptions are encountered. It might be worth opening up a new issue to look specifically at what JSONAPI does in these circumstances.

I've read carefully over #8 and #9, here is how I think we should proceed.

NULL is already a hinted return type of both ::getWidth and ::getHeight and @Roensby is correct, providers will likely have various capability to send either one, both or neither of these values. Lets make the foundation of the handling permissive with respect to these dimensions, for all resource types. In my case, despite requiring some special consideration for Facebook embeds during display, the underlying system rejecting the NULL value made the oembed system unusable.

I think it's acceptable for developers to make specific decisions and considerations towards the display of non-conforming resources, especially given these resources need to be explicitly enabled with a hook.

With that in mind, here is the test, IS/title update.

@Wim Leers, @Roensby, do you think JSON API needs a dedicated issue to discuss the exception handling component? I think this is probably not an API first initiative issue with the modified scope.

Could we get a FAIL patch here, to prove the bug and the fix?

1. +++ b/core/modules/media/src/OEmbed/Resource.php
   @@ -510,25 +510,20 @@ protected function setThumbnailDimensions($width, $height) {
   -    $width = (int) $width;
   -    $height = (int) $height;
   -
   -    if ($width > 0 && $height > 0) {
   -      $this->width = $width;
   -      $this->height = $height;
   -    }
   -    else {
   -      throw new \InvalidArgumentException('The dimensions must be numbers greater than zero.');
   +    if (($width !== NULL && (int) $width <= 0) || ($height !== NULL && (int) $height <= 0)) {
   +      throw new \InvalidArgumentException('The dimensions must be NULL or numbers greater than zero.');
        }
   +    $this->width = $width !== NULL ? (int) $width : NULL;
   +    $this->height = $height !== NULL ? (int) $height : NULL;
   

   I find this logic a bit hard to follow. Rather than doing $var !== NULL, we can use isset() instead? Also, I'm not sure we need to cast $width and $height to ints when using comparison operators; I'm pretty sure PHP will cast them automatically. So this can be:

   if ((isset($width) && $width <= 0) || (isset($height) && $height <= 0)) {
     throw;
   }
   $this->width = isset($width) ? (int) $width : NULL;
   $this->height = isset($height) ? (int) $height : NULL;
   

2. +++ b/core/modules/media/tests/src/Unit/ResourceTest.php
   @@ -0,0 +1,87 @@
   +  /**
   +   * Test cases for ::testSetDimensions.
   +   */
   +  public function setDimensionsTestCases() {
   +    return [
   +      'Standard rich dimensions' => [
   +        'rich',
   +        5,
   +        10,
   +      ],
   +      'Negative width and height' => [
   +        'rich',
   +        -5,
   +        -10,
   +        'The dimensions must be NULL or numbers greater than zero.',
   +      ],
   +      'Zero width' => [
   +        'rich',
   +        0,
   +        5,
   +        'The dimensions must be NULL or numbers greater than zero.',
   +      ],
   +      'NULL width' => [
   +        'rich',
   +        NULL,
   +        10,
   +      ],
   +      'NULL height' => [
   +        'rich',
   +        NULL,
   +        10,
   +      ],
   +      'NULL width and height' => [
   +        'rich',
   +        NULL,
   +        NULL,
   +      ],
   +      'Cast numeric dimenions' => [
   +        'rich',
   +        "1",
   +        "45",
   +        NULL,
   +        1,
   +        45,
   +      ],
   +      'Cast invalid zero value' => [
   +        'rich',
   +        "0",
   +        10,
   +        'The dimensions must be NULL or numbers greater than zero.',
   +      ],
   +      'Cast negative value' => [
   +        'rich',
   +        "-10",
   +        10,
   +        'The dimensions must be NULL or numbers greater than zero.',
   +      ],
   +    ];
   +  }
   +
   +  /**
   +   * @covers ::setDimensions
   +   * @dataProvider setDimensionsTestCases
   +   */
   +  public function testSetDimensions($factory, $width, $height, $exception = NULL, $expected_width = NULL, $expected_height = NULL) {
   +    if ($exception) {
   +      $this->setExpectedException(\InvalidArgumentException::class, $exception);
   +    }
   +    $resource = Resource::$factory('foo', $width, $height);
   +    $this->assertEquals($expected_width ?: $width, $resource->getWidth());
   +    $this->assertEquals($expected_height ?: $height, $resource->getHeight());
   +  }
   

   Oh my God, it's gorgeous!!! 😍 But, since this patch does some type juggling, can we use assertSame() instead of assertEquals()?

1. Sure thing, great improvements.
2. Great point, fixed.

I love this. RTBC when green on all backends. Thank you for the great bugfix!

Actually, one thing that concerns me...the photo resource type really requires dimensions. Should we perhaps add something to the Resource::photo() factory method, and related test coverage, to ensure that both width and height have a value (and throw exceptions if either doesn't)? Something like this:

function photo ($url, $width, $height...) {
  // Do some stuff

  // isset() is variadic and only returns TRUE if all arguments are set
  if (empty($width) || empty($height)) {
    throw new \InvalidArgumentException();
  }

  // More stuff
}

Also, the rich() factory method currently doesn't explicitly allow NULL values for $width and $height. Should we make that change here too?

If the system is really unusable for photos without dimensions, I definitely agree. Otherwise I think it would probably be a matter of time before, in order to support $some_provider, the constraints would simply be relaxed once more.

Here is a test to prove that photos render without dimensions, the hook_theme defaults them to NULL and template_preprocess_image optionally includes them as attributes.

Nice work @Sam152 looks like #22 is addressed

Committed f85f69c and pushed to 9.1.x. Thanks!

I think this is a good example of the robustness principle.

Going to get a +1 from release managers for a backport to 8.9.x and 9.0.x as they are in rc.

diff --git a/core/modules/media/tests/src/Unit/ResourceTest.php b/core/modules/media/tests/src/Unit/ResourceTest.php
index 6b398cfda7..5043565cd3 100644
--- a/core/modules/media/tests/src/Unit/ResourceTest.php
+++ b/core/modules/media/tests/src/Unit/ResourceTest.php
@@ -48,7 +48,7 @@ public function setDimensionsTestCases() {
         NULL,
         NULL,
       ],
-      'Cast numeric dimenions' => [
+      'Cast numeric dimensions' => [
         'rich',
         "1",
         "45",

Fixed spelling mistake on commit.

Discussed with @catch and we agreed to backport.