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

Update $removeparam syntax, add $queryprune as an alias #1384

Closed
ameshkov opened this issue Nov 26, 2020 · 7 comments
Closed

Update $removeparam syntax, add $queryprune as an alias #1384

ameshkov opened this issue Nov 26, 2020 · 7 comments

Comments

@ameshkov
Copy link
Member

Check the changes implemented in uBO's queryprune:
uBlockOrigin/uBlock-issues#760 (comment)

Tbh, I like the changes, especially the idea with inverted logic, and since $removeparam is currently only used in simple scenarios (removing a single parameter), we can afford to safely change its syntax to fully match $queryprune, and basically make them both interchangeable.

@sxgunchenko what do you think?

@sxgunchenko
Copy link

Hm... I agree with you that there are some good ideas in uBO's $queryprune rules behaviour, which would be nice to implement for $removeparam rules, like the inverted logic or the pattern derivation. But for me $removeparam rules' syntax is somewhat clearer. So, I would rather pick some ideas from the $queryprune rules instead of becoming fully compatible. Unless that is exactly our goal, of course.

@ameshkov
Copy link
Member Author

ameshkov commented Nov 27, 2020

Well, there're three options here.

  1. We could keep a slightly different syntax of $removeparam, and then implement the conversion logic to support $queryprune as well.
  2. The other alternative would be to have the same syntax and make $queryprune an alias to $removeparam.
  3. And finally, the third option, make $removeparam support both types of syntax and infer at runtime which one it is.

Which one do you prefer?

@sxgunchenko
Copy link

If it's acceptable not to be fully backward compatible with uBO, I'd prefer the first option.

@ameshkov
Copy link
Member Author

I am okay with either of these options so no prob.

@gorhill
Copy link

gorhill commented Nov 29, 2020

I am trying to be more compatible with removeparam=, ideally there would be no difference. I've done some work toward this and here are my thoughts.

I would prefer dropping the syntax allowing to enumerate more than one parameter name, i.e. removeparam=a|b|c. The reason is that it's very difficult to optimize, the same way a filter such as /a|b|c/ is difficult to optimize (can't tokenize, has to be executed against all non-blocked network requests, etc.). Ultimately, if a filter author really want to specify a list of parameter names, they can always use the regex form, i.e. removeparam=/^(a|b|c)=/, but at least the basic form won't encourage filters with poor performance.

I will support removeparam=a and removeparam=/.../ forms, and drop the special anchor characters |. With more thought, I think we should replace the negation character ! with the usual ~.

I understand the removeparam=a form means "remove query parameter a" from the URL. For the regex-based form though, i.e removeparam=/.../, I think it should means "remove query parameters normalized to a name=value string which match the regex /.../", so this would allow to remove query parameters by matching the value instead of the name.

Forgot to add that I added support for the special form removeparam=* which means remove all query parameters.

gorhill added a commit to gorhill/uBlock that referenced this issue Nov 29, 2020
Related discussions:
- uBlockOrigin/uBlock-issues#1356 (comment)
- AdguardTeam/CoreLibs#1384

Changes:

Negation character is `~` (instead of `!`).

Drop special anchor character `|` -- leading `|`
will be supported until no such filter is present
in uBO's own filter lists. For example, instance
of `queryprune=|ad` will have to be replaced with
`queryprune=/^ad/` (or `queryprune=ad` if the name
of the parameter to remove is exactly `ad`).

Align semantic with that of AdGuard's `removeparam=`,
except that specifying multiple `|`-separated names
is not supported.
@ameshkov ameshkov changed the title $removeparam improvements Update $removeparam syntax, add $queryprune as an alias Nov 30, 2020
@ameshkov
Copy link
Member Author

@sxgunchenko please check the updated spec here:
uBlockOrigin/uBlock-issues#1356 (comment)

@sxgunchenko
Copy link

core/pull-requests/2233

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants