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

Multiple select with [] in name can't be crawled #10514

Closed
dubcanada opened this issue Oct 7, 2015 · 15 comments
Closed

Multiple select with [] in name can't be crawled #10514

dubcanada opened this issue Oct 7, 2015 · 15 comments

Comments

@dubcanada
Copy link

Hello,

If you have a select, let's say it looks like

<select name="test[]" multiple>
    <optgroup value="Testing">
        <option value="1">Testing 1</option>
        <option value="2">Testing 2</option>
    </optgroup>
</select>

And if you try to use

$this->select([1,2], 'test[]');

It won't work, while it works in the Laravel side as soon as it goes to dom-crawler and enters the get function in FormFieldRegistry https://github.com/symfony/dom-crawler/blob/6bf50c092a5fcfb700e53addabb82ebd77625b27/FormFieldRegistry.php#L82 it breaks. The form field registry stores selects, regardless if they have a [] or not without it. So in this case if I changed it to

$this->select([1,2], 'test');

It would work on the dom-crawler, but it won't work for the Laravel CrawlerTrait. Basically if you have

$this->select([1,2], 'test[]');

In your test, and then add something like

if ($name == 'test[]') $name = 'test';

On https://github.com/symfony/dom-crawler/blob/6bf50c092a5fcfb700e53addabb82ebd77625b27/FormFieldRegistry.php#L84

It works.

So before the field names are sent to dom-crawler they need to have [] stripped.

dom-crawler version - v2.7.5
laravel foundation version - v5.1.19

@sebice
Copy link

sebice commented Oct 9, 2015

Are there any steps done in dom-crawler to address this issue?

@GrahamCampbell
Copy link
Member

This is the expected behaviour. [] is reserved in Laravel.

@arthurkirkosa
Copy link
Contributor

@GrahamCampbell how would you go about testing an input/select that sends and array to the request?

@GrahamCampbell
Copy link
Member

Rename it to drop the [], then you're good.

@dubcanada
Copy link
Author

What do you mean rename it to drop the [] ? I have

$this->actingAs($user)
         ->visit('stuff')
         ->select('3', 'location[]')

How do I rename it to drop the [] on Symfony dom-crawler but keep the [] on Laravel?

If I change it to location it doesn't work.

@GrahamCampbell

@ghost
Copy link

ghost commented Feb 23, 2016

I am still on L5.1.. however, update of https://github.com/laravel/framework/blob/5.2/src/Illuminate/Foundation/Testing/Concerns/InteractsWithPages.php#L845

with addition of:
$element = str_replace("[]", "", $element);

Would resolve the problem.

@iamthewit
Copy link

In case anyone else finds this and is struggling to write an integration test for multiselect, I managed to get it to work by doing what @spikerok said.

Taking the method @spikerok linked to : https://github.com/laravel/framework/blob/5.2/src/Illuminate/Foundation/Testing/Concerns/InteractsWithPages.php#L845

And adding in the line @spikerok said to add in:
$element = str_replace("[]", "", $element);

Gives you this:

protected function storeInput($element, $text)
    {
        $this->assertFilterProducesResults($element);
        $element = str_replace('#', '', $element);
        $element = str_replace("[]", "", $element);
        $this->inputs[$element] = $text;
        return $this;
    }

Simply copy and paste that into your test case class and you should be able to run your tests just fine.

Just incase you were wondering, my test looks like this:

$this->actingAs($adminUser)
             ->visit('/admin/project/create')
             ->type('James', 'name')
             ->select([1, 2], 'users[]')

@johnberberich
Copy link

johnberberich commented Apr 21, 2016

@GrahamCampbell I don't think you understand why simply removing the "[]" from the name of your multiselect is a problem. It gets around the testing issue brought up here, but breaks how Laravel PHP handles array form inputs in the $_POST superglobal. In short, by appending "[]" to your form input name (typically a select field with multiple selections enabled), Laravel $request->input('items') hands you an array of the option values when the form is submitted; if you remove "[]", you only get one option value back, thus breaking multiselect form input retrieval and validation.

@iamthewit Is there a better solution to this problem? Manually editing core framework files breaks upgradability and is generally a bad idea.

@johnberberich
Copy link

johnberberich commented Apr 22, 2016

For the time being, I've worked around this issue by submitting the form and its inputs together with InteractsWithPages@submitForm($buttonText, $inputs = [], $uploads = []). (You do this instead of calling type($text, $element), select($option, $element), press($buttonText), etc.)

Here's an example:

$this->submitForm('Submit', [
    'name' => 'Test name',
    'categories'  => [51, 52]
]);

@iamthewit
Copy link

@johnberberich I didn't edit any core framework code, I simply added the storeInput method (as per my comment above) to my test case class. This way it overrides the frameworks storeInput method.

@johnberberich
Copy link

I'm sorry, @iamthewit, I didn't read your comment thoroughly enough. I like that your solution lets you use the normal methods for filling a form, so I prefer it to mine. Thank you very much for clarifying.

@pelletiermaxime
Copy link

This is still a big problem with a quick and easy fix. I don't understand why this was closed.

@leewillis77
Copy link
Contributor

I'd agree that this should be re-opened - at the minute multi-value selects can't be tested with the Laravel framework.

Attempting to fill in a form with a multi-select element, e.g.

<select multiple="" name="hills[]">
<!-- options here -->
</select>

With a test that includes:
$this->select($hill->id, 'hills[]')

Fails, with the rather cryptic:
InvalidArgumentException: Unreachable field ""

Reproduced on latest Laravel.

@themsaid
Copy link
Member

themsaid commented Sep 8, 2016

@pelletiermaxime if you have a fix please feel free to open a Pull Request.

@pelletiermaxime
Copy link

The one liner fix a few comments above worked fine for me, but my test suite is really small, so not sure if there's any chance it breaks something. I will look at making a PR in the next few days if nobody else did.

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

No branches or pull requests

9 participants