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

6
src/Controller/PasteController.php
View file

@ -15,7 +15,7 @@ use Symfony\Component\HttpFoundation\Request;
class PasteController extends AbstractController
{
skobkin commented 2023-07-20 18:04:53 +00:00
Review
Copy link

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.
#[Route('/')]
#[Route('/', name: 'homepage')]
skobkin marked this conversation as resolved Outdated
skobkin commented 2023-07-20 18:06:44 +00:00
Outdated
Review
Copy link

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.
👍 1
public function new(Request $request, PasteRepository $pasteRepository): Response
skobkin marked this conversation as resolved Outdated
skobkin commented 2023-07-20 18:08:13 +00:00
Outdated
Review
Copy link

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.
👍 1
{
$pasteData = new PasteFormData();
@ -28,7 +28,7 @@ class PasteController extends AbstractController
$paste = Paste::fromFormDataAndIp($pasteData, $request->getClientIp());
$pasteRepository->save($paste, true);
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
return $this->redirectToRoute($request->attributes->get('_route'));
return $this->redirectToRoute('showpaste', ['id' => $paste->id, 'secret' => $paste->secret]);
}
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
return $this->render('paste.html.twig', [
@ -36,7 +36,7 @@ class PasteController extends AbstractController
]);
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
}
#[Route('/{id}/{secret}')]
#[Route('/{id}/{secret}', name: 'showpaste')]
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
public function showPaste(PasteRepository $pasteRepository, string $id, ?string $secret = NULL): Response
skobkin commented 2023-08-06 11:03:34 +00:00
Review
Copy link

show_paste at least.

`show_paste` at least.
{
$paste = $pasteRepository->findOneBy(['id' => $id, 'secret' => $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