silverstripe / silverstripe-framework

Silverstripe Framework, the MVC framework that powers Silverstripe CMS
https://www.silverstripe.org
BSD 3-Clause "New" or "Revised" License
719 stars 820 forks source link

ENH Create Requirements::customScriptWithAttributes #11076

Closed Ofthemasses closed 6 months ago

Ofthemasses commented 7 months ago

Allows custom type and crossorigin attributes when using the new method Requirements::customScriptWithAttributes, as suggested here: https://github.com/silverstripe/silverstripe-framework/pull/11061#discussion_r1402892340/.

Resolves #11060

Ofthemasses commented 7 months ago

Hi, I believe I fixed the errors from the last workflow.

Ofthemasses commented 7 months ago

Requests should be resolved in latest commit: 3f909d97e2508cf13669dd8e731a7fcbae88c159

Ofthemasses commented 6 months ago

Hi, I believe I fixed the errors from the last workflow.

Ofthemasses commented 6 months ago

Cant seem to reply to the left over ['content'] issue. But ill push a fix to this with a test for uniquenessID so this sort of issue is detected in future PRs.

Ofthemasses commented 6 months ago

Hi, I believe I fixed the errors from the last workflow.

Ofthemasses commented 6 months ago

Oops, misread that as space rather than new line. Pushed a new fix now

Ofthemasses commented 6 months ago

Something's not quite right with the $uniquenessID, it's showing custom attributes if a non attributes version is called after an attributes version with the same uniquenessID

    // PageController.php

    protected function init()
    {
        parent::init();
        Requirements::customScriptWithAttributes('console.log("Hello World abc!");', [
            'crossorigin' => 'abc'
        ], 'my-script');
        // this works correctly, it will overwrite the 'abc' script
        Requirements::customScriptWithAttributes('console.log("Hello World def!");', [
            'crossorigin' => 'def'
        ], 'my-script');
        // this incorrectly will include the custom crossorigin attribute
        Requirements::customScript('console.log("Hello World ghi!");', 'my-script');
    }

Gives an unexpected result:

<script type="application/javascript" crossorigin="def">//<![CDATA[
console.log("Hello World ghi!");
//]]></script>

Should be:

<script type="application/javascript">//<![CDATA[
console.log("Hello World ghi!");
//]]></script>

This makes sense, there isnt any clearing of the customScriptAttribute when Requirements::customScript is called. I'll push a fix to this with a test.

emteknetnz commented 6 months ago

Cheers for the contribution, this will be released in 5.2.0

Ofthemasses commented 6 months ago

Cheers for the contribution, this will be released in 5.2.0

Yay! Thank you for the support on this PR :grin:!