Umbraco-CMS
Umbraco-CMS copied to clipboard
Multiple warnings in source code
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
- Open IDE of choice
- Build
- 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
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:
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 :-)
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 🤗
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)
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
(Going to look into this as part of Umbraco UK Festival hackathon, if anyone else is then let me know!)
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)!
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
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:
Nice - and now we can tick off the UI project too!
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.
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
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
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:
- Umbraco.Cms.Api.Delivery: 16
- Umbraco.Cms.Api.Management: 92
- Umbraco.Cms.Persistence.EFCore: 9
- Umbraco.Cms.Persistence.SqlServer: 31
- Umbraco.Cms.Persistence.Sqlite: 89
- Umbraco.Cms.Targets: 525
- Umbraco.Core: 525
- Umbraco.Examine.Lucene: 10
- Umbraco.Infrastructure: 697
- Umbraco.JsonSchema: 34
- Umbraco.PublishedCache.NuCache: 81
- Umbraco.TestData: 11
- Umbraco.Tests.Benchmarks: 15
- Umbraco.Tests.Common: 5
- Umbraco.Tests.Integration: 1269
- Umbraco.Tests.UnitTests: 319
- Umbraco.Web.Common: 40
- Umbraco.Web.UI: 1 (cannot currently find this warning)
- Umbraco.Web.Website: 13
As these are resolved, we can ensure warnings as errors are activated per project.
Current status of 'main': 4862 warnings split over 31 projects (NB: these numbers sometimes change when rebuilding...):
- Umbraco.Templates: 0
- Umbraco.TestData: 11
- Umbraco.Tests.AcceptanceTest: 0
- Umbraco.Tests.AcceptanceTest.UmbracoProject: 0
- Umbraco.Tests.Benchmarks: 8
- Umbraco.Tests.Common: 0
- Umbraco.Tests.Integration: 1010
- Umbraco.Tests.UnitTests: 213
- Umbraco.Cms: 0
- Umbraco.JsonSchema: 0
- Umbraco.Cms.Api.Common: 12
- Umbraco.Cms.Api.Delivery: 118
- Umbraco.Cms.Api.Management: 295
- Umbraco.Cms.Imaging.ImageSharp: 0
- Umbraco.Cms.Imaging.ImageSharp2: 0
- Umbraco.Cms.Persistence.EFCore: 44
- Umbraco.Cms.Persistence.EFCore.SqlServer: 0
- Umbraco.Cms.Persistence.EFCore.Sqlite: 0
- Umbraco.Cms.Persistence.SqlServer: 40
- Umbraco.Cms.Persistence.Sqlite: 2
- Umbraco.Cms.StaticAssets: 0
- Umbraco.Cms.Targets: 39
- Umbraco.Core: 1069
- Umbraco.Examine.Lucene: 12
- Umbraco.Infrastructure: 1662
- Umbraco.PublishedCache.HybridCache: 155
- Umbraco.Web.Common: 154
- Umbraco.Web.UI: 1
- Umbraco.Web.UI.Client: 0
- Umbraco.Web.UI.Login: 0
- Umbraco.Web.Website: 17
As these are resolved, we can ensure warnings as errors are activated per project.