yiisoft / yii

Yii PHP Framework 1.1.x
http://www.yiiframework.com
BSD 3-Clause "New" or "Revised" License
4.85k stars 2.28k forks source link

Extend CClientScript::registerMetaTag with overwrite option #232

Closed qiangxue closed 12 years ago

qiangxue commented 12 years ago

I propose a change in CClientScript::registerMetaTag to allow overwrite of metatags. Some tags like keyword and description should in most cases never have two instances, and it would be nice to force a tag and overwrite if it exists.

The new method signature would have one additional parameter, $overwrite, which would default to false and hence nothing would break.

Another option would have been a method such as isMetaTagRegistered to keep tabs on which tags are registered or not.

Migrated from http://code.google.com/p/yii/issues/detail?id=2761


earlier comments

qiang.xue said, at 2012-01-01T03:37:10.000Z:

set for 1.1.10 milestone

qiang.xue said, at 2012-01-01T03:37:36.000Z:

set for 1.1.10 milestone

klimov-paul commented 12 years ago

Once I have already reported such issue:

http://code.google.com/p/yii/issues/detail?id=2460&can=1&q=metatag&colspec=ID%20Type%20Status%20Priority%20Milestone%20Owner%20Stars%20Summary

But you declined it. May be now you will reconsider.

samdark commented 12 years ago

@klimov-paul Your original report was declined for a reason. This one proposes better solution for the same problem so probably it will be OK to implement it.

What's your use-case for replacing meta-tags?

klimov-paul commented 12 years ago

The use case for the override Meta tags is very simple and comes from the “SEO rush”. For example: Lets create the bas controller for the particular Yii web application:


Class MyBaseController extends CController {
}

Now I wish the base controller register the “description” Meta tag with the default value:


