Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[Browserkit][Form][Tests] Add submitWithAdditionalValues() method in Symfony\Component\BrowserK…#18330

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

@alexislefebvre
Copy link
Contributor

QA
Branchmaster
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed tickets#3824,#4124
LicenseMIT
Doc PRsymfony/symfony-docs#6403

This PR adds asubmitWithAdditionalValues() method which you can use in order to submit a form with additional data during tests.

For example, when you have aCollectionType with the'allow_add' => true, option, you can't change the values with$form[FIELD_NAME][…] = 'foo', it triggers an error.

With thesubmitWithAdditionalValues() method you can add some fields to the request:

$client->submitWithAdditionalValues($crawler->filter('input')->form(),array(),// New values:array('foo[0]' =>'bar'),    );

Wherearray('foo[0]' => 'bar'), is an array which values won't be checked (Symfony won't check if the field exist).

sstok and HeahDude reacted with thumbs up emoji
@alexislefebvrealexislefebvreforce-pushed the3824-test-form-submit-additional-values branch 2 times, most recently from4c62292 to38e5d55CompareMarch 27, 2016 14:31
@alexislefebvrealexislefebvre changed the titleAdd submitWithAdditionalValues() method in Symfony\Component\BrowserK…[Tests] Add submitWithAdditionalValues() method in Symfony\Component\BrowserK…Mar 27, 2016
@alexislefebvre
Copy link
ContributorAuthor

I don't know why tests are failing, on AppVeyor the error is not related to my changes and on Travis CI I don't know what isdeps=low.

@alexislefebvrealexislefebvreforce-pushed the3824-test-form-submit-additional-values branch from38e5d55 to582a494CompareMarch 27, 2016 14:42
@HeahDude
Copy link
Contributor

Failures are unrelated :)

@HeahDude
Copy link
Contributor

Actually travis fails because#18275 has not been reverted in 3.0 and master yet.

alexislefebvre reacted with thumbs up emoji

@alexislefebvrealexislefebvre changed the title[Tests] Add submitWithAdditionalValues() method in Symfony\Component\BrowserK…[Form][Tests] Add submitWithAdditionalValues() method in Symfony\Component\BrowserK…Mar 27, 2016
@fabpot
Copy link
Member

It has been reverted in 3.0

@fabpot
Copy link
Member

and in master as well now.

@alexislefebvrealexislefebvreforce-pushed the3824-test-form-submit-additional-values branch from582a494 to36542ccCompareMarch 27, 2016 18:08
@alexislefebvre
Copy link
ContributorAuthor

I have force-pushed the commit to launch CI tests again.

@alexislefebvrealexislefebvreforce-pushed the3824-test-form-submit-additional-values branch 2 times, most recently fromad02c38 to31a542fCompareMarch 28, 2016 10:04
@javiereguiluz
Copy link
Member

@alexislefebvre thanks for sending this proposal. I'm not against it ... but adding a new method always "complicates" things (people must learn a new method, Symfony developers need to maintain that method forever, etc.)

I'd like to ask some Symfony Form experts, like@HeahDude, if they think this is absolutely necessary or if the problem to solve (which I don't understand) can be solved in a simpler way. Thanks!

@HeahDude
Copy link
Contributor

@javiereguiluz Please don't say I'm an expert :) "involved" seems more appropriate.

For me it's a 👍 (I've already add one it a few days ago on the description).

@javiereguiluz
Copy link
Member

@HeahDude could you please explain the problem to solve to people like me, not involved in the Form component?@alexislefebvre said:"I want to submit additional values in my tests but Symfony doesn't allow me to do that". Why does he want to submit additional values? Why doesn't Symfony allow to do that? Why should be "force" Symfony yo allow something it doesn't allow? Thanks.

@HeahDude
Copy link
Contributor

@javiereguiluz because a form view in a template (like used in a tested page with a crawler) might contain no fields.

Think of aCollectionType with an empty collection. Then it would require to add some fields with javascript to populate the form and submit.

Currently theDomCrawler/BrowserKit components do not allow to submit extra fields (which are not present in the view because they cannot be generated dynamically).

This PR fixes it by allowing to submit extra fields.

alexislefebvre reacted with thumbs up emoji

@javiereguiluz
Copy link
Member

@HeahDude thanks for the explanation. I think the "right" solution would be to allow the crawler to add nodes dynamically and then submit the form ... but my guess is that they want to maintain the DomCrawler as "read-only", so we can't add anything. Therefore, the proposed solution seems the way to go. Thanks!

HeahDude reacted with thumbs up emoji

@HeahDude
Copy link
Contributor

Maybe this one could have aBrowserKit label instead ofForm.

$this->assertEquals('http://www.example.com/foo',$client->getRequest()->getUri(),'->submit() submit forms');

// The field "foo[0]" have been converted to an array "foo => 0".
$this->assertEquals(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Maybe this test would be more readable assigning variables:

$client->setNextResponse(newResponse('<html><form action="/foo"><input type="submit" /></form></html>'));$crawler =$client->request('GET','http://www.example.com/foo/foobar');$additionalValues =array('foo[0]' =>'bar');$expectedParameters =array('foo' =>array('0' =>'bar'));$client->submitWithAdditionalValues($crawler->filter('input')->form(),array(),$additionalValues);$this->assertEquals('http://www.example.com/foo',$client->getRequest()->getUri(),'->submit() submit forms');$this->assertEquals($expectedParameters,$client->getRequest()->getParameters(),'parameters have not been added');

alexislefebvre reacted with thumbs up emoji
@javiereguiluzjaviereguiluz changed the title[Form][Tests] Add submitWithAdditionalValues() method in Symfony\Component\BrowserK…[Browserkit][Form][Tests] Add submitWithAdditionalValues() method in Symfony\Component\BrowserK…Mar 29, 2016
@alexislefebvre
Copy link
ContributorAuthor

Thanks@HeahDude for your explanation to@javiereguiluz. :)

Upon further reflection, one drawback of thissubmitWithAdditionalValues() method is that ithides what is happening, it can be used to add data, but not to remove data (e.g. remove an existing field from a collection) which is almost as important as adding data.

If you're not OK to add a new method for a not so common case, I suggest another approach:

  1. reduce this PR: implement theconvertFieldsToArray() method I added inSymfony/Component/DomCrawler/Form.php and remove the other changes from this PR. It would be useful because it allows to use a syntax close to the other tests (foo[bar] instead of a PHP array)
  2. document how to add or remove data during tests: examples in this PRhttps://github.com/symfony/symfony-docs/pull/6403/files or onStack Overflow
HeahDude reacted with heart emoji

@HeahDude
Copy link
Contributor

ping@jakzal :)

@alexislefebvrealexislefebvreforce-pushed the3824-test-form-submit-additional-values branch from31a542f tod18e89bCompareMarch 31, 2016 11:54
@alexislefebvrealexislefebvreforce-pushed the3824-test-form-submit-additional-values branch fromd18e89b to6cf66b1CompareApril 1, 2016 21:17
@alexislefebvre
Copy link
ContributorAuthor

Can someone please give some feedback about mylast comment? Thanks.

return$this->request($form->getMethod(),$form->getUri(),$form->getPhpValues(),$form->getPhpFiles());
}

/**
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

with additional values ?

alexislefebvre reacted with thumbs up emoji
@alexislefebvrealexislefebvreforce-pushed the3824-test-form-submit-additional-values branch fromc39e500 toea5d7deCompareApril 2, 2016 15:25
@HeahDude
Copy link
Contributor

Looks good :)

alexislefebvre reacted with thumbs up emoji

@HeahDude
Copy link
Contributor

Since the method is inForm wouldn't it make sense to call this methodsubmitWithAdditionnalFields ?

@HeahDude
Copy link
Contributor

orsubmitWithExtraFields ?

@HeahDude
Copy link
Contributor

Tests are failing because you need to update theDomCrawler dependency of theBrowerKit component to~3.1

…it\ClientExtract code that converts fields to arrays in a new method convertFieldsToArray()
@alexislefebvrealexislefebvreforce-pushed the3824-test-form-submit-additional-values branch fromea5d7de to3823aebCompareApril 2, 2016 15:49
@alexislefebvre
Copy link
ContributorAuthor

After further reflection, I don't like thissubmitWithAdditionalValues() method, I will remove it in order to have a simpler PR. Do you think it's better to close this PR and open a new one (while referencing this PR) instead of altering this PR?

@alexislefebvre
Copy link
ContributorAuthor

I'll work on the documentation instead, these changes are not necessary to test a collection of fields.

@alexislefebvrealexislefebvre deleted the 3824-test-form-submit-additional-values branchApril 2, 2016 18:30
@alexislefebvre
Copy link
ContributorAuthor

Thanks to@HeahDude and the others for your help and sorry for the inconvenience!

I opened aPR for the documentation, I hope that it will be more useful to document how to test a form collection for all the Symfony versions instead of adding a method that will be usable only with the last version of Symfony.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@alexislefebvre@HeahDude@fabpot@javiereguiluz@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp