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.

Sign up for GitHub

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

Jump to bottom

Link generation behaviour change: page to controller in area #4212

Closed
JamesNK opened this issue Nov 21, 2018 · 4 comments
Closed

Link generation behaviour change: page to controller in area #4212

JamesNK opened this issue Nov 21, 2018 · 4 comments
Assignees
@rynowak
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates Done This issue has been fixed
Milestone
3.0.0-preview3

Comments

@JamesNK
Copy link
Member

JamesNK commented Nov 21, 2018

Linking from a page in an area to a controller in the same area, without specifying the area is changing.

  • Succeeded in 2.1
  • Succeeded in 2.2 (unintentionally)
  • Fails in 3.0 (current behaviour)

We could make 3.0 succeed with a moderate amount of additional work. The question is whether anyone is doing this and if it is important to customers.

This issue is something to evaluate near the end of 3.0 to make a decision.

Affected test: PagesInAreas_CanGenerateLinksToControllersAndPages

<a asp-controller="Home" asp-action="Index">Link to area action</a>

PR that is changing this - aspnet/Routing#914
@rynowak
Copy link
Member

rynowak commented Nov 21, 2018 •
edited
Loading

I worked with James on tracking this down. We expected this behavior to change in 2.2 but due to a bug it didn't. I asked James to open an issue so that we could use it to consider the issue and try to figure out whether users will be impacted by this. It's possible for us to restore the 2.2 behavior if necessary but will require significant work.

@natemcmaster natemcmaster transferred this issue from aspnet/Routing Nov 22, 2018
@natemcmaster natemcmaster added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Nov 22, 2018
@natemcmaster natemcmaster added this to the 3.0.0-preview1 milestone Nov 22, 2018
@rynowak rynowak added the breaking-change This issue / pr will introduce a breaking change, when resolved / merged. label Nov 22, 2018
@JamesNK JamesNK mentioned this issue Nov 28, 2018
Merged
@davidbirchwork davidbirchwork mentioned this issue Dec 5, 2018
Closed
@mkArtakMSFT
Copy link
Member

@JamesNK, what else is pending here? We're trying to wrap up preview2 issues within couple of days.

@mkArtakMSFT
Copy link
Member

If this is not super critical, and work is pending, please move to preview3

@JamesNK JamesNK assigned rynowak and unassigned JamesNK Feb 7, 2019
rynowak added a commit that referenced this issue Feb 10, 2019
@rynowak
Fix #4212 area ambient value page->controller
5381bcc 
This change enhances our ambient value logic to also deal with required
values. In 2.2 we introduced a 'required values' semantic to allow route
values to appear "to the left" of a route pattern for the purpose of
ambient values copying. This is a complicated way of saying "when you
like to a different endpoint then discard the ambient values".

What we didn't consider is that some ambient values are special (like
area). So basically, we'll allow an ambient value to be used if it's
part of the required values - even if we've already decided to discard
the ambient values.

This is a pretty surgical fix and only affected the desired scenario
based on tests.

-----

I also removed an optimization that I think is broken. I put an earlier
optimization in place that attempted to count ambient values as they
were "seen" to try and avoid some extra copying. This copying loop has a
cost even if it no-ops which is what I was trying to prevent.

Unfortunately since we added 'required values' - it's now possible for
an ambient value to be double-counted, which makes this optimization
incorrect.
rynowak added a commit that referenced this issue Feb 11, 2019
@rynowak
Fix #4212 area ambient value page->controller
5e10f25 
This change enhances our ambient value logic to also deal with required
values. In 2.2 we introduced a 'required values' semantic to allow route
values to appear "to the left" of a route pattern for the purpose of
ambient values copying. This is a complicated way of saying "when you
like to a different endpoint then discard the ambient values".

What we didn't consider is that some ambient values are special (like
area). So basically, we'll allow an ambient value to be used if it's
part of the required values - even if we've already decided to discard
the ambient values.

This is a pretty surgical fix and only affected the desired scenario
based on tests.

-----

I also removed an optimization that I think is broken. I put an earlier
optimization in place that attempted to count ambient values as they
were "seen" to try and avoid some extra copying. This copying loop has a
cost even if it no-ops which is what I was trying to prevent.

Unfortunately since we added 'required values' - it's now possible for
an ambient value to be double-counted, which makes this optimization
incorrect.
@rynowak
Copy link
Member

rynowak commented Feb 11, 2019

This change in behaviour was fixed. There is no breaking change to speak of here.

@rynowak rynowak added Done This issue has been fixed and removed 2 - Working breaking-change This issue / pr will introduce a breaking change, when resolved / merged. labels Feb 11, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@rynowak rynowak

Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates Done This issue has been fixed
Projects
None yet
Milestone
3.0.0-preview3
Development

No branches or pull requests
4 participants
@JamesNK @rynowak @natemcmaster @mkArtakMSFT