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

Add missing elements to allow MVC5.2 site to run #1163

Merged
merged 1 commit into from Nov 2, 2014
Merged

Add missing elements to allow MVC5.2 site to run #1163

merged 1 commit into from Nov 2, 2014

Conversation

AerisG222
Copy link
Contributor

In combination with PR874, this allowed my site to work after trying to upgrade to the latest MVC version

@monoadmin
Copy link

Hello! I'm the build bot for the Mono project. I need approval from a Mono team member to build this pull request. A team member should reply with "approve" to approve a build of this pull request, "whitelist" to whitelist this and all future pull requests from this contributor, or "build" to explicitly request a build, even if one has already been done. Contributors can ignore this message.

@d0apga
Copy link

d0apga commented Aug 16, 2014

in master branch, HttpContextWrapper.GetService still not implement?

@monoadmin
Copy link

Hello! I'm the new build bot for the Mono project. I need approval from a Mono team member to build this pull request. A team member should reply with "approve" to approve a build of this pull request, "whitelist" to whitelist this and all future pull requests from this contributor, or "build" to explicitly request a build, even if one has already been done. Contributors can ignore this message.

@marxin
Copy link

marxin commented Sep 23, 2014

Hello.

I've just tried to apply the patch and my MVC+NHibernate application works correctly. I was using a different branch from matthid's fork. I would like to see the pull request in trunk, is there any blocker?

Thanks.
Martin

@knocte
Copy link
Contributor

knocte commented Sep 26, 2014

@AerisG222 can you squash your commits into just one please?

@AerisG222
Copy link
Contributor Author

Happy to try, am not so proficient with git. By any chance do you have a
reference you can point me at to help squash easily?

Thanks,
Mike

@AerisG222 https://github.com/AerisG222 can you squash your commits into
just one please?


Reply to this email directly or view it on GitHub
#1163 (comment).

@Therzok
Copy link
Contributor

Therzok commented Sep 26, 2014

Hey @AerisG222 !

Once you're on the more_mvc_fixes branch, you can do the following:

git rebase -i HEAD~6

While rebasing, on the first commit you made, to change the commit message you can write reword instead of pick. You can mark the others with fixup and they will be merged into the first.

@Therzok
Copy link
Contributor

Therzok commented Sep 26, 2014

See more here.

@AerisG222
Copy link
Contributor Author

Thanks for the pointers, that was very helpful!

@knocte
Copy link
Contributor

knocte commented Sep 27, 2014

Mike, thanks for squashing them. Unfortunately I've just realised there are still nitpicks to fix regarding style, I'll point them now:

