Improve behaviour when a site URL is not explicitly set #1726
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
Author
emmadesilva
force-pushed
the
ensure-a-better-state-when-a-site-url-is-not-set
branch
3 times, most recently
from
8efa960 to
e71b1d5
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1726 +/- ##
============================================
- Coverage 99.97% 99.95% -0.03%
- Complexity 1746 1747 +1
============================================
Files 181 181
Lines 4667 4670 +3
============================================
+ Hits 4666 4668 +2
- Misses 1 2 +1 ☔ View full report in Codecov by Sentry. |
emmadesilva
force-pushed
the
ensure-a-better-state-when-a-site-url-is-not-set
branch
from
8ed2cc1 to
0439c16
Compare
We now return the relative path even if the base URL is not set, as this is more likely to be the desired behavior the user's expecting. If the user is trying to get the base URL, but it's not set, we throw the exception.
emmadesilva
force-pushed
the
ensure-a-better-state-when-a-site-url-is-not-set
branch
from
0439c16 to
3cfa8a5
Compare
This better matches the documented method description of "Check if a site base URL has been set in config (or .env).", as this value is `localhost` by default and thus not set by the user. Revert "Method `Hyde::hasSiteUrl()` now returns false when URL is for localhost" This reverts commit 8a13ad9. Reapply "Method `Hyde::hasSiteUrl()` now returns false when URL is for localhost" This reverts commit 0c4ab34. Update HelpersTest.php
Only the Markdown pages can be made like this
emmadesilva
commented
Jun 13, 2024
| } | ||
|
|
||
| // User is trying to get the base URL, but it's not set | ||
| throw new BaseUrlNotSetException(); |
Member
Author
There was a problem hiding this comment.
We might consider this for deprecation
The hasSiteUrl method now checks if the URL is for localhost
This was referenced Jun 30, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Abstract
We're getting some issues by Hyde using
localhostas the default site URL. Initially, this PR will target the 1.x branch because I consider these things to be bugs that should be fixed in v1, however, there may be side effects of this: primarily that output HTML will change - but I do not think anyone will depend on those undesired effects before this patch, since localhost is never desired in production. This will resolve #1720Expanding on the reasoning behind allowing this in 1.x: Since v1 is LTS, I don't want to leave those users hanging with what is in my opinion a bug, that localhost links can "leak" through production. And I think the only way to fix this is by changing some logic which could have side effects. But I think I am okay with that tradeoff as it stands now, especially since I think any possible usages that may be affected probably don't actually desire the logic we have now.
Major changes
Method
Hyde::hasSiteUrl()now returns false if URL is localhostThis better matches the documented method description of "Check if a site base URL has been set in config (or .env).", as this value is
localhostby default and thus not set by the user.Method
Hyde::url()now resolves relative paths instead of throwingWe now return the relative path even if the base URL is not set, as this is more likely to be the desired behavior the user's expecting. If the user is trying to get the base URL, but it's not set, we throw the exception.