Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

submitSymfonyForm(): Mentioning name attribute #128

Merged
merged 3 commits into from Apr 6, 2021

Conversation

ThomasLandauer
Copy link
Member

@ThomasLandauer ThomasLandauer commented Apr 4, 2021

I'm not sure about this!
But I think the submitted fieldname now is myFormaddress, instead of myForm[address] - that's what I'm trying to fix.
I didn't take a look at the tests you mentioned in #54

BTW: I was first looking for this method in FormAssertionsTrait.php, is there a reason why you put it into BrowserAssertionsTrait.php?

I'm not sure about this!
But I think the submitted fieldname now is `myFormaddress`, instead of `myForm[address]` - that's what I'm trying to fix.
I didn't take a look at the tests you mentioned in Codeception#54
@TavoNiievez
Copy link
Member

TavoNiievez commented Apr 5, 2021

As shown in the code example,

$I->submitSymfonyForm('login_form', [
     '[email]'    => 'john_doe@example.com',
 	 '[password]' => 'secretForest'
]);

You need to add brackets to the key of the field that is passed as argument (eg. '[first_name]'). The reason behind this is that there are more 'complex' forms that might contain compound fields, for example:

[password][first] => 'mysupersecretpass'
[password][second] => 'mysupersecretpass'

and at the time I thought it would be better to leave the bracket syntax as it is ... Or at least, it was better than inventing a special syntax just to send compound fields to this function.

BTW: I was first looking for this method in FormAssertionsTrait.php, is there a reason why you put it into BrowserAssertionsTrait.php?

yes, the functions that are in BrowserAssertionTrait don't really use any framework-specific Symfony class. Neither of forms nor of anything else.

What is the same, they could be integrated in Innerbrowser and be available in all modules.
I did not send a PR to Innerbrowser for two reasons:

  • I think that the Innerbrowser module, despite working perfectly well, needs a little extra work to update its code base and include new PHP features, as well as a refactor in general since it is difficult in my opinion to navigate through that file.
    But that module affects all modules at the same time and any change of that style can be difficult to merge. (Too many people affected, too few reviewing).

  • The methods of that trait were ideas that I came up with to make some tests much easier to write and read, but knowing if they are being used as expected or there are difficulties (or redundancies) when using it is a feedback that I can only get later that it merges and that some people use that function in open source projects (and I can review them). For example, that drove the changes I made with seePageIsAvailable.

The idea of ​​doing this is to be very sure that both the API and the documentation are well designed and written, that it is understood and that they are used by people when writing their tests.

So my hope is that in some future those functions can be copied to InnerBrowser and removed from this module.

@TavoNiievez
Copy link
Member

btw, don't forget to tag me when you ask questions directed at me, I am not the only maintainer of this module :-)

@ThomasLandauer ThomasLandauer changed the title Fixing submitSymfonyForm() submitSymfonyForm(): Mentioning name attribute Apr 6, 2021
@ThomasLandauer
Copy link
Member Author

Sorry, I overlooked the brackets ;-)
You can merge this now.

@TavoNiievez TavoNiievez merged commit 259ee0e into Codeception:master Apr 6, 2021
@TavoNiievez
Copy link
Member

Thank you.

@ThomasLandauer ThomasLandauer deleted the patch-4 branch April 6, 2021 14:58
@TavoNiievez TavoNiievez added this to the 2.0.2 milestone Apr 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants