yiisoft / view

Yii view rendering library
https://www.yiiframework.com/
BSD 3-Clause "New" or "Revised" License
55 stars 43 forks source link

Ability to override script type in registerJs() #8

Closed nirvana-msu closed 3 years ago

nirvana-msu commented 8 years ago

I have a dynamically generated JSON-LD markup that needs to be registered in a head section with "application/ld+json" script type:

<script type="application/ld+json">...</script>

How can I achieve this? I think registerJs method should provide the ability to override script type, much like you can do in registerJsFile by passing options array with type in it .

nirvana-msu commented 8 years ago

I guess one workaround would be to pass the content of the script to layout using View::params, and then register where I need it with Html::script. But I feel like there should be a more straight-forward solution.

SilverFire commented 8 years ago

It's not possible to modify the script type with registerJs() method, because method adds scripts to View::js, which is grouped by position and key. We can not change this behavior because of BC.

Actually, I can suggest you a better workaround:

Register your own View component, extended from yii\web\View, add property jsBlocks and registerJsBlock() method with the following signature:

public function registerJsBlock($js, $options, $position)
{
    $this->jsBlocks[$position][] = Html::script($js, $options);
}

Then override renderBodyEndHtml() and renderHeadHtml() methods in order to use $this->jsBlocks array.

I don't think that we must include this behavior in core, so I'm closing the issue. If anybody has another ideas or arguments, why we must do it - feel free to post your opinions. Thanks

khalwat commented 7 years ago

I would like to echo @nirvana-msu 's comments... being able to pass in an additional parameter to View::registerJs() would be ideal, such as:

        $view->registerJs(
            $js,
            View::POS_HEAD,
            $key,
            $options
        );

And if $options is present, pass those in to Html::script() rather than just hardcoding ['type' => 'text/javascript']

That could still be passed in by default if no $options are provided, to preserve backwards compatibility.

JSON-LD can appear anywhere in the document, so it piggybacks on the various positions offered by View::js and wouldn't interfere with that in any way (browser just ignore the ld+json scripts).

The reason I can't use your solution @SilverFire is I'm operating inside of a framework (Craft CMS) that already has extended View -- so I have no opportunity to do as you've suggested. I'm just a component running inside of the existing app.

It seems like you have a perfect solution 99% in place, there just needs to be a way to pass down $options to $view->registerJs(), and you maintain BC 100% by just having the function be:

    public function registerJs($js, $position = self::POS_READY, $key = null, $options=['type' => 'text/javascript'])
brandonkelly commented 7 years ago

Guessing the BC concern here is, to be consistent with all the other View functions that accept an $options arg, $options must come before $key.

@SilverFire would you be at all open to some code like this, for BC’s sake? (Could be removed in the next “breaking change” release, whatever that ends up being.)

public function registerJs($js, $position = self::POS_READY, $options = [], $key = null)
{
    // Map $options -> $key for BC
    if (is_string($options) && $key === null) {
        $key = $options;
        $options = [];
    }

    // ...
}

From there the actual implementation would be easy - just add a new $jsOptions array on the class that stores the JS options per position and key, just like $js, and reference that from renderHeadHtml(), renderBodyBeginHtml(), and renderBodyEndHtml() when calling Html::script().

brandonkelly commented 7 years ago

Another argument I’d make for why this should be possible, beyond the JSON-LD use case, is just pure consistency. registerCss(), registerCssFile(), and registerJsFile() each have an $options argument; registerJs() is the only one that is awkwardly lacking it.

khalwat commented 7 years ago

Just an implementation note... it looks like if you pass in multiple JavaScripts to $view->registerJs() it puts them all into one <script> tag, with a ; after each script.

For JSON-LD, if you followed the same methodology, the JSON-LD should be wrapped in an array [] with a , after each one, like this:

<script type="application/ld+json">
[{
    "@context": "http://schema.org",
    "@type": "Person",
    "description": "wife",
    "name": "Polly",
    "url": "https://nystudio107.com"
},
{
    "@context": "http://schema.org",
    "@type": "Person",
    "description": "husband",
    "name": "Andrew",
    "url": "https://nystudio107.com"
}]
</script>

-OR-

Each one could simply be wrapped in their own <script> tags, such as this:

<script type="application/ld+json">
{
    "@context": "http://schema.org",
    "@type": "Person",
    "description": "wife",
    "name": "Polly",
    "url": "https://nystudio107.com"
}
</script>
<script type="application/ld+json">
{
    "@context": "http://schema.org",
    "@type": "Person",
    "description": "husband",
    "name": "Andrew",
    "url": "https://nystudio107.com"
}]
</script>

