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

Add proper types to the Doctrine\DBAL\Sharding namespace. #3571

Merged
merged 1 commit into from
May 30, 2019

Conversation

jwage
Copy link
Member

@jwage jwage commented May 30, 2019

Q A
Type improvement
BC Break no

Summary

Add proper types to the Doctrine\DBAL\Sharding namespace.

@jwage
Copy link
Member Author

jwage commented May 30, 2019

@morozov Is the shard id always an integer? I see we allow string|int|null but I see it casted to int here https://github.com/doctrine/dbal/blob/master/lib/Doctrine/DBAL/Sharding/PoolingShardConnection.php#L187

Can we change it to be int everywhere?

@morozov
Copy link
Member

morozov commented May 30, 2019

I believe the (int) cast here is wrong. The method didn't allow string prior to 7bafa27#diff-9b68bdf5dcd3705b351878bd1ee20492R56 and it caused a type mismatch:

$ git diff
@@ -131,7 +131,7 @@ class PoolingShardConnection extends Connection
     /**
      * Connects to a given shard.
      *
-     * @param string|int|null $shardId
+     * @param int|null $shardId

$ phpstan a
 ------ --------------------------------------------------------------------------------------- 
  Line   Doctrine/DBAL/Sharding/PoolingShardManager.php                                         
 ------ --------------------------------------------------------------------------------------- 
  46     Parameter #1 $shardId of method                                                        
         Doctrine\DBAL\Sharding\PoolingShardConnection::connect() expects int|null, int|string  
         given.                                                                                 
 ------ --------------------------------------------------------------------------------------- 

The major problem is that I personally don't know which sharding implementations we support and we don't test anything on CI. I'd assume that depending on the implementation, shards IDs may be integers or strings. E.g. in the Azure documentation, they refer to shards as A, B, C.

Enforcing the ID to int only might be the right approach, but it will require some more rework of the calling code.

@morozov
Copy link
Member

morozov commented May 30, 2019

Honestly, I'm not even sure that the sharding support is practically usable. The only implementation of ShardChoser is MultiTenantShardChoser:

public function pickShard($distributionValue, PoolingShardConnection $conn)
{
return $distributionValue;
}

The implementation means that for every single distribution value, there's a separate shard which does not make any sense. Is there a way to see this sharding in action?

@jwage
Copy link
Member Author

jwage commented May 30, 2019

To be honest, I am not sure. We might need to create a new issue for 3.0 to look at this and figure out how to properly test it and confirm it is working.

@jwage jwage self-assigned this May 30, 2019
@jwage jwage merged commit d794259 into doctrine:develop May 30, 2019
@jwage jwage deleted the sharding-types branch May 30, 2019 15:34
morozov pushed a commit to morozov/dbal that referenced this pull request May 31, 2019
Add proper types to the Doctrine\DBAL\Sharding namespace.
morozov pushed a commit to morozov/dbal that referenced this pull request May 31, 2019
Add proper types to the Doctrine\DBAL\Sharding namespace.
morozov pushed a commit to morozov/dbal that referenced this pull request May 31, 2019
Add proper types to the Doctrine\DBAL\Sharding namespace.
morozov pushed a commit to morozov/dbal that referenced this pull request May 31, 2019
Add proper types to the Doctrine\DBAL\Sharding namespace.
morozov pushed a commit that referenced this pull request Jun 13, 2019
Add proper types to the Doctrine\DBAL\Sharding namespace.
morozov pushed a commit that referenced this pull request Jun 27, 2019
Add proper types to the Doctrine\DBAL\Sharding namespace.
morozov pushed a commit that referenced this pull request Jun 27, 2019
Add proper types to the Doctrine\DBAL\Sharding namespace.
morozov pushed a commit that referenced this pull request Jun 27, 2019
Add proper types to the Doctrine\DBAL\Sharding namespace.
morozov pushed a commit to morozov/dbal that referenced this pull request Aug 26, 2019
Add proper types to the Doctrine\DBAL\Sharding namespace.
morozov pushed a commit to morozov/dbal that referenced this pull request Aug 26, 2019
Add proper types to the Doctrine\DBAL\Sharding namespace.
morozov pushed a commit to morozov/dbal that referenced this pull request Aug 26, 2019
Add proper types to the Doctrine\DBAL\Sharding namespace.
morozov pushed a commit to morozov/dbal that referenced this pull request Aug 27, 2019
Add proper types to the Doctrine\DBAL\Sharding namespace.
morozov pushed a commit that referenced this pull request Nov 2, 2019
Add proper types to the Doctrine\DBAL\Sharding namespace.
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 8, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants