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

Deduplicate and extract user/password extraction from alternative HTTP headers logic. #9735

Closed
wants to merge 2 commits into from

Conversation

bantu
Copy link

@bantu bantu commented Jul 19, 2014

The previous solution was not good because it is hard to see that there is no difference between the two blocks. This is now clear because we are iterating over a list of variable names. The variable name for the list can be kept short because the logic was also extracted into its own function.

Refs #9310

@ghost
Copy link

ghost commented Jul 19, 2014

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/6323/

@bantu bantu changed the title Deduplicate and extract "user/password extraction from alternative HTTP headers" logic. Deduplicate and extract user/password extraction from alternative HTTP headers logic. Jul 19, 2014
@bantu
Copy link
Author

bantu commented Jul 19, 2014

@owncloud-bot Retest this please.

@scrutinizer-notifier
Copy link

The inspection completed: 6 new issues, 2 updated code elements

@ghost
Copy link

ghost commented Jul 19, 2014

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/6324/

@LukasReschke
Copy link
Member

👍 - Great.

We can drop the strip_tags here. I'll create a follow-up PR based on this.

LukasReschke added a commit that referenced this pull request Jul 19, 2014
This `strip_tags` seems to be completely unneeded and will cause problems with passwords containing stripped characters. (e.g. `<` or `>`)

Needs #9735 to be merged first.
@bantu
Copy link
Author

bantu commented Jul 20, 2014

@PVince81 @DeepDiver1975 @icewind1991 @nickvergessen Please review

@PVince81
Copy link
Contributor

I suggest to close this in favor of #9738 where it's used ?

@DeepDiver1975
Copy link
Member

I suggest to close this in favor of #9738 where it's used ?

yes

@DeepDiver1975 DeepDiver1975 deleted the dry/base-http-header branch July 21, 2014 08:53
LukasReschke added a commit that referenced this pull request Jul 22, 2014
This `strip_tags` seems to be completely unneeded and will cause problems with passwords containing stripped characters. (e.g. `<` or `>`)

Needs #9735 to be merged first.
LukasReschke added a commit that referenced this pull request Jul 22, 2014
This `strip_tags` seems to be completely unneeded and will cause problems with passwords containing stripped characters. (e.g. `<` or `>`)

Needs #9735 to be merged first.
@lock lock bot locked as resolved and limited conversation to collaborators Aug 17, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants