Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow for managing third-party javascript integrations to be placed at the end of the head-section as well as the end of the page. #3931

Closed
jladage opened this issue Mar 26, 2024 · 22 comments · Fixed by plone/plone.base#60, plone/plone.app.layout#361 or plone/plone.app.upgrade#323

Comments

@jladage
Copy link

jladage commented Mar 26, 2024

PLIP (Plone Improvement Proposal)

Responsible Persons

Proposer: Jean-Paul Ladage

Seconder: Maurits van Rees

Abstract

Some Javascript integrations require injecting js at the end of the HEAD section, others require it at the bottom of the page.

Motivation

Currently Plone only allows for including js at the bottom of each page and the label Website Statistics makes it focused on limited use. E.g. Google Tag Manager requires inclusion of the tag in the head section. Imho Plone at least should allow website managers to include in either places.

Assumptions

Plone classic websites, no not every website will be a Volto site in the future.

Proposal & Implementation

Duplicate the webstats code for the head slot and assign it to the the IScripts viewletmanager.

Deliverables

Risks

No pain, just pleasure.

Participants

  • Jean-Paul Ladage
  • Maurits van Rees
@mauritsvanrees
Copy link
Sponsor Member

For me this would be a small addition that would be safe to add in Plone 6.0 already.
But I would want to hear opinions from others.

Main question: is two fields the right way to do this? For us it makes sense.

@gforcada
Copy link
Sponsor Contributor

LGTM 👍🏾

@davisagli
Copy link
Sponsor Member

I think it's fine. Should it be added at the top or bottom of the scripts viewlet manager?

@jladage
Copy link
Author

jladage commented Mar 28, 2024 via email

@yurj
Copy link
Contributor

yurj commented Mar 28, 2024

Preferable as the last script-tag in de head section, yes. You have an idea how to put it last, or should we create a separate provider after the plone.scripts one?

https://github.com/plone/plone.app.layout/blob/1e9df41d9f325f31fd3aa3cae0a4151ef917e3f9/plone/app/layout/links/configure.zcml#L9

I think that just adding here in the right position is enough, no need for a separate zcml.

@mauritsvanrees
Copy link
Sponsor Member

@davisagli Just checking: Volto does not use the current webstats_js field? Probably not, I don't see it on demo.plone.org in the Site control panel.

@davisagli
Copy link
Sponsor Member

@mauritsvanrees That's right.

@mauritsvanrees
Copy link
Sponsor Member

I think this is ready. The combined Jenkins jobs passed earlier today, but I am running them again with all the latest minor updates.

I have approved all three linked PRs, but I was involved in them (Jean-Paul is my former boss, and we are still working together), so it would be good if someone else checks them.

The script is added at the end of the html head now.

To check it:

  • bin/develop co plone.app.layout plone.base and use branch 3931.feature of those two packages plus plone.app.upgrade which is already checked out.
  • In portal_setup/manage_upgrade manually apply the latest upgrade step of Products.CMFPlone:plone ("Run to_isiteschema upgrade profile") or create a fresh Plone Classic UI site.
  • Go to the Site settings and add a console.log or alert to the two javascript fields.

@yurj
Copy link
Contributor

yurj commented Mar 29, 2024

Just to note that you can insert almost anything, not just javascript. Adding:

<link rel="stylesheet" href="`http url with a css`">

or

 <style>
  body {background-color: powderblue;}
  h1 {color: red;}
  p {color: blue;}
</style> 

or

<meta name="keywords" content="HTML, CSS, JavaScript">

is valid. <title> and <base> tags are already set in the same viewlet manager and it is recommended to don't have multiple <title> and <base> tags.

@erral
Copy link
Sponsor Member

erral commented Mar 30, 2024

Usually there's also the need to embed a JS script as a first thing in the <head> tag. It would be nice to have that too as part of this PLIP.

@jladage
Copy link
Author

jladage commented Mar 30, 2024

Just to note that you can insert almost anything, not just javascript. Adding:

<link rel="stylesheet" href="`http url with a css`">

or

 <style>
  body {background-color: powderblue;}
  h1 {color: red;}
  p {color: blue;}
</style> 

or

<meta name="keywords" content="HTML, CSS, JavaScript">

is valid. <title> and <base> tags are already set in the same viewlet manager and it is recommended to don't have multiple <title> and <base> tags.

