avoid overwriting params with falsey value

[namusyaka]

This behavior arises from the commit that mustermann was introduced.
see: 7a9ffcc

That breaks backward compatibility with Sinatra v1.4.X and is not intended behavior.
This changes it so that it overwrites params only if there is a possible value as a block argument.

Fixes #1417

I want a cautious review on this PR. After sufficient review, I'm planning to merge the commit.
/cc @rkh @zzak @nickpelone

[namusyaka]

To fix broken tests, applied #1419. Force pushed, but changes are nothing.

[nickpelone]

👍 thanks again!

[namusyaka]

@nickpelone Thanks for confirming my patch. I'll let @zzak and @rkh review this. Please wait for merging.

[rkh]

The change looks good for what it wants to do.

But I don't think we want this change, it might be opening up for security issue or at least bugs where someone is assuming a param can only be set via pattern (ie, imagine someone limiting what :id can capture and then relying on that as enough security):

set :pattern, capture: { ext: %w[home imprint] }

get '/:page?' do
  page = params[:page] || 'home'
  path = File.expand_path(page, page_directory)
  send_file path
end

[nickpelone]

I've never used the feature you describe above - I sanitize my params (wherever they originated in the stack: optional captures, request bodies, etc) with an external gem.

Without getting too in the weeds here, my approach worked in Sinatra 1.4, but is broken in 2.x, where one of the stated desires was for people to file a ticket if their app stopped working. (and for what it's worth, I imagine the situation described above would happen in Sinatra 1.4.x too, re: assuming captures could only come from the route matcher).

I could even live with an option I had to toggle on, but for this behavior to go totally away, forever, would be very depressing to me. (In addition to necessitating a revision of my API I design / deciding to backport to the entire Sinatra 1.x / Rack 1.x ecosystem and live in the past...eww!)

I have strong interest in seeing this functionality restored - so if another approach is needed I could dive into the code and propose one, failing all else.

Thanks.

[namusyaka]

Since received @rkh's comment, I have reconsidered this issue.
As a result, I thought this behavior was certainly undesirable.
This is because even if there are duplicate parameters in path and request body (or query), they should be handled separately.
For example, unlike what is included in the request body, parameters included in path do validation by URI at the previous stage, the request body is not validated.

What I'd like to say is that it is not important that validation is carried out, but the two elements are different in nature.
In other words, the original behavior is wrong, I think.

However, we should look to the fact that this errant behavior was working with traditional sinatra.
For example, we can explicitly write in the document that this behavior has been fixed.

/cc @nickpelone @rkh

In any case, I decided to close this PR as an unwanted change.
Last, @nickpelone Thank you so much for bringing this up.

[nickpelone]

☹️

[nickpelone]

Upon further reflection, I figure I can make this "lax" style I was using work with an extra helper method in those route blocks so I guess I'll stop being so grumpy. Thanks for taking a look.