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…
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=dev
APP_SECRET=90c78b17302e6ff2e14213f129e3f5f4
You 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
schema
parameter for yourTable
attributes.We can discuss PostgreSQL specifics later.
@ -0,0 +10,4 @@
/**
* Auto-generated Migration: Please modify to your needs!
*/
final class Version20230720125632 extends AbstractMigration
Before 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 @@
<?php
I can't leave you a comment on
src/Controller/.gitignore
, so I'll ask here.Why do you need it?
@ -0,0 +1,54 @@
<?php
Please 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
EntityManager
here.Make your repository implement
save
like 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
Paste
entity 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
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.@ -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 @@
<?php
namespace 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
Table
attribute here.Especially since you're defining
Column
attributes 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=false
here since it's a Primary Key.But I'd suggest to explicitly set it for every other
Column
and also set aname
for 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 readonly
instead of justprivate
properties. 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
bool
flag?You can just
return $this->secret !== null
.@ -0,0 +1,28 @@
<?php
namespace 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
128
to 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 (
select
in HTML terms) with a list of available periods like "10 minutes", "1 year" and "Never".@ -0,0 +1,14 @@
<?php
Can't leave a comment for
src/Repository/.gitignore
, so I'll ask here.Do you need it?
@ -0,0 +1,14 @@
<?php
Please 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.twig
and use it'sbody
block?feature_pasteto WIP: feature_pasteGood job 👍
A couple more things.
@ -0,0 +10,4 @@
final class Version20230720115905 extends AbstractMigration
{
public function getDescription(): string
Do 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
string
andAssert\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
bool
field 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): void
Do 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
$highlighted
and$highlighted_text
.$highlighted_text
is 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.twig
here.@ -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_paste
at 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 @@
<?php
declare(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): Paste
You can use
self
as 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.