Hi @yurj, I added some lxml code to filter only script tags to be rendered in the viewlet, see plone/plone.app.layout@a95f03d

Please review this code, I had some UTF weirdness in the doctest, but it seems to work fine.

@jladage
Copy link
Author

jladage commented Mar 30, 2024

Usually there's also the need to embed a JS script as a first thing in the <head> tag. It would be nice to have that too as part of this PLIP.

Oh really, I didn't see one yet. Do you have examples of integrations that require a script tag before loading any other script, but for hackers? ;)

@erral
Copy link
Sponsor Member

erral commented Mar 30, 2024

Google Tag Manager requires to be inserted as a first thing in the head tag:

https://developers.google.com/tag-platform/gtagjs/install

Similarly CMPs like CookieBot require the same.

@jladage
Copy link
Author

jladage commented Mar 30, 2024

Ah, okay! That's the use case I should provide. Do we really want 3 fields (head start, head end and page end) or would it suffice when I change the viewlet to render at the top of the head?

@jladage
Copy link
Author

jladage commented Mar 30, 2024

Well, I already switch to using IHtmlHead viewlet manager, so it will be loaded before the other script tags. Earlier is not really necessary, except that they would register a fraction more bounces of people clicking the cancel button really fast. ;)

@yurj
Copy link
Contributor

yurj commented Mar 30, 2024

Just to note that you can insert almost anything, not just javascript. Adding:
<link rel="stylesheet" href="`http url with a css`">
or

 <style>
  body {background-color: powderblue;}
  h1 {color: red;}
  p {color: blue;}
</style> 

or
<meta name="keywords" content="HTML, CSS, JavaScript">
is valid. <title> and <base> tags are already set in the same viewlet manager and it is recommended to don't have multiple <title> and <base> tags.

Hi @yurj, I added some lxml code to filter only script tags to be rendered in the viewlet, see plone/plone.app.layout@a95f03d

Please review this code, I had some UTF weirdness in the doctest, but it seems to work fine.

Hi!

I think it is not necessary to allow only script tags. The webstat js field can be filled only by site admins or managers, so there's no real security problem because it is supposed they know what they're doing. Also this code will do the filtering at every rendering, you can use the registry modification time (see _RESOURCE_REGISTRY_MTIME from https://github.com/plone/Products.CMFPlone/blob/master/Products/CMFPlone/resources/browser/resource.py) as a cachekey if you wish.

@erral
Copy link
Sponsor Member

erral commented Mar 31, 2024

Ah, okay! That's the use case I should provide. Do we really want 3 fields (head start, head end and page end) or would it suffice when I change the viewlet to render at the top of the head?

We have added this kind of fields several times in some of our projects.

Some site admins are really picky about this (it should be possible because our SEO guys say it is possible and bla bla bla...).

@jladage
Copy link
Author

jladage commented Apr 2, 2024

Just to note that you can insert almost anything, not just javascript. Adding:
<link rel="stylesheet" href="`http url with a css`">
or

 <style>
  body {background-color: powderblue;}
  h1 {color: red;}
  p {color: blue;}
</style> 

or
<meta name="keywords" content="HTML, CSS, JavaScript">
is valid. <title> and <base> tags are already set in the same viewlet manager and it is recommended to don't have multiple <title> and <base> tags.

Hi @yurj, I added some lxml code to filter only script tags to be rendered in the viewlet, see plone/plone.app.layout@a95f03d
Please review this code, I had some UTF weirdness in the doctest, but it seems to work fine.

Hi!

I think it is not necessary to allow only script tags. The webstat js field can be filled only by site admins or managers, so there's no real security problem because it is supposed they know what they're doing. Also this code will do the filtering at every rendering, you can use the registry modification time (see _RESOURCE_REGISTRY_MTIME from https://github.com/plone/Products.CMFPlone/blob/master/Products/CMFPlone/resources/browser/resource.py) as a cachekey if you wish.

Hi @yurj , I changed the code so base and title tags are stripped and all remaining valid tag are rendered. I'm totally not familiar with the code you pointed at, but considering it's 4 light calls extra, it will hardly make a difference iyam. ;) In case you or others insist, feel free to help out.
plone/plone.app.layout@5880d2e

@thet
Copy link
Member

thet commented Apr 4, 2024

Another use case of adding something first is to set a variable other scripts depend upon. We did something like that in older versions of Quaive.

@jladage
Copy link
Author

jladage commented Apr 4, 2024

Another use case of adding something first is to set a variable other scripts depend upon. We did something like that in older versions of Quaive.

That's now working. the head js is injected before all javascripts. ;)

mister-roboto pushed a commit to plone/buildout.coredev that referenced this issue Apr 23, 2024
Branch: refs/heads/master
Date: 2024-03-26T16:39:23+01:00
Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org>
Commit: plone/plone.app.upgrade@216036b

Add upgrade to add registry record ``webstats_head_js`` to ``ISiteSchema``.

This is part of plone/Products.CMFPlone#3931.

Files changed:
A news/3931.bugfix
A plone/app/upgrade/v60/profiles/to_isiteschema/registry.xml
M plone/app/upgrade/v60/configure.zcml
M plone/app/upgrade/v60/profiles.zcml
M plone/app/upgrade/v61/configure.zcml
Repository: plone.app.upgrade

Branch: refs/heads/master
Date: 2024-04-23T09:54:16+02:00
Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org>
Commit: plone/plone.app.upgrade@eaaa75e

Merge remote-tracking branch 'origin/master' into 3931.feature

Files changed:
A news/324.feature
A plone/app/upgrade/v61/alpha.py
M plone/app/upgrade/utils.py
M plone/app/upgrade/v52/final.py
M plone/app/upgrade/v60/tests.py
M plone/app/upgrade/v61/configure.zcml
Repository: plone.app.upgrade

Branch: refs/heads/master
Date: 2024-04-23T09:54:38+02:00
Author: Maurits van Rees (mauritsvanrees) <m.van.rees@zestsoftware.nl>
Commit: plone/plone.app.upgrade@7caacae

Merge pull request #323 from plone/3931.feature

Add upgrade to add registry record ``webstats_head_js`` to ``ISiteSchema``

Files changed:
A news/3931.bugfix
A plone/app/upgrade/v60/profiles/to_isiteschema/registry.xml
M plone/app/upgrade/v60/configure.zcml
M plone/app/upgrade/v60/profiles.zcml
M plone/app/upgrade/v61/configure.zcml
mister-roboto pushed a commit to plone/buildout.coredev that referenced this issue Apr 23, 2024
Branch: refs/heads/master
Date: 2024-03-26T16:39:23+01:00
Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org>
Commit: plone/plone.app.upgrade@216036b

Add upgrade to add registry record ``webstats_head_js`` to ``ISiteSchema``.

This is part of plone/Products.CMFPlone#3931.

Files changed:
A news/3931.bugfix
A plone/app/upgrade/v60/profiles/to_isiteschema/registry.xml
M plone/app/upgrade/v60/configure.zcml
M plone/app/upgrade/v60/profiles.zcml
M plone/app/upgrade/v61/configure.zcml
Repository: plone.app.upgrade

Branch: refs/heads/master
Date: 2024-04-23T09:54:16+02:00
Author: Maurits van Rees (mauritsvanrees) <maurits@vanrees.org>
Commit: plone/plone.app.upgrade@eaaa75e

Merge remote-tracking branch 'origin/master' into 3931.feature

Files changed:
A news/324.feature
A plone/app/upgrade/v61/alpha.py
M plone/app/upgrade/utils.py
M plone/app/upgrade/v52/final.py
M plone/app/upgrade/v60/tests.py
M plone/app/upgrade/v61/configure.zcml
Repository: plone.app.upgrade

Branch: refs/heads/master
Date: 2024-04-23T09:54:38+02:00
Author: Maurits van Rees (mauritsvanrees) <m.van.rees@zestsoftware.nl>
Commit: plone/plone.app.upgrade@7caacae

Merge pull request #323 from plone/3931.feature

Add upgrade to add registry record ``webstats_head_js`` to ``ISiteSchema``

Files changed:
A news/3931.bugfix
A plone/app/upgrade/v60/profiles/to_isiteschema/registry.xml
M plone/app/upgrade/v60/configure.zcml
M plone/app/upgrade/v60/profiles.zcml
M plone/app/upgrade/v61/configure.zcml
@mauritsvanrees
Copy link
Sponsor Member

All merged, and will be released this week.
Thanks for pushing this one @jladage !

@erral
Copy link
Sponsor Member

erral commented Apr 23, 2024

I have updated the translation strings in plone.app.locales and tried to keep the existing translations for the languages the texts were translated it 🤞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment