Fix parameter type hints in EntityManager methods#6
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aims to adjust method parameter typing/optionality in Kdyby\Doctrine\EntityManager to better reflect intended usage and improve type hints.
Changes:
- Modifies
EntityManager::flush()signature (currently making the parameter mandatory). - Modifies
EntityManager::create()signature to use a nullableEventManagertype (currently making the parameter mandatory and omitting a default). - Changes the lazy-initialization check in
safePersist()(currently introducing a syntax error).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/Kdyby/Doctrine/EntityManager.php
Outdated
| * @return EntityManager | ||
| */ | ||
| public function flush($entity = null) | ||
| public function flush($entity ) |
There was a problem hiding this comment.
flush() now requires an argument ($entity) even though the docblock and Doctrine API allow calling flush() with no args. This is a backwards-incompatible change and will break callers that do $em->flush(). Restore the default value (e.g., $entity = NULL) so the parameter remains optional; you can still keep/improve type information via PHPDoc.
| public function flush($entity ) | |
| public function flush($entity = NULL) |
src/Kdyby/Doctrine/EntityManager.php
Outdated
| public function safePersist($entity) | ||
| { | ||
| if ($this->nonLockingUniqueInserter === NULL) { | ||
| if ($this->nonLockingUniqueInserter ==) { |
There was a problem hiding this comment.
This if condition is a PHP parse error (== without a right-hand operand). It should compare $this->nonLockingUniqueInserter against NULL (and ideally use strict comparison) so the lazy initialization works as intended.
| if ($this->nonLockingUniqueInserter ==) { | |
| if ($this->nonLockingUniqueInserter === null) { |
src/Kdyby/Doctrine/EntityManager.php
Outdated
| public static function create($conn, Doctrine\ORM\Configuration $config, ?Doctrine\Common\EventManager $eventManager ) | ||
| { |
There was a problem hiding this comment.
create() previously allowed omitting $eventManager (default NULL), but it is now a required parameter and the signature is missing a default. This is backwards-incompatible and also inconsistent with the implementation which treats it as optional ($eventManager ?: new ...). Add a default = NULL (and keep ?Doctrine\\Common\\EventManager if you want nullability). Also consider updating the PHPDoc to reflect that it can be null.
No description provided.