libwww-perl / WWW-Mechanize

Handy web browsing in a Perl object
https://metacpan.org/pod/WWW::Mechanize
Other
68 stars 52 forks source link

Document that :content_file names containing "?" need to have it URL encoded: "%3F". #290

Closed jidanni closed 2 years ago

jidanni commented 4 years ago

Mention local (file:) names containing "?" need to have it URL encoded: "%3F".

coveralls commented 4 years ago

Pull Request Test Coverage Report for Build 298


Files with Coverage Reduction New Missed Lines %
lib/WWW/Mechanize.pm 23 92.82%
<!-- Total: 23 -->
Totals Coverage Status
Change from base Build 294: 0.0%
Covered Lines: 729
Relevant Lines: 782

💛 - Coveralls
oalders commented 4 years ago

Can you demonstrate with the problem is here?

use WWW::Mechanize;
my $ua = WWW::Mechanize->new;

$ua->get( 'http://example.com', ':content_file' => 'foo?bar' );

creates a local file for me.

jidanni commented 4 years ago

:content_file names? I'm not talking about :content_file names!

Here we see when we add "file:", we also need to URL encode the "?", else the file is no longer found. I'll use mech-dump here for the example.

$ touch /tmp/x?y\&z=w
$ mech-dump /tmp/x?y\&z=w
file:///tmp/x%3Fy&z=w returns type "text/plain", not "text/html"
$ mech-dump file:/tmp/x?y\&z=w
file:/tmp/x?y&z=w returns type "", not "text/html" (#292)
$ mech-dump file:/tmp/x%3Fy\&z=w
file:/tmp/x%3Fy&z=w returns type "text/plain", not "text/html" (And no way to change it, #63)
simbabque commented 4 years ago

I'm trying to understand what the exact issue is. I think you're saying that when there is a file with a question mark ? in the name in the local file system, and we're trying to download that file, something goes wrong and the file isn't found. That could be down to your shell expanding the ? as a single character wildcard and not finding anything (although it should find the ? anyway, shouldn't it?).

Therefore telling the user to escape this does make sense, but we need to keep in mind that content_file is an LWP::UserAgent thing that changes the behaviour of Mechanize, and your case is an edge case.

I like the idea of building up this knowledge in the documentation, but I wonder if it is not better placed in an FAQ style part of the documentation. We don't have one right now, but we could make one.

jidanni commented 4 years ago

I don't know anything about "content_file". All I am saying is if a person has a local file that has a "?" in its name, then there is no way for him to access it with WWW::Mechanize, unless someone tells him that he needs to use %3F to represent the "?". And there is little chance of him guessing that on his own. Therefore it should be mentioned on the man page.

jidanni commented 4 years ago

...

creates a local file for me.

I am not talking about creating files. I am talking about reading existing files from disk.

jidanni commented 4 years ago

(P.S., apparently there are some "diffs" that you are trying to show me. However whatever diffs you made are very hard for me to see here on GitHub.)

oalders commented 4 years ago

I don't see any diffs in this ticket.

jidanni commented 4 years ago

OK. But I see "Pull Request Test Coverage" and "Some checks were not successful" which I don't know what I need to look at.

jidanni commented 4 years ago

OK somehow I made a patch instead of a normal issue or something.

simbabque commented 4 years ago

You made a Pull Request with a documentation change. This triggered a build in the connected TravisCI system, which failed for some reason (I haven't looked yet), and the finished build in turn triggered Coveralls, which does a test coverage report. I don't understand why the report shows a loss of coverage if all you changed was POD. I will investigate this now.

But please note that there are two different people (@oalders and me, @simbabque) talking to you, as well as the @coveralls bot leaving a comment. It looks to me like you're responding to all comments like there is a single person on the other end.

I think @oalders has pointed out :content_file (and maybe misunderstood your point) because the change you suggested with your PR is directly adjacent to the documentation with a note on :content_file, with not even an empty line between it. And it's also talking about files, so it's reasonable to get confused about the intend of the extra text. I understand you're talking about something else, and so will Olaf now I am sure, as we've clarified.

We would like to improve Mechanize's documentation, and you make a good point. Because of the confusion I've addressed above I just think we should add this to a different place in the docs. Let's discuss where we can put it instead so people who need it find it, but it does not confuse others.