Handle oEmbed resources without an explicit height

To set up a provider like Twitter, follow the "Extending" part of https://www.drupal.org/node/2966029.

Let's see how many tests this breaks.

Maybe it is good idea to allow the modules to alter the resource data. Because anyway the source for Twitter or Instagram should be created manually and core is not providing it, the developer in his/her module can decide what height or width to set to the resource if it is not present.

The patch is attached.

I don't see how this alter will fix issues for oEmbed support of Twitter statuses, per #3005539: Building rich oembed resource requires width and height. The developer adding the resources shouldn't need to do extra work for the embedding of remote objects, as that's the whole point of oEmbed - it should "just work".

EDIT:

Also, instead of an alter this means we do need a Twitter extends OEmbed plugin. We'll need a series of contrib that add provider-specific workarounds, then.

EDIT 2:

For what it is worth, Twitter embeds work fine if $resource->setDimensions($width, $height); is commented out in theResource::rich factory method. It's not very wide, but it works.

I think Twitter is anyway not allowed by default in the core, or I am missing something? Remote videos with Youtube and Vimeo are only ones that are allowed, right?

@a.dmitriiev It is not supported by core, but there is oembed_providers which extends core to use a wide range of oembed providers. See the related issue #3164271: Read height of Facebook/Twitter/Insta posts

I agree with @phenaproxima. I think setting the height to NULL by default should make the oEmbed system a bit more friendly.

Nevertheless, displaying an iframe without an explicit height is tricky and will not look nice. So this is only one part of the solution.

I have created an MR which marks the height of photos, rich, and video as optional.

From what I can tell Drupal\Tests\media\Unit\ResourceTest::testSetDimensions() already covers the ability to set a rich item with a NULL height.

...
'NULL height' => [
  'rich',
  NULL,
  10,
],
'NULL width and height' => [
  'rich',
  NULL,
  NULL,
],
...

Also, Drupal\media\OEmbed\Resource::setDimensions() seems to already handle the ability to have a NULL height

protected function setDimensions($width, $height) {
    if ((isset($width) && $width <= 0) || (isset($height) && $height <= 0)) {
      throw new \InvalidArgumentException('The dimensions must be NULL or numbers greater than zero.');
    }
    $this->width = isset($width) ? (int) $width : NULL;
    $this->height = isset($height) ? (int) $height : NULL;
}

So are additional tests required in this case?

I agree that the int type cast and exception in ::setDimensions() would probably be enough to ensure weight and height are positive integers or NULL. While I agree a ?int typehint could also work here, none of the other parameters currently have it, and I think it would be out of scope to refactor all of them.

I think this is good to go from my perspective.

I think this could use a test - we have oembed test fixtures in core already, and could add one without an explicit height and make sure it can be parsed.

Modifying the existing twitter fixture sounds like a good plan.

I am personally OK with the approach and the test changes.

One issue that came up when discussing this in slack, is that by setting a non-null formatter max_height value, we fix the test failure here, but I'm not sure if we wouldn't be hiding the potential scenario where a formatter _hasn't_ been configured (so it would have max_height default to 0), _and_ a provider returns no height. Not sure if this is in scope for this ticket?

Tentatively moving to RTBC since I am personally not really too concerned about this edge-case, but no strong opinions.
(assuming the tests go green).

It doesn't feel like we need test coverage for this, but what's the consequences on an actual site if something like that happens, do we need a follow-up?

I've asked around for folks running twitter oembeds via media module on a production site to manually test this if possible

Tested this on chrome, safari and firefox and followed the following test steps:

• Took screenshots before applying the patch
• Applied this patch via composer.json
• Cleared caches

Please grant credit to ramya.shankaralingam and xtineroque who helped with the testing.

Crediting @tapscolaM, @ramya.shankaralingam and @xtineroque who helped with manual testing

Committed f87d33a and pushed to 9.3.x. Thanks!

Added and published a change record too. Please amend if I've missed anything https://www.drupal.org/node/3225613