Skip to content

v10: Fix build warnings in Web.Common#12349

Merged
nikolajlauridsen merged 14 commits intov10/devfrom
v10/bugfix/fix-build-warnings-Web.Common
May 9, 2022
Merged

v10: Fix build warnings in Web.Common#12349
nikolajlauridsen merged 14 commits intov10/devfrom
v10/bugfix/fix-build-warnings-Web.Common

Conversation

@Zeegaan
Copy link
Copy Markdown
Member

@Zeegaan Zeegaan commented May 4, 2022

Notes

  • Removed lots of warnings again, i think the interesting stuff is all the redundant "?" checks, cause even though the compiler will give a warning for some of them, its not all that are actually redudant (see the fix test commit, some where failing cause i had removed the ? checks)

  • Like in V10: Build warnings in Web.Website #12332 please be aware of the [Obsolete] attrubute, dotnet format adds these when using obsolete methods -.-

How to test

  • Be on v10/dev
  • Open your .globalconfig file
  • On line 44-49 change the severity now to warning instead of suggestion
  • Build the project and note the number of build warnings
  • Switch branch to this one and build again
  • Build warnings should be reduced 🙏

Copy link
Copy Markdown
Contributor

@nikolajlauridsen nikolajlauridsen left a comment

Choose a reason for hiding this comment

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

Code overall looks good to me 👍, only found a couple of minor things :D Also din't find any new obsolete attributes, so looks like you caught em all :)

I haven't tested this yet though

Zeegaan and others added 5 commits May 6, 2022 08:26
Co-authored-by: Mole <nikolajlauridsen@protonmail.ch>
…ttribute.cs

Co-authored-by: Mole <nikolajlauridsen@protonmail.ch>
…b.Common' into v10/bugfix/fix-build-warnings-Web.Common
# Conflicts:
#	src/Umbraco.Web.Common/Hosting/UmbracoHostBuilderDecorator.cs
@nikolajlauridsen nikolajlauridsen merged commit c576bbe into v10/dev May 9, 2022
@nikolajlauridsen nikolajlauridsen deleted the v10/bugfix/fix-build-warnings-Web.Common branch May 9, 2022 07:39
@bjarnef
Copy link
Copy Markdown
Contributor

bjarnef commented May 16, 2022

It seems after merging these changing into v10/contrib branch I now get an compile error here: https://github.com/umbraco/Umbraco-CMS/blob/v10/contrib/src/Umbraco.Web.Common/Views/UmbracoViewPage.cs#L42

image

In .globalconfig it has this on line 43-49:

dotnet_analyzer_diagnostic.category-StyleCop.CSharp.DocumentationRules.severity = suggestion
dotnet_analyzer_diagnostic.category-StyleCop.CSharp.ReadabilityRules.severity = suggestion
dotnet_analyzer_diagnostic.category-StyleCop.CSharp.NamingRules.severity = suggestion
dotnet_analyzer_diagnostic.category-StyleCop.CSharp.SpacingRules.severity = suggestion
dotnet_analyzer_diagnostic.category-StyleCop.CSharp.OrderingRules.severity = suggestion
dotnet_analyzer_diagnostic.category-StyleCop.CSharp.MaintainabilityRules.severity = suggestion
dotnet_analyzer_diagnostic.category-StyleCop.CSharp.LayoutRules.severity = suggestion

If I change line https://github.com/umbraco/Umbraco-CMS/pull/12349/files#diff-f9c020492385e8ed7a6066d6edafd07b849779a3c533eeb24995235b93f6329bR42 the following instead the code compiles as ViewData.Model returns TModel?:

TModel? model = ViewData.Model;

// cc @bergmania

@bjarnef bjarnef mentioned this pull request May 16, 2022
1 task
@Zeegaan
Copy link
Copy Markdown
Member Author

Zeegaan commented May 17, 2022

Hey @bjarnef I'm not seeing a compile error on either v10/dev or v10/contrib

image

