improve PHP 8 support w/ rector, removes legacy code, deprecates unused methods#1128
Conversation
jrushlow
changed the title
| try { | ||
| $metadata = $this->doctrineHelper->getMetadata($classOrNamespace); | ||
| } catch (MappingException|LegacyCommonMappingException|PersistenceMappingException $mappingException) { | ||
| } catch (MappingException|LegacyCommonMappingException|PersistenceMappingException) { |
There was a problem hiding this comment.
Doctrine\Common namespaced imports e.g. LegacyCommonMappingException will be removed in a separate PR as there are a lot of changes related to outdated legacy classes that need to be made based on a git grep
| // @legacy @todo - refactor for better naming conventions BEFORE next release. | ||
| $useAttributesForDoctrineMapping = $userClassConfiguration->isEntity() && ($this->doctrineHelper->isDoctrineSupportingAttributes()) && $this->doctrineHelper->doesClassUsesAttributes($userClassNameDetails->getFullName()); | ||
| $entityUsesAttributes = ($isEntity = $userClassConfiguration->isEntity()) && $this->doctrineHelper->doesClassUsesAttributes($userClassNameDetails->getFullName()); | ||
|
|
||
| if ($isEntity && !$entityUsesAttributes) { | ||
| throw new \RuntimeException('MakeUser only supports attribute mapping with doctrine entities.'); | ||
| } | ||
|
|
||
| // B) Implement UserInterface | ||
| $manipulator = new ClassSourceManipulator( | ||
| sourceCode: $this->fileManager->getFileContents($classPath), | ||
| overwrite: true, | ||
| useAttributesForDoctrineMapping: $useAttributesForDoctrineMapping | ||
| useAttributesForDoctrineMapping: $entityUsesAttributes |
There was a problem hiding this comment.
For make user we can gen a user that is not an entity - in which case we don't add any attributes to the generated class.. But if the user is an entity, we only generate it using attributes, otherwise we complain. Is my thinking correct on this?
There was a problem hiding this comment.
Yes. This is correct :)
| // @legacy @todo - refactor for better naming conventions BEFORE next release. | ||
| $useAttributesForDoctrineMapping = $userClassConfiguration->isEntity() && ($this->doctrineHelper->isDoctrineSupportingAttributes()) && $this->doctrineHelper->doesClassUsesAttributes($userClassNameDetails->getFullName()); | ||
| $entityUsesAttributes = ($isEntity = $userClassConfiguration->isEntity()) && $this->doctrineHelper->doesClassUsesAttributes($userClassNameDetails->getFullName()); | ||
|
|
||
| if ($isEntity && !$entityUsesAttributes) { | ||
| throw new \RuntimeException('MakeUser only supports attribute mapping with doctrine entities.'); | ||
| } | ||
|
|
||
| // B) Implement UserInterface | ||
| $manipulator = new ClassSourceManipulator( | ||
| sourceCode: $this->fileManager->getFileContents($classPath), | ||
| overwrite: true, | ||
| useAttributesForDoctrineMapping: $useAttributesForDoctrineMapping | ||
| useAttributesForDoctrineMapping: $entityUsesAttributes |
There was a problem hiding this comment.
Yes. This is correct :)
| * @internal | ||
| */ | ||
| final class ClassNameValue | ||
| final class ClassNameValue implements \Stringable |
There was a problem hiding this comment.
Do you need to implement this explicitly? I thought that any classes with __toString() automatically get the Stringable interface. I only see implements \Stringable 3 times in symfony/symfony, which leads me to believe that we generally don't add this.
There was a problem hiding this comment.
This was one of the rector "auto fix it up" jobber dooers. Looking at the PHP Docs:
Unlike most interfaces, Stringable is implicitly present on any class that has the magic __toString() method defined, although it can and should be declared explicitly.
It's not really necessary per the docs other than they we it should be declared explicitly. Should we remove this?
There was a problem hiding this comment.
although it can and should be declared explicitly
Ok, fair enough - we can keep it :)
|
Thanks Jesse - huge cleanup! |
2 participants
make:userfinalorinternalsrcup to PHP 8 standards