Deployments not rooted fail to load the Fontello icons

@thet could you please elaborate why we need the fonts rooted? What's the reasoning behind? This will break any deployment that involves not having Plone in the root.

I filed this: Deployments not rooted fail to load the Fontello icons · Issue #203 · plone/plone.staticresources · GitHub

Current state in a non-rooted deployment:

Plone 6.0.0a3

Just after start the site:

image

Plone 5.2.7

Also present in 5.2.7 that I upgraded some days ago.

image

See also Revert "Revert "use / for font url to fix production-bundle issues"" by thet · Pull Request #175 · plone/plone.staticresources · GitHub

Related: Problem with fontello resources with 1.4.3 version · Issue #162 · plone/plone.staticresources · GitHub

Revert "use / for font url to fix production-bundle issues" by erral · Pull Request #168 · plone/plone.staticresources · GitHub and

I can confirm that 5.2.6 and 5.2.5 have this bug present there as well. 5.2.4 do not.

I'd like to figure this out, discuss it with whoever is necessary and fix it right away. I can help with the implementation too.

/cc @mauritsvanrees @tisto @erral @cekk @ericof @agitator @thet @jensens

From what I read in Problem with fontello resources with 1.4.3 version · Issue #162 · plone/plone.staticresources · GitHub

an attempt was already made to revert the use of / prefix for loading the font resources, but there is an issue with it being bundled in a released package as @cekk reported there as a last message.

IMHO the problem is not in rooted/non rooted. In css, resources are relative to the css that declares them. So, if the css is:

/++plone++production/++unique++2022-03-31T04:05:17.238056/default.css

then fontello mus be loaded as

../../ (-> ++unique++ -> ++plone++production) ++plone++static/fonts/plone-fontello.woff

I don't understand what is so complicated. We also have preprocessors, so use them to set the levels of ../../ (plone 6 needs 3 ../) in css. This is not a bug but mismanagement due to abuse of traversal.

++production++, ++static++, ++webresources++ are not regular objects to traverse, you will get a lot of insufficient privileges if you rely on it.

We've the same problem in Diazo when using alternative config loaded (see backend.xml in barceloneta Plone 6, backend.xml. <link rel="stylesheet" href="++theme++barceloneta/css/barceloneta.css" /> fails if the content is private · Issue #261 · plone/plonetheme.barceloneta · GitHub) using relative path. In Diazo there's no root concept but Diazo knows how to handle relative links to the ++mytheme++ namespace, which is 1 level below root.

These bugs exists because we want to handle them with Acquisition, which is wrong and unneeded in this case because we know the paths (actually we create them, so...).

1 Like

Wouldn't be better to fix those traversal issues, so these resource paths are evergreen and fulfil their initial purpose? If they are incapable of doing that purpose well, then, is it worth to even continue using them?

@jensens do you think it is possible to fix these traversal issues when the traversed paths are fully/partially private or the user has no permissions over them?

I just talked with @agitator about that.
The revert from this PR Revert "Revert "use / for font url to fix production-bundle issues"" by thet · Pull Request #175 · plone/plone.staticresources · GitHub which changes the fontello font path to be absolute was because otherwise the fonts are not found in production mode due to the traversal problems already discussed here.

The problem originates from Move fonts to extra bundle by agitator · Pull Request #131 · plone/plone.staticresources · GitHub
This PR tried to reduce bundle sizes by moving out the icons into it's own bundles.

@sneridagh could you please try to not merge the plone-fontello and plone-glyphicons bundles? Maybe this solves the issue. Try to set:

  <records prefix="plone.bundles/plone-fontello"
            interface='Products.CMFPlone.interfaces.IBundleRegistry'>
    <value key="merge_with"></value>
  </records>

  <records prefix="plone.bundles/plone-glyphicons"
            interface='Products.CMFPlone.interfaces.IBundleRegistry'>
    <value key="merge_with"></value>
  </records>

Please note, in the next Plone 6 alpha release which will introduce the change to ES6, inline SVG icons and the new resource registry this should not be an issue anymore. Fontello isn't used anymore and we include all icons inline. Also the resource registry does not do this bundle merging and path mangling with ++unique++ and ++production++ urls.

If nothing helps you could revert (manually, selectively) Move fonts to extra bundle by agitator · Pull Request #131 · plone/plone.staticresources · GitHub and restore the situation where we had fontello and glyphicons included for each bundle.

this significantly increases the bundle size but should also completly solve this problem here.

Yes, not merging "should" be the easiest fix to use it with relative path.
Also it's probably not much use for anonymous users and should get a only-for-logged-in-condition.

Ok! So in Plone 6 we will be ok in the next release. One down.

Regarding the workaround you proposed, I think that this is beyond to find a workaround. The fact is that everybody upgrading their sites to that versions, will get the issue, no matter what they do. I don't know if we could have minimised it with an upgrade step (I can see one in p.resources, but apparently not for dealing with this).

I understand the purpose of Move fonts to extra bundle by agitator · Pull Request #131 · plone/plone.staticresources · GitHub and the nature of the improvement. Also, tbh it seems to me quite a breaking change introduced in a minor version without notice. Also, the fact that it happened several minor versions ago it hardens the situation.

So I do not see reverting that PR an option, because we will break again some deployments, and we will have to write an inverse upgrade step.

Also I can't see now how a link to a static resource in ++plone++static has to do with a bundle. When the production bundle is created, then it gets it from the instance root at the moment of compilation, right? There is no way to continue using relative resources instead of using the root? I would need someone to explain it to me and show me the issue. Why we cannot revert this?

Why does production bundle breaks?

OK, I tried it. But it doesn't work anyways. The generated URL is /Plone/++plone++static/++unique++2021-10-22%2014%3A35%3A54.260820/plone-glyphicons-compiled.css, so still this ++unique++ in between.

Sorry for my ignorance, but who puts that string in between? The RR?

I don't really understand the problem anymore.

I tried to access the fontello resources with a relative path.
If the calling resource is:

http://localhost:8080/Plone/++plone++static/++unique++2021-10-22%2014%3A35%3A54.260820/plone-fontello-compiled.css

then I can successfully access the plone-fontello.woff font via:

http://localhost:8080/Plone/++plone++static/++unique++2021-10-22%2014%3A35%3A54.260820/++plone++static/fonts/plone-fontello.woff

as well as:

http://localhost:8080/Plone/++plone++static/++unique++2021-10-22%2014%3A35%3A54.260820/fonts/plone-fontello.woff

So, changing the paths from absolute /++plone++static/fonts/ to relative ++plone++static/fonts/ or even fonts/ should just work fine with the added benefit, that the resulting URL will contain the ++unique++ part for caching/cache breaking.

That is probably what you want, right @sneridagh ?

Do you see any problem with that approach, @agitator ?

the ++unique++ string is a feature of plone.resource and ensures unique URLs which can easily be cached. I just before talked with @jensens about that - we need that ++unique++ (++webresource++ in Plone 6) because cache-breaking URL search strings (?t=1234) are not allowed with some popular CDNs (right @jensens ?).

The ++production++ is part of the resource merging into production bundles and done in CMFPlone.

See my comment from: Revert "use / for font url to fix production-bundle issues" by erral · Pull Request #168 · plone/plone.staticresources · GitHub

the default.css is created here: Products.CMFPlone/Products/CMFPlone/resources/browser/styles.py at c4014de40e21cd02c089861da0a7b6baf8850ab2 · plone/Products.CMFPlone · GitHub
The ++plone++production traversal is handled here: Products.CMFPlone/Products/CMFPlone/traversal.py at c4014de40e21cd02c089861da0a7b6baf8850ab2 · plone/Products.CMFPlone · GitHub
And the ++unique++ traversal here: plone.resource/plone/resource/traversal.py at 2ded97029aca909232940e30ab058da92660edb2 · plone/plone.resource · GitHub
For completness and because I just found it along the way, the ++theme++ traverser is here plone.app.theming/src/plone/app/theming/traversal.py at e6b0ea42e9078e4ed2365fab6cc656098abe727b · plone/plone.app.theming · GitHub

hope that helps!

yeah, at least is how I thought that it used to work, right? Sorry for being so rusty on RR matters :frowning: I can barely remember how they work, least to say internally! :')

I hope this fixes it: Fix icon font loading by thet · Pull Request #207 · plone/plone.staticresources · GitHub

The fix is to use relative font paths AND not merge the plone-glyphicons and plone-fontello bundles in production mode.

2 Likes

I have installed the 1.x branch in a client's site running Plone 5.2.7, and after running the upgrade step the icons are showing OK in both deployment scenarios (full domain and a path under the domain) and with debug mode on and off.

1 Like

Awesome! Thanks for the swift response @thet! Thanks, @erral for giving it a shot! I guess we can make a plone.staticresources release now and then include it in the upcoming Plone 5.2.8 release.

2 Likes

@erral @sneridagh plone.staticresources · PyPI 1.4.5 has been released with the fix.

I briefly talked to @mauritsvanrees and @thet on Discord and then went ahead and did the release. Please give it a shot folks and report back if the release fixed the issue.

3 Likes

I have just installed the new plone.staticresources 1.4.5 in a client's production site deployed in a path and the icons are showing ok

2 Likes

Awesome. Thanks for the feedback @erral!