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

Trim spaces from user given url #1778

Merged
merged 3 commits into from Feb 5, 2018
Merged

Trim spaces from user given url #1778

merged 3 commits into from Feb 5, 2018

Conversation

keltroth
Copy link
Contributor

@keltroth keltroth commented Feb 2, 2018

Forget the other PR, I made this one to avoid new commit on my own dev branch to come to you :)

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.

As I said, lgtm.

@Frenzie
Copy link
Member

Frenzie commented Feb 2, 2018

See #1777 for some (minimal) discussion. As I said in #1777 (comment)

Note that while trim does strip tabs (which you might accidentally get from tables) it doesn't strip non-breaking spaces (which you might also get from websites). I'm not sure if that matters. :-)

@@ -39,12 +39,14 @@ public function firstAction() {
* @throws FreshRSS_Feed_Exception
* @throws Minz_FileNotExistException
*/
public static function addFeed($url, $title = '', $cat_id = 0, $new_cat_name = '', $http_auth = '') {
public static function addFeed($param_url, $title = '', $cat_id = 0, $new_cat_name = '', $http_auth = '') {
Copy link
Member

Choose a reason for hiding this comment

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

Please either keep $url (and later below use $url = trim($url);) or update the @param documentation a few lines higher up. I would better like option 1.

Copy link
Member

Choose a reason for hiding this comment

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

I thought of writing something like that and decided against it without realizing that GitHub was hiding the documentation a few lines up. :-P

I'd also go with option 1.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did option 1 👍

@Alkarex Alkarex added this to the 1.10.0 milestone Feb 5, 2018
@Alkarex
Copy link
Member

Alkarex commented Feb 5, 2018

Thanks @keltroth 👍
Please add your name to https://github.com/FreshRSS/FreshRSS/blob/dev/CREDITS.md

@Alkarex Alkarex modified the milestones: 1.11.0, 1.10.0 Feb 5, 2018
@Alkarex Alkarex merged commit 81df36e into FreshRSS:dev Feb 5, 2018
@keltroth keltroth deleted the trimurl branch February 5, 2018 19:58
Alkarex added a commit that referenced this pull request Feb 8, 2018
@Alkarex Alkarex mentioned this pull request Feb 14, 2018
javerous pushed a commit to javerous/FreshRSS that referenced this pull request Jan 20, 2020
Trim spaces from user given url
javerous pushed a commit to javerous/FreshRSS that referenced this pull request Jan 20, 2020
mdemoss pushed a commit to mdemoss/FreshRSS that referenced this pull request Mar 25, 2021
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.

None yet

3 participants