Pass strict/verbose constructor args to HTML::Form #256

simbabque · 2018-10-11T16:04:30Z

Closes #255

oalders · 2018-10-11T19:28:24Z

Thanks so much for this. Should we add a test that demonstrates that strict_forms can be disabled on a case by case basis when calling submit_form?

oalders · 2018-10-11T19:29:30Z

Also, it would be great to document this new behaviour in the best practices section as well as the SYNOPSIS.

simbabque · 2018-10-11T19:33:47Z

I think I have this actually.

oalders · 2018-10-11T19:47:56Z

I was looking specifically for strict_forms => 0 in a submit_form call in the test, but I don't see it.

simbabque · 2018-10-11T19:55:09Z

Line 228 in the new test file should have been 0, my bad. The name of the text is correct. I'll fix this tomorrow. At the London Perl tech meeting now.

oalders · 2018-10-11T19:55:56Z

Thanks! Enjoy your meeting. :)

…Form

simbabque · 2018-10-12T11:00:24Z

Should we add a test that demonstrates that strict_forms can be disabled on a case by case basis when calling submit_form?

I've updated my commit to fix the test.

Also, it would be great to document this new behaviour in the best practices section as well as the SYNOPSIS.

I've done the best practices, but I am not sure about the SYNOPSIS.

From the amount of questions I've answered about Mech on Stack Overflow I believe a lot of new users start by copying the SYNOPSIS code without really understanding what's going on there. For them, the strict forms option would not really make their code more robust, but just confuse them more because they will likely be confronted with even more errors they don't understand.

On the other hand, it helps to keep these new users on track. I've added it as a separate commit so we can cherry-pick it out if needed.

I do not think we should be talking about verbose mode a lot. Catching these warnings is quite an advanced topic. Having it documented as an option is probably enough for people who know what they are doing.

petdance · 2018-10-15T19:34:45Z

I think it would be good if the docs explained what the strict/verbose flags do when passed through to HTML::Form. It doesn't need to replicate the docs, but a one-sentence summary would be helpful to the reader.

And is verbose not a best practice?

simbabque · 2018-10-15T20:30:33Z

I think it would be good if the docs explained what the strict/verbose flags do when passed through to HTML::Form. It doesn't need to replicate the docs, but a one-sentence summary would be helpful to the reader.

I included that. Please see https://github.com/libwww-perl/WWW-Mechanize/pull/256/files#diff-9b033b946ad0ade000c98612745f570eR248.

And is verbose not a best practice?

I guess it should be, but it's not the default in HTML::Form and I believe we should not just turn on a new feature that will introduce fatal errors where none were there before. That would break a lot of users' code.

petdance · 2018-10-15T20:33:43Z

I included that.

Sorry, my mistake. That looks good.

As to verbose being a best practice, I didn't think that listing it in "best practices" would include making it be the default. We'd just be specifying that it's a good idea, right?

Note that I haven't used either of these features myself, but will be eager to play with them when they go live.

simbabque · 2018-10-15T20:41:51Z

As to verbose being a best practice, I didn't think that listing it in "best practices" would include making it be the default. We'd just be specifying that it's a good idea, right?

Oh, I misunderstood. Sorry about that. However, was already in the best practices section. I've extended it in my commit. Please see https://github.com/libwww-perl/WWW-Mechanize/pull/256/files#diff-9b033b946ad0ade000c98612745f570eR3219.

petdance · 2018-10-15T20:52:17Z

Is there anything you'd like me to do to test this out?

simbabque · 2018-10-15T20:55:47Z

If you're asking me, feel free to play with it. I wrote unit tests, but I didn't actually use it. I don't use Mech a lot lately as my focus at $work has shifted. At the moment I'm doing these tickets for fun and for the sake of doing them. :)

PatrickCronin

Olaf was looking for volunteers to review this PR. I suspect he's trying to draw more attention to this dist. Very clever Olaf! Anyway, here's my two cents.

PatrickCronin · 2018-10-12T18:49:58Z

lib/WWW/Mechanize.pm

+=item * C<< strict_forms => [0|1] >>
+
+Globally sets the HTML::Form strict flag which causes form submission to
+croak if any of the passed fields don't exist on the page, and/or a value


"any of the passed fields don't exist on the page" seems slightly inaccurate. Perhaps something like "all passed fields can't be found in a single form on the page"

