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
#7978 - fix for user agent quotes issue #8050
#7978 - fix for user agent quotes issue #8050
Conversation
Simply replacing all quotes is imho not the best way. A quote is still a valid character for useragents and thus might occur in one. So we should not make it impossible to exclude useragents including a quote. To better handle enclosing quotes maybe it might be better to use |
@sgiehl I studied "user agent" strings thoroughly before starting writing the fix. To my knowledge, no user agent contains single or double quotes. So, this might be a good solution. I suggest you guys to checkout the site http://www.useragentstring.com/pages/useragentstring.php which lists numerous user agent strings. As per Matt's suggestion, we do not user comma separated (user agent) strings for now. So, "str_getcsv" doesn't help in this case. |
I'm the lead developer of device detector. Believe me or not, there are useragents containing quotes some examples:
The current implementation uses comma separated values, and your changes doesn't change that behaviour as |
Okay, thanks for sharing such user agents which are quite new to me. We On Wed, Jun 24, 2015 at 3:59 PM, Stefan Giehl notifications@github.com
|
3bb26c9
to
7b57f0f
Compare
@mnapoli @mattab I have updated the quotes removal with trim() function as per suggestion and tested with single & more pair of double quotes in the end, works great. Please review and comment if we need any improvements here. Sorry for the extended delay, I had been quite busy in religious activities during Ramadan fasting period. |
Isn't the goal of this PR the same as of #8368 |
Ah I started working on #7978 today, we have actually many problems on top of the original issue:
I started to work on a solution where all similar options work with one item per line (replacing the "comma-separated" formatting). Enclosing quotes are accepted and trimmed. The goal is to have consistency between all the different form fields, between the fields and their helps and between site-specific settings and global settings. Also it would allow any character in user agent strings (both quotes and commas). |
Before this commit, users can configure excluded user agents by separating them with commas. Enclosing quotes are not trimmed. With this commit, users can configure excluded user agents one per line. Enclosing quotes are trimmed. This is meant to be simpler and more consistent. It also allows to exclude user agents containing both commas and quotes. This change applies both to global settings as well as site-specific settings. This commit also changes the help text, which now explicitly says that user agents should be separated by new lines. A migration script is included: it will replace commas by new lines. Related: #7978, #8050
FYI I've started working on a new fix in #8482 |
…turns Before this commit, users can configure excluded query parameters by separating them with commas. With this commit, users can configure them one per line. This is meant to be simpler and more consistent (see #8448). This is now also consistent with form inline helps. This change applies both to global settings as well as site-specific settings. A migration script is included: it will replace commas by new lines. Related: #7978, #8050
Before this commit, users can configure excluded IPs by separating them with commas. With this commit, users can configure them one per line. This is meant to be simpler and more consistent (see #8448). This is now also consistent with form inline helps. This change applies both to global settings as well as site-specific settings. A migration script is included: it will replace commas by new lines. Related: #7978, #8050
Really appreciate your work done here but have to close in favour of #8368 . |
actually because of #8482 |
Fix for issue #7978.