public override object GetService (Type serviceType)
{
throw new NotImplementedException ();
return ((IServiceProvider)w).GetService(serviceType);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Space between GetService and (.

@Therzok
Copy link
Contributor

Therzok commented Sep 28, 2014

Looks good on my end.

@akoeplinger
Copy link
Member

The first commit by @chrisfcarroll is actually part of PR #874 and should be excluded here I think. I wonder why the other PR wasn't merged yet, it looks good from my POV.

@martinjt
Copy link
Contributor

@knocte is there any hold up on getting this merged now that the changes you've suggested have been made?

@knocte
Copy link
Contributor

knocte commented Oct 12, 2014

I guess not, but I'm no maintainer. You need to bother @grendello I guess.

@martinjt
Copy link
Contributor

@grendello what does one need to bribe you with to get a final review and merge on this? I have a nice single malt here?

private readonly string _minRequiredPasswordLengthError = "{0} must have at least {1} characters";
private readonly string _minNonAlphanumericCharactersError = "{0} must have at least {1} special characters";
private readonly string _passwordStrengthError = "{0} is weak";

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't need private here.

@martinjt
Copy link
Contributor

@AerisG222 I'm going to try some unit tests for this, are you going to be able to review and accept them as a pull?

@AerisG222
Copy link
Contributor Author

I'm happy to review and will try to pull when there is something available.
though might look for help to make sure everything is pulled and wrapped
up nicely, as I've struggled with that before ;)

In all honesty, I've tried to move on to reworking my site to vnext as that
should be a much better platform to build from given the cross platform
focus. Granted, this is a ways away, but even with the patch we've been
commenting on, my site still hits an error on the webapi side. I
understand for others they won't have the luxury of upgrading, but
depending on your needs/interests, time might be better spent helping to
ensure that project has a quality experience on mono...

On Thu, Oct 16, 2014 at 6:51 PM, martinjt notifications@github.com wrote:

@AerisG222 https://github.com/AerisG222 I'm going to try some unit
tests for this, are you going to be able to review and accept them as a
pull?


Reply to this email directly or view it on GitHub
#1163 (comment).

@martinjt
Copy link
Contributor

I'm having the same issue with WebAPI, I think I've got a handle on all the things that need implementing. I've submitted a pull around encryption, I think there are 2 more to come, on is the HttpTaskAsyncHandler, but I can't remember what the other one is. After that, 5.2 (WebAPI and MVC) should "work", it's then a bug fix exercise.

Could you fix the "private" issue I commented on? and squash like you did with the last one? Also, any help with writing unit tests for this would be helpful.

@AerisG222
Copy link
Contributor Author

Yea, let me try to find some time this weekend to fix the private issue you
identified, and will re-squash. I'll try to have a look at some unit tests
but timing is difficult...

On Thu, Oct 16, 2014 at 7:09 PM, martinjt notifications@github.com wrote:

I'm having the same issue with WebAPI, I think I've got a handle on all
the things that need implementing. I've submitted a pull around encryption,
I think there are 2 more to come, on is the HttpTaskAsyncHandler, but I
can't remember what the other one is. After that, 5.2 (WebAPI and MVC)
should "work", it's then a bug fix exercise.

Could you fix the "private" issue I commented on? and squash like you did
with the last one? Also, any help with writing unit tests for this would be
helpful.


Reply to this email directly or view it on GitHub
#1163 (comment).

@chrisfcarroll
Copy link

You were waiting for #874 ? I did the (hopefully) final correction and rebased.

@martinjt
Copy link
Contributor

This all looks fine to me now. We just need someone to review and merge.

I am planning on putting a full list of the Pulls needed for the aspnetwebstack features to work, then we can try and get someone to review & merge them all at once.

If we can't get them merged, I'm going to create a fork so we can collate a work aspnetwebstack piece.

@AerisG222
Copy link
Contributor Author

I've cleaned up the 'private' issue, and squashed.

@kumpera
Copy link
Contributor

kumpera commented Oct 18, 2014

@grendello what do you think? can we merge this one?

@migueldeicaza
Copy link
Contributor

Hey guys,

This code contains some obvious code that was lifted from .NET (the MembershipValidator).

@AerisG222
Copy link
Contributor Author

Apologies again for this, I have removed the file that I did not author.

@@ -7,6 +7,7 @@ System.ComponentModel.DataAnnotations/EmailAddressAttribute.cs
System.ComponentModel.DataAnnotations/PhoneAttribute.cs
System.ComponentModel.DataAnnotations/FileExtensionsAttribute.cs
System.ComponentModel.DataAnnotations/CompareAttribute.cs
System.ComponentModel.DataAnnotations/UrlAttribute.cs
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isn't required, it's defined in the base file

@migueldeicaza
Copy link
Contributor

Thanks for removing the third-party code and squashing the commits.

migueldeicaza added a commit that referenced this pull request Nov 2, 2014
Add missing elements to allow MVC5.2 site to run
@migueldeicaza migueldeicaza merged commit e4d8f21 into mono:master Nov 2, 2014
@AerisG222 AerisG222 deleted the more_mvc_fixes branch November 2, 2014 00:45
@liuxm6
Copy link

liuxm6 commented Feb 12, 2017

mvc 5.2.3 on mono 4.8.0 error
The view found at '~/Views/Home/Index.cshtml' was not created.

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
Add missing elements to allow MVC5.2 site to run

Commit migrated from mono/mono@e4d8f21
AlexKnauth pushed a commit to AlexKnauth/mono that referenced this pull request Nov 2, 2023
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

Successfully merging this pull request may close these issues.

None yet