Umbraco-CMS icon indicating copy to clipboard operation
Umbraco-CMS copied to clipboard

Multiple warnings in source code

Open emmagarland opened this issue 2 years ago • 13 comments

Which Umbraco version are you using? (Please write the exact version, example: 10.1.0)

~~12.3.0+ (but applies to any version)~~ v14 (latest)

Bug summary

There are thousands of warnings within the Umbraco source code. I was hoping to try and start to reduce these as use this issue as a base to work from (unless there already is one?).

My idea is to go through each project and check that there are no warnings, and if there are, remove them.

This is until perhaps we can set warnings as errors to keep it clean for future? Bigger step/decision! I have started an initial PR for this at #15019.

Specifics

I recommend ticking off each project if there are no warnings, or if all warnings have since been removed. To maintain this, we would also set warnings as errors (something for HQ to consider)

Steps to reproduce

  1. Open IDE of choice
  2. Build
  3. Observe warnings

Expected result / actual result

Minimal to no warnings ultimately

### Tasks
- [x] Remove warnings from Templates/Umbraco.Templates project
- [ ] Remove warnings from  Tests/Umbraco.TestData project
- [ ] Remove warnings from Tests/Umbraco.Tests.AcceptanceTest project
- [ ] Remove warnings from Tests/Umbraco.Tests.Benchmarks project
- [ ] Remove warnings from Tests/Umbraco.Tests.Common project
- [ ] Remove warnings from Tests/Umbraco.Tests.Integration project
- [ ] Remove warnings from  Tests/Umbraco.Tests.UnitTests project
- [ ] Remove warnings from Tools/Umbraco.JsonSchema project
- [x] Remove warnings from Umbraco.Cms project
- [x] Remove warnings from Umbraco.Cms.Api.Common project
- [ ] Remove warnings from Umbraco.Cms.Api.Delivery project
- [x] Remove warnings from Umbraco.Cms.Imaging.ImageSharp project
- [x] Remove warnings from Umbraco.Cms.Imaging.ImageSharp2 project
- [ ] Remove warnings from Umbraco.Cms.Persistence.EFCore project
- [x] Remove warnings from Umbraco.Cms.Persistence.EFCore.Sqlite project
- [x] Remove warnings from Umbraco.Cms.Persistence.EFCore.SqlServer project
- [ ] Remove warnings from Umbraco.Cms.Persistence.Sqlite project
- [ ] Remove warnings from Umbraco.Cms.Persistence.SqlServer project
- [x] Remove warnings from Umbraco.Cms.StaticAssets project
- [x] Remove warnings from Umbraco.Cms.Targets project
- [ ] Remove warnings from Umbraco.Core project
- [ ] Remove warnings from  Umbraco.Examine.Lucene project
- [ ] Remove warnings from Umbraco.Infrastructure project
- [ ] Remove warnings from Umbraco.PublishedCache.NuCache project
- [ ] Remove warnings from Umbraco.Web.BackOffice project
- [ ] Remove warnings from Umbraco.Web.Common project
- [x] Remove warnings from Umbraco.Web.UI project
- [x] Remove warnings from Umbraco.Web.UI.Client project
- [ ] Remove warnings from Umbraco.Web.Website project

emmagarland avatar Oct 21 '23 10:10 emmagarland

Hi there @emmagarland!

Firstly, a big thank you for raising this issue. Every piece of feedback we receive helps us to make Umbraco better.

We really appreciate your patience while we wait for our team to have a look at this but we wanted to let you know that we see this and share with you the plan for what comes next.

  • We'll assess whether this issue relates to something that has already been fixed in a later version of the release that it has been raised for.
  • If it's a bug, is it related to a release that we are actively supporting or is it related to a release that's in the end-of-life or security-only phase?
  • We'll replicate the issue to ensure that the problem is as described.
  • We'll decide whether the behavior is an issue or if the behavior is intended.

We wish we could work with everyone directly and assess your issue immediately but we're in the fortunate position of having lots of contributions to work with and only a few humans who are able to do it. We are making progress though and in the meantime, we will keep you in the loop and let you know when we have any questions.

Thanks, from your friendly Umbraco GitHub bot :robot: :slightly_smiling_face:

github-actions[bot] avatar Oct 21 '23 10:10 github-actions[bot]

Hi @emmagarland,

