This issue documents a set of improvements identified during a review of the codebase.
1. pendingWaitersCount property iterates the entire waiters queue on every pool capacity check
PoolManager computes pendingWaitersCount via a foreach over $this->waiters, calling isPending() on every entry. This property is evaluated inside get() every time $maxWaiters > 0 and the pool is at capacity. Under heavy concurrent load this is O(n) per connection request, where n is the number of callers waiting.
A separately maintained $pendingWaiterCount integer, incremented when a waiter is enqueued and decremented when one is dequeued or times out, would make this O(1) with no behaviour change.
File: src/Manager/PoolManager.php
2. releaseClean() allocates a new SplQueue instance when the waiter queue is empty
When $this->waiters is empty after serving a waiter, releaseClean() replaces it with new SplQueue(). An empty SplQueue is already fully reusable with no excess memory. The replacement allocation is unnecessary and happens on every connection release that finds no pending waiters.
File: src/Manager/PoolManager.php
3. fromUri() defaults enableServerSideCancellation to true, inconsistent with the constructor and fromArray()
When building a MysqlConfig from a URI string, enable_server_side_cancellation defaults to true if the query parameter is absent. The constructor and fromArray() both default this to false. A caller switching from array-based to URI-based configuration would silently pick up a different cancellation behaviour. All three construction paths should share the same default.
File: src/ValueObjects/MysqlConfig.php
4. checkShutdownComplete() redundantly sets $this->activeConnections = 0
The guard at the top of checkShutdownComplete() returns early unless $this->activeConnections === 0. The unconditional assignment $this->activeConnections = 0; several lines later is therefore always assigning zero to a value that is already zero. This line is a no-op and misleads readers into thinking the counter could still be non-zero at that point.
File: src/Manager/PoolManager.php
5. PoolManager::$MysqlConfig property uses PascalCase
private MysqlConfig $MysqlConfig; names a property using PascalCase, which matches the class name and conflicts with PSR-12 and the camelCase property naming convention used throughout the rest of the library. It should be renamed to $mysqlConfig and its internal references updated accordingly.
File: src/Manager/PoolManager.php
6. Duplicate docblock on createNewConnection()
The @internal and @return docblock for createNewConnection() appears twice, back-to-back, directly above the method definition. One copy is a copy-paste remnant and should be removed.
File: src/Manager/PoolManager.php
7. healthCheck() duplicates the same queue-drain logic in both the resolve and reject callbacks
Inside Promise::all(), both the resolve callback and the reject callback contain an identical while (! $tempQueue->isEmpty()) loop that re-enqueues or removes healthy connections. Extracting this into a local closure would eliminate the duplication and make future changes to the drain logic apply to both paths automatically.
File: src/Manager/PoolManager.php
8. Connection class is not final
All other internal classes in the library (ManagedPreparedStatement, Transaction, PreparedStatement, RowStream, NameParamParser) are declared final. Connection is not. It is marked @internal, but without final there is nothing preventing accidental subclassing. Adding final makes the intent consistent with the rest of the codebase.
File: src/Internals/Connection.php
9. MysqlClient::stream() uses an anonymous class to carry two fields
The $state object used in stream() is an anonymous class with two public properties ($connection and $released). An anonymous class definition is created per call site. Two closure-captured-by-reference variables ($connection = null and $released = false) would carry the same state without the overhead of a class instantiation, and reduce the cognitive load when reading the method.
File: src/MysqlClient.php
10. NameParamParser uses string literals for its state machine instead of the existing ParserState enum
NameParamParser::parse() drives its state machine using plain string literals ('NORMAL', '--', '/*', etc.). The library already defines src/Enums/ParserState.php for exactly this purpose. Using the enum would make state transitions refactoring-safe, remove the risk of typos in string comparisons, and align with how other handlers in the library track their state.
File: src/Internals/NameParamParser.php
Happy to submit a pull request addressing any or all of the above. I will start with an issue per the contributing guidelines and await your direction on which items to prioritize.
This issue documents a set of improvements identified during a review of the codebase.
1.
pendingWaitersCountproperty iterates the entire waiters queue on every pool capacity checkPoolManagercomputespendingWaitersCountvia aforeachover$this->waiters, callingisPending()on every entry. This property is evaluated insideget()every time$maxWaiters > 0and the pool is at capacity. Under heavy concurrent load this is O(n) per connection request, where n is the number of callers waiting.A separately maintained
$pendingWaiterCountinteger, incremented when a waiter is enqueued and decremented when one is dequeued or times out, would make this O(1) with no behaviour change.File:
src/Manager/PoolManager.php2.
releaseClean()allocates a newSplQueueinstance when the waiter queue is emptyWhen
$this->waitersis empty after serving a waiter,releaseClean()replaces it withnew SplQueue(). An emptySplQueueis already fully reusable with no excess memory. The replacement allocation is unnecessary and happens on every connection release that finds no pending waiters.File:
src/Manager/PoolManager.php3.
fromUri()defaultsenableServerSideCancellationtotrue, inconsistent with the constructor andfromArray()When building a
MysqlConfigfrom a URI string,enable_server_side_cancellationdefaults totrueif the query parameter is absent. The constructor andfromArray()both default this tofalse. A caller switching from array-based to URI-based configuration would silently pick up a different cancellation behaviour. All three construction paths should share the same default.File:
src/ValueObjects/MysqlConfig.php4.
checkShutdownComplete()redundantly sets$this->activeConnections = 0The guard at the top of
checkShutdownComplete()returns early unless$this->activeConnections === 0. The unconditional assignment$this->activeConnections = 0;several lines later is therefore always assigning zero to a value that is already zero. This line is a no-op and misleads readers into thinking the counter could still be non-zero at that point.File:
src/Manager/PoolManager.php5.
PoolManager::$MysqlConfigproperty uses PascalCaseprivate MysqlConfig $MysqlConfig;names a property using PascalCase, which matches the class name and conflicts with PSR-12 and the camelCase property naming convention used throughout the rest of the library. It should be renamed to$mysqlConfigand its internal references updated accordingly.File:
src/Manager/PoolManager.php6. Duplicate docblock on
createNewConnection()The
@internaland@returndocblock forcreateNewConnection()appears twice, back-to-back, directly above the method definition. One copy is a copy-paste remnant and should be removed.File:
src/Manager/PoolManager.php7.
healthCheck()duplicates the same queue-drain logic in both the resolve and reject callbacksInside
Promise::all(), both the resolve callback and the reject callback contain an identicalwhile (! $tempQueue->isEmpty())loop that re-enqueues or removes healthy connections. Extracting this into a local closure would eliminate the duplication and make future changes to the drain logic apply to both paths automatically.File:
src/Manager/PoolManager.php8.
Connectionclass is notfinalAll other internal classes in the library (
ManagedPreparedStatement,Transaction,PreparedStatement,RowStream,NameParamParser) are declaredfinal.Connectionis not. It is marked@internal, but withoutfinalthere is nothing preventing accidental subclassing. Addingfinalmakes the intent consistent with the rest of the codebase.File:
src/Internals/Connection.php9.
MysqlClient::stream()uses an anonymous class to carry two fieldsThe
$stateobject used instream()is an anonymous class with two public properties ($connectionand$released). An anonymous class definition is created per call site. Two closure-captured-by-reference variables ($connection = nulland$released = false) would carry the same state without the overhead of a class instantiation, and reduce the cognitive load when reading the method.File:
src/MysqlClient.php10.
NameParamParseruses string literals for its state machine instead of the existingParserStateenumNameParamParser::parse()drives its state machine using plain string literals ('NORMAL','--','/*', etc.). The library already definessrc/Enums/ParserState.phpfor exactly this purpose. Using the enum would make state transitions refactoring-safe, remove the risk of typos in string comparisons, and align with how other handlers in the library track their state.File:
src/Internals/NameParamParser.phpHappy to submit a pull request addressing any or all of the above. I will start with an issue per the contributing guidelines and await your direction on which items to prioritize.