WIP: feature_paste #1

Draft
Miroslavsckaya wants to merge 24 commits from feature_paste into master
4 changed files with 148 additions and 111 deletions
Showing only changes of commit ffcfa29968 - Show all commits

View file

@ -1,53 +1,53 @@
<?php <?php
skobkin marked this conversation as resolved
Review

I can't leave you a comment on src/Controller/.gitignore, so I'll ask here.

Why do you need it?

I can't leave you a comment on `src/Controller/.gitignore`, so I'll ask here. Why do you need it?
declare(strict_types = 1); declare(strict_types = 1);
skobkin marked this conversation as resolved Outdated

Please do not forget declare(strict_types=1);

Please do not forget `declare(strict_types=1);`
namespace App\Controller; namespace App\Controller;
use App\Form\Type\PasteForm; use App\Form\Type\PasteForm;
use App\Entity\Paste; use App\Entity\Paste;
use App\Repository\PasteRepository; use App\Repository\PasteRepository;
use Symfony\Bundle\FrameworkBundle\Controller\AbstractController; use Symfony\Bundle\FrameworkBundle\Controller\AbstractController;
use Symfony\Component\Routing\Annotation\Route; use Symfony\Component\Routing\Annotation\Route;
use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpFoundation\Response;
use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Request;
class PasteController extends AbstractController class PasteController extends AbstractController
{ {
#[Route('/')] #[Route('/')]
Review

I'd suggest explicitly defining which request methods we're processing here.

P.S. We'll move this to YAML in the end.

I'd suggest explicitly defining which request methods we're processing here. P.S. We'll move this to YAML in the end.
public function new(Request $request, PasteRepository $pasteRepository): Response { public function new(Request $request, PasteRepository $pasteRepository): Response
skobkin marked this conversation as resolved Outdated

You don't really need EntityManager here.

Make your repository implement save like I showed you and use it instead.

You don't really need `EntityManager` here. Make your repository implement `save` like I showed you and use it instead.
$paste = new Paste(); {
skobkin marked this conversation as resolved Outdated

I'd suggest we refactor this to to DTO usage and make our Paste entity immutable.

I'd suggest we refactor this to to DTO usage and make our `Paste` entity immutable.
$form = $this->createForm(PasteForm::class, $paste); $paste = new Paste();
$form = $this->createForm(PasteForm::class, $paste);
$form->handleRequest($request); $form->handleRequest($request);
if ($form->isSubmitted() && $form->isValid()) { if ($form->isSubmitted() && $form->isValid()) {
$paste = $form->getData(); $paste = $form->getData();
$paste->setIp($request->getClientIp()); $paste->setIp($request->getClientIp());
skobkin marked this conversation as resolved Outdated

You can just set it in the constructor.

You can just set it in the constructor.
$paste->setPublishDate(new \DateTime()); $paste->setPublishDate(new \DateTime());
skobkin marked this conversation as resolved Outdated

Do you need IP in PasteData?

Do you need IP in `PasteData`?
if ($paste->isPrivate()) { if ($paste->isPrivate()) {
skobkin marked this conversation as resolved Outdated

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

This still could be done in the entity itself.

This still could be done in the entity itself.
$paste->setSecret(hash('sha1', random_bytes(25))); $paste->setSecret(hash('sha1', random_bytes(25)));
} }
$pasteRepository->save($paste); $pasteRepository->save($paste);
skobkin marked this conversation as resolved Outdated

I still suggest to make Paste::fromFormData().

I still suggest to make `Paste::fromFormData()`.
return $this->redirectToRoute($request->attributes->get('_route')); return $this->redirectToRoute($request->attributes->get('_route'));
skobkin marked this conversation as resolved Outdated

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.

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.
} }
skobkin marked this conversation as resolved Outdated

I see you needed some space 🤔

I see you needed some space 🤔
return $this->render('paste.html.twig', [ return $this->render('paste.html.twig', [
'form' => $form, 'form' => $form,
skobkin marked this conversation as resolved Outdated

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.
]); ]);
Review

show_paste at least.

`show_paste` at least.
} }
skobkin marked this conversation as resolved Outdated
  • 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.
- 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.
#[Route('/{id}/{secret}')] #[Route('/{id}/{secret}')]
public function show_paste(PasteRepository $pasteRepository, Request $request, string $id, ?string $secret=NULL): Response { public function showPaste(PasteRepository $pasteRepository, Request $request, string $id, ?string $secret=NULL): Response
{
skobkin marked this conversation as resolved Outdated

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.
$paste = $pasteRepository->findOneBy(['id' => $id, 'secret' => $secret]); $paste = $pasteRepository->findOneBy(['id' => $id, 'secret' => $secret]);
skobkin marked this conversation as resolved Outdated

Do you need double quotes here too?

Do you need double quotes here too?
$form = $this->createForm(PasteForm::class, $paste); $form = $this->createForm(PasteForm::class, $paste);
return $this->render('paste.html.twig', [ return $this->render('paste.html.twig', [
Review

I'd suggest you implementing Twig extension wih filter and function to use highlighter in the template.

I'd suggest you implementing Twig extension wih filter and function to use highlighter in the template.
'form' => $form, 'form' => $form,
]); ]);
} }
} }