So to abstract this, one of the $options you could pass in could be a "uniqueTags" setting, to render the <script> tag pairs for each script registered, or you'd need to be able to pass in the "before" ([), "after" (]), and "delimiter" (,) for each script to make it work.

So it's either that, or an additional registerJsonLd() method that understands how to do the right thing for JSON-LD.

Validation can be done here: https://search.google.com/structured-data/testing-tool/

khalwat commented 7 years ago

So I implemented this myself in yii\web\View this is tested and does exactly what's needed. I'd be happy to do a PR if you like. It's pretty simple.

Since browsers just ignore JSON-LD, it doesn't matter where the JSON-LD goes on the page, so we don't need to offer up a $position as Yii2 does with $view->registerJs(), but rather we can treat it the way Yii2 treats CSS in $view->registerCss().

That is, we render it in one specific place (after the <body> tag) and we enclose each registered JSON-LD block in its own <script type="application/ld+json"> & </script> tags.

yii\web\View:

Added:

    /**
     * @var array the registered JsonLd blocks
     * @see registerJsonLd()
     */
    public $jsonLd;

Added:

    /**
     * Registers a JsonLd block.
     * @param string $jsonLd the JsonLd block to be registered
     *
     * @param string $key the key that identifies the JsonLd block. If null, it will use
     * $jsonLd as the key. If two JsonLd blocks are registered with the same key, the latter
     * will overwrite the former.
     */
    public function registerJsonLd($jsonLd, $key = null)
    {
        $key = $key ?: md5($jsonLd);
        $this->jsonLd[$key] = Html::script($jsonLd, ['type' => 'application/ld+json']);
    }

Modified:

    /**
     * Renders the content to be inserted at the beginning of the body section.
     * The content is rendered using the registered JS code blocks and files.
     * @return string the rendered content
     */
    protected function renderBodyBeginHtml()
    {
        $lines = [];
        if (!empty($this->jsFiles[self::POS_BEGIN])) {
            $lines[] = implode("\n", $this->jsFiles[self::POS_BEGIN]);
        }
        if (!empty($this->js[self::POS_BEGIN])) {
            $lines[] = Html::script(implode("\n", $this->js[self::POS_BEGIN]), ['type' => 'text/javascript']);
        }
        if (!empty($this->jsonLd)) {
            $lines[] = implode("\n", $this->jsonLd);
        }

        return empty($lines) ? '' : implode("\n", $lines);
    }
khalwat commented 7 years ago

Here's the PR for it: https://github.com/yiisoft/yii2/pull/13949

khalwat commented 7 years ago

One more implementation note... I think when you do get around to looking at this, piggybacking on $view->registerJs() is a bad idea. The reason is that Js = JavaScript, which is a specific thing.

The <script> tag is a general thing, which JavaScript is a subset of. JSON-LD is emphatically not JavaScript, so allowing people to register it (or other potential future scripts) via registerJs() really would be incorrect.

Probably the best way to do it would be to have a generic registerScript which you could pass in the $options and then have it special-case based on the type of script that it is, in terms of handling the special-casing you do for JavaScript right now.

samdark commented 5 years ago

https://github.com/yiisoft/yii2/issues/15651

AScarj commented 5 years ago

JSON-LD rules require the location of the code in <head>, so you need to modify the renderHeadHtml() method.

    /**
     * Renders the content to be inserted in the head section.
     * The content is rendered using the registered meta tags, link tags, CSS/JS code blocks and files.
     * @return string the rendered content
     */
    protected function renderHeadHtml()
    {
        $lines = [];
        if (!empty($this->metaTags)) {
            $lines[] = implode("\n", $this->metaTags);
        }

        if (!empty($this->linkTags)) {
            $lines[] = implode("\n", $this->linkTags);
        }
        if (!empty($this->cssFiles)) {
            $lines[] = implode("\n", $this->cssFiles);
        }
        if (!empty($this->css)) {
            $lines[] = implode("\n", $this->css);
        }
        if (!empty($this->jsFiles[self::POS_HEAD])) {
            $lines[] = implode("\n", $this->jsFiles[self::POS_HEAD]);
        }
        if (!empty($this->js[self::POS_HEAD])) {
            $lines[] = Html::script(implode("\n", $this->js[self::POS_HEAD]));
        }
        if (!empty($this->jsonLd)) {
            $lines[] = implode("\n", $this->jsonLd);
        }

        return empty($lines) ? '' : implode("\n", $lines);
    }
samdark commented 5 years ago

See #7