Skip to content
This repository has been archived by the owner on Feb 24, 2022. It is now read-only.

Fix twitter uses name not property attribute #40

Closed
wants to merge 1 commit into from

Conversation

wilsonpage
Copy link

No description provided.

@coveralls
Copy link

Coverage Status

Coverage decreased (-4.3%) to 95.745% when pulling e887705 on wilsonpage:patch-1 into 82ac24e on mozilla:master.

@jaredlockhart
Copy link
Collaborator

@wilsonpage can you rebase this and update the tests? Thanks!

@wilsonpage
Copy link
Author

Sorry haven't had chance yet.

On Fri, 19 Aug 2016 18:13 Jared Kerim, notifications@github.com wrote:

Please update the tests and rebase on master.


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#40 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AA-ShxvsDbzehKGX5CIgNbmxaeijpYNdks5qheQ-gaJpZM4JjHgn
.

@pdehaan
Copy link
Contributor

pdehaan commented Sep 1, 2016

@wilsonpage I can submit a new PR for this against master and update the tests, but I wanted to ask how you felt about leaving the old [invalid] tags in, as well as adding the correct Twitter <meta>tags.

Basically feedback on #51.
Should we just try and "do the right thing", even if the user used incorrect meta tags? We'd get better results back in weird edge cases of invalid markup, at the cost of bloating our codebase with extra meta lookups.

@wilsonpage
Copy link
Author

@pdehaan that's a fair point. TBH the use of name instead of property is a gotcha, I'm sure others will make the same mistake.

@pdehaan
Copy link
Contributor

pdehaan commented Sep 2, 2016

Per @wilsonpage, yeah, apparently Reddit uses some invalid/un-recommended tags, per #51 (comment)

<meta property="og:description" content="Welcome to r/Funny: reddit&apos;s largest humour depository">
<meta property="og:image" content="https://www.redditstatic.com/icon.png">
<meta property="og:site_name" content="reddit">
<meta property="og:title" content="funny &#x2022; /r/funny">
<meta property="twitter:card" content="summary">
<meta property="twitter:site" content="reddit">
<meta property="twitter:title" content="funny &#x2022; /r/funny">

I think we have no choice but to do checks for both <meta name=""> and <meta property=""> if we want to get consistent results. Interestingly, running that page through the Twitter Card Validator shows that it's a valid page, so I'm guessing that Twitter looks for name and property since they know a reasonable %% of people will mess it up.

@jaredlockhart
Copy link
Collaborator

@pdehaan @wilsonpage we must absolutely include both cases in the rules. The parser doesn't 'care' what the 'intended' structure of markup is, it only cares about all the ways to get the relevant data from pages. That markup is messy and non-conformant is a founding precept of this library. @pdehaan if you could add both cases to the rules, update the tests, and rebase that would be a huge help!

@pdehaan
Copy link
Contributor

pdehaan commented Sep 15, 2016

jaredkerim says: "@pdehaan if you could add both cases to the rules, update the tests, and rebase that would be a huge help!"

Consider yourself helped, #66!

I totally forgot about #51, but I can do that PR separately, where we basically look for the incorrect format for Open Graph tags as well.

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

Successfully merging this pull request may close these issues.

None yet

4 participants