Miroslavsckaya/copypaste
1
0
Fork 0
Code Issues 1 Pull requests 1 Packages Projects Releases Wiki Activity

WIP: feature_paste #1

Draft
Miroslavsckaya wants to merge 24 commits from feature_paste into master
pull from: feature_paste
merge into: Miroslavsckaya:master
Conversation 0 Commits 24 Files changed 31 +30 -113
2 changed files with 30 additions and 113 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit 21d651f377 - Show all commits

19
src/Controller/PasteController.php
View file

@ -26,18 +26,18 @@ class PasteController extends AbstractController
$pasteData = $form->getData();
skobkin marked this conversation as resolved Outdated
skobkin commented 2023-07-20 18:09:30 +00:00
Outdated
Review
Copy link

You can just set it in the constructor.

You can just set it in the constructor.
👍 1
skobkin marked this conversation as resolved Outdated
skobkin commented 2023-07-22 17:48:11 +00:00
Outdated
Review
Copy link

Do you need IP in PasteData?

Do you need IP in `PasteData`?
👍 1
$secret = null;
if ($pasteData->isPrivate()) {
if ($pasteData->private) {
skobkin marked this conversation as resolved Outdated
skobkin commented 2023-07-20 18:09:54 +00:00
Outdated
Review
Copy link

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.
👍 1
skobkin marked this conversation as resolved Outdated
skobkin commented 2023-07-21 22:38:05 +00:00
Outdated
Review
Copy link

This still could be done in the entity itself.

This still could be done in the entity itself.
👍 1
$secret = hash('sha1', random_bytes(25));
}
$paste = new Paste(
skobkin marked this conversation as resolved Outdated
skobkin commented 2023-07-21 22:29:02 +00:00
Outdated
Review
Copy link

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

I still suggest to make `Paste::fromFormData()`.
👍 1
$pasteData->getText(),
$pasteData->getLanguage(),
$pasteData->getDescription(),
$pasteData->getFilename(),
$pasteData->getAuthor(),
$pasteData->text,
$pasteData->language,
skobkin marked this conversation as resolved Outdated
skobkin commented 2023-07-20 18:12:15 +00:00
Outdated
Review
Copy link

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.
👍 1
$pasteData->description,
skobkin marked this conversation as resolved Outdated
skobkin commented 2023-07-20 18:11:14 +00:00
Outdated
Review
Copy link

I see you needed some space 🤔

I see you needed some space 🤔
👍 1
$pasteData->filename,
$pasteData->author,
new \DateTimeImmutable(),
skobkin marked this conversation as resolved Outdated
skobkin commented 2023-07-21 22:24:36 +00:00
Outdated
Review
Copy link

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.
👍 1
$pasteData->getExpirationDate(),
$pasteData->expirationDate,
skobkin commented 2023-08-06 11:03:34 +00:00
Review
Copy link

show_paste at least.

`show_paste` at least.
$request->getClientIp(),
$secret
skobkin marked this conversation as resolved Outdated
skobkin commented 2023-07-21 22:40:57 +00:00
Outdated
Review
Copy link
  • 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.
👍 1
);
@ -52,10 +52,11 @@ class PasteController extends AbstractController
}
#[Route('/{id}/{secret}')]
public function showPaste(PasteRepository $pasteRepository, Request $request, string $id, ?string $secret=NULL): Response
public function showPaste(PasteRepository $pasteRepository, string $id, ?string $secret=NULL): Response
{
$paste = $pasteRepository->findOneBy(['id' => $id, 'secret' => $secret]);
$form = $this->createForm(PasteForm::class, $paste);
$pasteData = new PasteFormData($paste);
$form = $this->createForm(PasteForm::class, $pasteData);
return $this->render('paste.html.twig', [
'form' => $form,

120
src/DTO/PasteFormData.php
View file

@ -3,120 +3,36 @@ declare(strict_types = 1);
namespace App\DTO;
use App\Entity\Paste;
use Symfony\Component\Validator\Constraints as Assert;
class PasteFormData
{
private int $id;
#[Assert\NotBlank]
private string $text;
public string $text;
#[Assert\Type(\boolean::class)]
skobkin marked this conversation as resolved Outdated
skobkin commented 2023-07-21 22:36:05 +00:00
Outdated
Review
Copy link

Huh?

Huh?
👍 1
private bool $private;
private ?string $language = null;
private ?string $description = null;
private ?string $filename = null;
public bool $private;
public ?string $language = null;
public ?string $description = null;
public ?string $filename = null;
#[Assert\NotBlank]
private string $author = 'anonymous';
public string $author = 'anonymous';
skobkin marked this conversation as resolved
skobkin commented 2023-07-21 22:36:37 +00:00
Review
Copy link

Why string and Assert\NotBlank?

Why `string` and `Assert\NotBlank`?
👍 1
#[Assert\Type(\DateTimeImmutable::class)]
private ?\DateTimeImmutable $expirationDate;
public ?\DateTimeImmutable $expirationDate;
skobkin marked this conversation as resolved
skobkin commented 2023-07-22 18:00:09 +00:00
Review
Copy link

Is this validation being processed BEFORE storing the data in the DTO?
If not, it's pointless as with bool field earlier.

Is this validation being processed BEFORE storing the data in the DTO? If not, it's pointless as with `bool` field earlier.
👍 1
private ?string $secret;
public function getId(): int
public function __construct(?Paste $paste=null)
skobkin marked this conversation as resolved
skobkin commented 2023-07-21 22:33:47 +00:00
Review
Copy link

Or you can make constructor private, use property promotion and add fromPaste() method to create it from the entity.

Or you can make constructor `private`, use property promotion and add `fromPaste()` method to create it from the entity.
👍 1
{
return $this->id;
}
public function setId(int $id): void
if ($paste === null)
{
$this->id = $id;
return;
}
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;
$this->text = $paste->text;
$this->private = $paste->secret !== null;
$this->language = $paste->language;
$this->description = $paste->description;
$this->filename = $paste->filename;
$this->author = $paste->author;
$this->expirationDate = $paste->expirationDate;
}
}