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

Allow configuration of Base URL via website #5656

Merged
merged 4 commits into from Sep 13, 2023

Conversation

samc1213
Copy link
Contributor

@samc1213 samc1213 commented Sep 13, 2023

Closes #5246

Changes proposed in this pull request:

  • Add a configuration for base_url in the System Configuration page

How to test the feature manually:

  1. Open System Configuration
  2. Change "Base URL"
  3. Refresh page, and ensure Base URL is updated
  4. Check config.php and ensure base_url is updated

Pull request checklist:

  • clear commit messages
  • code manually tested
  • unit tests written (optional if too hard)
  • documentation updated

Additional information can be found in the documentation.

app/i18n/fr/admin.php Outdated Show resolved Hide resolved
@Alkarex Alkarex added this to the 1.22.0 milestone Sep 13, 2023
@Alkarex
Copy link
Member

Alkarex commented Sep 13, 2023

Thanks, and welcome 👍🏻

@Alkarex
Copy link
Member

Alkarex commented Sep 13, 2023

I have added an automatic detection hint:
image

Copy link
Member

@Frenzie Frenzie left a comment

Choose a reason for hiding this comment

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

makes sense

@samc1213
Copy link
Contributor Author

I have added an automatic detection hint: image

Love this idea - I tested it as well and it looks good to me.

@Alkarex Alkarex merged commit 52d87c3 into FreshRSS:edge Sep 13, 2023
1 check passed
@math-GH
Copy link
Contributor

math-GH commented Sep 14, 2023

I am a bit late to the party...
I would expect an improved documentation and would like to have here a warning message. It is very easy to destroy the application.

I changed http://localhost/freshrss/p to http://localhost/freshrss and it crashed the system.

grafik

It is to easy to fail here

@Frenzie
Copy link
Member

Frenzie commented Sep 14, 2023

True, something like this might need a new UI element. Or at least I don't think we have something like an input disabled with an unlock button next to it anywhere.

I mean exactly the same thing as the show password button basically, just to get rid of the disabled attribute. You could additionally add a little warning underneath the title if that's not sufficiently saying "dangerous" all by itself.
image

Alkarex added a commit to Alkarex/FreshRSS that referenced this pull request Sep 14, 2023
FreshRSS#5656 (comment)
Make read-only while waiting for a better approach (which can wait till release 1.23)
@Alkarex Alkarex mentioned this pull request Sep 14, 2023
@Alkarex
Copy link
Member

Alkarex commented Sep 14, 2023

Right, indeed @math-GH . This went a bit too fast indeed, and should have waited for 1.23.
Temporary mitigation #5657

@math-GH
Copy link
Contributor

math-GH commented Sep 14, 2023

Let me think loud:
If the base URL is wrong, then the front end does not work. The admin cannot change it because (s)he cannot access.
If the base URL is correct, then it does not need to be changed.

What is the use case that needs the option? (I read the issue ticket but does not understand the use case)

@Frenzie
Copy link
Member

Frenzie commented Sep 14, 2023

If the base URL is wrong, then the front end does not work. The admin cannot change it because (s)he cannot access.
If the base URL is correct, then it does not need to be changed.

This is incorrect. I had a wrong base URL for years before I even noticed, unless I'm thinking of a different similar setting.

@math-GH
Copy link
Contributor

math-GH commented Sep 14, 2023

If the base URL is wrong, then the front end does not work. The admin cannot change it because (s)he cannot access.
If the base URL is correct, then it does not need to be changed.

This is incorrect. I had a wrong base URL for years before I even noticed, unless I'm thinking of a different similar setting.

That leads us back to the documentation: for what is the base URL used/needed?

@Alkarex
Copy link
Member

Alkarex commented Sep 14, 2023

The base URL is used for the generation of various addresses, including relative addresses, some redirections, and for constructing absolute URLs as needed by e.g. API, WebSub

@math-GH
Copy link
Contributor

math-GH commented Sep 14, 2023

Any example for my setup that will work (default: http://localhost/freshrss/p)?

Alkarex added a commit that referenced this pull request Sep 15, 2023
#5656 (comment)
Make read-only while waiting for a better approach (which can wait till release 1.23)
@Alkarex
Copy link
Member

Alkarex commented Sep 15, 2023

@math-GH An example of place where you can see it in action is in one of the RSS outputs. Check the <atom:link href="".../>

@Frenzie
Copy link
Member

Frenzie commented Sep 15, 2023

I believe in my case it wasn't wrong per se but the IP address instead of the domain which led to some subtle problem with something API-related iirc, like the inability to log on when entering the domain in a client.

@marien-probesys
Copy link

What about guessing the recommended base_url with JavaScript instead? window.location.href would return a reliable URL, just having to strip the query part.

@Alkarex
Copy link
Member

Alkarex commented Sep 16, 2023

What about guessing the recommended base_url with JavaScript instead? window.location.href would return a reliable URL, just having to strip the query part.

Indeed Marien, as a tip to autofill the information in the configuration page. But that cannot be used the rest of the time (e.g. from API, WebSub, HTTP redirects) as the request is not passing through a JavaScript client environment.

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.

[BUG] Instance update base_url
5 participants