We're writing to let you know that we would love some help with this issue. We feel that this issue is ideal to flag for a community member to work on it. Once flagged here, folk looking for issues to work on will know to look at yours. Of course, please feel free work on this yourself ;-). If there are any changes to this status, we'll be sure to let you know.

For more information about issues and states, have a look at this blog post.

Thanks muchly, from your friendly Umbraco GitHub bot :-)

github-actions[bot] avatar Oct 23 '23 09:10 github-actions[bot]

Hey Emma!

As a new HQ'er this is something that popped on my radar as well and I am very pleased to see some community initiative on this.

I will talk this trough with the team on what our vision for this is, feel free to continue making smaller PR's and ill get back to you asap 🤗

Migaroez avatar Oct 23 '23 09:10 Migaroez

This would be great! I did some work on this a while ago, just trying to relieve some of this, by reformatting our code files: https://github.com/umbraco/Umbraco-CMS/pull/12438 (this is just for the core project)

Zeegaan avatar Oct 24 '23 07:10 Zeegaan

Thanks @Migaroez and @Zeegaan! Glad that you like the idea, looking forward to hearing more.

Nice to see you did a bunch of updates already @Zeegaan 👍

Emma

emmagarland avatar Oct 24 '23 07:10 emmagarland

(Going to look into this as part of Umbraco UK Festival hackathon, if anyone else is then let me know!)

jacksorjacksor avatar Nov 02 '23 10:11 jacksorjacksor

Sounds good @jacksorjacksor !

I have a PR open to set warnings as errors on projects without warnings, but @Migaroez is going to let me know HQ stance on that.

In the meantime, removing warnings is fair game for all (there are lots)!

emmagarland avatar Nov 02 '23 11:11 emmagarland

Data analysis:

My build created 2299 errors, split over these 15 projects:

Project Count
Umbraco.Infrastructure 700
Umbraco.Tests.Integration 512
Umbraco.Web.BackOffice 316
Umbraco.Tests.UnitTests 302
Umbraco.Core 269
Umbraco.PublishedCache.NuCache 102
Umbraco.Cms.Persistence.SqlServer 31
Umbraco.Web.Common 24
Umbraco.Tests.Benchmarks 18
Umbraco.Tests.Common 7
Umbraco.Web.Website 6
Umbraco.Cms.Persistence.Sqlite 5
Umbraco.Cms.Persistence.EFCore 3
Umbraco.Examine.Lucene 3
Umbraco.Web.UI 1
Grand Total 2299

Data set: BuildWarnings.csv

jacksorjacksor avatar Nov 02 '23 13:11 jacksorjacksor

Data analysis:

My build created 2299 errors, split over these 15 projects:

|Project|Count|

|---|---|

|Umbraco.Infrastructure|700|

|Umbraco.Tests.Integration|512|

|Umbraco.Web.BackOffice|316|

|Umbraco.Tests.UnitTests|302|

|Umbraco.Core|269|

|Umbraco.PublishedCache.NuCache|102|

|Umbraco.Cms.Persistence.SqlServer|31|

|Umbraco.Web.Common|24|

|Umbraco.Tests.Benchmarks|18|

|Umbraco.Tests.Common|7|

|Umbraco.Web.Website|6|

|Umbraco.Cms.Persistence.Sqlite|5|

|Umbraco.Cms.Persistence.EFCore|3|

|Umbraco.Examine.Lucene|3|

|Umbraco.Web.UI|1|

|Grand Total|2299|

Data set:

BuildWarnings.csv

Nice - and now we can tick off the UI project too!

emmagarland avatar Nov 02 '23 15:11 emmagarland

Hey y'all

We had a chat about this internally and I don't think it comes as a surprise that we would love all the warnings to go away as well.

Below are a couple of pointers/ideas

  • Keep the PR's coming but keep them small(ish) to avoid double work and support faster merging, don't forget to reference this ticket.
  • We can keep this ticket open as a tracking ticket if @emmagarland doesn't mind maintaining it.
  • We will further internally discus when the right time would be to turn on warnings as errors.
  • We are making our Internal review process a bit stricter to hopefully catch any new warnings from popping up.

Migaroez avatar Nov 03 '23 09:11 Migaroez

All sounds brilliant, thanks @Migaroez for following this all up!

Agreed re small PRs.

Happy to keep this open as a tracking ticket, and I'll keep on top of it.