View file

@ -1,12 +1,12 @@
<?php <?php
declare(strict_types = 1); declare(strict_types = 1);
skobkin marked this conversation as resolved Outdated

Please do not forget declare(strict_types=1);

Please do not forget `declare(strict_types=1);`
namespace App\Entity; namespace App\Entity;
use App\Repository\PasteRepository; use App\Repository\PasteRepository;
use Doctrine\DBAL\Types\Types; use Doctrine\DBAL\Types\Types;
use Doctrine\ORM\Mapping as ORM; use Doctrine\ORM\Mapping as ORM;
use Symfony\Component\Validator\Constraints as Assert; use Symfony\Component\Validator\Constraints as Assert;
Review

I'd suggest explicitly defining Table attribute here.

Especially since you're defining Column attributes below ⬇️

I'd suggest explicitly defining `Table` attribute here. Especially since you're defining `Column` attributes below ⬇️
Review

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)] #[ORM\Entity(repositoryClass: PasteRepository::class)]
class Paste class Paste
@ -14,128 +14,150 @@ class Paste
#[ORM\Id] #[ORM\Id]
skobkin marked this conversation as resolved Outdated

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.

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\GeneratedValue] #[ORM\GeneratedValue]
Review

Why not also constructor promotion though?

Why not also constructor promotion though?
#[ORM\Column(nullable: false)] #[ORM\Column(nullable: false)]
private int $id; private int $id;
skobkin marked this conversation as resolved Outdated

No need for double quotes here too.

No need for double quotes here too.
#[ORM\Column(type: 'text', nullable: false)] #[ORM\Column(type: 'text', nullable: false)]
#[Assert\NotBlank] #[Assert\NotBlank]
private string $text; private string $text;
#[ORM\Column(type: 'boolean', nullable: false)] #[ORM\Column(type: 'boolean', nullable: false)]
#[Assert\Type(\boolean::class)] #[Assert\Type(\boolean::class)]
private bool $private; private bool $private;

What type? Why 25 exactly?

What type? Why 25 exactly?
Review

Why 128?

Why 128?
#[ORM\Column(length: 25, nullable: true)] #[ORM\Column(length: 25, nullable: true)]
skobkin marked this conversation as resolved Outdated

Why not nullable?

Why not nullable?
private ?string $language; private ?string $language;
#[ORM\Column(type: 'text', nullable: true)] #[ORM\Column(type: 'text', nullable: true)]
private ?string $description; private ?string $description;
#[ORM\Column(length: 128, nullable: true)] #[ORM\Column(length: 128, nullable: true)]
skobkin marked this conversation as resolved
Review

IPv6?

IPv6?
private ?string $filename; private ?string $filename;
#[ORM\Column(length: 128, nullable: false)] #[ORM\Column(length: 128, nullable: false)]
skobkin marked this conversation as resolved Outdated

Why double quotes?

Why double quotes?
#[Assert\NotBlank] #[Assert\NotBlank]
private string $author = 'anonymous'; private string $author = 'anonymous';
Review

You can use self as return type hint too.

You can use `self` as return type hint too.
#[ORM\Column(type: Types::DATETIME_MUTABLE, nullable: false)] #[ORM\Column(type: Types::DATETIME_MUTABLE, nullable: false)]
skobkin marked this conversation as resolved Outdated

Why mutable date? Do you plan to change publish date?

Why mutable date? Do you plan to change publish date?
#[Assert\Type(\DateTime::class)] #[Assert\Type(\DateTime::class)]
private \DateTime $publishDate; private \DateTime $publishDate;
#[ORM\Column(type: Types::DATETIME_MUTABLE, nullable: true)] #[ORM\Column(type: Types::DATETIME_MUTABLE, nullable: true)]
#[Assert\Type(\DateTime::class)] #[Assert\Type(\DateTime::class)]
private ?\DateTime $expirationDate; private ?\DateTime $expirationDate;
#[ORM\Column(length: 15, nullable: false)] #[ORM\Column(length: 15, nullable: false)]
private string $ip; private string $ip;
#[ORM\Column(length: 40, nullable: true)] #[ORM\Column(length: 40, nullable: true)]
private ?string $secret; private ?string $secret;
skobkin marked this conversation as resolved Outdated

Same code style problem like above. You're leaving { on the same line with return type.

It's ok if you're doing multi-line arguments, but not in this case.

Same code style problem like above. You're leaving `{` on the same line with return type. It's ok if you're doing multi-line arguments, but not in this case.
public function getId(): int { public function getId(): int
return $this->id; {
return $this->id;
skobkin marked this conversation as resolved Outdated

Let this thread be for now, but we'd better make this entity immutable and remove every setter method from here.

This way we can use private readonly instead of just private properties. Or even public readonly 🤔

Let this thread be for now, but we'd better make this entity immutable and remove every setter method from here. This way we can use `private readonly` instead of just `private` properties. Or even `public readonly` 🤔
} }
public function setId(int $id): void { public function setId(int $id): void
$this->id = $id; {
$this->id = $id;
} }
public function getText(): string { public function getText(): string
return $this->text; {
return $this->text;
} }
public function setText(string $text): void { public function setText(string $text): void
$this->text = $text; {
$this->text = $text;
} }
public function getLanguage(): ?string { public function getLanguage(): ?string
return $this->language; {
return $this->language;
} }
public function setLanguage(?string $language): void { public function setLanguage(?string $language): void
$this->language = $language; {
$this->language = $language;
} }
public function getDescription(): ?string { public function getDescription(): ?string
return $this->description; {
return $this->description;
} }
public function setDescription(?string $description): void { public function setDescription(?string $description): void
$this->description = $description; {
$this->description = $description;
} }
public function getFilename(): ?string { public function getFilename(): ?string
return $this->filename; {
return $this->filename;
} }
public function setFilename(?string $filename): void { public function setFilename(?string $filename): void
$this->filename = $filename; {
$this->filename = $filename;
} }
public function getAuthor(): string { public function getAuthor(): string
skobkin marked this conversation as resolved Outdated

You don't need this.

You don't need this.
return $this->author; {
return $this->author;
} }
public function setAuthor(string $author): void { public function setAuthor(string $author): void
$this->author = $author; {
$this->author = $author;
} }
public function getPublishDate(): \DateTime { public function getPublishDate(): \DateTime
return $this->publishDate; {
return $this->publishDate;
} }
public function setPublishDate(\DateTime $date): void { public function setPublishDate(\DateTime $date): void
$this->publishDate = $date; {
$this->publishDate = $date;
} }
public function getExpirationDate(): ?\DateTime { public function getExpirationDate(): ?\DateTime
return $this->expirationDate; {
return $this->expirationDate;
} }
skobkin marked this conversation as resolved Outdated

You don't need this.

You don't need this.
public function setExpirationDate(?\DateTime $date): void { public function setExpirationDate(?\DateTime $date): void
$this->expirationDate = $date; {
$this->expirationDate = $date;
} }
skobkin marked this conversation as resolved Outdated

Do you really need this bool flag?

You can just return $this->secret !== null.

Do you really need this `bool` flag? You can just `return $this->secret !== null`.
public function getIp(): string { public function getIp(): string
return $this->ip; {
return $this->ip;
} }
public function setIP(string $ip): void { public function setIP(string $ip): void
$this->ip = $ip; {
$this->ip = $ip;
} }
public function getSecret(): ?string { public function getSecret(): ?string
return $this->secret; {
return $this->secret;
} }
public function setSecret(?string $secret): void { public function setSecret(?string $secret): void
$this->secret = $secret; {
$this->secret = $secret;
} }
public function isPrivate(): bool { public function isPrivate(): bool
return $this->private; {
return $this->private;
} }
public function setPrivate(bool $private): void { public function setPrivate(bool $private): void
$this->private = $private; {
$this->private = $private;
} }
} }

View file

@ -1,30 +1,43 @@
<?php <?php
declare(strict_types = 1); declare(strict_types = 1);
skobkin marked this conversation as resolved Outdated

Please do not forget declare(strict_types=1);

Please do not forget `declare(strict_types=1);`
namespace App\Form\Type; namespace App\Form\Type;
use Symfony\Component\Form\AbstractType; use Symfony\Component\Form\AbstractType;
use Symfony\Component\Form\FormBuilderInterface; use Symfony\Component\Form\FormBuilderInterface;
use Symfony\Component\Form\Extension\Core\Type\ChoiceType; use Symfony\Component\Form\Extension\Core\Type\ChoiceType;
use Symfony\Component\Form\Extension\Core\Type\TextareaType; use Symfony\Component\Form\Extension\Core\Type\TextareaType;
use Symfony\Component\Form\Extension\Core\Type\TextType; use Symfony\Component\Form\Extension\Core\Type\TextType;
use Symfony\Component\Form\Extension\Core\Type\SubmitType; use Symfony\Component\Form\Extension\Core\Type\SubmitType;
use Symfony\Component\Form\Extension\Core\Type\DateTimeType; use Symfony\Component\Form\Extension\Core\Type\DateTimeType;
use Symfony\Component\Form\Extension\Core\Type\CheckboxType; use Symfony\Component\Form\Extension\Core\Type\CheckboxType;
class PasteForm extends AbstractType class PasteForm extends AbstractType
{ {
public function buildForm(FormBuilderInterface $builder, array $options): void { public function buildForm(FormBuilderInterface $builder, array $options): void
skobkin marked this conversation as resolved Outdated

Double quotes?

Also please multi-line format arrays with more than one element for readability.

Double quotes? Also please multi-line format arrays with more than one element for readability.
{
$builder $builder
->add('language', ChoiceType::class, ['choices' => ['Python' => 'python', 'PHP' => 'php', 'Plain text' => NULL]]) ->add('language', ChoiceType::class, [
skobkin marked this conversation as resolved Outdated

You can probably extract 128 to the constant and use here and in the validation attribute.

You can probably extract `128` to the constant and use here and in the validation attribute.
'choices' => [
'Python' => 'python',
skobkin marked this conversation as resolved Outdated

This should be a combo-box (select in HTML terms) with a list of available periods like "10 minutes", "1 year" and "Never".

This should be a combo-box (`select` in HTML terms) with a list of available periods like "10 minutes", "1 year" and "Never".
'PHP' => 'php',
'Plain text' => NULL,
]
]
)
->add('description', TextType::class, ['required' => false]) ->add('description', TextType::class, ['required' => false])
->add('text', TextareaType::class) ->add('text', TextareaType::class)
->add('author', TextType::class, ['attr' => ['maxlength' =>128]]) ->add('author', TextType::class, ['attr' => ['maxlength' =>128]])
skobkin marked this conversation as resolved Outdated

required => false?

`required => false`?
->add('filename', TextType::class, ['required' => false, 'attr' => ['maxlength' =>128]]) ->add('filename', TextType::class, ['required' => false, 'attr' => ['maxlength' =>128]])
Review

Why 128?

Why 128?
->add('expirationDate', DateTimeType::class, ['required' => false, 'date_widget' => 'single_text', 'input' => 'datetime']) ->add('expirationDate', DateTimeType::class, [
'required' => false,
'date_widget' => 'single_text',
'input' => 'datetime',
]
)
->add('private', CheckboxType::class, ['required' => false]) ->add('private', CheckboxType::class, ['required' => false])
->add('save', SubmitType::class) ->add('save', SubmitType::class)
; ;
} }
} }

View file

@ -1,21 +1,23 @@
<?php <?php
skobkin marked this conversation as resolved
Review

Can't leave a comment for src/Repository/.gitignore, so I'll ask here.

Do you need it?

Can't leave a comment for `src/Repository/.gitignore`, so I'll ask here. Do you need it?
declare(strict_types = 1); declare(strict_types = 1);
skobkin marked this conversation as resolved Outdated

Please do not forget declare(strict_types=1);

Please do not forget `declare(strict_types=1);`
namespace App\Repository; namespace App\Repository;
use App\Entity\Paste; use App\Entity\Paste;
use Doctrine\Bundle\DoctrineBundle\Repository\ServiceEntityRepository; use Doctrine\Bundle\DoctrineBundle\Repository\ServiceEntityRepository;
use Doctrine\Persistence\ManagerRegistry; use Doctrine\Persistence\ManagerRegistry;
class PasteRepository extends ServiceEntityRepository class PasteRepository extends ServiceEntityRepository
{ {
public function __construct(ManagerRegistry $registry) { public function __construct(ManagerRegistry $registry)
parent::__construct($registry, Paste::class); {
skobkin marked this conversation as resolved Outdated

I'd suggest adding save() here like I showed you.

I'd suggest adding `save()` here like I showed you.

Better with optional flush.
Flushng from the repo itself should be a rare action.

Better with optional `flush`. Flushng from the repo itself should be a rare action.
parent::__construct($registry, Paste::class);
} }
public function save(Paste $paste): void { public function save(Paste $paste): void
skobkin marked this conversation as resolved Outdated

Do not forget to use code formatting. It'd surrounded = with spaces for you.

Do not forget to use code formatting. It'd surrounded `=` with spaces for you.
$entityManager = $this->getEntityManager(); {
$entityManager->persist($paste); $entityManager = $this->getEntityManager();
$entityManager->flush(); $entityManager->persist($paste);
$entityManager->flush();
skobkin marked this conversation as resolved Outdated

You shouldn't do flush here by default. Only in certain cases if needed.

You shouldn't do flush here by default. Only in certain cases if needed.
} }
} }