Issue 29801609: Issue 6733 - Allow empty values in filter options (Closed)
|
Created:
June 7, 2018, 11:51 a.m. by Manish Jethani Modified:
July 12, 2018, 11:11 a.m. CC:
sergei Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionIssue 6733 - Allow empty values in filter options
Patch Set 1 #
Total comments: 4
Patch Set 2 : Rebase and simplify #
Total comments: 6
MessagesTotal messages: 8
Patch Set 1
https://codereview.adblockplus.org/29801609/diff/29801610/lib/filterClasses.js File lib/filterClasses.js (left): https://codereview.adblockplus.org/29801609/diff/29801610/lib/filterClasses.j... lib/filterClasses.js:592: if (sitekeys != null) How come you remove this check here, but add it for some of the options later? https://codereview.adblockplus.org/29801609/diff/29801610/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29801609/diff/29801610/lib/filterClasses.j... lib/filterClasses.js:790: else if (option == "REWRITE" && value != null) Come to think of it, why don't we just allow the $rewrite option to be used without the =? Then we don't have to mess with the regular expression or other logic? I figure we could do something like this: else if (option == "REWRITE") rewrite = value || "";
Patch Set 2: Rebase and simplify https://codereview.adblockplus.org/29801609/diff/29801610/lib/filterClasses.js File lib/filterClasses.js (left): https://codereview.adblockplus.org/29801609/diff/29801610/lib/filterClasses.j... lib/filterClasses.js:592: if (sitekeys != null) On 2018/06/07 13:21:56, kzar wrote: > How come you remove this check here, but add it for some of the options later? There's an explanation, but no longer relevant. https://codereview.adblockplus.org/29801609/diff/29801610/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29801609/diff/29801610/lib/filterClasses.j... lib/filterClasses.js:790: else if (option == "REWRITE" && value != null) On 2018/06/07 13:21:56, kzar wrote: > Come to think of it, why don't we just allow the $rewrite option to be used > without the =? Then we don't have to mess with the regular expression or other > logic? I figure we could do something like this: > > else if (option == "REWRITE") > rewrite = value || ""; We could do that but it would be weird to allow $rewrite without a value but still disallow blank values. What do you think? Anyway, this version of the patch just allows `$rewrite=` but not just `$rewrite`. https://codereview.adblockplus.org/29801609/diff/29815555/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29801609/diff/29815555/lib/filterClasses.j... lib/filterClasses.js:804: if (value == null) Blank value is allowed for $rewrite here while still an error for $domain, $sitekeys, and $csp. https://codereview.adblockplus.org/29801609/diff/29815555/test/filterClasses.js File test/filterClasses.js (right): https://codereview.adblockplus.org/29801609/diff/29815555/test/filterClasses.... test/filterClasses.js:486: test.equal(Filter.normalize("$csp= "), This is important now because we want to return InvalidFilter on blank value for $csp.
https://codereview.adblockplus.org/29801609/diff/29815555/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29801609/diff/29815555/lib/filterClasses.j... lib/filterClasses.js:804: if (value == null) On 2018/06/25 13:13:44, Manish Jethani wrote: > Blank value is allowed for $rewrite here while still an error for $domain, > $sitekeys, and $csp. So by checking for `value == null` instead of `!value` we'd allow "...$rewrite=" but not "rewrite$", or do I misunderstand? Either way, why don't you check for `!value` here?
https://codereview.adblockplus.org/29801609/diff/29815555/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29801609/diff/29815555/lib/filterClasses.j... lib/filterClasses.js:804: if (value == null) On 2018/07/12 10:36:13, kzar wrote: > On 2018/06/25 13:13:44, Manish Jethani wrote: > > Blank value is allowed for $rewrite here while still an error for $domain, > > $sitekeys, and $csp. > > So by checking for `value == null` instead of `!value` we'd allow "...$rewrite=" > but not "rewrite$", or do I misunderstand? Either way, why don't you check for > `!value` here? Yes, we're allowing $rewrite= but not $rewrite, because this is not a "flag" like $match-case, it needs a value but it can take an empty value.
LGTM https://codereview.adblockplus.org/29801609/diff/29815555/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29801609/diff/29815555/lib/filterClasses.j... lib/filterClasses.js:804: if (value == null) On 2018/07/12 10:52:55, Manish Jethani wrote: > On 2018/07/12 10:36:13, kzar wrote: > > On 2018/06/25 13:13:44, Manish Jethani wrote: > > > Blank value is allowed for $rewrite here while still an error for $domain, > > > $sitekeys, and $csp. > > > > So by checking for `value == null` instead of `!value` we'd allow > "...$rewrite=" > > but not "rewrite$", or do I misunderstand? Either way, why don't you check for > > `!value` here? > > Yes, we're allowing $rewrite= but not $rewrite, because this is not a "flag" > like $match-case, it needs a value but it can take an empty value. Ah right I see, we anticipate the confusion about adding the "=" for the users who do want an empty value is less than the confusion of users who don't understand the option at all and attempt to use it without a value.
On 2018/07/12 11:04:20, kzar wrote: > LGTM Thanks! https://codereview.adblockplus.org/29801609/diff/29815555/lib/filterClasses.js File lib/filterClasses.js (right): https://codereview.adblockplus.org/29801609/diff/29815555/lib/filterClasses.j... lib/filterClasses.js:804: if (value == null) On 2018/07/12 11:04:20, kzar wrote: > On 2018/07/12 10:52:55, Manish Jethani wrote: > > On 2018/07/12 10:36:13, kzar wrote: > > > On 2018/06/25 13:13:44, Manish Jethani wrote: > > > > Blank value is allowed for $rewrite here while still an error for $domain, > > > > $sitekeys, and $csp. > > > > > > So by checking for `value == null` instead of `!value` we'd allow > > "...$rewrite=" > > > but not "rewrite$", or do I misunderstand? Either way, why don't you check > for > > > `!value` here? > > > > Yes, we're allowing $rewrite= but not $rewrite, because this is not a "flag" > > like $match-case, it needs a value but it can take an empty value. > > Ah right I see, we anticipate the confusion about adding the "=" for the users > who do want an empty value is less than the confusion of users who don't > understand the option at all and attempt to use it without a value. Yup. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
