Testing v9-rc1 on Laravel Passport

[hafezdivandari]

Trying to add support and test v9-rc1 before stable release on Laravel Passport, PR laravel/passport#1734, there are 2 issues:

1. User ID doesn't accept integer
2. Client ID doesn't accept integer

You may check the failed tests here: https://github.com/laravel/passport/actions/runs/8482967241/job/23243211337?pr=1734

Both User and Client classes of Laravel Passport are using League\OAuth2\Server\Entities\Traits\EntityTrait.

• This trait accepts mixed id on set but return string on get:

  oauth2-server/src/Entities/Traits/EntityTrait.php

  Lines 22 to 33 in ca511c1

  	/**
  	* @return non-empty-string
  	*/
  	public function getIdentifier(): string
  	{
  	return $this->identifier;
  	}
  	public function setIdentifier(mixed $identifier): void
  	{
  	$this->identifier = $identifier;
  	}

• Also on the TokenInterface class, $userIdentifier type doesn't match on set / get :

  oauth2-server/src/Entities/TokenInterface.php

  Lines 39 to 49 in ca511c1

  	/**
  	* Set the identifier of the user associated with the token.
  	*
  	* @param non-empty-string $identifier
  	*/
  	public function setUserIdentifier(string $identifier): void;
  	/**
  	* Get the token user's identifier.
  	*/
  	public function getUserIdentifier(): string|int|null;

• On AccessTokenRepositoryInterface the $userIdentifier property accepts mixed:

  oauth2-server/src/Repositories/AccessTokenRepositoryInterface.php

  Line 33 in ca511c1

  mixed $userIdentifier = null

• On AbstractGrant class, getClientEntityOrFail method accepts only string $clientId and not int:

  oauth2-server/src/Grant/AbstractGrant.php

  Line 187 in ca511c1

  protected function getClientEntityOrFail(string $clientId, ServerRequestInterface $request): ClientEntityInterface

• On ClientRepositoryInterface class, getClientEntity and validateClient methods, $clientIdentifier argument doesn't accept int:

  oauth2-server/src/Repositories/ClientRepositoryInterface.php

  Lines 25 to 30 in ca511c1

  	public function getClientEntity(string $clientIdentifier): ?ClientEntityInterface;
  	/**
  	* Validate a client's secret.
  	*/
  	public function validateClient(string $clientIdentifier, ?string $clientSecret, ?string $grantType): bool;

[Sephster]

Thanks so much for this @hafezdivandari. This is super useful. I suspected the change of IDs might cause issues downstream so great to have something concrete to work with. Really appreciate you doing this

[hafezdivandari]

Thank you @Sephster for your hard work in maintaining this package and the new major release. I'll keep an eye on this issue until it is resolved. Please let me know when we are able to move the PR forward. Thanks.

[Sephster]

I think I've fixed all the issues raised here now @hafezdivandari - thanks again for creating the PR for Passport. Would you be able to check if this has resolved all issues now? Once we have a working, v9 compatible version of Passport, I will look to tag v9 proper. Thanks again for progressing this. Much appreciated.

[hafezdivandari]

@Sephster I applied the latest changes. I'm checking failed test...

[hafezdivandari]

@Sephster all tests are passing. We may close this issue as completed now, Thank you.

[eugene-borovov]

Hi, @Sephster
This library should treat any identifiers as strings. This will avoid constant conversions to a string when composing a response. The only thing that needs to be provided is the conversion of identifiers from a query to a string in order to avoid type mismatch errors. Knowing the true data type of the identifier is the task of the application that uses this library.

The introduction of an integer identifier is a step back in the implementation of typing.

[Sephster]

I can see the case for this argument. Part of the reason I originally had the identifiers as strings initially was to avoid casting when things come in from http requests.

@hafezdivandari what are your thoughts on this? Would a simple case of casting fix most compatibility issues with Passport?

You still raised some excellent points so I won't revert all changes but I'm leaning towards treating identifiers as strings only as @eugene-borovov suggests

[eugene-borovov]

@Sephster
I`d like to remind about this issue. This change help to cast all request parameters to string.

[hafezdivandari]

No problem @Sephster, let's make the types more strict and see what happens on Laravel Passport tests. I'll try to cast the arguments.

[Sephster]

Changes made now @hafezdivandari - if you can see how you get on with the PR, I can merge this into main assuming all is ok. Apologies for the delay with this as well, I was travelling with my family.

[hafezdivandari]

Thank you @Sephster, have a nice trip. Just commented a little issue on the PR: #1402 (comment)

[Sephster]

It was great thank you. Thanks for spotting that. I've fixed the issue reported now.

[hafezdivandari]

@Sephster 6 tests are failing on Laravel Passport rn, I added a comment on the PR: #1402 (comment) I hope this is the last one.