WIP: feature_paste #1
|
@ -3,8 +3,9 @@ declare(strict_types = 1);
|
|||
|
||||
namespace App\Controller;
|
||||
|
||||
use App\Form\Type\PasteForm;
|
||||
use App\DTO\PasteFormData;
|
||||
use App\Entity\Paste;
|
||||
use App\Form\Type\PasteForm;
|
||||
use App\Repository\PasteRepository;
|
||||
use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
|
||||
use Symfony\Component\Routing\Annotation\Route;
|
||||
|
@ -17,19 +18,29 @@ class PasteController extends AbstractController
|
|||
#[Route('/')]
|
||||
skobkin marked this conversation as resolved
Outdated
|
||||
public function new(Request $request, PasteRepository $pasteRepository): Response
|
||||
skobkin marked this conversation as resolved
Outdated
skobkin
commented
I'd suggest we refactor this to to DTO usage and make our I'd suggest we refactor this to to DTO usage and make our `Paste` entity immutable.
|
||||
{
|
||||
$paste = new Paste();
|
||||
$form = $this->createForm(PasteForm::class, $paste);
|
||||
$pasteData = new PasteFormData();
|
||||
$form = $this->createForm(PasteForm::class, $pasteData);
|
||||
|
||||
$form->handleRequest($request);
|
||||
if ($form->isSubmitted() && $form->isValid()) {
|
||||
$paste = $form->getData();
|
||||
$paste->setIp($request->getClientIp());
|
||||
$paste->setPublishDate(new \DateTime());
|
||||
$pasteData = $form->getData();
|
||||
skobkin marked this conversation as resolved
Outdated
skobkin
commented
You can just set it in the constructor. You can just set it in the constructor.
|
||||
|
||||
skobkin marked this conversation as resolved
Outdated
skobkin
commented
Do you need IP in Do you need IP in `PasteData`?
|
||||
if ($paste->isPrivate()) {
|
||||
$paste->setSecret(hash('sha1', random_bytes(25)));
|
||||
$secret = null;
|
||||
if ($pasteData->isPrivate()) {
|
||||
skobkin marked this conversation as resolved
Outdated
skobkin
commented
Do we need double quotes here? By the way, we can also do that in the constructor. Do we need double quotes here?
By the way, we can also do that in the constructor.
skobkin marked this conversation as resolved
Outdated
skobkin
commented
This still could be done in the entity itself. This still could be done in the entity itself.
|
||||
$secret = hash('sha1', random_bytes(25));
|
||||
}
|
||||
|
||||
$paste = new Paste(
|
||||
skobkin marked this conversation as resolved
Outdated
skobkin
commented
I still suggest to make I still suggest to make `Paste::fromFormData()`.
|
||||
$pasteData->getText(),
|
||||
$pasteData->getLanguage(),
|
||||
skobkin marked this conversation as resolved
Outdated
skobkin
commented
Just set a BTW, where are you redirecting exactly? 🤔 You should be redirecting to the route which is processed by Just set a `name` for your route and you won't need such workarounds.
BTW, where are you redirecting exactly? 🤔 You should be redirecting to the route which is processed by `PasteController::show_paste()` as far as I understand your code.
|
||||
$pasteData->getDescription(),
|
||||
skobkin marked this conversation as resolved
Outdated
skobkin
commented
I see you needed some space 🤔 I see you needed some space 🤔
|
||||
$pasteData->getFilename(),
|
||||
$pasteData->getAuthor(),
|
||||
new \DateTimeImmutable(),
|
||||
skobkin marked this conversation as resolved
Outdated
skobkin
commented
I mean you don't need to pass creation date here at all. I mean you don't need to pass creation date here at all.
|
||||
$pasteData->getExpirationDate(),
|
||||
skobkin
commented
`show_paste` at least.
|
||||
$request->getClientIp(),
|
||||
$secret
|
||||
skobkin marked this conversation as resolved
Outdated
skobkin
commented
- it's a good idea to add `,` to the last argument too if you use multi-line formatting
- if you're doing this in the controller, you can just do something like `$pasteData->private ? \hash('sha1', \random_bytes(25)) : null`. But I suggest to do that in the static method (named constructor) as I also suggested nearby.
|
||||
);
|
||||
$pasteRepository->save($paste);
|
||||
|
||||
skobkin marked this conversation as resolved
Outdated
skobkin
commented
You're violating Symfony code style here which as I can guess you used above. You can just set Symfony style in the settings of your IDE for this project and auto-format this file to fix. Or fix it manually if you want. You're violating [Symfony code style](https://symfony.com/doc/current/contributing/code/standards.html#symfony-coding-standards-in-detail) here which as I can guess you used above.
You can just set Symfony style in the settings of your IDE for this project and auto-format this file to fix. Or fix it manually if you want.
|
||||
return $this->redirectToRoute($request->attributes->get('_route'));
|
||||
skobkin marked this conversation as resolved
Outdated
skobkin
commented
Do you need double quotes here too? Do you need double quotes here too?
|
||||
|
|
122
src/DTO/PasteFormData.php
Normal file
|
@ -0,0 +1,122 @@
|
|||
<?php
|
||||
declare(strict_types = 1);
|
||||
|
||||
namespace App\DTO;
|
||||
|
||||
use Symfony\Component\Validator\Constraints as Assert;
|
||||
|
||||
|
||||
class PasteFormData
|
||||
{
|
||||
private int $id;
|
||||
|
||||
#[Assert\NotBlank]
|
||||
private string $text;
|
||||
skobkin marked this conversation as resolved
Outdated
|
||||
|
||||
#[Assert\Type(\boolean::class)]
|
||||
private bool $private;
|
||||
|
||||
private ?string $language = null;
|
||||
|
||||
skobkin marked this conversation as resolved
skobkin
commented
Why Why `string` and `Assert\NotBlank`?
|
||||
private ?string $description = null;
|
||||
|
||||
skobkin marked this conversation as resolved
skobkin
commented
Is this validation being processed BEFORE storing the data in the DTO? Is this validation being processed BEFORE storing the data in the DTO?
If not, it's pointless as with `bool` field earlier.
|
||||
private ?string $filename = null;
|
||||
|
||||
skobkin marked this conversation as resolved
skobkin
commented
Or you can make constructor Or you can make constructor `private`, use property promotion and add `fromPaste()` method to create it from the entity.
|
||||
#[Assert\NotBlank]
|
||||
private string $author = 'anonymous';
|
||||
|
||||
#[Assert\Type(\DateTimeImmutable::class)]
|
||||
private ?\DateTimeImmutable $expirationDate;
|
||||
|
||||
private ?string $secret;
|
||||
|
||||
public function getId(): int
|
||||
{
|
||||
return $this->id;
|
||||
}
|
||||
|
||||
public function setId(int $id): void
|
||||
{
|
||||
$this->id = $id;
|
||||
}
|
||||
|
||||
public function getText(): string
|
||||
{
|
||||
return $this->text;
|
||||
}
|
||||
|
||||
public function setText(string $text): void
|
||||
{
|
||||
$this->text = $text;
|
||||
}
|
||||
|
||||
public function getLanguage(): ?string
|
||||
{
|
||||
return $this->language;
|
||||
}
|
||||
|
||||
public function setLanguage(?string $language): void
|
||||
{
|
||||
$this->language = $language;
|
||||
}
|
||||
|
||||
public function getDescription(): ?string
|
||||
{
|
||||
return $this->description;
|
||||
}
|
||||
|
||||
public function setDescription(?string $description): void
|
||||
{
|
||||
$this->description = $description;
|
||||
}
|
||||
|
||||
public function getFilename(): ?string
|
||||
{
|
||||
return $this->filename;
|
||||
}
|
||||
|
||||
public function setFilename(?string $filename): void
|
||||
{
|
||||
$this->filename = $filename;
|
||||
}
|
||||
|
||||
public function getAuthor(): string
|
||||
{
|
||||
return $this->author;
|
||||
}
|
||||
|
||||
public function setAuthor(string $author): void
|
||||
{
|
||||
$this->author = $author;
|
||||
}
|
||||
|
||||
public function getExpirationDate(): ?\DateTime
|
||||
{
|
||||
return $this->expirationDate;
|
||||
}
|
||||
|
||||
public function setExpirationDate(?\DateTime $date): void
|
||||
{
|
||||
$this->expirationDate = $date;
|
||||
}
|
||||
|
||||
public function getSecret(): ?string
|
||||
{
|
||||
return $this->secret;
|
||||
}
|
||||
|
||||
public function setSecret(?string $secret): void
|
||||
{
|
||||
$this->secret = $secret;
|
||||
}
|
||||
|
||||
public function isPrivate(): bool
|
||||
{
|
||||
return $this->private;
|
||||
}
|
||||
|
||||
public function setPrivate(bool $private): void
|
||||
{
|
||||
$this->private = $private;
|
||||
}
|
||||
}
|
|
@ -6,158 +6,33 @@ namespace App\Entity;
|
|||
use App\Repository\PasteRepository;
|
||||
use Doctrine\DBAL\Types\Types;
|
||||
use Doctrine\ORM\Mapping as ORM;
|
||||
use Symfony\Component\Validator\Constraints as Assert;
|
||||
|
||||
skobkin
commented
I'd suggest explicitly defining Especially since you're defining I'd suggest explicitly defining `Table` attribute here.
Especially since you're defining `Column` attributes below ⬇️
skobkin
commented
I'd also suggest to make this entity readonly. It'll give some performance benefits. You can read about that here. I'd also suggest to make this entity readonly. It'll give some performance benefits.
You can read about that [here](https://www.doctrine-project.org/projects/doctrine-orm/en/2.15/reference/attributes-reference.html#attrref_entity).
|
||||
#[ORM\Entity(repositoryClass: PasteRepository::class)]
|
||||
class Paste
|
||||
{
|
||||
#[ORM\Id]
|
||||
#[ORM\GeneratedValue]
|
||||
skobkin marked this conversation as resolved
Outdated
skobkin
commented
I don't think you need But I'd suggest to explicitly set it for every other I don't think you need `nullable=false` here since it's a Primary Key.
But I'd suggest to explicitly set it for every other `Column` and also set a `name` for them.
|
||||
#[ORM\Column(nullable: false)]
|
||||
private int $id;
|
||||
#[ORM\Column]
|
||||
skobkin
commented
Why not also constructor promotion though? Why not also constructor promotion though?
|
||||
public readonly int $id;
|
||||
|
||||
skobkin marked this conversation as resolved
Outdated
skobkin
commented
No need for double quotes here too. No need for double quotes here too.
|
||||
#[ORM\Column(type: 'text', nullable: false)]
|
||||
#[Assert\NotBlank]
|
||||
private string $text;
|
||||
|
||||
#[ORM\Column(type: 'boolean', nullable: false)]
|
||||
#[Assert\Type(\boolean::class)]
|
||||
private bool $private;
|
||||
|
||||
#[ORM\Column(length: 25, nullable: true)]
|
||||
private ?string $language;
|
||||
|
||||
#[ORM\Column(type: 'text', nullable: true)]
|
||||
private ?string $description;
|
||||
|
||||
#[ORM\Column(length: 128, nullable: true)]
|
||||
private ?string $filename;
|
||||
|
||||
#[ORM\Column(length: 128, nullable: false)]
|
||||
#[Assert\NotBlank]
|
||||
private string $author = 'anonymous';
|
||||
|
||||
#[ORM\Column(type: Types::DATETIME_MUTABLE, nullable: false)]
|
||||
#[Assert\Type(\DateTime::class)]
|
||||
private \DateTime $publishDate;
|
||||
|
||||
#[ORM\Column(type: Types::DATETIME_MUTABLE, nullable: true)]
|
||||
#[Assert\Type(\DateTime::class)]
|
||||
private ?\DateTime $expirationDate;
|
||||
|
||||
#[ORM\Column(length: 15, nullable: false)]
|
||||
private string $ip;
|
||||
|
||||
#[ORM\Column(length: 40, nullable: true)]
|
||||
private ?string $secret;
|
||||
|
||||
public function getId(): int
|
||||
{
|
||||
return $this->id;
|
||||
}
|
||||
|
||||
public function setId(int $id): void
|
||||
{
|
||||
$this->id = $id;
|
||||
}
|
||||
|
||||
public function getText(): string
|
||||
{
|
||||
return $this->text;
|
||||
}
|
||||
|
||||
public function setText(string $text): void
|
||||
{
|
||||
$this->text = $text;
|
||||
}
|
||||
|
||||
public function getLanguage(): ?string
|
||||
{
|
||||
return $this->language;
|
||||
}
|
||||
|
||||
public function setLanguage(?string $language): void
|
||||
{
|
||||
$this->language = $language;
|
||||
}
|
||||
|
||||
public function getDescription(): ?string
|
||||
{
|
||||
return $this->description;
|
||||
}
|
||||
|
||||
public function setDescription(?string $description): void
|
||||
{
|
||||
$this->description = $description;
|
||||
}
|
||||
|
||||
public function getFilename(): ?string
|
||||
{
|
||||
return $this->filename;
|
||||
}
|
||||
|
||||
public function setFilename(?string $filename): void
|
||||
{
|
||||
$this->filename = $filename;
|
||||
}
|
||||
|
||||
public function getAuthor(): string
|
||||
{
|
||||
return $this->author;
|
||||
}
|
||||
|
||||
public function setAuthor(string $author): void
|
||||
{
|
||||
$this->author = $author;
|
||||
}
|
||||
|
||||
public function getPublishDate(): \DateTime
|
||||
{
|
||||
return $this->publishDate;
|
||||
}
|
||||
|
||||
public function setPublishDate(\DateTime $date): void
|
||||
{
|
||||
$this->publishDate = $date;
|
||||
}
|
||||
|
||||
public function getExpirationDate(): ?\DateTime
|
||||
{
|
||||
return $this->expirationDate;
|
||||
}
|
||||
|
||||
public function setExpirationDate(?\DateTime $date): void
|
||||
{
|
||||
$this->expirationDate = $date;
|
||||
}
|
||||
|
||||
public function getIp(): string
|
||||
{
|
||||
return $this->ip;
|
||||
}
|
||||
|
||||
public function setIP(string $ip): void
|
||||
{
|
||||
$this->ip = $ip;
|
||||
}
|
||||
|
||||
public function getSecret(): ?string
|
||||
{
|
||||
return $this->secret;
|
||||
}
|
||||
|
||||
public function setSecret(?string $secret): void
|
||||
{
|
||||
$this->secret = $secret;
|
||||
}
|
||||
|
||||
public function isPrivate(): bool
|
||||
{
|
||||
return $this->private;
|
||||
}
|
||||
|
||||
public function setPrivate(bool $private): void
|
||||
{
|
||||
$this->private = $private;
|
||||
}
|
||||
public function __construct(
|
||||
#[ORM\Column(type: 'text', nullable: false)]
|
||||
public readonly string $text,
|
||||
#[ORM\Column(length: 25, nullable: true)]
|
||||
public readonly ?string $language,
|
||||
#[ORM\Column(type: 'text', nullable: true)]
|
||||
public readonly ?string $description,
|
||||
#[ORM\Column(length: 128, nullable: true)]
|
||||
skobkin
commented
What type? Why 25 exactly? What type? Why 25 exactly?
skobkin
commented
Why 128? Why 128?
|
||||
public readonly ?string $filename,
|
||||
#[ORM\Column(length: 128, nullable: false)]
|
||||
skobkin marked this conversation as resolved
Outdated
skobkin
commented
Why not nullable? Why not nullable?
|
||||
public readonly string $author,
|
||||
#[ORM\Column(type: Types::DATETIME_IMMUTABLE, nullable: false)]
|
||||
public readonly \DateTimeImmutable $publishDate,
|
||||
#[ORM\Column(type: Types::DATETIME_IMMUTABLE, nullable: true)]
|
||||
public readonly ?\DateTimeImmutable $expirationDate,
|
||||
#[ORM\Column(length: 15, nullable: false)]
|
||||
public readonly string $ip,
|
||||
#[ORM\Column(length: 40, nullable: true)]
|
||||
public readonly ?string $secret,
|
||||
skobkin marked this conversation as resolved
Outdated
skobkin
commented
Why double quotes? Why double quotes?
|
||||
) {}
|
||||
}
|
||||
skobkin
commented
You can use You can use `self` as return type hint too.
|
You don't really need
EntityManager
here.Make your repository implement
save
like I showed you and use it instead.