Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unable to set a webpage as a valid source URL (fallback to iframe) since v1.3.11 #101

Closed
juban opened this issue Feb 25, 2022 · 5 comments
Closed

Comments

@juban
Copy link
Contributor

juban commented Feb 25, 2022

We're heavily using your excelent plugin for many projects.

Until version 1.3.10, it was possible to give any kind of url as a source for the Embed. In that case, the plugin was kindly switching to an iframe version.
There is an annoying regression since version 1.3.11 and the introduction of the FallbackAdapter. In that context, even if the source url is valid, it will result in a "Please check your URL." error and blank output in the templates.

The reason this is happening is related to a silent error thrown in the embed method of the OembedService class.
When the $media = Embed::create($url, $options, $dispatcher); is called with a standard webpage url, the resulting Embed object will give an empty result when calling $media->getCode().
So the following code (line 84 and 85) will result in an exception:

$dom = new DOMDocument;
$dom->loadHTML($media->getCode()); // exception here

Changing that part with the following code revert the previous plugin behavior and works well with "normal" embedable urls:

$dom = new DOMDocument;
$code = $media->getCode();
if (empty($code)) {
    $code = Utils::iframe($media->url);
}
$dom->loadHTML($code);

I can send you a PR for that matter if it helps.

reganlawton added a commit that referenced this issue Feb 27, 2022
@juban
Copy link
Contributor Author

juban commented Mar 8, 2022

@reganlawton
Do you think it would be possible to push a tag for the fix soon? That would be really helpful to us. Thanks.

@reganlawton
Copy link
Member

OMFG I completely forgot about that cos I normally do it myself and accepted the PR via the Github Mobile. I'll sort this right this second.

@reganlawton
Copy link
Member

@juban that has now got a release tag.

@juban
Copy link
Contributor Author

juban commented Mar 9, 2022

@reganlawton
No worry, thank you so much for the new release, it will save us a lot of trouble.

@reganlawton
Copy link
Member

@juban thanks for contributing

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants