Do not inject portal if portal.url is set to 'false' in config file.#21936
Conversation
WalkthroughA guard clause was introduced in the 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms (9)
🔇 Additional comments (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
cathysarisky
left a comment
cathysarisky
left a comment
There was a problem hiding this comment.
This change makes a lot of sense to me. If the scriptUrl isn't set or is set to false, there's no reason to continue with the function.
It also brings the behavior in line with what the docs have said for a long time. :)
|
Hey @ltoinel , I'm not sure if you're still interested in pursuing this change or not, but I was able to snag a moment of dev attention for it. To be mergeable, it's going to need one or more tests added, showing that it results in the desired behavior. You might take a look at the tests for the ghost_head excludes behavior here as a starting point: |
|
Hey @ltoinel ! Since you didn't reply, I've pushed some tests that would cover the change in your PR, in hopes of getting this functionality ready to merge! |
cathysarisky
changed the title
9larsons
force-pushed
the
fix-member-js-injection-when-portal-off
branch
5 times, most recently
from
a2bae04 to
324ebf4
Compare
If portal URL is false, we return no value.
The original approach used an early `return ''` that suppressed the portal script, member styles (cta_styles) AND the Stripe script when `portal.url` was falsy. Per maintainer guidance, only the portal script should be skipped; member styles and Stripe must still be emitted. Wrapped just the portal-script construction in `if (scriptUrl)`, mirroring the falsy check used by the sibling getSearchHelper. This removes the spurious `<script src="false">` (a 404 on every page) when `portal.url: false` is configured, while leaving styles and Stripe untouched. Dropped the special-case for the literal string 'false' in favour of a plain falsy check, matching the documented boolean contract. Tests assert the portal script is absent while cta_styles and Stripe remain present; obsolete snapshots regenerated.
9larsons
force-pushed
the
fix-member-js-injection-when-portal-off
branch
from
324ebf4 to
f6d1e5f
Compare
4 participants
Original PR comment:
If portal URL is set to false in the config as described in the Ghost documentation, we have to return nothing.
https://ghost.org/docs/config/
"The script can be relocated by changing the URL, or disabled entirely by setting "url": false."
Cathy's addition, upon 'adopting' this PR:
Tests added, confirming the behavior is as expected, including if the url is set to string false, boolean false, null, etc.
While I'd generally encourage self hosters who don't want the portal code to load to just turn off memberships, this leaves a route for a site to use the api functionality behind members, donations, etc, without including the portal loading code.
Likely to be a common want? No. Minimal impact on code maintainability? I think so.