I think neither is fully correct. This is not about searching for forms as far as I understand. It's for once it's found a form, that form will be strict, so it will not silently ignore if you try to pass in extra fields. It doesn't matter how it found that form. If you use a method that will find a form based on fields, then it simply won't find it if you ask for superfluous (or maybe misspelled) fields. That behaviour hasn't changed, it was already there.

I believe saying any of the passed fields don't exist on the form makes more sense.

PatrickCronin · 2018-10-12T18:51:46Z

lib/WWW/Mechanize.pm

+
+This behavior can also be turned on globally by passing C<< strict_forms => 1>> to
+C<<WWW::Mechanize->new>>. If you do that, you can still disable it for individual calls
+by passing C<< strict_forms => 0>> here.


PatrickCronin · 2018-10-12T18:52:18Z

lib/WWW/Mechanize.pm

+            password     => 'lost-and-alone',
+            extra_field  => 123,               # field does not exist
+        }
+    );


PatrickCronin · 2018-10-16T13:36:10Z

t/form_with_fields_passthrough_params.t

+    );
+    isa_ok( $form, 'HTML::Form' );
+    is($form->attr('name'), '4th_form_1', 'fourth form matches');
+}


This test seems very similar to the one two blocks above. Does it test different functionality?

I copied all of those from the existing form_with_fields.t, because I wanted to make sure all the tests we already have also pass when strict is on. I did not actually prove those tests to be correct in this PR. I think the idea for these two is that in the 1st_form test, it tests the outcome of calling form_with_fields() with one argument, while the 4th_form_1 test calls it with two arguments. I'd like to keep those in for consistency.

PatrickCronin · 2018-10-16T13:37:37Z

t/form_with_fields_passthrough_params.t

+    isa_ok( $forms[0], 'HTML::Form' );
+    isa_ok( $forms[1], 'HTML::Form' );
+    is($forms[0]->attr('name'), '3rd_form_ambiguous', 'first result of 3rd_form_ambiguous');
+    is($forms[0]->attr('name'), '3rd_form_ambiguous', 'second result of 3rd_form_ambiguous');


I'm not sure how this is different from the previous line.

Good spot. See above please. This seems to have been broken in form_with_fields.t already, but I missed it. I will fix it in both tests.

petdance · 2018-10-16T14:36:29Z

I'd like to suggest/request that when all this is churned over that a new PR gets made with all the changes and feedback. It's difficult to look over multiple commits on something like this.

simbabque · 2018-10-16T14:39:26Z

I'd like to suggest/request that when all this is churned over that a new PR gets made with all the changes and feedback. It's difficult to look over multiple commits on something like this.

I agree. When everyone's happy, I'll squash down the branch and create a single-commit new PR.

petdance · 2018-10-17T19:30:13Z

One other request: I'd like it if we used proper capitalization and punctuation in comments, so that instead of:

# enable strict form processing to catch typos and non-existant form fields

we have

# Enable strict form processing to catch typos and non-existent form fields.

Makes it easier to read. (Also, it's "non-existent" not "non-existant".)

petdance · 2018-10-17T19:35:25Z

Is there a timeframe for making a release of WWW::Mechanize with this PR in it? I'm going to put out a release of Test::WWW::Mechanize soon, and I'd like to mention in it that the underlying WWW::Mechanize has new features in it that may be useful to testers.

oalders · 2018-10-17T19:44:49Z

I can release this on the same day as this gets merged. I'm happy with the current state of things. The code comments could be cleaned up here or in a different merge. I'm not picky about that.

petdance · 2018-10-17T19:55:15Z

I just ran a small subset of our test suite using this Mech branch and the new options enabled and already turned up a couple of problems.

strict_forms caused an error of "Input 'sb_selectedoption' is readonly at /home/alester/...."

verbose_forms threw a warning saying " outside

", which combined with Test::Warnings, made my test fail (good!).

👍 to this level of pickiness. I'll start testing in earnest once it hits the CPAN.

simbabque · 2018-10-18T09:38:34Z

I'd like it if we used proper capitalization and punctuation in comments

I don't want to nitpick, but it seems that actually most of the code base has single sentence comments that start with a capital letter, but do not have a full stop. There are only full stops if the comment has more than one sentence. However, this is in the synopsis. So it's not even a real comment.

I am all for consistency, so I'll change that.

@oalders, do you want me to merge them down and create a new PR or are we OK with a bit of history going in?

oalders · 2018-10-18T18:54:02Z

@simbabque in the interest of getting this done, I'm going to merge what we have here and get a release out there. I think it looks good.

oalders · 2018-10-18T18:54:46Z

Thanks @simbabque, @petdance and @PatrickCronin!

oalders · 2018-10-18T23:25:09Z

New version is now on CPAN.

simbabque force-pushed the pr/gh255 branch from 7e0dff2 to 1e95aa1 Compare October 12, 2018 10:38

simbabque added 3 commits October 12, 2018 11:52

Issue libwww-perl#255: Pass strict/verbose constructor args to HTML::…

fdb5251

…Form

Issue libwww-perl#255: include strict_forms in best practices

2a254b2

Issue libwww-perl#255: include strict_forms in SYNOPSIS

23e22a2

simbabque force-pushed the pr/gh255 branch from 1e95aa1 to 23e22a2 Compare October 12, 2018 10:59

oalders approved these changes Oct 12, 2018

View reviewed changes

oalders requested review from genio and petdance October 12, 2018 22:41

PatrickCronin reviewed Oct 16, 2018

View reviewed changes

Issue libwww-perl#255: improve docs and fix typo in test

d29d910

Issue libwww-perl#255: clean up comments

2fa11f5

oalders merged commit 51872f0 into libwww-perl:master Oct 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass strict/verbose constructor args to HTML::Form #256

Pass strict/verbose constructor args to HTML::Form #256

simbabque commented Oct 11, 2018

oalders commented Oct 11, 2018

oalders commented Oct 11, 2018

simbabque commented Oct 11, 2018 via email •

edited

Loading

oalders commented Oct 11, 2018

simbabque commented Oct 11, 2018 via email •

edited

Loading

oalders commented Oct 11, 2018

simbabque commented Oct 12, 2018

petdance commented Oct 15, 2018

simbabque commented Oct 15, 2018 •

edited

Loading

petdance commented Oct 15, 2018

simbabque commented Oct 15, 2018

petdance commented Oct 15, 2018

simbabque commented Oct 15, 2018

PatrickCronin left a comment

PatrickCronin Oct 12, 2018

simbabque Oct 16, 2018

PatrickCronin Oct 12, 2018

PatrickCronin Oct 12, 2018

PatrickCronin Oct 16, 2018

simbabque Oct 16, 2018

PatrickCronin Oct 16, 2018

simbabque Oct 16, 2018

petdance commented Oct 16, 2018

simbabque commented Oct 16, 2018

petdance commented Oct 17, 2018

petdance commented Oct 17, 2018

oalders commented Oct 17, 2018

petdance commented Oct 17, 2018

simbabque commented Oct 18, 2018

oalders commented Oct 18, 2018

oalders commented Oct 18, 2018

oalders commented Oct 18, 2018

Pass strict/verbose constructor args to HTML::Form #256

Pass strict/verbose constructor args to HTML::Form #256

Conversation

simbabque commented Oct 11, 2018

oalders commented Oct 11, 2018

oalders commented Oct 11, 2018

simbabque commented Oct 11, 2018 via email • edited Loading

oalders commented Oct 11, 2018

simbabque commented Oct 11, 2018 via email • edited Loading

oalders commented Oct 11, 2018

simbabque commented Oct 12, 2018

petdance commented Oct 15, 2018

simbabque commented Oct 15, 2018 • edited Loading

petdance commented Oct 15, 2018

simbabque commented Oct 15, 2018

petdance commented Oct 15, 2018

simbabque commented Oct 15, 2018

PatrickCronin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petdance commented Oct 16, 2018

simbabque commented Oct 16, 2018

petdance commented Oct 17, 2018

petdance commented Oct 17, 2018

oalders commented Oct 17, 2018

petdance commented Oct 17, 2018

simbabque commented Oct 18, 2018

oalders commented Oct 18, 2018

oalders commented Oct 18, 2018

oalders commented Oct 18, 2018

simbabque commented Oct 11, 2018 via email •

edited

Loading

simbabque commented Oct 11, 2018 via email •

edited

Loading

simbabque commented Oct 15, 2018 •

edited

Loading