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

Changes necessary for compatibility with ORM 3 #373

Closed
wants to merge 1 commit into from
Closed

Changes necessary for compatibility with ORM 3 #373

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
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/Persistence/Mapping/ClassMetadata.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
*
* @return ReflectionClass<T>
*/
public function getReflectionClass(): ReflectionClass;
public function getReflectionClass();

Check failure on line 38 in src/Persistence/Mapping/ClassMetadata.php

View workflow job for this annotation

GitHub Actions / Coding Standards / Coding Standards (8.3) 

Method \Doctrine\Persistence\Mapping\ClassMetadata::getReflectionClass() does not have native return type hint for its return value but it should be possible to add it based on @return annotation "ReflectionClass<T>".
Copy link
Member Author

Choose a reason for hiding this comment

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

In ORM 3, the return type is ReflectionClass|null


/** Checks if the given field name is a mapped identifier for this class. */
public function isIdentifier(string $fieldName): bool;
Expand Down
4 changes: 2 additions & 2 deletions src/Persistence/Mapping/Driver/FileDriver.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,7 @@
*
* @throws MappingException
*/
public function getElement(string $className): ClassMetadata
public function getElement(string $className)

Check failure on line 74 in src/Persistence/Mapping/Driver/FileDriver.php

View workflow job for this annotation

GitHub Actions / Coding Standards / Coding Standards (8.3) 

Method \Doctrine\Persistence\Mapping\Driver\FileDriver::getElement() does not have native return type hint for its return value but it should be possible to add it based on @return annotation "ClassMetadata".
Copy link
Member Author

Choose a reason for hiding this comment

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

When running the ORM 3 test suite, line 95 causes lots of issues:

Doctrine\Persistence\Mapping\Driver\FileDriver::getElement(): Return value must be of type Doctrine\Persistence\Mapping\ClassMetadata, SimpleXMLElement returned

Copy link
Member

Choose a reason for hiding this comment

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

to me, this looks like invalid tests

Copy link
Member

Choose a reason for hiding this comment

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

hmm, actually, it might be that the new types don't match the actual usages. The XmlDriver of the orm returns array<string, SimpleXMLElement> in loadMappingFile, which is why getElement also returns a SimpleXMLElement.

My guess is that the types added for this method are totally wrong, as it looks like each FileDriver child class is free to choose the format of the data they want to cache per class anyway (as the implementation of the public API loadMetadataForClass is left to the child classes).
And this method should probably have been defined as protected instead.

{
if ($this->classCache === null) {
$this->initialize();
Expand Down Expand Up @@ -139,7 +139,7 @@
* @return ClassMetadata[]
* @psalm-return array<class-string, ClassMetadata<object>>
*/
abstract protected function loadMappingFile(string $file): array;
abstract protected function loadMappingFile(string $file);

Check failure on line 142 in src/Persistence/Mapping/Driver/FileDriver.php

View workflow job for this annotation

GitHub Actions / Coding Standards / Coding Standards (8.3) 

Method \Doctrine\Persistence\Mapping\Driver\FileDriver::loadMappingFile() does not have native return type hint for its return value but it should be possible to add it based on @return annotation "ClassMetadata[]".
Copy link
Member Author

Choose a reason for hiding this comment

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

The type declaration was simply not updated in ORM 3

Copy link
Member

Choose a reason for hiding this comment

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

does the ORM need to return something else than an array here ?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it's just that the return type declaration was not added in ORM 3 (it was forgotten)


/**
* Initializes the class cache from all the global files.
Expand Down
2 changes: 1 addition & 1 deletion src/Persistence/Proxy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@
*
* Acts as a no-op if already initialized.
*/
public function __load(): void;
public function __load();

Check failure on line 29 in src/Persistence/Proxy.php

View workflow job for this annotation

GitHub Actions / Static Analysis / PHPStan (8.3) 

Method Doctrine\Persistence\Proxy::__load() has no return type specified.

Check failure on line 29 in src/Persistence/Proxy.php

View workflow job for this annotation

GitHub Actions / Coding Standards / Coding Standards (8.3) 

Method \Doctrine\Persistence\Proxy::__load() does not have void return type hint.
greg0ire marked this conversation as resolved.
Show resolved Hide resolved

/** Returns whether this proxy is initialized or not. */
public function __isInitialized(): bool;
Expand Down
Loading