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 +21 -19
3 changed files with 21 additions and 19 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit c164ed446b - Show all commits

18
src/Controller/PasteController.php
View file

@ -24,23 +24,9 @@ class PasteController extends AbstractController
$form->handleRequest($request);
if ($form->isSubmitted() && $form->isValid()) {
$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
$pasteData->ip = $request->getClientIp();
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->private) {
$secret = hash('sha1', random_bytes(25));
}
$paste = new Paste(
$pasteData->text,
$pasteData->language,
$pasteData->description,
$pasteData->filename,
$pasteData->author,
new \DateTimeImmutable(),
$pasteData->expirationDate,
$request->getClientIp(),
$secret
);
$paste = Paste::fromFormData($pasteData);
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
$pasteRepository->save($paste);
return $this->redirectToRoute($request->attributes->get('_route'));

1
src/DTO/PasteFormData.php
View file

@ -20,6 +20,7 @@ class PasteFormData
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)]
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
public string $ip;
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
public function __construct(?Paste $paste=null)
{

21
src/Entity/Paste.php
View file

@ -2,7 +2,7 @@
declare(strict_types = 1);
skobkin marked this conversation as resolved Outdated
skobkin commented 2023-07-20 18:30:23 +00:00
Outdated
Review
Copy link

Please do not forget declare(strict_types=1);

Please do not forget `declare(strict_types=1);`
👍 1
namespace App\Entity;
use App\DTO\PasteFormData;
use App\Repository\PasteRepository;
use Doctrine\DBAL\Types\Types;
use Doctrine\ORM\Mapping as ORM;
@ -15,7 +15,7 @@ class Paste
#[ORM\Column]
skobkin commented 2023-07-22 18:02:29 +00:00
Review
Copy link

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 2023-07-20 18:20:17 +00:00
Outdated
Review
Copy link

No need for double quotes here too.

No need for double quotes here too.
👍 1
public function __construct(
private function __construct(
#[ORM\Column(type: 'text', nullable: false)]
public readonly string $text,
#[ORM\Column(length: 25, nullable: true)]
@ -35,4 +35,19 @@ class Paste
#[ORM\Column(length: 40, nullable: true)]
public readonly ?string $secret,
skobkin marked this conversation as resolved Outdated
skobkin commented 2023-07-20 18:22:04 +00:00
Outdated
Review
Copy link

Why double quotes?

Why double quotes?
👍 1
) {}
}
skobkin commented 2023-08-06 11:17:44 +00:00
Review
Copy link

You can use self as return type hint too.

You can use `self` as return type hint too.
public static function fromFormData(PasteFormData $pasteFormData): Paste
{
skobkin marked this conversation as resolved Outdated
skobkin commented 2023-07-20 18:22:53 +00:00
Outdated
Review
Copy link

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

Why mutable date? Do you plan to change publish date?
👍 1
return new self(
$pasteFormData->text,
$pasteFormData->language,
$pasteFormData->description,
$pasteFormData->filename,
$pasteFormData->author,
new \DateTimeImmutable(),
$pasteFormData->expirationDate,
$pasteFormData->ip,
$pasteFormData->private ? \hash('sha1', \random_bytes(25)) : null,
);
}
skobkin marked this conversation as resolved Outdated
skobkin commented 2023-07-20 18:27:40 +00:00
Outdated
Review
Copy link

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.
👍 1
}