dotnet / aspnetcore

ASP.NET Core is a cross-platform .NET framework for building modern cloud-based web applications on Windows, Mac, or Linux.
https://asp.net
MIT License
35.36k stars 9.99k forks source link

Please make EditForm always return its EditContext #12238

Closed PeterHimschoot closed 4 years ago

PeterHimschoot commented 5 years ago

Describe the bug

When the EditForm component binds through the Model property, accessing the EditContext property returns null. Why not return the _fixedEditContext?

To Reproduce

I have a component with this EditForm:

<EditForm @ref="editForm"
          OnValidSubmit="@Submit"
          Model="@Customer">

In its @code I have this:

private EditForm editForm;

private bool isInvalid = false;

protected  override void OnInit()
{
    if( editForm?.EditContext == null)
  {
    Console.WriteLine("??? EditContext is null???");
  } else
  {
    isInvalid = editForm.EditContext.Validate();
  }

Expected behavior

I would expect that EditContext has a value, even though not set myself.

Additional context

This is on Blazor Preview 6

mkArtakMSFT commented 5 years ago

Thanks for contacting us, @PeterHimschoot. Feel free to send us a PR for this and we'll happily consider it.

yasz commented 5 years ago

Please enhance it

neville-nazerane commented 4 years ago

Is this about initializing the property EditForm to _fixedEditContext? Or will it make sense to ensure EditForm was never assigned?

I can't tell you why someone will, but what if someone assigns EditForm to null? Will it make sense for EditForm to return null after that point?

Blackleones commented 4 years ago

Are there any updates about this bug? Is it going to be fixed soon or we need to find a workaround?

uabarahona commented 4 years ago

I would like to take a look at this if nobody worked or is working on this yet

uabarahona commented 4 years ago

So the fix looks very easy but I would like to discuss more about it, basically we have:

We change the EditContext property to something like this

[Parameter] public EditContext EditContext 
{ 
    get{ return _customEditContext ??  _fixedEditContext; }  
    set { _customEditContext = value; } 
}

but we now loose the ability to know whether the EditContext was set explicitly or not, if that is a scenario we would have then, I think it will be better to have a new property FixedEditContext

Let me know your thoughts on this

mwinkler commented 4 years ago

I ran in this issue today, when I try to validate the from without a submit button. Please fix this.

nunomaia commented 4 years ago

I have created a new Property in my project to return the current EditContext. This allows me to force form validation without using a submit button. I unfortunately had to duplicate EditForm class in my project to overcome this.

public EditContext CurrentEditContext => _fixedEditContext

_fixedEditContext is already updated on OnParametersSet. .


// Update _fixedEditContext if we don't have one yet, or if they are supplying a
// potentially new EditContext, or if they are supplying a different Model

if (_fixedEditContext == null || EditContext != null || Model != _fixedEditContext.Model)
{
    _fixedEditContext = EditContext ?? new EditContext(Model);
}
chrissainty commented 4 years ago

@SteveSandersonMS Hi Steve, I would like to have a go at this one as part of the July sprint if that's ok?

In terms of the solution I've had a read through the comments and had a look through the code. The two proposed options so far are to make EditContext non-null and just return _fixedEditContext. However, this will break the functionality in OnParmaetersSet. The other option would be to add a new parameter to return _fixedEditContext.

As a third option, I thought we could add an additional field which would look something like:

private EditContext? _providedEditContext;

Then update the current EditContext parameter to look like this:

[Parameter]
public EditContext? EditContext
{
    get => _fixedEditContext;
    set => _providedEditContext = value;
}

The OnParametersSet method could then be updated to use the _providedEditContext field:

protected override void OnParametersSet()
{
    if ((_providedEditContext == null) == (Model == null))
    {
        throw new InvalidOperationException($"{nameof(EditForm)} requires a {nameof(Model)} " +
            $"parameter, or an {nameof(EditContext)} parameter, but not both.");
    }

    if (OnSubmit.HasDelegate && (OnValidSubmit.HasDelegate || OnInvalidSubmit.HasDelegate))
    {
        throw new InvalidOperationException($"When supplying an {nameof(OnSubmit)} parameter to " +
            $"{nameof(EditForm)}, do not also supply {nameof(OnValidSubmit)} or {nameof(OnInvalidSubmit)}.");
    }

    if (_fixedEditContext == null || _providedEditContext != null || Model != _fixedEditContext.Model)
    {
        _fixedEditContext = _providedEditContext ?? new EditContext(Model!);
    }
}

This would seem to give us everything we're after without breaking existing behavour in OnParametersSet, with the cost being an extra field. The user can always retrieve the current EditContext, even when it's created via the Model parameter. But we don't break the check for developers providing both EditContext and Model.

What do you think?

SteveSandersonMS commented 4 years ago

This sounds great, @chrissainty! Your design looks exactly optimal to me. Do you feel able to proceed with implementing this? Let me know if you think there are other outstanding questions.

chrissainty commented 4 years ago

Yep, already done :)

Are there any tests needed for this? I couldn't find any but I might have just missed them. I'll raise the PR but can add tests if needed.

SteveSandersonMS commented 4 years ago

Historically EditForm has only been tested via E2E tests since it was originally so trivial there wasn't really anything else to cover.

However it's now aggregating functionality so would justify adding a new unit test class in src\Components\Web\test\Forms. I wouldn't expect you to retrofit unit tests for other behaviors, but something covering the new ones would be good. And then we have a place to cover more stuff in the future too.

captainsafia commented 4 years ago

This is closed and will ship in RC1.