You've linked to a line, but the link doesn't seem to work for me, just takes me to the changes :(

@bjarnef
Copy link
Copy Markdown
Contributor

bjarnef commented May 17, 2022

@Zeegaan the link should work, but you may need to click "Load diff" at UmbracoViewPage to see the linked line:

image

Strange that it is your screenshot says TModel isn't nullable.

When I inspect ViewData.Model I get this:

image

https://docs.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.mvc.viewfeatures.viewdatadictionary.model?view=aspnetcore-6.0#microsoft-aspnetcore-mvc-viewfeatures-viewdatadictionary-model

@Zeegaan
Copy link
Copy Markdown
Member Author

Zeegaan commented May 17, 2022

@bjarnef Really strange indeed, I've just switched to my laptop to test aswell, seems like its also not nullable there 🤔
When i inspect in Rider i get this:
image

Whats ur dotnet version? 🤔 Mine is 6.0.202
image

@bjarnef
Copy link
Copy Markdown
Contributor

bjarnef commented May 17, 2022

Currently I have .NET v6.0.101 installed :)

@Zeegaan
Copy link
Copy Markdown
Member Author

Zeegaan commented May 17, 2022

@bjarnef Seems like that is the issue!

I've switched to 6.0.101 and now i get the same
image

@bjarnef
Copy link
Copy Markdown
Contributor

bjarnef commented May 17, 2022

@Zeegaan strange if it has changed in a patch update of .NET6 🤔

@Zeegaan
Copy link
Copy Markdown
Member Author

Zeegaan commented May 17, 2022

@bjarnef I'd personally consider wrong nullability a bug, so i don't see why that cannot change in a patch 🤔

Heres the PR that changed it to non-nullable in 6.0.2
https://github.com/dotnet/aspnetcore/pull/39416/files

@bjarnef
Copy link
Copy Markdown
Contributor

bjarnef commented May 17, 2022

@Zeegaan okay, maybe it should be mentioned in the contributing guide that it now requires .NET 6.0.2+ 😁
https://github.com/umbraco/Umbraco-CMS/blob/v10/contrib/.github/CONTRIBUTING.md

// cc @nul800sebastiaan @bergmania

@nul800sebastiaan
Copy link
Copy Markdown
Member

Well that is terribly annoying! Fixed: https://github.com/umbraco/Umbraco-CMS/blob/v10/contrib/.github/BUILD.md#debugging-with-vs-code

But also needs to be fixed in the docs for minimum requirements.

bergmania added a commit that referenced this pull request May 30, 2022
nikolajlauridsen pushed a commit that referenced this pull request Jun 2, 2022
* Fixed issues with basic auth middleware to support Umbraco Cloud usecase

* Fix redirects to return url, now allows website urls

* Strip potential domain part of returnPath

* Fix default value in appsettings schema

* Reintroduce check of basic auth enabled.

* Fix wrong negation introduced in #12349

* Fixed issues with redirects

* Also check external login cookie, while authenticating backoffice
nikolajlauridsen pushed a commit that referenced this pull request Jun 2, 2022
* Fixed issues with basic auth middleware to support Umbraco Cloud usecase

* Fix redirects to return url, now allows website urls

* Strip potential domain part of returnPath

* Fix default value in appsettings schema

* Reintroduce check of basic auth enabled.

* Fix wrong negation introduced in #12349

* Fixed issues with redirects

* Also check external login cookie, while authenticating backoffice
if (!(content is null))
{
_helper.AssignedContentItem = content;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Zeegaan couldn't this part be simplified to:

TModel model = ViewData.Model;
var content = model as IPublishedContent;
if (content is null && model is IContentModel contentModel)
{
    content = contentModel.Content;
}

_helper = Context.RequestServices.GetRequiredService<UmbracoHelper>();

content ??= UmbracoContext?.PublishedRequest?.PublishedContent;

if (content is not null)
{
    _helper.AssignedContentItem = content;
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hey @bjarnef you are totally right that could be simplified, could i trouble you to make a PR with the changes? I will gladly review it 😄

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@Zeegaan here you go 😁 #12528

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.

4 participants