Parity between Controller and Page/PageModel #6155
Conversation
| /// A <see cref="Task{TResult}"/> which, when completed, asynchronously returns a | ||
| /// <see cref="CompositeValueProvider"/>. | ||
| /// </returns> | ||
| public static async Task<CompositeValueProvider> CreateAsync(ActionContext actionContext) |
There was a problem hiding this comment.
The purpose of resource filters is that they get to change the set of VPF's that apply to the action
| return await CreateAsync(actionContext, factories); | ||
| } | ||
|
|
||
| private static async Task<CompositeValueProvider> CreateAsync( |
There was a problem hiding this comment.
I think it's fine to make this public
| throw new ArgumentNullException(nameof(actionContext)); | ||
| } | ||
|
|
||
| var mvcOptions = actionContext.HttpContext.RequestServices.GetRequiredService<IOptions<MvcOptions>>(); |
There was a problem hiding this comment.
Any time you use GetRequiredService you have to pay @davidfowl a dollar.
If you see yourself using it, then stop and think about it. It's usually wrong.
| var parameter = handler.Parameters[i]; | ||
| arguments[i] = await page.Binder.BindModelAsync( | ||
| arguments[i] = await binder.BindModelAsync( |
There was a problem hiding this comment.
Is all of this change just because we're removing Page.Binder?
There was a problem hiding this comment.
To this file, basically. The only other solution I can come up with is passing the binder into CreateExecutor from PageActionInvokerProvider. Considering PageActionInvokerProvider already has a ton of private readonly's I guess that's cleaner as it contains the DI to one spot.
There was a problem hiding this comment.
Yeah or we can just move the binding up a level.
So the invoker calls model binding, and then calls the executor. The executor knows nothing about binding. This is the way it works for controllers. https://github.com/aspnet/Mvc/blob/dev/src/Microsoft.AspNetCore.Mvc.Core/Internal/ControllerActionInvoker.cs#L980
We should see if we can use ParameterBinder here.
It might be beneficial at this point to separate the Binder cleanup from the rest of this PR.
ryanbrandenburg
force-pushed
the
rybrande/PagePageModelAPISurface
branch
from
f815979 to
abc2705
Compare
|
🆙📅 The prep-work for removing |
| /// <summary> | ||
| /// Gets the dynamic view bag. | ||
| /// </summary> | ||
| public dynamic ViewBag |
There was a problem hiding this comment.
Let's remove this for now. It makes us all sad including @DamianEdwards
🔥 🏑
ryanbrandenburg
force-pushed
the
rybrande/PagePageModelAPISurface
branch
from
135dbaf to
c9bfd22
Compare
Resolves part of #6071.