oEmbed field formatter creates wrong hash when leaving empty dimensions

Thank you @amourow for reporting

the MR should be updated to 11.x branch as that's the current development branch

Also will need a test case showing the issue.

Thanks

Please review.

Comment on MR.

Checking the max_width and max_height in the iframe src is 0 even when the formatter set to empty string.
Test passed. Ready for review.

Believe this is ready for committer review now.

I'm triaging RTBC issues → . I read the IS and the comments. I didn't find any unanswered questions.

@amourow, thanks for reporting this and creating the MR with test.

There is no fail test here. So, I tested this on 10.0.x and 11.x, using the steps in the issue summary, and was not able to reproduce the problem. I then applied the MR, removed the fix and ran the test. The test did fail.

So, maybe there is a problem here. I see that this is adding a new test, one that is nearly identical to testRender. I think we should build on testRender instead of making a whole new test for someone to work on later. I played with this a bit using the test case below and some modifications to the test body to test the settings if $formatter_settings is not empty. If we do this, then it will be easier to maintain in the future and we are adding assertion for the settings to all the existing test cases.

      'Vimeo video empy max size' => [
        'https://vimeo.com/7073899',
        'video_vimeo.json',
        ['max_width' => '', 'max_height' => ''],
        [
          'iframe' => [
            'src' => '/media/oembed?url=https%3A//vimeo.com/7073899',
            'width' => '480',
            'height' => '360',
            'title' => 'Drupal Rap Video - Schipulcon09',
            'loading' => 'lazy',
          ],
        ],
        'self_closing' => TRUE,
      ],

And this first attempt and testing the formatter settings.

    if (!empty($formatter_settings)) {
      $element = $assert->elementExists('css', 'iframe');
      $expected = '';
      foreach ($formatter_settings as $setting => $value) {
        $value = empty($value) ? '0' : $value;
        $expected = "$setting=$value";
        $this->assertStringContainsString($expected, $element->getAttribute('src'));
      }
    }

james.williams → made their first commit to this issue’s fork.

I've had a go at adapting that suggestion from @quietone for testing this.

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Good work queue bot; serves me right for editing directly in GitLab!

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

May be completely unrelated but does ✨ Negotiate max width/height of oEmbed assets more intelligently Needs work impact this at all?

The Needs Review Queue Bot → tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

@smustgrave I actually worked on using work from the two together on a client project. They are indeed very closely related, but can still be treated separately (if preferred). That other issue deals with using better dimensions (as supplied from the oEmbed provider), whereas this one fixes an error on Drupal’s side that results from when no dimensions being set.

I’m confused by the bot’s latest report, it says there’s a spelling issue without saying where?

May just need a rebase with the latest changes from 11.x