Constructor property promotion not working correctly with properties that start with the same characters as the json property

[Gerben-T]

Running netresearch/jsonmapper: 4.5.0

Consider the following class

/**
 * @param string $firstName First name of the recipient.
 * @param string $lastName Last name of the recipient.
 * @param IdType $idType The recipient ID type. This is used to indicate the format used for the ID.
 * @param string $nickname Nickname or display name of the recipient.
 * @param string $id The recipient ID specific to the provider.
 */
public function __construct(
	public string $firstName,
	public string $lastName,
	public IdType $idType,
	public string $nickname,
	public string $id
) {}

And this is my json payload

{
	"id": "messaging-user@externalservice.com",
	"idType": "email"
}

When running

$mapper->map($json, MyClass::class);

it tries to put "id": "messaging-user@externalservice.com" into $idType which will fail because "messaging-user@externalservice.com" is not a valid value for enum IdType.

This is because of the following check https://github.com/cweiske/jsonmapper/blob/v4.5.0/src/JsonMapper.php#L580-L588
It seems to match $id to $idType because it starts with $id which is wrong in this case.
It should check if the whole variable name matches, so that
"$id" does not match "$idType".

Another thing is if my docblock contains $id somewhere in the description it would still match

@param string $firstName First name of the recipient. And some reference to $id maybe?

This would match for the json key id.

[SvenRtbg]

My analysis shows the surprising fact that annotations still take precedence over actual types assigned.

But I can reproduce the error, and it is correct that the search for the annotation simply scans for "string containing $+key" in the annotation array, without checking that there might be nonwhitespace after "key" in the property name.

I would love to fix the precedence issue here first. If proper types are assigned, they are valid and required to be followed, as you'd be unable to assign an "IdType" to a field requiring "string" and vice versa.

And then, you'd still need to update to version 5.0, which isn't a big step, but might require setting a flag back to it's previous default.

As a workaround, if you'd not include the annotation at all, i.e. remove the last asterisk in /** (make it /* or remove entirely), it will work.

[Gerben-T]

I would love to fix the precedence issue here first. If proper types are assigned, they are valid and required to be followed, as you'd be unable to assign an "IdType" to a field requiring "string" and vice versa.

I agree that the precedence is more important here.

And then, you'd still need to update to version 5.0, which isn't a big step, but might require setting a flag back to it's previous default.

I'm not sure how big the changes are in 5.0
If this is a small fix wouldn't it be possible to fix this in a minor version?

As a workaround, if you'd not include the annotation at all, i.e. remove the last asterisk in /** (make it /* or remove entirely), it will work.

Yes, but then i will lose some phpdoc functionality in me IDE (Phpstorm) since it can't parse the docblock.
Honestly the only reason for this docblock is the description for each param. Maybe i can get the same effect with single /** @var */ lines. I'll have to test this and see if they do not get parsed by the jsonmapper.

[Gerben-T]

I did some testing and found another weird thing
When i add /** @required */ to my $id property everything works..

This script

enum IdType: string {
	case email = "email";
	case other = "other";
}

readonly class SomeClass
{
	/**
	 * @param string $firstName First name of the recipient.
	 * @param string $lastName Last name of the recipient.
	 * @param IdType $idType The recipient ID type. This is used to indicate the format used for the ID.
	 * @param string $nickname Nickname or display name of the recipient.
	 * @param string $id The recipient ID specific to the provider.
	 */
	public function __construct(
		public string $firstName,
		public string $lastName,
		public IdType $idType,
		public string $nickname,
		public string $id
	) {}
}

$mapper = new JsonMapper();
$mapper->bExceptionOnMissingData = true;

$jsonObject = json_decode('{
	"id": "messaging-user@externalservice.com",
	"idType": "email"
}');
try {
	$result = $mapper->map($jsonObject, SomeClass::class);
	var_dump($result);
} catch(JsonMapper_Exception $e) {
	echo $e->getMessage();
}

Results in

Fatal error: Uncaught ValueError: "messaging-user@externalservice.com" is not a valid backing value for enum IdType in /vendor/netresearch/jsonmapper/src/JsonMapper.php on line 708

But if i simply change public string $id into

/** @required  */
public string $id

i get

class SomeClass#7 (5) {
  public readonly string $firstName =>
  *uninitialized*
  public readonly string $lastName =>
  *uninitialized*
  public readonly IdType $idType =>
  enum IdType::email : string("email");
  public readonly string $nickname =>
  *uninitialized*
  public readonly string $id =>
  string(34) "messaging-user@externalservice.com"
}

i was expecting the same error but somehow it now correctly understands the order?

[Gerben-T]

Maybe i can get the same effect with single /** @var */ lines. I'll have to test this and see if they do not get parsed by the jsonmapper.

I tested this and it works but not completely

Replacing the @param for @var does fix the jsonmapper issue (obviously because @var is not parsed and @param is), but the phpdoc only seems to work on the property itself and not on the constructor, which is honestly not a big deal for me atm.

	public function __construct(
		/** @var string $firstName First name of the recipient. */
		public string $firstName,
		/** @var string $lastName Last name of the recipient. */
		public string $lastName,
		/** @var IdType $idType The recipient ID type. This is used to indicate the format used for the ID. */
		public IdType $idType,
		/** @var string $nickname Nickname or display name of the recipient. */
		public string $nickname,
		/** @var string $id The recipient ID specific to the provider. */
		public string $id
	) {}

Inspection on the property works fine

Inspection on the constructor does not (it doesn't show the descriptions correctly)

The most important part works; showing the description when accessing the properties, so i'll use this as a workaround for now.

[SvenRtbg]

I believe we uncovered two issues here - at least that's what I found yesterday:

1. PHPDoc is preferred over the actual type of the property.
2. Parsing the PHPDoc also incorrectly finds properties with identical prefixes.

To keep this from happening, we'd need a proper test case, but I'll try to do my best there.

Hint: The reason I mentioned version 5 is because I don't think there will be maintenance releases for earlier versions, even if there are gaps in the version numbers. As major versions don't increase the required PHP version as of now, that should not be a big issue.

[SvenRtbg]

I like to add another problem: Even though JSON allows to distinguish between strings, ints and floats, the JsonMapper will call settype() prior to setting the value, thus typecasting the original value. This prevents the JSON value from ever having the wrong scalar type, however there is no flag to enable stricter checks which would trigger a TypeError when attempting to populate an int property with a string or vice versa.