Rename User::getSex()/setSex() to getGender()/setGender()

[ADmad]

In context of user's profile "gender" is the appropriate term rather than "sex". For example profile info provided by Google and Facebook also uses "gender".

So I propose that the methods, constants and property of User entity class should be renamed according.

Ideally this should have been done before the 3.0 release but perhaps we can still do this and maintain old names as aliases to avoid backwards compatible break.

[ovr]

Hey!

I am agree with this ide, but 3.0 was released :(

Let's create aliases and mark old names as @deprecated in PHPDoc

Will be cool if anyone can help with PR

Thanks

[ADmad]

There is another issue to consider. You force the gender(sex) to be either male or female only and setSex() throws exception for any other values.. That's not a good idea IMO.

For e.g. as per Meetup's API docs the gender value can be either male, female, other or none. So it would be best to simply set the gender/sex to whatever value the provider returns.

[heiglandreas]

I can understand the idea to have a defined interface. But I'd also promote at least to have a third option 'other'. Gender is not binary. Whether one likes it or not, if this library tries to enforce a binary solution here it will backfire.

So instead of forcing male of female I'd default to unknown and be able to set either male, female or other. That way the getGender-method could always return a string so there would not be the necessity to do a null-check.

A future extension could then be to set the provided additional information in a separate field so that the user can fetch the gender and either get male or female or f.e. the value provided by the third-aprty entity when they suppot different values or other if they don't. But that's for a future release IMO

[ADmad]

That way the getGender-method could always return a string so there would not be the necessity to do a null-check.

The method should still be able to return null to handle cases where gender isn't set in profile or for providers which don't return a gender at all.

[ADmad]

There are issues with setting gender/sex in specific provider classes too.

For e.g. the Facebook has

auth/src/OAuth2/Provider/Facebook.php

Line 63 in 4872935

$user->setSex($value === 1 ? User::SEX_FEMALE : User::SEX_MALE);

which seems pretty wrong given what the FB's Graph API says regarding the gender field https://developers.facebook.com/docs/graph-api/reference/user.

The gender selected by this person, male or female. If the gender is set to a custom value, this value will be based off of the preferred pronoun; it will be omitted if the preferred pronoun is neutral

So the the value could be either male, female or unset which the code checks for 1 and incorrectly always fallbacks to male.

[heiglandreas]

That way the getGender-method could always return a string so there would not be the necessity to do a null-check.

The method should still be able to return null to handle cases where gender isn't set in profile or for providers which don't return a gender at all.

In that case the method would return unknown instead of NULL. On the consumer side you always get a string back. The consumer anyhow needs to decide what to do with that string. But the API now is much clearer on what the consumer will get.

[ADmad]

Values for all other properties can be null so there's no problem allowing the return value of getGender() to be null too.

[heiglandreas]

Yeah. Not all. There are a lot of propreties that can – according to their docblock – not be null . But according to the code they can actually be anything as they are public properties, so anyone can alter them and set any type one wants. That is a completely different discussion IMO.

Still: A dedicated type that is not nullable for a valid reason is a much cleaner and explicit approach and interface. The returned string unknown tells the consumer exactly that we do not know the gender of the user. A value of null does jsust say that there is nothing. By convention the consumre shall then assume that we do not know the gender. Such implicit information often leads to missinterpretations. especially in the case of the current implementation that could also mean that the client tried to set a gender, got an exception that they then caught to not disturb the flow and now null can either mean "ew do not know" or "it's neither male nor female". Two completely different informations. With returning either unknown or other we explicitly tell the consumer what we know.

[ADmad]

Always returning unknown prevents differentiating between whether the provider returned unknown (for e.g. none in case of Meetup) or the provider doesn't allow / has ability to return gender at all.

Anyway considering the various possible values returned by providers it would be best to have two methods to get the gender. One that returns the marshalled value and another that returns the raw value.

[heiglandreas]

That was the idea I proposed in #94 (comment) - be able to get the value that was set if it is self::GENDER_OTHER

[ADmad]

Personally I would just return the raw values and let the users marshal / convert it to standard values depending on what's used in their apps.

[heiglandreas]

Always returning unknown prevents differentiating between whether the provider returned unknown (for e.g. none in case of Meetup) or the provider doesn't allow / has ability to return gender at all.

That problem is already existent with the null return value.