WIP: feature_paste #1
No reviewers
Labels
No labels
bug
enhancement
invalid
wontfix
No milestone
No project
No assignees
2 participants
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference
Miroslavsckaya/copypaste!1
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feature_paste"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
feature_pasteto WIP: feature_pasteWIP: feature_pasteto WIP: feature_pasteWIP: feature_pasteto feature_pasteI left a couple of suggestions for improvement.
@ -0,0 +16,4 @@###> symfony/framework-bundle ###APP_ENV=devAPP_SECRET=90c78b17302e6ff2e14213f129e3f5f4You should use some placeholder value here like
your_secret_please_set_on_local_env.@ -0,0 +1,5 @@controllers:I'd suggest we move to route list in YAML here before merging it. But it's not a main priority for now. Just let this thread be until we ready with everything else.
@ -0,0 +7,4 @@use Doctrine\DBAL\Schema\Schema;use Doctrine\Migrations\AbstractMigration;/**Either describe what these migrations do or remove meaningless comments.
@ -0,0 +27,4 @@public function down(Schema $schema): void{// this down() migration is auto-generated, please modify it to your needs$this->addSql('CREATE SCHEMA public');This should probably not be here.
I'd say that it's most likely a but caused by you not setting the
schemaparameter for yourTableattributes.We can discuss PostgreSQL specifics later.
@ -0,0 +10,4 @@/*** Auto-generated Migration: Please modify to your needs!*/final class Version20230720125632 extends AbstractMigrationBefore merging this we should ideally have only one migration which does everything from scratch or from previous database version. There is no need to repeat entire development process on the production.
@ -0,0 +29,4 @@public function down(Schema $schema): void{// this down() migration is auto-generated, please modify it to your needs$this->addSql('CREATE SCHEMA public');same.
@ -0,0 +1,54 @@<?phpI can't leave you a comment on
src/Controller/.gitignore, so I'll ask here.Why do you need it?
@ -0,0 +1,54 @@<?phpPlease do not forget
declare(strict_types=1);@ -0,0 +14,4 @@class PasteController extends AbstractController{#[Route('/')]I'd suggest explicitly defining which request methods we're processing here.
P.S. We'll move this to YAML in the end.
@ -0,0 +15,4 @@class PasteController extends AbstractController{#[Route('/')]public function new(Request $request, EntityManagerInterface $entityManager): Response {You don't really need
EntityManagerhere.Make your repository implement
savelike I showed you and use it instead.@ -0,0 +16,4 @@{#[Route('/')]public function new(Request $request, EntityManagerInterface $entityManager): Response {$paste = new Paste();I'd suggest we refactor this to to DTO usage and make our
Pasteentity immutable.@ -0,0 +23,4 @@if ($form->isSubmitted() && $form->isValid()) {$paste = $form->getData();$paste->setIp($request->getClientIp());$paste->setPublishDate(new \DateTime());You can just set it in the constructor.
@ -0,0 +26,4 @@$paste->setPublishDate(new \DateTime());if ($paste->isPrivate()) {$paste->setSecret(hash("sha1", random_bytes(25)));Do we need double quotes here?
By the way, we can also do that in the constructor.
@ -0,0 +32,4 @@$entityManager->persist($paste);$entityManager->flush();return $this->redirectToRoute($request->attributes->get('_route'));Just set a
namefor 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.@ -0,0 +33,4 @@$entityManager->flush();return $this->redirectToRoute($request->attributes->get('_route'));I see you needed some space 🤔
@ -0,0 +42,4 @@}#[Route('/{id}/{secret}')]public function show_paste(PasteRepository $pasteRepository, Request $request, string $id, ?string $secret=NULL): Response {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.
@ -0,0 +43,4 @@#[Route('/{id}/{secret}')]public function show_paste(PasteRepository $pasteRepository, Request $request, string $id, ?string $secret=NULL): Response {$paste = $pasteRepository->findOneBy(["id" => $id, "secret" => $secret]);Do you need double quotes here too?
@ -0,0 +1,139 @@<?phpnamespace App\Entity;Please do not forget
declare(strict_types=1);@ -0,0 +6,4 @@use Doctrine\ORM\Mapping as ORM;use Symfony\Component\Validator\Constraints as Assert;#[ORM\Entity(repositoryClass: PasteRepository::class)]I'd suggest explicitly defining
Tableattribute here.Especially since you're defining
Columnattributes below ⬇️I'd also suggest to make this entity readonly. It'll give some performance benefits.
You can read about that here.
@ -0,0 +11,4 @@{#[ORM\Id]#[ORM\GeneratedValue]#[ORM\Column(nullable: false)]I don't think you need
nullable=falsehere since it's a Primary Key.But I'd suggest to explicitly set it for every other
Columnand also set anamefor them.@ -0,0 +14,4 @@#[ORM\Column(nullable: false)]private int $id;#[ORM\Column(type: "text", nullable: false)]No need for double quotes here too.
@ -0,0 +22,4 @@#[Assert\Type(\boolean::class)]private bool $private;#[ORM\Column(length: 25, nullable: true)]What type? Why 25 exactly?
@ -0,0 +33,4 @@#[ORM\Column(length: 128, nullable: false)]#[Assert\NotBlank]private string $author = "anonymous";Why double quotes?
@ -0,0 +37,4 @@#[ORM\Column(type: Types::DATETIME_MUTABLE, nullable: false)]#[Assert\Type(\DateTime::class)]private \DateTime $publishDate;Why mutable date? Do you plan to change publish date?
@ -0,0 +49,4 @@#[ORM\Column(length: 40, nullable: true)]private ?string $secret;public function getId(): int {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.
@ -0,0 +53,4 @@return $this->id;}public function setId(int $id): void {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 readonlyinstead of justprivateproperties. Or evenpublic readonly🤔@ -0,0 +101,4 @@return $this->publishDate;}public function setPublishDate(\DateTime $date): void {You don't need this.
@ -0,0 +125,4 @@return $this->secret;}public function setSecret(?string $secret): void {You don't need this.
@ -0,0 +130,4 @@}public function isPrivate(): bool {return $this->private;Do you really need this
boolflag?You can just
return $this->secret !== null.@ -0,0 +1,28 @@<?phpnamespace App\Form\Type;Please do not forget
declare(strict_types=1);@ -0,0 +15,4 @@{public function buildForm(FormBuilderInterface $builder, array $options): void {$builder->add("language", ChoiceType::class, ["choices" => ["Python" => "python", "PHP" => "php", "Plain text" => NULL]])Double quotes?
Also please multi-line format arrays with more than one element for readability.
@ -0,0 +18,4 @@->add("language", ChoiceType::class, ["choices" => ["Python" => "python", "PHP" => "php", "Plain text" => NULL]])->add("description", TextType::class, ["required" => false])->add("text", TextareaType::class)->add("author", TextType::class, ["attr" => ["maxlength" =>128]])You can probably extract
128to the constant and use here and in the validation attribute.@ -0,0 +20,4 @@->add("text", TextareaType::class)->add("author", TextType::class, ["attr" => ["maxlength" =>128]])->add("filename", TextType::class, ["required" => false, "attr" => ["maxlength" =>128]])->add("expirationDate", DateTimeType::class, ["required" => false, "date_widget" => "single_text", "input" => "datetime"])This should be a combo-box (
selectin HTML terms) with a list of available periods like "10 minutes", "1 year" and "Never".@ -0,0 +1,14 @@<?phpCan't leave a comment for
src/Repository/.gitignore, so I'll ask here.Do you need it?
@ -0,0 +1,14 @@<?phpPlease do not forget
declare(strict_types=1);@ -0,0 +10,4 @@{public function __construct(ManagerRegistry $registry) {parent::__construct($registry, Paste::class);}I'd suggest adding
save()here like I showed you.Better with optional
flush.Flushng from the repo itself should be a rare action.
@ -0,0 +1 @@{{ form(form) }}Did you forget to extend
base.html.twigand use it'sbodyblock?feature_pasteto WIP: feature_pasteGood job 👍
A couple more things.
@ -0,0 +10,4 @@final class Version20230720115905 extends AbstractMigration{public function getDescription(): stringDo you need it here if it's empty?
@ -0,0 +23,4 @@public function down(Schema $schema): void{$this->addSql('CREATE SCHEMA public');This shouldn't be here.
@ -0,0 +26,4 @@$pasteData = $form->getData();$secret = null;if ($pasteData->private) {This still could be done in the entity itself.
@ -0,0 +30,4 @@$secret = hash('sha1', random_bytes(25));}$paste = new Paste(I still suggest to make
Paste::fromFormData().@ -0,0 +36,4 @@$pasteData->description,$pasteData->filename,$pasteData->author,new \DateTimeImmutable(),I mean you don't need to pass creation date here at all.
@ -0,0 +39,4 @@new \DateTimeImmutable(),$pasteData->expirationDate,$request->getClientIp(),$secret,to the last argument too if you use multi-line formatting$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.@ -0,0 +11,4 @@{#[Assert\NotBlank]public string $text;#[Assert\Type(\boolean::class)]Huh?
@ -0,0 +17,4 @@public ?string $description = null;public ?string $filename = null;#[Assert\NotBlank]public string $author = 'anonymous';Why
stringandAssert\NotBlank?@ -0,0 +21,4 @@#[Assert\Type(\DateTimeImmutable::class)]public ?\DateTimeImmutable $expirationDate;public function __construct(?Paste $paste=null)Or you can make constructor
private, use property promotion and addfromPaste()method to create it from the entity.@ -0,0 +24,4 @@public readonly ?string $description,#[ORM\Column(length: 128, nullable: true)]public readonly ?string $filename,#[ORM\Column(length: 128, nullable: false)]Why not nullable?
@ -0,0 +30,4 @@public readonly \DateTimeImmutable $publishDate,#[ORM\Column(type: Types::DATETIME_IMMUTABLE, nullable: true)]public readonly ?\DateTimeImmutable $expirationDate,#[ORM\Column(length: 15, nullable: false)]IPv6?
@ -0,0 +28,4 @@)->add('description', TextType::class, ['required' => false])->add('text', TextareaType::class)->add('author', TextType::class, ['attr' => ['maxlength' =>128]])required => false?@ -0,0 +18,4 @@{$entityManager = $this->getEntityManager();$entityManager->persist($paste);$entityManager->flush();You shouldn't do flush here by default. Only in certain cases if needed.
Closed old threads, added a couple of comments.
@ -0,0 +24,4 @@$form->handleRequest($request);if ($form->isSubmitted() && $form->isValid()) {$pasteData = $form->getData();$pasteData->ip = $request->getClientIp();Do you need IP in
PasteData?@ -0,0 +19,4 @@#[Assert\NotBlank]public string $author = 'anonymous';#[Assert\Type(\DateTimeImmutable::class)]public ?\DateTimeImmutable $expirationDate;Is this validation being processed BEFORE storing the data in the DTO?
If not, it's pointless as with
boolfield earlier.@ -0,0 +12,4 @@#[ORM\Id]#[ORM\GeneratedValue]#[ORM\Column]public readonly int $id;Why not also constructor promotion though?
@ -0,0 +14,4 @@parent::__construct($registry, Paste::class);}public function save(Paste $paste, bool $flush=false): voidDo not forget to use code formatting. It'd surrounded
=with spaces for you.Good start, but there are things to improve and other things to do.
@ -0,0 +10,4 @@"doctrine/doctrine-bundle": "^2.10","doctrine/doctrine-migrations-bundle": "^3.2","doctrine/orm": "^2.15","scrivo/highlight.php": "v9.18.1.10",Why so specific?
@ -0,0 +46,4 @@return $this->render('show_paste.html.twig', ['form' => $form,'highlighted_text' => CodeHighlighter::highlight($pasteData->language, $pasteData->text)I'd suggest you implementing Twig extension wih filter and function to use highlighter in the template.
@ -0,0 +9,4 @@{public static function highlight(?string $language, string $code): string{$hl = new Highlighter();Why not inject into constructor?
@ -0,0 +14,4 @@try {// Highlight some code.$highlighted = $hl->highlight($language, $code);$highlighted_text = "<pre><code class=\"hljs {$highlighted->language}\">".$highlighted->value.'</code></pre>';Why do you use HTML here and not in the template? Check the comment I left in
PasterController::showPaste().Also naming here is not good. You have
$highlightedand$highlighted_text.$highlighted_textis not in$camelCase@ -0,0 +18,4 @@}catch (\DomainException $e) {// This is thrown if the specified language does not exist$highlighted_text = '<pre><code>'.htmlentities($code).'</code></pre>';Again why HTML here?
@ -0,0 +4,4 @@<meta charset="UTF-8"><title>{% block title %}Copypaste{% endblock %}</title><link rel="icon" href="data:image/svg+xml,<svg xmlns=%22http://www.w3.org/2000/svg%22 viewBox=%220 0 128 128%22><text y=%221.2em%22 font-size=%2296%22>⚫️</text></svg>"><link href="https://cdn.jsdelivr.net/npm/bootstrap@5.0.2/dist/css/bootstrap.min.css" rel="stylesheet" integrity="sha384-EVSTQN3/azprG1Anm3QDgpJLIm9Nao0Yz1ztcQTwFspd3yD65VohhpuuCOmLASjC" crossorigin="anonymous">It's better not to rely on external CDN's when you can.
You can just save needed resources in the repository since we're not making our front-end too complex to make dynamic build.
@ -0,0 +10,4 @@</head><body>{% block content %}{% endblock %}<script src="https://cdn.jsdelivr.net/npm/bootstrap@5.0.2/dist/js/bootstrap.bundle.min.js" integrity="sha384-MrcW6ZMFYlzcLA8Nl+NtUVF0sA7MsXsP1UyJoMp4YLEuNSfAP+JcXn/tWtIaxVXM" crossorigin="anonymous"></script>Same here.
@ -0,0 +1,6 @@{% extends "base.html.twig" %}Why double quotes again?
BTW, if you're including it in the
show_paste.html.twig, then you don't need to extendbase.html.twighere.@ -0,0 +5,4 @@{% endblock %}{% block content %}<ul class="nav nav-tabs" id="myTab" role="tablist">Do not forget to name classes and ID's properly.
@ -0,0 +10,4 @@<button class="nav-link active" id="show-tab" data-bs-toggle="tab" data-bs-target="#show" type="button" role="tab" aria-controls="show" aria-selected="true">Show</button></li><li class="nav-item" role="presentation"><button class="nav-link" id="profile-tab" data-bs-toggle="tab" data-bs-target="#edit" type="button" role="tab" aria-controls="edit" aria-selected="false">Edit</button>profile-tab?@ -0,0 +15,4 @@</ul><div class="tab-content" id="myTabContent"><div class="tab-pane fade show active" id="show" role="tabpanel" aria-labelledby="show-tab">{{ highlighted_text|raw }}As I said above, better implement Twig extension and use it here.
@ -0,0 +37,4 @@]);}#[Route('/{id}/{secret}', name: 'showpaste')]show_pasteat least.@ -0,0 +22,4 @@#[ORM\Column(type: 'text', nullable: true)]public readonly ?string $description,#[ORM\Column(type: 'string', length: 128, nullable: true)]public readonly ?string $filename,Why 128?
@ -0,0 +1,26 @@<?phpdeclare(strict_types = 1);namespace App\Entity;Why
Entity?Are you storing it in the database? Does it have an identity?
@ -0,0 +18,4 @@public function down(Schema $schema): void{$this->addSql('CREATE SCHEMA public');This shouldn't be here.
@ -0,0 +35,4 @@public readonly ?string $secret,) {}public static function fromFormDataAndIp(PasteFormData $pasteFormData, $ip): PasteYou can use
selfas return type hint too.@ -0,0 +29,4 @@->add('description', TextType::class, ['required' => false])->add('text', TextareaType::class)->add('author', TextType::class, ['attr' => ['maxlength' => 128], 'required' => false])->add('filename', TextType::class, ['required' => false, 'attr' => ['maxlength' =>128]])Why 128?
View command line instructions
Checkout
From your project repository, check out a new branch and test the changes.Merge
Merge the changes and update on Forgejo.Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.