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

Additional ServiceCollection extension methods #339

Conversation

andrewmckaskill
Copy link

These overloads allow for backwards compatibility between v3 and v4 when used inside Umbraco - allowing a v4 package to be substituted without re-compiling the entirety of Umbraco.

Reason:
Even though the new FacetsConfig parameter is optional and has a default value provided, existing packages that are built against v3 fail to bind to this method, producing a System.MissingMethodException:

System.MissingMethodException: Method not found: 'Microsoft.Extensions.DependencyInjection.IServiceCollection Examine.ServicesCollectionExtensions.AddExamineLuceneIndex(Microsoft.Extensions.DependencyInjection.IServiceCollection, System.String, Examine.FieldDefinitionCollection, Lucene.Net.Analysis.Analyzer, Examine.IValueSetValidator, System.Collections.Generic.IReadOnlyDictionary`2<System.String,Examine.Lucene.IFieldValueTypeFactory>)'.
   at Umbraco.Cms.Infrastructure.Examine.DependencyInjection.UmbracoBuilderExtensions.AddExamineIndexes(IUmbracoBuilder umbracoBuilder)

By changing the new parameter to be made available as a traditional overload instead of an optional parameter, the original method signature is kept the same and is found via reflection.

@Shazwazza
Copy link
Owner

Looks good, but I'm not sure what will happen if we are passing in a null facets config. @nikcio any ideas?

There's still a bunch of work for v4 to go, just needs time for testing/review.

@nikcio
Copy link
Contributor

nikcio commented Jul 5, 2023

@Shazwazza if the facetsconfig is null it will instanceiate the default facetsconfig so I'll should be fine. (This happens on line 115 in this file) 😉

@nikcio
Copy link
Contributor

nikcio commented Jul 5, 2023

@Shazwazza once we get #313 merged it should also be easy to see if we are passing null where we shouldn't (in new code from PRs) as it will create a warning. 😄

@nikcio
Copy link
Contributor

nikcio commented Jul 26, 2023

@andrewmckaskill Same here: #340 (comment)

@nikcio nikcio mentioned this pull request Jul 28, 2023
5 tasks
@nzdev
Copy link
Contributor

nzdev commented Jul 30, 2023

#347 ensures api compatibility for all of the v3 public api

@andrewmckaskill
Copy link
Author

Closing in favour of #347

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