Looking forward to the outcome of this!

Emma

emmagarland avatar Nov 03 '23 09:11 emmagarland

Hey @Migaroez, thanks for merging the first phase of this task!

I'll reopen this issue to keep it as a tracking issue if that's cool.

Cheers,

Emma

emmagarland avatar Aug 27 '24 17:08 emmagarland

Data analysis:

My build created 2299 errors, split over these 15 projects:

Project Count Umbraco.Infrastructure 700 Umbraco.Tests.Integration 512 Umbraco.Web.BackOffice 316 Umbraco.Tests.UnitTests 302 Umbraco.Core 269 Umbraco.PublishedCache.NuCache 102 Umbraco.Cms.Persistence.SqlServer 31 Umbraco.Web.Common 24 Umbraco.Tests.Benchmarks 18 Umbraco.Tests.Common 7 Umbraco.Web.Website 6 Umbraco.Cms.Persistence.Sqlite 5 Umbraco.Cms.Persistence.EFCore 3 Umbraco.Examine.Lucene 3 Umbraco.Web.UI 1 Grand Total 2299 Data set: BuildWarnings.csv

Current status of 'contrib', 3636 warnings split over 19 projects:

  1. Umbraco.Cms.Api.Delivery: 16
  2. Umbraco.Cms.Api.Management: 92
  3. Umbraco.Cms.Persistence.EFCore: 9
  4. Umbraco.Cms.Persistence.SqlServer: 31
  5. Umbraco.Cms.Persistence.Sqlite: 89
  6. Umbraco.Cms.Targets: 525
  7. Umbraco.Core: 525
  8. Umbraco.Examine.Lucene: 10
  9. Umbraco.Infrastructure: 697
  10. Umbraco.JsonSchema: 34
  11. Umbraco.PublishedCache.NuCache: 81
  12. Umbraco.TestData: 11
  13. Umbraco.Tests.Benchmarks: 15
  14. Umbraco.Tests.Common: 5
  15. Umbraco.Tests.Integration: 1269
  16. Umbraco.Tests.UnitTests: 319
  17. Umbraco.Web.Common: 40
  18. Umbraco.Web.UI: 1 (cannot currently find this warning)
  19. Umbraco.Web.Website: 13

As these are resolved, we can ensure warnings as errors are activated per project.

emmagarland avatar Aug 27 '24 18:08 emmagarland

Current status of 'main': 4862 warnings split over 31 projects (NB: these numbers sometimes change when rebuilding...):

  1. Umbraco.Templates: 0
  2. Umbraco.TestData: 11
  3. Umbraco.Tests.AcceptanceTest: 0
  4. Umbraco.Tests.AcceptanceTest.UmbracoProject: 0
  5. Umbraco.Tests.Benchmarks: 8
  6. Umbraco.Tests.Common: 0
  7. Umbraco.Tests.Integration: 1010
  8. Umbraco.Tests.UnitTests: 213
  9. Umbraco.Cms: 0
  10. Umbraco.JsonSchema: 0
  11. Umbraco.Cms.Api.Common: 12
  12. Umbraco.Cms.Api.Delivery: 118
  13. Umbraco.Cms.Api.Management: 295
  14. Umbraco.Cms.Imaging.ImageSharp: 0
  15. Umbraco.Cms.Imaging.ImageSharp2: 0
  16. Umbraco.Cms.Persistence.EFCore: 44
  17. Umbraco.Cms.Persistence.EFCore.SqlServer: 0
  18. Umbraco.Cms.Persistence.EFCore.Sqlite: 0
  19. Umbraco.Cms.Persistence.SqlServer: 40
  20. Umbraco.Cms.Persistence.Sqlite: 2
  21. Umbraco.Cms.StaticAssets: 0
  22. Umbraco.Cms.Targets: 39
  23. Umbraco.Core: 1069
  24. Umbraco.Examine.Lucene: 12
  25. Umbraco.Infrastructure: 1662
  26. Umbraco.PublishedCache.HybridCache: 155
  27. Umbraco.Web.Common: 154
  28. Umbraco.Web.UI: 1
  29. Umbraco.Web.UI.Client: 0
  30. Umbraco.Web.UI.Login: 0
  31. Umbraco.Web.Website: 17

As these are resolved, we can ensure warnings as errors are activated per project.

emmagarland avatar Jun 19 '25 14:06 emmagarland