r111439 MediaWiki - Code Review archive

Repository:MediaWiki
Revision:< r111438‎ | r111439 | r111440 >
Date:02:22, 14 February 2012
Author:dantman
Status:reverted (Comments)
Tags:
Comment:
Introduce property: and itemprop: support for addMeta to add RDFa <meta property="..." content="..."> and Microdata <meta itemprop="..." content="..."> to the <head>.
This is done in a way that can also be feature tested, and technically could be expanded by extensions.
Modified paths:
  • /trunk/phase3/includes/OutputPage.php (modified) (history)

Diff [purge]

Index: trunk/phase3/includes/OutputPage.php
——@@ -232,6 +232,15 @@
233233 private $mRedirectedFrom = null;
234234
235235 /**
 236+ * Name prefixes that can be used in addMeta
 237+ */
 238+ public static $metaAttrPrefixes = array(
 239+ 'http' => 'http-equiv',
 240+ 'itemprop' => 'itemprop',
 241+ 'property' => 'property',
 242+ );
 243+
 244+ /**
236245 * Constructor for OutputPage. This should not be called directly.
237246 * Instead a new RequestContext should be created and it will implicitly create
238247 * a OutputPage tied to that context.
——@@ -278,7 +287,13 @@
279288 /**
280289 * Add a new <meta> tag
281290 * To add an http-equiv meta tag, precede the name with "http:"
 291+ * To add a Microdata itemprop meta tag, precede the name with "itemprop:"
 292+ * To add a RDFa property meta tag, precede the name with "property:"
282293 *
 294+ * itemprop: and property: were introduced in 1.20, you can feature
 295+ * test for them by checking for the key in the new
 296+ * OutputPage::$metaAttrPrefixes variable.
 297+ *
283298 * @param $name String tag name
284299 * @param $val String tag value
285300 */
——@@ -2995,11 +3010,16 @@
29963011 }
29973012
29983013 foreach ( $this->mMetatags as $tag ) {
2999 - if ( 0 == strcasecmp( 'http:', substr( $tag[0], 0, 5 ) ) ) {
3000 - $a = 'http-equiv';
3001 - $tag[0] = substr( $tag[0], 5 );
3002 - } else {
3003 - $a = 'name';
 3014+ $a = 'name'; // default attribute
 3015+ foreach ( self::$metaAttrPrefixes as $prefix => $attribute ) {
 3016+ // Check if the name starts with the prefix
 3017+ if ( strpos( $tag[0], "$prefix:" ) === 0 ) {
 3018+ // Set the attribute name we're using
 3019+ $a = $attribute;
 3020+ // Strip the prefix from the name
 3021+ $tag[0] = substr( $tag[0], strlen( $prefix ) + 1 );
 3022+ break;
 3023+ }
30043024 }
30053025 $tags[] = Html::element( 'meta',
30063026 array(

Follow-up revisions

RevisionCommit summaryAuthorDate
r111975Revert RDFa and Microdata improvements to addMeta in r111439 till after the g...dantman22:48, 20 February 2012

Comments

#Comment by Nikerabbit (talk | contribs)   16:38, 14 February 2012

Why public? Don't we usually use hooks for these.

#Comment by Dantman (talk | contribs)   19:32, 14 February 2012

Got a better way to feature test than `isset( OutputPage::$metaAttrPrefixes ) && isset( OutputPage::$metaAttrPrefixes['property'] ) );`?

#Comment by Werdna (talk | contribs)   19:09, 17 February 2012

What's the use context for checking if this feature is enabled or not? (I was confused for a moment by 'feature test' as a verb ;-), I guess I do less JS work than you).

#Comment by Dantman (talk | contribs)   19:24, 17 February 2012

So that an extension can actually write `$out->addMeta( 'property:og:title', 'Foo' );` and be sure that it's actually implemented and they're actually going to get <meta property="og:title" content="Foo"> and not <meta name="property:og:title" content="Foo">. Because you can't use class_exists or method_exists to test for this feature like you can with others.

eg: r1=111441&r2=111440&pathrev=111441 http://svn.wikimedia.org/viewvc/mediawiki/trunk/extensions/OpenGraphMeta/OpenGraphMeta.php?r1=111441&r2=111440&pathrev=111441

#Comment by GWicke (talk | contribs)   19:45, 17 February 2012

It would be nice if itemprops were wrapped in an itemscope, which does not yet seem to be the case. An itemtype might also make sense to make the items more useful.

#Comment by Dantman (talk | contribs)   20:16, 17 February 2012

<meta itemprop=>'s are usually scoped to the body. Extensions that need meta itemprop with a scope and type can use OutputPageBodyAttributes to add them. The primary reason for itemprops in the head currently seem to be for the +1 button.

#Comment by GWicke (talk | contribs)   20:28, 17 February 2012

There seems to be no itemscope set on the body right now (at least grep did not turn up anything in phase3), and the spec does not mention any implicit scoping to the body. So I would think that regular microdata parsers cannot use these itemprops. Are there specialized consumers that are still able to use them?

#Comment by Dantman (talk | contribs)   21:03, 17 February 2012

And there are no itemprops set on <meta> items right now. OutputPageBodyAttributes lets extensions define an itemscope and itemtype on the body. And this change I made allows extensions to use $out->addMeta( 'itemscope:name', $name ); to define an itemprop on a meta element in the <head>.

#Comment by GWicke (talk | contribs)   21:28, 17 February 2012

Sorry- I somehow ignored OutputPageBodyAttributes when first reading it. In that case, it appears to be necessary to add an itemref to all itemprops in the head to associate them with an itemscope set on the body. This does not seem to be possible right now, so this would produce invalid Microdata.

It might be cleaner to set the itemscope on the head instead, and avoid the itemref and unwanted scoping on the body that way. Could a simple itemscope be implicitly added to the head element whenever a microdata item is added? This would make it easier to produce valid microdata.

#Comment by Dantman (talk | contribs)   23:45, 17 February 2012

What? Why would you add an itemref to a meta element in the head? Hell, last I checked itemref is only valid on an itemscope, a is invalid Microdata.

Ok, I made one small mistake. It's not the body that we add the itemscope to, it's the <html> element. I'll need to add a an extra hook for that.

And you wouldn't want to avoid scoping in the body. The whole point of itemprop in the head is that it applies to the entire page. Things like the schema.org WebPage itemtype.

#Comment by GWicke (talk | contribs)   10:33, 18 February 2012

For WebPage, that definitely makes more sense. The only trouble I see with the global scope is that stray itemprops in the content would end up in the outermost itemscope. But that should be fixable in the parser.

I am still not that happy with the need to use an extra hook to avoid producing invalid microdata. Some of the WebPage schema data would be naturally added in the skin (mainContentOfPage for example), so it might make sense to add this itemtype and itemscope on the html element in the skin as well. Header itemprops from other itemtypes could still be added using absolute URLs in itemprop names.

If we allow users to replace the itemtype on the html element arbitrarily, then we could not use something like WebPage to mark up parts of the page in the skin- at least not without using absolute URLs throughout. Afaik right now search engine crawlers don't yet use itemprops with absolute URLs, but I guess that might change (hopefully also in the HTML spec) once mixing multiple itemtypes becomes more common. Until then, we have to decide if something like WebPage or an arbitrary user itemtype should take precedence. I would vote for a static itemtype like WebPage, as it covers most of the metadata already published with pages.

#Comment by Dantman (talk | contribs)   17:59, 18 February 2012
  • Sure perhaps we should fix stray itemprops in the parser. Even without a <html> scoped WebPage the Microdata spec basically indicates we should be stripping out itemprops that way anyways.
  • I have no plans to add any global Microdata into the skin. This feature is added for extensions that have a reason to do that. Whether it's a WebPage Microdata item, or there is some other type of spec they want to add.
  • http://schema.org/WebPage actually departs a little from the Microdata spec's rules. You can actually completely omit an itemscope and itemtype, and search engines will assume that html is a WebPage itemtype and stray itemprops belong to it.
#Comment by GWicke (talk | contribs)   09:13, 20 February 2012

I believe it would be cleaner and less damaging to future microdata use if we instead provided a method for extensions to add an (optional) itemscope / id and multiple contained itemprops within the header. This way microdata with custom itemtypes can be safely added without breaking the global itemtype for a page. By making the itemscope optional, bare itemprops can still be added to extend a global itemtype (presumably WebPage). Using the itemref mechanism, extension output in the body can also extend a microdata item introduced in the header.

I would be happy to create such a microdata-specific method, and would prefer if microdata support was not added to addMeta().

#Comment by Dantman (talk | contribs)   23:50, 20 February 2012

I'm not going to bother getting this reviewed till after the git migration, so for now it's reverted anyways (thankfully I feature tested so extensions won't break anyways).

A few notes though:

  • I'm not opposed to the idea of a separate defined api in OutputPage for defining the page-wide Microdata of a page in an intuitive way.
  • However I see no value in having extensions outputting Microdata in-body and using itemref to point to properties in the <head>.
    • Microdata items have absolutely no connection to the non-microdata contents of the document. Whether the data was inserted inside the head, or inside the body doesn't affect the Microdata at all. So if an extension outputs any itemscope into the body then there is absolutely no value in putting the itemprops inside the <head> instead of putting them inside the itemscope in the body.
    • The only value at all in putting an itemprop inside the head is as a place to add a few of the general properties to the one global itemscope, namely WebPage, like itemprop="name".
  • If we do opt instead for an api to add page-wide Microdata to a page, we should probably do the same for RDFa instead of this addMeta method.
    • It would also be good for said RDFa API to embed xmlns/prefix handling as well, instead of having extensions modify $wgXhtmlNamespaces.
    • We should probably also take into account that the spec defining RDFa+HTML5 seams to say that for RDFa purposes an xmlns:*= is valid in HTML5 and tweak our code to not omit namespaces when $wgHtml5 is true.

Status & tagging log