aspnet / AspNetWebStack

ASP.NET MVC 5.x, Web API 2.x, and Web Pages 3.x (not ASP.NET Core)
Other
858 stars 354 forks source link

GetSelectListWithDefaultValue Modifies IEnumerable<SelectListItem> selectList #271

Closed Gabby-Paolucci closed 4 years ago

Gabby-Paolucci commented 4 years ago

The GetSelectListWithDefaultValue method modifies the selectList given as a parameter. A copy of the list is made, but selectList is modified (specifically the Selected properties) as part of the process and since the object is passed by reference, the original object is modified. This is confusing behavior.

src/System.Web.Mvc/Html/SelectExtensions.cs

    private static IEnumerable<SelectListItem> GetSelectListWithDefaultValue(IEnumerable<SelectListItem> selectList, object defaultValue, bool allowMultiple)
    {
        ...
        List<SelectListItem> newSelectList = new List<SelectListItem>();

        foreach (SelectListItem item in selectList)
        {
            item.Selected = (item.Value != null) ? selectedValues.Contains(item.Value) : selectedValues.Contains(item.Text);
            newSelectList.Add(item);
        }
        return newSelectList;
    }

Here's an example of where this can cause confusion: @Html.DropDownListFor(model => model.Field1, Model.SelectListItems) @Html.DropDownListFor(model => model.Field2, Model.SelectListItems)

If Field1 has a value set but Field2 is null, Field1 will still render with the value of Field1 set since Model.SelectListItems will have had Selected set to true for that SelectListItem.

If anything, the documentation should be updated to note this effect. Resolving this bug could be a breaking change—I imagine there is existing code that depends on this behavior.

Gabby-Paolucci commented 4 years ago

I'll also note, this was submitted as an issue for the project back in 2014 when it was hosted in CodePlex: ASP.NET MVC / Web API / Web Pages - View Issue #1913: DropdownListFor modifies the SelectListItem collection passed to it (Wayback Machine Archive)

It was determined that the developers were "we are concerned that some users might have taken dependency on the behavior (although this is probably rare)."

And there was a post about this in 2013 in the ASP.NET Forums: DropDownListFor modifying passed SelectListItems | The ASP.NET Forums

mkArtakMSFT commented 4 years ago

Thanks for contacting us. @Rick-Anderson can this be clarified in the ASP.NET Web Stack docs?

mkArtakMSFT commented 4 years ago

This issue was moved to dotnet/AspNetCore.Docs#18841