GDRP: Extend script tags with additional attributes

3l73 commented 2 years ago

Is your feature request related to a problem? Please describe.

The script tag placed inside the header, does not allowed additional attributes for ie. GDPR frameworks.

As the code is marked as final, an inheritance is not possible.

Describe the solution you'd like

Since not all GDPR frameworks require additional attributes, there are two possible options:

extending the site management section with a text area which contains key/value pairs that will be rendered as attributes
trigger an event that allows to modifiy the script tag

Describe alternatives you've considered

Replace the extension with a simple TypoScript code.

Teachability, Documentation, Adoption, Migration Strategy

-

Additional context

-

3l73 commented 2 years ago

If you want, I would provide a pull request for option 1, since this is easier to use for non developers.

brotkrueml commented 2 years ago

Thanks for the feature request. Option 1 is not possible as this requires IRRE capabilities for extensions in site handling. By now, it is hardcoded for core (like languages). It is possible to do so in yaml directly, but that is not the way for non-developers.

So I would go with a PSR-14 event. Can you give an example for the adoption of the script tag?

3l73 commented 2 years ago

First step is a new event AddScriptTagParameterEvent, which allows a developer to add additional attributes.

It has methods to add, change, get and remove attributes. Additionally it offers the current SiteLanguage.

Before call $pageRenderer->addHeaderData the event is processed.

If the event contains any attributes, these will be added to the script tag.

brotkrueml commented 2 years ago

I see the need for the possibility to add additional attributes. But I don't get the concrete use case by now.

I assume you mean

<script>
    var _paq=window._paq||[];
    // ...
</script>`

So, an event can adjust this script tag, for example:

<script data-foo="bar">
    var _paq = window._paq || [];
    // ...
</script>

It is restricted to data attributes or can there be other attributes as well (and which ones, maybe type)? What is the GDPR tool/content manager then do? Sorry, have to ask, as I don't know/use that (I run Matomo only cookie-less and with IP anonymisation, so no hassle with GDPR). So a concrete example for understanding would be useful for me ;-)

The event should then be dispatched in Classes/Hooks/PageRenderer/TrackingCodeInjector.php as this is the extensions' hook for the page renderer which takes care the <script> code.

I also would suggest to name the event EnrichScriptTagEvent as this is more neutral and can later be enhanced to do possibly more. The methods describe then what can be done (addAttribute(), getAttributes()).

Why do you want to add the methods change and remove to the event? I don't see the use case here, but the code may get unnecessary more complex (which I want to avoid without reason).

Why do you need the SiteLanguage in the event? I can think of German pages: yes, adjust the script attribute, but for Brasilian pages maybe not or in another way. The Request object has a broader scope, from the request you can retrieve the current site language - so that would be preferrable.

3l73 commented 2 years ago

I see the need for the possibility to add additional attributes. But I don't get the concrete use case by now.

If you use for example klaro.js you need to place attributes in order that klaro can identify each external resource.

These attributes define the service i.e. Matomo., the source (URI) etc.

I assume you mean
<script>
    var _paq=window._paq||[];
    // ...
</script>`
So, an event can adjust this script tag, for example:
<script data-foo="bar">
    var _paq = window._paq || [];
    // ...
</script>
It is restricted to data attributes or can there be other attributes as well (and which ones, maybe type)?

Data attributes as well as the type attribute in context of klaro.js

What is the GDPR tool/content manager then do? Sorry, have to ask, as I don't know/use that (I run Matomo only cookie-less and with IP anonymisation, so no hassle with GDPR). So a concrete example for understanding would be useful for me ;-)

According to the GDPR regulations the customer must consent (since the other options doesn't match my use case). This is an active decisions, even if the tracking software respects the "Do not track" setting of the browser.

The event should then be dispatched in Classes/Hooks/PageRenderer/TrackingCodeInjector.php as this is the extensions' hook for the page renderer which takes care the <script> code.

I also would suggest to name the event EnrichScriptTagEvent as this is more neutral and can later be enhanced to do possibly more. The methods describe then what can be done (addAttribute(), getAttributes()).

Sounds great.

Why do you want to add the methods change and remove to the event? I don't see the use case here, but the code may get unnecessary more complex (which I want to avoid without reason).

Depending of the project this could be useful. But true, it will be of rate use.

Why do you need the SiteLanguage in the event? I can think of German pages: yes, adjust the script attribute, but for Brasilian pages maybe not or in another way. The Request object has a broader scope, from the request you can retrieve the current site language - so that would be preferrable.

True, the idea was may be to add additional language information if required.

brotkrueml commented 2 years ago

Okay, got it. We also use Klaro and trigger events to Matomo Tag Manager which then embeds the script. But that's tricky, when Matomo Integration extension is used. So, a use case would be to set, for example, the type of the script tag to "text/plain" and after consent then alter it to "text/javascript" (or remove it).

So: The methods of the event can be:

addAttribute(string $name, string $value): void to add an attribute. Already existing attributes would be overridden.
getAttributes(): array which returns the attribute pairs
getRequest(): ServerRequestInterface which returns the request object

I usually tag the getters as @internal as it is not needed for the user, but can use at own risk (getRequest() of course not).

Or, alternatively, make it more strict:

addDataAttribute($name, $value): void to add an attribute (without data-). Already existing attributes would be overridden.
getDataAttributes(): array which returns the attribute pairs
setType(string $value): void to set a type (for example text/plain). An empty string means, no type to use.
getType(): string which returns the type attribute (also @internal)
getRequest(): ServerRequestInterface which returns the request object

I assume, then also

setId(string $value): void. An empty string means, no id to use.
getId(): string

are necessary.

Personally, I like the strict approach more ;-) This way, you can use empty data attribute values (then only data-foo is used instead of data-foo="". But avoid type="" or id="" (or don't have to check which attributes can be empty and which not when iterating over all attributes from variant 1 in the hook).

If you like you can make a PR and test it against your installation. Plus points are available for adding unit tests and adjusting the documentation/changelog. One or more use cases in the chapter "Use Case" would be also great as this helps other users to understand the capability of the extension and guide to do things :-)

3l73 commented 2 years ago

Okay, I hopefully be able to offer a pull request tomorrow or the next days.

3l73 commented 2 years ago

The pull request is now available.

It is currently a draft in order to talk about the implementation.

brotkrueml / typo3-matomo-integration