plone / plone.staticresources

Static resources for Plone
https://pypi.org/project/plone.staticresources/
5 stars 12 forks source link

Revert "Revert "use / for font url to fix production-bundle issues"" #175

Closed thet closed 2 years ago

thet commented 2 years ago

This reverts commit 9bc2a08fc8dc439dd4b33b495daa1816aec0af23.

In production mode, icon fonts are not found due to urls like: http://localhost:8080/Plone/++plone++production/++unique++2021-10-22T12:54:21.278234/++plone++static/++plone++static/fonts/plone-fontello.ttf?89786008

mister-roboto commented 2 years ago

@thet thanks for creating this Pull Request and help improve Plone!

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass.

Whenever you feel that the pull request is ready to be tested, either start all jenkins jobs pull requests by yourself, or simply add a comment in this pull request stating:

@jenkins-plone-org please run jobs

With this simple comment all the jobs will be started automatically.

Happy hacking!

thet commented 2 years ago

@jenkins-plone-org please run jobs

sneridagh commented 2 years ago

@thet could you please elaborate why did you revert this? Also, why is it needed?

image

In a site deployed in a different directory it will fail... because there's no Plone in the root...

Also, I realised of this is present also in 5.2.7 after upgrading a site to that.

/cc @erral @tisto

erral commented 2 years ago

There was an error for Plone sites not deployed in the root of the domain, reported by @cekk here:

https://github.com/plone/plone.staticresources/issues/162

I thought I found the issue and added a PR to fix that:

https://github.com/plone/plone.staticresources/pull/168

But it looks like it did not fixed the issue, and I think it was merged without properly testing it. I think @agitator pinged me about it, so I reverted my PR and reopened the original issue:

https://github.com/plone/plone.staticresources/pull/171

erral commented 2 years ago

So the current status should be the original: fontello icons are no properly rendered it Plone is not deployed in the root of the domain.

tisto commented 2 years ago

@thet we really need feedback from you here. This is a serious issue for us and IMHO for the Plone community as well. We shipped the latest Plone version to lots of clients that are important to us, and all icons of the site are broken.

I understand if you don't have time right now to look into this. Though, then the right cause of action here would be to revert whatever commit/PR caused this and ship a new plone.staticresources package right away. IMHO this issue is serious enough to ask the @plone/release-team to ship Plone 5.2.8 asap after creating a fix.

@erral can you pinpoint the PR/commit that initially caused this?

erral commented 2 years ago

I think that the issue came after this commit:

https://github.com/plone/plone.staticresources/commit/0228cd7ccceba2fed4ae7ff9b43459defbcd2957

thet commented 2 years ago

@tisto I agree this needs to be fixed. If I remember correctly making the font paths relative works in development mode but not in production mode, where the ++unique++ and ++production++ paths are inserted. @agitator should know about some other implications on this, he was providing an initial fix on that.

In current not-yet-released Plone 6 with the new resource registry this shouldn't happen anymore AFAIK /cc @jensens

I'm open for any good solution on this and do not have strong opinions other than a fix should not break another valid and common use case. so go ahead with this!

jensens commented 2 years ago

++unique++ is replaced by ++webresource++. ++production++ is no longer needed since merging does not make sense anymore.

Overall all three traversers are based on the same plone.resource.traversal.UniqueResourceTraverser, so they work exactly the same. The only difference is the way the unique part is created. So overall, without looking further, the behavior should be the same and the failure as well, I guess, just with a different name between the 2 ++.

The idea to base the traversers on the root/navigation root is to make all cachable at its best. This happens here: https://github.com/plone/Products.CMFPlone/blob/1f09a8585c152ed559fdc24056bd8b77941b248a/Products/CMFPlone/resources/browser/resource.py#L250-L256

sneridagh commented 2 years ago

But this is all about Plone 6. I assume Plone 6 will be fine.

How we address the problem that in production mode a relative link to that is broken? Also, I remember that something like that happen in Diazo, but it manages to inject the right path always.

jensens commented 2 years ago

Right, in 6 we also do not have fontello anymore, we use the new icon sub-system.