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 +8 -4
2 changed files with 8 additions and 4 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit 561b016e38 - Show all commits

6
src/Controller/PasteController.php
View file

@ -6,7 +6,6 @@ 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 Doctrine\ORM\EntityManagerInterface;
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;
@ -16,7 +15,7 @@ use Symfony\Component\HttpFoundation\Request;
class PasteController extends AbstractController class PasteController extends AbstractController
{ {
#[Route('/')] #[Route('/')]
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.
public function new(Request $request, EntityManagerInterface $entityManager): Response { public function new(Request $request, PasteRepository $pasteRepository): Response {
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
$paste = new Paste(); $paste = new Paste();
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
$form = $this->createForm(PasteForm::class, $paste); $form = $this->createForm(PasteForm::class, $paste);
@ -30,8 +29,7 @@ class PasteController extends AbstractController
$paste->setSecret(hash('sha1', random_bytes(25))); $paste->setSecret(hash('sha1', random_bytes(25)));
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
} }
$entityManager->persist($paste); $pasteRepository->save($paste);
$entityManager->flush();
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->redirectToRoute($request->attributes->get('_route')); return $this->redirectToRoute($request->attributes->get('_route'));
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

6
src/Repository/PasteRepository.php
View file

@ -12,4 +12,10 @@ class PasteRepository extends ServiceEntityRepository
public function __construct(ManagerRegistry $registry) { public function __construct(ManagerRegistry $registry) {
parent::__construct($registry, Paste::class); parent::__construct($registry, Paste::class);
skobkin marked this conversation as resolved Outdated
skobkin commented 2023-07-20 18:38:57 +00:00
Outdated
Review
Copy link

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

I'd suggest adding `save()` here like I showed you.
👍 1
skobkin commented 2023-07-21 21:56:34 +00:00
Outdated
Review
Copy link

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.
👍 1
} }
public function save(Paste $paste): void {
$entityManager = $this->getEntityManager();
skobkin marked this conversation as resolved Outdated
skobkin commented 2023-07-22 17:51:11 +00:00
Outdated
Review
Copy link

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.
👍 1
$entityManager->persist($paste);
$entityManager->flush();
}
} }
skobkin marked this conversation as resolved Outdated
skobkin commented 2023-07-21 20:28:25 +00:00
Outdated
Review
Copy link

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.