Inject ILogger to RequestLocalization Middleware

[Kahbazi]

#6532

[gfoidl]

Why did you take the change in 16739e0?

So to not inject the ILogger<RequestLocalizationMiddleware>.

[Kahbazi]

I saw that other middlewares inject ILoggerFactory so I wanted to keep the same pattern

[Tratcher]

The breaking change guidelines are to add a new constructor and obsolete the old one. For weird DI reasons the new constructor should be placed above the old one.

[Kahbazi]

So how should we create logger object for the old constructor. Maybe I should put back the EnsureLogger!!?

[rynowak]

For weird DI reasons the new constructor should be placed above the old one.

Does this actually matter to DI? the order used by reflection is not a stable guarantee 😢

[pranavkm]

There's an attribute that you can use to specify the constructor to be used - ActivatorUtilitiesConstructorAttribute

[pranavkm]

@Kahbazi here's what I'd do:

1. Add a new ctor which accepts a ILoggerFactory \ ILogger<T> instance. I personally prefer the latter, but there isn't a clear guidance for one over the other.
2. Add [Obsolete] attribute to the current attribute. Add ActivatorUtilitiesConstructorAttribute to the new ctor
3. Change the current constructor to call the other ctor. Have it pass a NullLogger instance. This should avoid the need to have EnsureLogger

[Tratcher]

@Kahbazi here's an example, we use a NullLogger for the other constructor:
https://github.com/aspnet/AspNetCore/blob/62351067ff4c1401556725b401478e648b66acdc/src/Security/CookiePolicy/src/CookiePolicyMiddleware.cs#L20-L32

[Kahbazi]

So the log will be disabled if someone directly uses the old constructor, is that ok?

[Tratcher]

Yes, it's quite unlikely anyone will use the old one, but we leave it for binary compat.

[pranavkm]

[Obsolete("This constructor is obsolete and will be removed in a future version. Use RequestLocalizationMiddleware(RequestDelegate next, IOptions<RequestLocalizationOptions> options, ILoggerFactory loggerFactory) instead")]

[Tratcher]

So far so good. @pranavkm can you track merging this?

[pranavkm]

@Kahbazi 👍

src\Middleware\Localization\ref\Microsoft.AspNetCore.Localization.netcoreapp3.0.cs(,): error : Generated code is not up to date in src/Middleware/Localization/ref/Microsoft.AspNetCore.Localization.netcoreapp3.0.cs. You might need to regenerate the reference assemblies or project list (see docs/ReferenceAssemblies.md and docs/ReferenceResolution.md)
PowerShell exited with code '1'.

[Kahbazi]

@pranavkm Done!

[pranavkm]

Thanks for the PR!