Facebook

[MichaelPetrinolis]

Facebook module ->Facebook Login integration

[sebastienros]

Do you want to do a demo next Tuesday?

[MichaelPetrinolis]

Thank you for the invitation, I would love to do so.
My only concern is that this will be my first time.

[Skrypt]

There's a first time for everything. Better sooner than later 😉

[sebastienros]

Just require IEnumerable in interfaces. No need for ImmutableArray.

[agriffard]

@MichaelPetrinolis Can you please fix the conflicts?

[MichaelPetrinolis]

@agriffard done.

[KANekT]

When you planed finish FB login and close / merge in beta 3 this issue?

[MichaelPetrinolis]

That depends. There are #1786 and #2029 which are related and might change the way third party authentication services are implemented and probably that's the reason that is not yet merged.

[KANekT]

oh... #1786 planed in rc :(

[MichaelPetrinolis]

Don't know if this is the reason not merged. I tried to make a guess.

[sebastienros]

I was just worried by the dependencies between components, I need to re-focus on this PR to understand why and if they are necessary. Also it is related to the Layout changes from @MatthijsKrempel

[MatthijsKrempel]

@sebastienros had a look at the code, I don't see any overlap,.

[sebastienros]

@MichaelPetrinolis I updated your PR, do you want to review my changes? If you are ok with these then I will merge.

[kevinchalet]

Should the other display driver be updated to perform the authorization check at the same stage?

[sebastienros]

I looked for other patterns, but could only find the same one. I assume you are talking about having the security check inside the shape lambda.

[kevinchalet]

No, I meant why moving the authorization check inside the if block? It's not consistent with what exists elsewhere (but maybe it's the right thing to do, I dunno):

OrchardCore/src/OrchardCore.Modules/OrchardCore.Email/Drivers/SmtpSettingsDisplayDriver.cs

Lines 72 to 82 in 80670ba

	public override async Task<IDisplayResult> UpdateAsync(SmtpSettings section, BuildEditorContext context)
	{
	var user = _httpContextAccessor.HttpContext?.User;
	if (!await _authorizationService.AuthorizeAsync(user, Permissions.ManageEmailSettings))
	{
	return null;
	}
	if (context.GroupId == GroupId)
	{

[kevinchalet]

Out of curiosity, why these changes?

[sebastienros]

I don't see the point of requiring an immutable array on the interface. I remember you had explained why you use them, please remind me.

[kevinchalet]

ImmutableArray is a struct type (that has dedicated LINQ operators so things like Any() don't allocate enumerators). If you end up exposing it as an IEnumerable, this benefit is lost and there's no point using ImmutableArray to build the list.

Personally, I like having ImmutableArray instead of IEnumerable when we know the implementation won't be done using an iterator (async methods are very poor candidates for that anyway. We'll have to wait for C# 8 and .NET Core 3.0 to have something better) or when the enumeration could be easily cached. There are many places in OC that could benefit from that, e.g all the IPermissionProvider implementations could expose the permissions list as a cached ImmutableArray instead of returning new arrays each time.

[MichaelPetrinolis]

I saw two issues, can I commit in this PR or should I create a new one?
1.The update for core settings does not work(it checks the settings validity without assigning the values from model)
2. When the appsecret is not set, the edit display driver fails due to unprotecting null string ….

[kevinchalet]

@MichaelPetrinolis sadly @sebastienros already merged your PR, so it's too late to include new commits in this PR.

[kevinchalet]

Using PathString like we now do in the OpenID module could have been a good idea 😄

[MichaelPetrinolis]

Thanx , i will double check it in dev branch and create a new PR if necessary