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

Add the portal_url parameter to be used with Diazo rules #376

Merged
merged 4 commits into from
Jul 25, 2024

Conversation

yurj
Copy link
Contributor

@yurj yurj commented Jul 23, 2024

Looking at this rule, for example:

    <!-- We can't control the bundle from here due to include. Just hard code -->
    <after css:theme-children="head">
      <link href="++theme++barceloneta/css/barceloneta.css"
            rel="stylesheet"
      />
    </after>

the css will be loaded relative to the content. This is problematic:

  • different url for the same css file, cache problems
  • traversing private content can lead to "Insufficient privileges" while the resource itself is public

This PR add the portal_url variable in the theme parameters (manifest.cfg) and apply it in backend.xml

yurj added 3 commits July 23, 2024 15:58
Add the portal_url parameter to be used with Diazo rules
Use the portal_url theme parameter in backend.xml
@mister-roboto
Copy link

@yurj thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

@yurj
Copy link
Contributor Author

yurj commented Jul 23, 2024

@jenkins-plone-org please run jobs

@yurj
Copy link
Contributor Author

yurj commented Jul 23, 2024

@thet can this PR be a problem for collective.lineage? It just use the variable in backend.xml.

@yurj
Copy link
Contributor Author

yurj commented Jul 23, 2024

Note: Jenkins failures have nothing to do with this PR

Use TAL expression, safer.
@yurj yurj requested a review from mtrebron July 23, 2024 14:53
@yurj
Copy link
Contributor Author

yurj commented Jul 23, 2024

@jenkins-plone-org please run jobs

@yurj
Copy link
Contributor Author

yurj commented Jul 24, 2024

Let's wait @petschki before merging it.

@yurj yurj requested a review from petschki July 24, 2024 05:56
@petschki
Copy link
Member

I thought this is automatically done when you add a starting slash to the resources in your theme template like /++theme++/... ... maybe someone can check this in plone.app.theming.

Greetings from Sardegna ☀️🏖️😅

@yurj
Copy link
Contributor Author

yurj commented Jul 24, 2024

I thought this is automatically done when you add a starting slash to the resources in your theme template like /++theme++/... ... maybe someone can check this in plone.app.theming.

Greetings from Sardegna ☀️🏖️😅

backend.xml haven't it (no slash in front of it). In custom themes there's no problem because they're custom. For VHM rewrited urls, portal_url should return the correct url (not / but /path/my/site/), so it should cover most of the supported cases. This PR does not use the portal_url parameter other than in backend.xml which actually is a relative link and should be fixed (different urls for the same file, traversing private content problem).

Great Sardegna! ☀️

Copy link
Member

@thet thet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 and ❤️ for this PR!
Minor comments though - I think I'd remove the curly braces, if they are not needed.
Check that again, please.

I'm approving, so you can check my comments and apply my suggestion if applicable or otherwise just merge, if the curlys are needed.

Greetings from our Wales/England vacation :D

plonetheme/barceloneta/theme/backend.xml Show resolved Hide resolved
plonetheme/barceloneta/theme/manifest.cfg Show resolved Hide resolved
@yurj yurj merged commit 0cbb049 into master Jul 25, 2024
12 checks passed
@yurj yurj deleted the yurj-theme-portal_url branch July 25, 2024 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants