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.

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

Rector code cleanup #321

Draft
wants to merge 26 commits into
base: develop
Choose a base branch
from
Draft

Rector code cleanup #321

wants to merge 26 commits into from

Conversation

rimi-itk
Copy link
Contributor

@rimi-itk rimi-itk commented Feb 22, 2023

Uses Rector to clean up code.

Some annotations are still present.

A commit per change type has been made.

  • Added Rector
  • Updated constructor property promotion
  • get_class → ::class
  • Updated constructor property promotion
  • catch exceptions only by type
  • Added JSON_THROW_ON_ERROR
  • Arrow functions
  • catch exceptions only by type
  • final const
  • Cleaned up
  • Implemented Stringable
  • Added property types
  • Set property default values
  • Changed switch statements to match expressions
  • Made sure that strings are strings
  • Changed pow to **
  • Cleaned up
  • Declared variables
  • Cleaned up
  • Added property types
  • Cast to string
  • First class callable syntax
  • Fixed lists
  • Converted annotations to attributes
  • Rectorified tests/
  • Applied coding standards

Copy link
Collaborator

@jekuaitk jekuaitk left a comment

Choose a reason for hiding this comment

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

It seems rector has done a lot of good, but on some occasions made our setup not function correctly.

Comment on lines +20 to +22
#[ORM\ManyToOne(targetEntity: 'CaseEntity', inversedBy: 'caseDocumentRelation')]
#[ORM\JoinColumn(nullable: false)]
private ?\App\Entity\CaseEntity $case = null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know all the changes is done by rector, but should we consider removing the ? and = null when the properties are not nullable?

{
return $this->caseNumber;
return (string) $this->caseNumber;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I dont think this type-casting is necessary, but i see why rector has done it. caseNumber is not nullable, but when we use make:entity it still creates the getCaseNumber method with return value ?string. However, it does no harm so lets just keep it.

@@ -44,9 +35,9 @@ public function setType(?string $type): self
return $this;
}

public function getIdentifier(): ?string
public function getIdentifier(): string
{
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does it make this change? identifier is nullable so why the change to empty string?


#[ORM\Column(type: 'string', length: 255)]
#[Groups(['mail_template'])]
private ?string $title = null;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Another case of a not nullable property being given type ? and set to null by default. Maybe rector doesn't recognize that properties are not nullable by default? Or perhaps it takes it from the getter which also incorrectly has the return type ?string?

Comment on lines +36 to +37
#[ORM\Column(type: 'string', length: 255)]
private string $templateFilename;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This one seems to be handled differently (but correct!) compared to other properties, so it seems the @var string ensures correctly processing by rector?

@@ -104,7 +104,7 @@ public function getNextRid()
foreach ($this->xmlRelationships as $xmlRelationship) {
$attributes = $xmlRelationship->attributes();
if (isset($attributes['Id'])) {
if ('rId' === substr($attributes['Id'], 0, 3)) {
if (str_starts_with($attributes['Id'], 'rId')) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

private $mockMunicipalityRepository;
private $propertyAccessor;
private $documentUploader;
private \App\Repository\BoardRepository & \PHPUnit\Framework\MockObject\MockObject $mockBoardRepository;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks funky.

*/
private $assignedCases;
#[ORM\OneToMany(targetEntity: CaseEntity::class, mappedBy: 'assignedTo')]
private \Doctrine\Common\Collections\ArrayCollection|array $assignedCases;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will fail if we try loading fixtures. It should probably say \Doctrine\Common\Collections\Collection rather than \Doctrine\Common\Collections\ArrayCollection|array.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants