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.

Sign up for GitHub

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

Migrate to Doctrine CS 10 on 3.0.x #9886

Closed
wants to merge 1 commit into from
Closed

Migrate to Doctrine CS 10 on 3.0.x #9886

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
2 changes: 1 addition & 1 deletion composer.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@
},
"require-dev": {
"doctrine/annotations": "^1.13",
"doctrine/coding-standard": "^9.0",
"doctrine/coding-standard": "^10@dev",
"phpbench/phpbench": "^1.0",
"phpstan/phpstan": "1.8.0",
"phpunit/phpunit": "^9.5",
Expand Down
66 changes: 29 additions & 37 deletions lib/Doctrine/ORM/AbstractQuery.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,7 @@ abstract class AbstractQuery
/**
* The user-specified ResultSetMapping to use.
*/
protected ?ResultSetMapping $_resultSetMapping = null;
protected ResultSetMapping|null $_resultSetMapping = null;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the rationale behind enforcing the usage of union types instead of nullable types?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

doctrine/coding-standard#266 (comment)

Copy link
Member

@morozov morozov Jul 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I believe arbitrary union types (e.g. getValue(sting $key): int|string|false) often reflect poor API design while nullable types are fine (e.g. findUser(int $id): ?User).

I think that enforcing a feature introduced primarily to support the existing poor design of the PHP's own API and the type system as a one-size-fits-all solution is not the right move.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Union types are fine. I must disagree that union types often reflect poor API design. I think the premise is too simplified.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have two ways of expressing the same thing: a union of a type and null. Normalizing that is reasonable imho. Whether we use the old nullable syntax or the new union syntax does not matter that much to me, as long as we agree on a single way to do it.

And I don't follow that "poor API design" argument either.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All right, so how do you suggest to proceed here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not forbid nullable types in the coding standard: doctrine/coding-standard#266 (comment).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do not forbid nullable types in the coding standard

That means, we would allow both ways of expressing nullable types? Or would we let the fixer change Foo|null to ?Foo?

Do you want to open a PR on the coding-standards repo that makes the necessary config adjustments? I think, this change should be discussed there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think different topic is discussed here. The sniff is not about whether or not it is ok to design API with unions. It's not about API at all.

The sniff is about consistent usage of union types. If there are no unions, the sniff does not apply.

If there's union of Type and null, it should be written as union: Type|null. The archaic ?Type was introduced because of absence of union types in the language. Since there's unions support now, there's no reason to use ? because it's inconsistent. We already have null value and will have null standalone type in 8.2.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That means, we would allow both ways of expressing nullable types? Or would we let the fixer change Foo|null to ?Foo?

If the latter is possible, I would prefer that. Otherwise, the former is fine.

Do you want to open a PR on the coding-standards repo that makes the necessary config adjustments?

No.


/**
* The map of query hints.
Expand All @@ -104,14 +104,14 @@ abstract class AbstractQuery
*/
protected string|int $_hydrationMode = self::HYDRATE_OBJECT;

protected ?QueryCacheProfile $_queryCacheProfile = null;
protected QueryCacheProfile|null $_queryCacheProfile = null;

/**
* Whether or not expire the result cache.
*/
protected bool $_expireResultCache = false;

protected ?QueryCacheProfile $_hydrationCacheProfile = null;
protected QueryCacheProfile|null $_hydrationCacheProfile = null;

/**
* Whether to use second level cache, if available.
Expand All @@ -123,16 +123,16 @@ abstract class AbstractQuery
/**
* Second level cache region name.
*/
protected ?string $cacheRegion = null;
protected string|null $cacheRegion = null;

/**
* Second level query cache mode.
*
* @psalm-var Cache::MODE_*|null
*/
protected ?int $cacheMode = null;
protected int|null $cacheMode = null;

protected ?CacheLogger $cacheLogger = null;
protected CacheLogger|null $cacheLogger = null;

protected int $lifetime = 0;

Expand All @@ -143,7 +143,7 @@ public function __construct(
/**
* The entity manager used by this query object.
*/
protected EntityManagerInterface $em
protected EntityManagerInterface $em,
) {
$this->parameters = new ArrayCollection();
$this->_hints = $em->getConfiguration()->getDefaultQueryHints();
Expand All @@ -168,17 +168,13 @@ public function setCacheable(bool $cacheable): static
return $this;
}

/**
* @return bool TRUE if the query results are enabled for second level cache, FALSE otherwise.
*/
/** @return bool TRUE if the query results are enabled for second level cache, FALSE otherwise. */
public function isCacheable(): bool
{
return $this->cacheable;
}

/**
* @return $this
*/
/** @return $this */
public function setCacheRegion(string $cacheRegion): static
{
$this->cacheRegion = $cacheRegion;
Expand All @@ -191,14 +187,12 @@ public function setCacheRegion(string $cacheRegion): static
*
* @return string|null The cache region name; NULL indicates the default region.
*/
public function getCacheRegion(): ?string
public function getCacheRegion(): string|null
{
return $this->cacheRegion;
}

/**
* @return bool TRUE if the query cache and second level cache are enabled, FALSE otherwise.
*/
/** @return bool TRUE if the query cache and second level cache are enabled, FALSE otherwise. */
protected function isCacheEnabled(): bool
{
return $this->cacheable && $this->hasCache;
Expand All @@ -221,10 +215,8 @@ public function setLifetime(int $lifetime): static
return $this;
}

/**
* @psalm-return Cache::MODE_*|null
*/
public function getCacheMode(): ?int
/** @psalm-return Cache::MODE_*|null */
public function getCacheMode(): int|null
{
return $this->cacheMode;
}
Expand Down Expand Up @@ -287,7 +279,7 @@ public function getParameters(): ArrayCollection
*
* @return Parameter|null The value of the bound parameter, or NULL if not available.
*/
public function getParameter(int|string $key): ?Parameter
public function getParameter(int|string $key): Parameter|null
{
$key = Parameter::normalizeName($key);

Expand Down Expand Up @@ -449,7 +441,7 @@ public function setResultSetMapping(ResultSetMapping $rsm): static
/**
* Gets the ResultSetMapping used for hydration.
*/
protected function getResultSetMapping(): ?ResultSetMapping
protected function getResultSetMapping(): ResultSetMapping|null
{
return $this->_resultSetMapping;
}
Expand Down Expand Up @@ -485,7 +477,7 @@ private function translateNamespaces(ResultSetMapping $rsm): void
* $query->setHydrationCacheProfile(new QueryCacheProfile());
* $query->setHydrationCacheProfile(new QueryCacheProfile($lifetime, $resultKey));
*/
public function setHydrationCacheProfile(?QueryCacheProfile $profile): static
public function setHydrationCacheProfile(QueryCacheProfile|null $profile): static
{
if ($profile === null) {
$this->_hydrationCacheProfile = null;
Expand All @@ -505,7 +497,7 @@ public function setHydrationCacheProfile(?QueryCacheProfile $profile): static
return $this;
}

public function getHydrationCacheProfile(): ?QueryCacheProfile
public function getHydrationCacheProfile(): QueryCacheProfile|null
{
return $this->_hydrationCacheProfile;
}
Expand All @@ -518,7 +510,7 @@ public function getHydrationCacheProfile(): ?QueryCacheProfile
*
* @return $this
*/
public function setResultCacheProfile(?QueryCacheProfile $profile): static
public function setResultCacheProfile(QueryCacheProfile|null $profile): static
{
if ($profile === null) {
$this->_queryCacheProfile = null;
Expand All @@ -541,7 +533,7 @@ public function setResultCacheProfile(?QueryCacheProfile $profile): static
/**
* Defines a cache driver to be used for caching result sets and implicitly enables caching.
*/
public function setResultCache(?CacheItemPoolInterface $resultCache): static
public function setResultCache(CacheItemPoolInterface|null $resultCache): static
{
if ($resultCache === null) {
if ($this->_queryCacheProfile) {
Expand All @@ -567,7 +559,7 @@ public function setResultCache(?CacheItemPoolInterface $resultCache): static
*
* @return $this
*/
public function enableResultCache(?int $lifetime = null, ?string $resultCacheId = null): static
public function enableResultCache(int|null $lifetime = null, string|null $resultCacheId = null): static
{
$this->setResultCacheLifetime($lifetime);
$this->setResultCacheId($resultCacheId);
Expand All @@ -594,7 +586,7 @@ public function disableResultCache(): static
*
* @return $this
*/
public function setResultCacheLifetime(?int $lifetime): static
public function setResultCacheLifetime(int|null $lifetime): static
{
$lifetime = (int) $lifetime;

Expand Down Expand Up @@ -638,7 +630,7 @@ public function getExpireResultCache(): bool
return $this->_expireResultCache;
}

public function getQueryCacheProfile(): ?QueryCacheProfile
public function getQueryCacheProfile(): QueryCacheProfile|null
{
return $this->_queryCacheProfile;
}
Expand Down Expand Up @@ -853,7 +845,7 @@ public function getHints(): array
*/
public function toIterable(
ArrayCollection|array $parameters = [],
string|int|null $hydrationMode = null
string|int|null $hydrationMode = null,
): iterable {
if ($hydrationMode !== null) {
$this->setHydrationMode($hydrationMode);
Expand Down Expand Up @@ -885,7 +877,7 @@ public function toIterable(
*/
public function execute(
ArrayCollection|array|null $parameters = null,
string|int|null $hydrationMode = null
string|int|null $hydrationMode = null,
): mixed {
if ($this->cacheable && $this->isCacheEnabled()) {
return $this->executeUsingQueryCache($parameters, $hydrationMode);
Expand All @@ -902,7 +894,7 @@ public function execute(
*/
private function executeIgnoreQueryCache(
ArrayCollection|array|null $parameters = null,
string|int|null $hydrationMode = null
string|int|null $hydrationMode = null,
): mixed {
if ($hydrationMode !== null) {
$this->setHydrationMode($hydrationMode);
Expand Down Expand Up @@ -973,7 +965,7 @@ private function getHydrationCache(): CacheItemPoolInterface
*/
private function executeUsingQueryCache(
ArrayCollection|array|null $parameters = null,
string|int|null $hydrationMode = null
string|int|null $hydrationMode = null,
): mixed {
$rsm = $this->getResultSetMapping();
if ($rsm === null) {
Expand All @@ -985,7 +977,7 @@ private function executeUsingQueryCache(
$this->getHash(),
$this->lifetime,
$this->cacheMode ?: Cache::MODE_NORMAL,
$this->getTimestampKey()
$this->getTimestampKey(),
);

$result = $queryCache->get($queryKey, $rsm, $this->_hints);
Expand All @@ -1012,7 +1004,7 @@ private function executeUsingQueryCache(
return $result;
}

private function getTimestampKey(): ?TimestampCacheKey
private function getTimestampKey(): TimestampCacheKey|null
{
assert($this->_resultSetMapping !== null);
$entityName = reset($this->_resultSetMapping->aliasMap);
Expand Down Expand Up @@ -1059,7 +1051,7 @@ protected function getHydrationCacheId(): array
* If this is not explicitly set by the developer then a hash is automatically
* generated for you.
*/
public function setResultCacheId(?string $id): static
public function setResultCacheId(string|null $id): static
{
if (! $this->_queryCacheProfile) {
return $this->setResultCacheProfile(new QueryCacheProfile(0, $id));
Expand Down
8 changes: 4 additions & 4 deletions lib/Doctrine/ORM/Cache.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,9 @@ interface Cache
*/
public const MODE_REFRESH = 4;

public function getEntityCacheRegion(string $className): ?Region;
public function getEntityCacheRegion(string $className): Region|null;

public function getCollectionCacheRegion(string $className, string $association): ?Region;
public function getCollectionCacheRegion(string $className, string $association): Region|null;

/**
* Determine whether the cache contains data for the given entity "instance".
Expand Down Expand Up @@ -90,7 +90,7 @@ public function containsQuery(string $regionName): bool;
/**
* Evicts all cached query results under the given name, or default query cache if the region name is NULL.
*/
public function evictQueryRegion(?string $regionName = null): void;
public function evictQueryRegion(string|null $regionName = null): void;

/**
* Evict data from all query regions.
Expand All @@ -102,5 +102,5 @@ public function evictQueryRegions(): void;
*
* @param string|null $regionName Query cache region name, or default query cache if the region name is NULL.
*/
public function getQueryCache(?string $regionName = null): QueryCache;
public function getQueryCache(string|null $regionName = null): QueryCache;
}
14 changes: 7 additions & 7 deletions lib/Doctrine/ORM/Cache/CacheConfiguration.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,12 @@
*/
class CacheConfiguration
{
private ?CacheFactory $cacheFactory = null;
private ?RegionsConfiguration $regionsConfig = null;
private ?CacheLogger $cacheLogger = null;
private ?QueryCacheValidator $queryValidator = null;
private CacheFactory|null $cacheFactory = null;
private RegionsConfiguration|null $regionsConfig = null;
private CacheLogger|null $cacheLogger = null;
private QueryCacheValidator|null $queryValidator = null;

public function getCacheFactory(): ?CacheFactory
public function getCacheFactory(): CacheFactory|null
{
return $this->cacheFactory;
}
Expand All @@ -26,7 +26,7 @@ public function setCacheFactory(CacheFactory $factory): void
$this->cacheFactory = $factory;
}

public function getCacheLogger(): ?CacheLogger
public function getCacheLogger(): CacheLogger|null
{
return $this->cacheLogger;
}
Expand All @@ -49,7 +49,7 @@ public function setRegionsConfiguration(RegionsConfiguration $regionsConfig): vo
public function getQueryValidator(): QueryCacheValidator
{
return $this->queryValidator ??= new TimestampQueryCacheValidator(
$this->cacheFactory->getTimestampRegion()
$this->cacheFactory->getTimestampRegion(),
);
}

Expand Down
2 changes: 1 addition & 1 deletion lib/Doctrine/ORM/Cache/CacheFactory.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public function buildCachedCollectionPersister(EntityManagerInterface $em, Colle
/**
* Build a query cache based on the given region name
*/
public function buildQueryCache(EntityManagerInterface $em, ?string $regionName = null): QueryCache;
public function buildQueryCache(EntityManagerInterface $em, string|null $regionName = null): QueryCache;

/**
* Build an entity hydrator
Expand Down
4 changes: 1 addition & 3 deletions lib/Doctrine/ORM/Cache/CollectionCacheEntry.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,7 @@ class CollectionCacheEntry implements CacheEntry
*/
public array $identifiers;

/**
* @param CacheKey[] $identifiers List of entity identifiers hold by the collection
*/
/** @param CacheKey[] $identifiers List of entity identifiers hold by the collection */
public function __construct(array $identifiers)
{
$this->identifiers = $identifiers;
Expand Down
10 changes: 3 additions & 7 deletions lib/Doctrine/ORM/Cache/CollectionHydrator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,9 @@
*/
interface CollectionHydrator
{
/**
* @param mixed[]|Collection $collection The collection.
*/
/** @param mixed[]|Collection $collection The collection. */
public function buildCacheEntry(ClassMetadata $metadata, CollectionCacheKey $key, array|Collection $collection): CollectionCacheEntry;

/**
* @return mixed[]|null
*/
public function loadCacheEntry(ClassMetadata $metadata, CollectionCacheKey $key, CollectionCacheEntry $entry, PersistentCollection $collection): ?array;
/** @return mixed[]|null */
public function loadCacheEntry(ClassMetadata $metadata, CollectionCacheKey $key, CollectionCacheEntry $entry, PersistentCollection $collection): array|null;
}
2 changes: 1 addition & 1 deletion lib/Doctrine/ORM/Cache/ConcurrentRegion.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ interface ConcurrentRegion extends Region
*
* @throws LockException Indicates a problem accessing the region.
*/
public function lock(CacheKey $key): ?Lock;
public function lock(CacheKey $key): Lock|null;

/**
* Attempts to read unlock the mapping for the given key.
Expand Down
Loading