Class MyBaseController extends CController {
    public function init() {
        Yii::app()->clientScript->registerMetaTag('Default Description', 'description’);
    }
}

The reason I wish to do it is simple: not every page in the web application really require unique Meta description, however in order to make the site valid every page should have at least some Meta description. While web application in development the new pages can raise and vanish every day, as far as the site default description can change at the same rate. So, I wish to make SEO things easy for myself and more reliable in outlook.

Now begins an interesting part. Lest image I have append a catalog section at my web application. This catalog introduces the item detail view page. This page according to the SEO requirements must contain the Meta description composed from the item description. So while creating the view file for the “catalog/view” I write the following:


clientScript->registerMetaTag($itemModel->meta_description, 'description’);

$this->widget('zii.widgets.CDetailView', array(
    'data'=>$itemModel,
    …

In this case I wish to replace previously set default page description (which is not suitable according the SEO requirements) with the new one, which is suitable for this particular page. But in result HTML code I see the following:


So my SEO is broken. See http://www.seoptimise.com/blog/2011/04/google-test-multiple-meta-descriptions-work-as-expected-social-search-does-not.html

Of course, now you can say something like this: “You should set your Meta tags at the single same place and maintain them on your own”. But such approach limits the framework extension ability. Let’s image that in example above on item details page I wish to create my own detail view widget, which will not only compose the model attribute representation, but will also compose the Meta data from the model attributes. If such widget is created, it can not be passed to another application safely, as far Yii by default does not allow Meta tags overridden or specification of their default values.

For now, if any isolated component tries to register its own Meta information, the final output HTML is unpredictable.

klimov-paul commented 12 years ago

I have mentioned the issue http://code.google.com/p/yii/issues/detail?id=2460&can=1&q=metatag&colspec=ID%20Type%20Status%20Priority%20Milestone%20Owner%20Stars%20Summary for a reason. We have a plenty of discussion back there, which is earned to be reread.

First of all, you should ask yourself: what Meta tag functionality if more common: appending any Meta tags to the global heap without bothering to identify or track them, or usage the Meta tags list where every tag has its own unique identifier.

You can find the use cases for the both approaches, as far as you can find a web application, which does not post any Meta tags or even does not output a HTML.

The main question is “what the developer expects from CClientScript::registerMetaTag()?”.

klimov-paul commented 12 years ago

I can accept, you don’t want to allow “CClientScript::registerMetaTag()” override Meta tags by default. You can create a flag “override” at the “CClientScript::registerMetaTag()”, or create a separated method (or methods), for example: “CClientScript::registerMetaTagOverride()”, “CClientScript::setMetaTag()”, “CClientScript::appendMetaTag()” and so on.

But I wonder: how you expect to allow developer both appending and override Meta tags with the same name?

For example: Let’s image we have created a method “CClientScript::registerMetaTagOverride()”, which allows to override meta tag value, if it was already registered. In meantime the old method “CClientScript::registerMetaTag()” always appends meta tag to the list. So what should (and what will) happen in result of following code:


Yii::app()->clientScript->registerMetaTag(‘append value 1’, ‘some_meta_tag’);
Yii::app()->clientScript->registerMetaTag(‘append value 2’, ‘some_meta_tag’);
…
Yii::app()->clientScript->registerMetaTagOverride(‘overriden value’, ‘some_meta_tag’);

I do not know how such case should be resolved…

mdomba commented 12 years ago

You made your point on the old issue and now too... ( about writing too much :D )

In general:

Problem is when writing code for a framework and especially when we want to have a fast and performant core to put there only what is essential and important.

Any problem can be solved in different ways and many times developers propose some code for the core that solves a problem just in the way they like it... but not in the way that is the best for a framework.

This problem particularly:

I really don't see a problem here as you can solve this differently... I can compare this to breadcrumbs (maybe not the best comparison but just an idea) - someone could for example want to set a default breadcrumbs and override them from other code parts...

Compare how breadcrumbs works in Yii, you can do the same for the description text... you can set the default in the base controller and then set specific text on specific controllers...

And I really think that a widget should not set a meta tag at all... where would we come if an extension would set a meta tag for the application that uses that extension.

kidol commented 12 years ago

After all in a widget $this->owner is available and for anything else Yii::app()->controller is available.

But as mdomba said, it's pretty odd anyway to set a meta description in such places like a widget.

klimov-paul commented 12 years ago

There is a significant difference between a Meta tags and breadcrumbs management in Yii. Yii framework does NOT provide any storage for breadcrumbs: class “CController” does not have any breadcrumbs, you should add it manually while extending the base Yii controller class.

In the same time the “CClientScript” IS a container for meta tags! “CClientScript” is included Yii core container(!) for the meta tags. This is a source of problem.

Remember “CClientScript” manages not only Meta tags. It also manages CSS and JS files. And in that case “CClientScript” DOES track the uniqueness of the registered CSS or JS files. Remember: the entire batch of widgets (for example “CGridView”) uses “CClientScript::registerCssFile()” and “CClientScript::registerScriptFile()” to publish their own assets. More of that: see the method “CClientScript::registerScript()”, its first parameter: $id - “ID that uniquely identifies this piece of JavaScript code”.

Perhaps this issue is a result of the confusion, which the inconsistent code in “CClientScript” has made. May be we should start the problem resolving from the division of “CClientScript” class into 2 separated ones: first for the files and second for the meta data.

kidol commented 12 years ago

Remember “CClientScript” manages not only Meta tags. It also manages CSS and JS files. And in that case “CClientScript” DOES track the uniqueness of the registered CSS or JS files.

There can be several meta tags with the same name. The only way you can distinct between different meta tags is to take the whole $options into account (which includes the name).

Yes this could be solved by changing the way the tags will be stored. For example:

// Now
$this->metaTags[serialize($options)]=$options;
// After (allow to override by name)
$this->metaTags[$name][serialize($options)]=$options;

Then we could add a 5th argument $override to registerMetaTag() or we could add additional methods like registerMetaTagOverride(). But that would bring problems like you already mentioned: Should we overwrite all meta tags with name "X" or just the first one? etc...

But whatever the solution would look like, I really don't see the need for it.

Also why do you want to call an ugly long method every time? In your use case you could simple do:

$this->metaDescription = $itemModel->meta_description;

There are other obvious benefits like implementing a setter that will make sure the description is not too long.

klimov-paul commented 12 years ago

Let’s imagine I will use my own controller field to store Meta description value. This means I have to choose a single place of code, where I will actually register its value as Meta tag. Such place should be chosen really carefully, so its code never would be run twice during the single application run. The first thing, which comes in mind: place it inside the main overall layout view file:


<?php Yii::app()->clientScript->registerMetaTag($this->metaDescription,’description’); ?>
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html xmlns="http://www.w3.org/1999/xhtml" xml:lang="en" lang="en">
<head>
    <meta http-equiv="Content-Type" content="text/html; charset=utf-8" />
    <meta name="language" content="en" />
…

Looking at this code I wonder: why I ever need “CClientScript::registerMetaTag()” at all? It is useless. I can write my Meta description tag value using the old fashion HTML composing or using “CHtml::metaTag()” function (you seems forgotten of its existence).

Note: I know I should use “CController::beforeRender()” for this purpose, this is not the case and does not changes the problem.

If I can not rely on “CClientScript::registerMetaTag()” inside the isolated component and should choose the single(!) place to actually use it, so why this function ever exists? “CHtml::metaTag()” should be just enough to manage all meta data, why not?

You should remember why you ever created “CClientScript” class. It manages the HTML document head section. This section is important because its stores such artifacts as linked CSS and JavaScript files, page Meta data and so on. The problem is this section is single and the same for the entire page, and there should be a way for isolated component to somehow manage this area. If I am creating widget, which introduces JavaScript visual effect based on JQuery library, I need a way to append “jquery.js” link inside the HTML page head section. If I wish this widget be pretty, I need a way to append “mywidget.css” link inside the HTML page head section. And so on and so on…

Now what if I tell you: you want your widget manage HTML page head section - do it on your own: place the “cssFiles” and “jsFiles” fields inside your controller and use them to manage your files, and you can always access your controller using “Yii::app()->controller”. What would you answer?

“CClientScript” was created to manage the HTML page head section, allowing the isolated component, like widget, registering its data without making inappropriate HTML code. It is a global storage (as far as any application component share singleton ability with the application itself) of page CSS files, JS files, Meta tags and so on. And this storage makes sure the output HTML code will be appropriate, such as no duplicating entry will appear.

This issue has been risen, because the tracking of meta tags uniqueness is different from the tracking CSS and JavaScript file links uniqueness. Inside the tag “link” there is not attribute, which identifies it: no name, no id. So for this only the combination of all its attributes (the main is “href” of course) can be used as its key. But Meta tags are different: they have name and this name is counted.

When I come across Meta tag override problem at the first place and found its source, I was sure it was just an editorial error. See the source code of the “CClientScript::registerMetaTag()”:


public function registerMetaTag($content,$name=null,$httpEquiv=null,$options=array())
{
    $this->hasScripts=true;
    if($name!==null)
        $options['name']=$name;
    if($httpEquiv!==null)
        $options['http-equiv']=$httpEquiv;
    $options['content']=$content;

            // Here is the place when data is actually stored:
    $this->metaTags[serialize($options)]=$options;

    $params=func_get_args();
    $this->recordCachingAction('clientScript','registerMetaTag',$params);
    return $this;
}

It seems someone simply forgot to exclude the “content” key, while creating unique identifier for the Meta data. So I suggested changing this code as following:


public function registerMetaTag($content,$name=null,$httpEquiv=null,$options=array())
{
    $this->hasScripts=true;
    if($name!==null)
        $options['name']=$name;
    if($httpEquiv!==null)
        $options['http-equiv']=$httpEquiv;

            // Lets create the key
            $metaTagKey = serialize($options);
            // and now we can save content as well
    $options['content']=$content;

    $this->metaTags[$metaTagKey]=$options;
    $params=func_get_args();
    $this->recordCachingAction('clientScript','registerMetaTag',$params);
    return $this;
}

However you insist “this is not a bug but feature”. You say: there should be a way to register several Meta tags with the same name and options but different content. However, even with such philosophy your code still inconsistent. Let’s image I am a developer, which is agreed with possibility to register several Meta with the same name and I even want to do so. Now I am creating the following code:


$model = SomeArClass::model()->find(…);
Yii::app()->clientScript->registerMetaTag($model->value, ’some_meta_tag’);
…
$anotherModel = SomeArClass::model()->find(…);
Yii::app()->clientScript->registerMetaTag($anotherModel ->value, ’some_meta_tag’);

As result of this code I am expecting the 2 entries of Meta tag “some_meta_tag” appear at the result HTML. However, who knows what the value of “$model->value” and “$anotherModel ->value”? If these values are equal: $model->value == $anotherModel ->value, result HTML code will contain only the single value of “some_meta_tag” Meta tag. So now we have one more confusing developer on the same Meta data matter…

It is obvious that something is wrong in the current logic.

kidol commented 12 years ago

Looking at this code I wonder: why I ever need “CClientScript::registerMetaTag()” at all? It is useless. I can write my Meta description tag value using the old fashion HTML composing or using “CHtml::metaTag()” function (you seems forgotten of its existence).

If you rate the method's value based on "can it filter out duplicates?" only, then yes, use CHtml, but obviously the method does a lot more than CHtml::metaTag().

However you insist “this is not a bug but feature”. You say: there should be a way to register several Meta tags with the same name and options but different content. However, even with such philosophy your code still inconsistent. Let’s image I am a developer, which is agreed with possibility to register several Meta with the same name and I even want to do so. Now I am creating the following code:

$model = SomeArClass::model()->find(…);
Yii::app()->clientScript->registerMetaTag($model->value, ’some_meta_tag’);
…
$anotherModel = SomeArClass::model()->find(…);
Yii::app()->clientScript->registerMetaTag($anotherModel ->value, ’some_meta_tag’);

As result of this code I am expecting the 2 entries of Meta tag “some_meta_tag” appear at the result HTML. However, who knows what the value of “$model->value” and “$anotherModel ->value”? If these values are equal: $model->value == $anotherModel ->value, result HTML code will contain only the single value of “some_meta_tag” Meta tag.

No this example is not valid. You are saying that by coincidence 2 instances of the same meta tag will be created from different sources. That should never be the case. A (head-) meta tag is page-wide and not bound to a fragmet of a page. css/js on the other hand may be bound to a fragment of a page and thus it may happen that by coincidence 2 widgets require the same file (e.g. jquery).

It is obvious that something is wrong in the current logic.

The current logic is not wrong, but one can argue whether it's worth extending the functionality (i.e. adding a 5th argument or adding more methods). In my opinion, it's not.

klimov-paul commented 12 years ago

At the beginning of this discussion @samdark asked me “What's your use-case for replacing meta-tags?” Now I wish to ask all of you an opposite question: What is your use case for appending the Meta tags with the same name, same options but different content?

samdark commented 12 years ago

@klimov-paul

http://www.ietf.org/rfc/rfc2731.txt — see how author is specified via meta tags. http://googlewebmastercentral.blogspot.com/2007/03/using-robots-meta-tag.html — Google allows multiple robots.

Still I agree that there are valid and common cases for inserting-or-replacing tags. The way @kidol suggested is OK but since it's very common for description, title etc. I think it worth having this in the core. Instead of adding another parameter I suggest to think about new method with the same parameters.

mdomba commented 12 years ago

The $options parameter can be used... something like array("replace"=>true)

samdark commented 12 years ago

Since it will be used often for most common tags, I think it worth adding another method.

mdomba commented 12 years ago

Well in the end it's same for me... but I'm not really sure this will be used so often as this feature was not requested from other developers... even the old issue on googlecode did not have more supporters for this problem.

samdark commented 12 years ago

True. Probably it worth waiting for more requests on it since if your application designed properly it will only set this once.

kidol commented 12 years ago

@samdark

Probably it worth waiting for more requests on it since if your application designed properly it will only set this once.

Agree. Also I've never noticed someone complaining in the forum.

@klimov-paul While you are right that the functionality could be extended or changed, you did never point out why the property approach is no acceptable solution for you (or did I miss that?). Here is an example I've already posted in the old googlecode issue, look at the source of this page: http://www.golem.de/ You can also see that there are several link tags with the same rel attribute. So the same "problem" also affects registerLinkTag(). But again it's obvious that one should and usually will set such tags in a single place.

klimov-paul commented 12 years ago

Actually I can not see a use case description in your examples.

From @samdark :

1) http://www.ietf.org/rfc/rfc2731.txt - here is description of Dublin Core Meta data convention, which is not a standard of any kind. This description says there can be “in theory” (!) several Meta tags with the same name and options but different contents. Also this document indicates that there can be several identical (same name, same options and same content) Meta tags at the same page, which current Yii functionality does not allow. Of course, if there is no markup rule, which may deny the possibility of several same named Meta tags existence. As far as HTML can be considered as XML the following code is valid:


<head>
<meta name="description" content="Description 1" />
<meta name="description" content="Description 2" />
</head>

But according to XML standard the following code also will be valid:


<body>
<div id=”same_id”></div>
<div id=”same_id”></div>
</body>

while it is not valid according to HTML standards (or to be more precise to JavaScript standards).

2) http://googlewebmastercentral.blogspot.com/2007/03/using-robots-meta-tag.html - here is a document, which says Google “may” merge several “robots” Meta tag values. However this document also says “We recommend that you place all content values in one meta tag”!

From @kidol :

3) http://www.golem.de/ - here is a site, which introduces unknown purpose Meta tag named “msapplication-task”. I can only image, what purpose this Meta tag serves, but from its name I suggest it is some sort of XML style data resource for some external Microsoft application. There is no clue why this site has introduced this meta tag and why it duplicated in the HTML source. Looks the creations simply never heard about such things as SOAP, REST and so on.

samdark commented 12 years ago

@klimov-paul

1) It's actually used by people in practice. That's why I've referenced it. 3) That's unknown purpose for you but I guess it's a pretty valid purpose for website creators. Another example of using same named meta is Facebook: http://developers.facebook.com/docs/opengraphprotocol/ :

You may include multiple og:image tags to associate multiple images with your page.

klimov-paul commented 12 years ago

I have never said the tracking of some Meta tags uniqueness is impossible in the current Yii implementation. If you read my comments, you can see, I have mentioned “CController::beforeRender()” and “CHtml::metaTag()” and showed any functionality can be achieved.

I suppose the “ClientScript::registerMetaTag()” should override the Meta tag value, because otherwise it makes confusion and leads to latent mistakes. See the “ClientScript” interface (public methods):

The entire “ClientScript” class has been designed to allow isolated components maintaining the HTML page “head”. However when I tell you isolated component should be able to invoke “ClientScript::registerMetaTag()” you say “this is not a good idea”.

I suppose “ClientScript” is not enough to manage Meta tags. Some more sophisticated component should be created for such purpose, something like “CPageMetaData”. However I doubt the core team will agree with me.

In any rate I am out of arguments… I can not deny you have a point, and so I can not demand the current functionality to be changed. All I can say now: it is up to you to make final decision.

samdark commented 12 years ago

registerCss and registerScript are "safe" only if IDs are matching. So to sum up:

  1. There are valid use cases for both replacing and adding same named meta-tags. So it's good to have both behaviors.
  2. It's not clear what to do if there are 2 same-named tags registered with "adding" and then "replacing" is invoked.
  3. It's not clear yet how API should look like.
  4. We should not break backwards compatibility.
mdomba commented 12 years ago

This thread has gone too long.. and mostly it's a repeat of the googlecode thread - http://code.google.com/p/yii/issues/detail?id=2460

CClientScript is designed to maintain the page "head", but as explained already numerous times we cannot compare meta tags to CSS or JS files...

Meta tags can be duplicated, that is a fact... a fact is that a developer maybe would like to replace a meta tag instead of duplicating it... but as pointed by samdark... there can be unexpected problems like what to do if there are two same name meta tags already registered and now a replace meta tag is issued... which one of the previously two should be replaced?

A "manual" solution for replacing meta tags has been suggested.

Because of all this and because a proper solution that would make happy all would be really too complicated... and especially because this is very rarely needed I suggest we close this issue and leave all as it is like we did the first time.

samdark commented 12 years ago

@mdomba that's too easy :)

  1. I suggest replacing all same-named tags if "replace" is called.
  2. How about adding registerMetaTagOnce() method? Adding parameter to the already long parameters list isn't good. Also we can't change default behavior since it will break BC.
mdomba commented 12 years ago

not sure I follow your idea...

if there are two "same name" meta tags registered it does not mean they are equal.. after the 3rd "once()" call... by replacing them both there would be two equal meta tags.

kidol commented 12 years ago

@klimov-paul You miss registerLinkTag() which is unsafe as well. Link/meta-tags and css/js scripts/files have a fundamental difference. So just comparing those methods in one aspect and say "look there is a difference in how they work, this class is flawed" is not a valid argument.

@mdomba

if there are two "same name" meta tags registered it does not mean they are equal..

True, but for things like meta description, @klimov-paul simply wants the ability to register a default one (he doesn't need any lang attribute etc). So if we would compare to html: a non-unique name would act as a unique id.

I think 2. by samdark (this is what klimov wants as well) would be the best solution if this has to get implemented. So instead of a new $replace argument or 'replace' in $options, add a new method.

But again I think this is not needed at all and should instead be solved by the developer.

samdark commented 12 years ago

@mdomba I meant removing all same-named tags first and then adding one that we're "replacing" with.

klimov-paul commented 12 years ago

Related issue has been opened at #1351.

I suppose this one can be closed.