Skip to content

Fix Twittercards provider type #291

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

Merged
merged 1 commit into from
Sep 13, 2018

Conversation

Cavanna-dev
Copy link

@Cavanna-dev Cavanna-dev commented Sep 13, 2018

Description

We need to access to twitter:card. But in some cases, websites use value instead of content.

Related to: #290

@oscarotero
Copy link
Collaborator

I would accept to get values from value in addition to content but without change the getType() method that must return a value from a limited and known set of values (like an enum).
If you need a specific value from a provider, you can access to that value from the provider bag.
So, once the twittercard provider get the value of twitter:card, you can access to the raw value using following code:

$twitterProvider = $info->getProviders()['twittercards'];
$twitterProvider->getBag()->get('card');

@Cavanna-dev
Copy link
Author

Cavanna-dev commented Sep 13, 2018

Hi @oscarotero,

Thanks for your reply, I linked the PR to the issue I created too.

Ok fine for me, so you do prefer something like this for the getType()?

switch ($type) {
    case 'video':
    case 'photo':
    case 'link':
    case 'rich':
    case 'summary':
    case 'summary_large_image':
    case 'app':
        return $type;

    case 'player':
        return 'video';
}

@Cavanna-dev Cavanna-dev force-pushed the fix-twitter-cards branch 2 times, most recently from ddce7b6 to e0f3beb Compare September 13, 2018 11:26
@Cavanna-dev
Copy link
Author

Btw I fixed every tests I could, but I have one error that I'm not sure about how to fix it.

1) Embed\Tests\CustomAdaptersNamespaceTest::testTwo
Embed\Exceptions\InvalidUrlException: gnutls_handshake() failed: Handshake failed
/home/travis/build/oscarotero/Embed/src/Embed.php:144
/home/travis/build/oscarotero/Embed/src/Embed.php:81
/home/travis/build/oscarotero/Embed/tests/AbstractTestCase.php:35
/home/travis/build/oscarotero/Embed/tests/CustomAdaptersNamespaceTest.php:47

Can you help me with this one? 😊

@oscarotero
Copy link
Collaborator

Hi.

$info->type must return always one of the following values: video, photo, link and rich. Sorry but any other value is not allowed. If you want to get the specific value returned by a provider like twittercards, you must get the provider and get the value by yourself. This is based in oembed spec (sections 2.3.4.x) and ensures consistency between all providers that can return different and contradictory values. So I'd leave this function as it was.

And about the tests, some fails are temporary, others only happens in the travis platform but not in local, etc. That's a tedious work so I appreciate your help here. When I'm unable to fix a specific test, I add it to the ignore group. Example: https://github.com/oscarotero/Embed/blob/46e19804e330065c899657c047288f792e2bca02/tests/RedditTest.php#L8
That allows to fix the test in a future.

@Cavanna-dev Cavanna-dev force-pushed the fix-twitter-cards branch 4 times, most recently from 18b3b9b to 4538c95 Compare September 13, 2018 12:52
@Cavanna-dev
Copy link
Author

Cavanna-dev commented Sep 13, 2018

@oscarotero Thanks for the tips !

I fixed what I could, but I start getting weird results:

1) Embed\Tests\ScribdTest::testOne
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-rich
+link

In local it's ok, and I check on the website too, it appears that it's really a rich type. So maybe we should let it this way and if you're OK with my changes you could merge and release the version? I need it for my project :D

@oscarotero oscarotero merged commit 4b4ab5f into php-embed:master Sep 13, 2018
@oscarotero
Copy link
Collaborator

Thanks, I'll try to fix these issues and release a new version

@oscarotero oscarotero mentioned this pull request Sep 13, 2018
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

Successfully merging this pull request may close these issues.

2 participants