WIP: feature_paste #1

Draft
Miroslavsckaya wants to merge 24 commits from feature_paste into master
No description provided.
Miroslavsckaya added 2 commits 2023-07-20 16:37:47 +00:00
Miroslavsckaya changed title from feature_paste to WIP: feature_paste 2023-07-20 16:38:06 +00:00
Miroslavsckaya changed title from WIP: feature_paste to WIP: feature_paste 2023-07-20 16:38:33 +00:00
Miroslavsckaya changed title from WIP: feature_paste to feature_paste 2023-07-20 16:39:38 +00:00
Miroslavsckaya requested review from skobkin 2023-07-20 16:42:34 +00:00
Miroslavsckaya was assigned by skobkin 2023-07-20 17:02:19 +00:00
skobkin requested changes 2023-07-20 18:41:13 +00:00
skobkin left a comment
Collaborator

I left a couple of suggestions for improvement.

I left a *couple* of suggestions for improvement.
.env Outdated
@ -0,0 +16,4 @@
###> symfony/framework-bundle ###
APP_ENV=dev
APP_SECRET=90c78b17302e6ff2e14213f129e3f5f4
Collaborator

You should use some placeholder value here like your_secret_please_set_on_local_env.

You should use some placeholder value here like `your_secret_please_set_on_local_env`.
skobkin marked this conversation as resolved
@ -0,0 +1,5 @@
controllers:
Collaborator

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.

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;
/**
Collaborator

Either describe what these migrations do or remove meaningless comments.

Either describe what these migrations do or remove meaningless comments.
skobkin marked this conversation as resolved
@ -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');
Collaborator

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 your Table attributes.

We can discuss PostgreSQL specifics later.

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 your `Table` attributes. We can discuss PostgreSQL specifics later.
skobkin marked this conversation as resolved
@ -0,0 +10,4 @@
/**
* Auto-generated Migration: Please modify to your needs!
*/
final class Version20230720125632 extends AbstractMigration
Collaborator

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.

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.
skobkin marked this conversation as resolved
@ -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');
Collaborator

same.

same.
skobkin marked this conversation as resolved
@ -0,0 +1,54 @@
<?php
Collaborator

I can't leave you a comment on src/Controller/.gitignore, so I'll ask here.

Why do you need it?

I can't leave you a comment on `src/Controller/.gitignore`, so I'll ask here. Why do you need it?
skobkin marked this conversation as resolved
@ -0,0 +1,54 @@
<?php
Collaborator

Please do not forget declare(strict_types=1);

Please do not forget `declare(strict_types=1);`
skobkin marked this conversation as resolved
@ -0,0 +14,4 @@
class PasteController extends AbstractController
{
#[Route('/')]
Collaborator

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.
@ -0,0 +15,4 @@
class PasteController extends AbstractController
{
#[Route('/')]
public function new(Request $request, EntityManagerInterface $entityManager): Response {
Collaborator

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.
skobkin marked this conversation as resolved
@ -0,0 +16,4 @@
{
#[Route('/')]
public function new(Request $request, EntityManagerInterface $entityManager): Response {
$paste = new Paste();
Collaborator

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.
skobkin marked this conversation as resolved
@ -0,0 +23,4 @@
if ($form->isSubmitted() && $form->isValid()) {
$paste = $form->getData();
$paste->setIp($request->getClientIp());
$paste->setPublishDate(new \DateTime());
Collaborator

You can just set it in the constructor.

You can just set it in the constructor.
skobkin marked this conversation as resolved
@ -0,0 +26,4 @@
$paste->setPublishDate(new \DateTime());
if ($paste->isPrivate()) {
$paste->setSecret(hash("sha1", random_bytes(25)));
Collaborator

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.
skobkin marked this conversation as resolved
@ -0,0 +32,4 @@
$entityManager->persist($paste);
$entityManager->flush();
return $this->redirectToRoute($request->attributes->get('_route'));
Collaborator

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.
skobkin marked this conversation as resolved
@ -0,0 +33,4 @@
$entityManager->flush();
return $this->redirectToRoute($request->attributes->get('_route'));
Collaborator

I see you needed some space 🤔

I see you needed some space 🤔
skobkin marked this conversation as resolved
@ -0,0 +42,4 @@
}
#[Route('/{id}/{secret}')]
public function show_paste(PasteRepository $pasteRepository, Request $request, string $id, ?string $secret=NULL): Response {
Collaborator

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.

You're violating [Symfony code style](https://symfony.com/doc/current/contributing/code/standards.html#symfony-coding-standards-in-detail) 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.
skobkin marked this conversation as resolved
@ -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]);
Collaborator

Do you need double quotes here too?

Do you need double quotes here too?
skobkin marked this conversation as resolved
@ -0,0 +1,139 @@
<?php
namespace App\Entity;
Collaborator

Please do not forget declare(strict_types=1);

Please do not forget `declare(strict_types=1);`
skobkin marked this conversation as resolved
@ -0,0 +6,4 @@
use Doctrine\ORM\Mapping as ORM;
use Symfony\Component\Validator\Constraints as Assert;
#[ORM\Entity(repositoryClass: PasteRepository::class)]
Collaborator

I'd suggest explicitly defining Table attribute here.

Especially since you're defining Column attributes below ⬇️

I'd suggest explicitly defining `Table` attribute here. Especially since you're defining `Column` attributes below ⬇️
Collaborator

I'd also suggest to make this entity readonly. It'll give some performance benefits.

You can read about that here.

I'd also suggest to make this entity readonly. It'll give some performance benefits. You can read about that [here](https://www.doctrine-project.org/projects/doctrine-orm/en/2.15/reference/attributes-reference.html#attrref_entity).
@ -0,0 +11,4 @@
{
#[ORM\Id]
#[ORM\GeneratedValue]
#[ORM\Column(nullable: false)]
Collaborator

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 a name for them.

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 a `name` for them.
skobkin marked this conversation as resolved
@ -0,0 +14,4 @@
#[ORM\Column(nullable: false)]
private int $id;
#[ORM\Column(type: "text", nullable: false)]
Collaborator

No need for double quotes here too.

No need for double quotes here too.
skobkin marked this conversation as resolved
@ -0,0 +22,4 @@
#[Assert\Type(\boolean::class)]
private bool $private;
#[ORM\Column(length: 25, nullable: true)]
Collaborator

What type? Why 25 exactly?

What type? Why 25 exactly?
@ -0,0 +33,4 @@
#[ORM\Column(length: 128, nullable: false)]
#[Assert\NotBlank]
private string $author = "anonymous";
Collaborator

Why double quotes?

Why double quotes?
skobkin marked this conversation as resolved
@ -0,0 +37,4 @@
#[ORM\Column(type: Types::DATETIME_MUTABLE, nullable: false)]
#[Assert\Type(\DateTime::class)]
private \DateTime $publishDate;
Collaborator

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

Why mutable date? Do you plan to change publish date?
skobkin marked this conversation as resolved
@ -0,0 +49,4 @@
#[ORM\Column(length: 40, nullable: true)]
private ?string $secret;
public function getId(): int {
Collaborator

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.
skobkin marked this conversation as resolved
@ -0,0 +53,4 @@
return $this->id;
}
public function setId(int $id): void {
Collaborator

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 just private properties. Or even public readonly 🤔

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 just `private` properties. Or even `public readonly` 🤔
skobkin marked this conversation as resolved
@ -0,0 +101,4 @@
return $this->publishDate;
}
public function setPublishDate(\DateTime $date): void {
Collaborator

You don't need this.

You don't need this.
skobkin marked this conversation as resolved
@ -0,0 +125,4 @@
return $this->secret;
}
public function setSecret(?string $secret): void {
Collaborator

You don't need this.

You don't need this.
skobkin marked this conversation as resolved
@ -0,0 +130,4 @@
}
public function isPrivate(): bool {
return $this->private;
Collaborator

Do you really need this bool flag?

You can just return $this->secret !== null.

Do you really need this `bool` flag? You can just `return $this->secret !== null`.
skobkin marked this conversation as resolved
@ -0,0 +1,28 @@
<?php
namespace App\Form\Type;
Collaborator

Please do not forget declare(strict_types=1);

Please do not forget `declare(strict_types=1);`
skobkin marked this conversation as resolved
@ -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]])
Collaborator

Double quotes?

Also please multi-line format arrays with more than one element for readability.

Double quotes? Also please multi-line format arrays with more than one element for readability.
skobkin marked this conversation as resolved
@ -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]])
Collaborator

You can probably extract 128 to the constant and use here and in the validation attribute.

You can probably extract `128` to the constant and use here and in the validation attribute.
skobkin marked this conversation as resolved
@ -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"])
Collaborator

This should be a combo-box (select in HTML terms) with a list of available periods like "10 minutes", "1 year" and "Never".

This should be a combo-box (`select` in HTML terms) with a list of available periods like "10 minutes", "1 year" and "Never".
skobkin marked this conversation as resolved
@ -0,0 +1,14 @@
<?php
Collaborator

Can't leave a comment for src/Repository/.gitignore, so I'll ask here.

Do you need it?

Can't leave a comment for `src/Repository/.gitignore`, so I'll ask here. Do you need it?
skobkin marked this conversation as resolved
@ -0,0 +1,14 @@
<?php
Collaborator

Please do not forget declare(strict_types=1);

Please do not forget `declare(strict_types=1);`
skobkin marked this conversation as resolved
@ -0,0 +10,4 @@
{
public function __construct(ManagerRegistry $registry) {
parent::__construct($registry, Paste::class);
}
Collaborator

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

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

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.
skobkin marked this conversation as resolved
@ -0,0 +1 @@
{{ form(form) }}
Collaborator

Did you forget to extend base.html.twig and use it's body block?

Did you forget to extend `base.html.twig` and use it's `body` block?
skobkin marked this conversation as resolved
Miroslavsckaya changed title from feature_paste to WIP: feature_paste 2023-07-21 13:34:47 +00:00
Miroslavsckaya added 10 commits 2023-07-21 20:09:38 +00:00
skobkin requested changes 2023-07-21 22:41:59 +00:00
skobkin left a comment
Collaborator

Good job 👍

A couple more things.

Code Refactoring

Good job 👍 A couple more things. ![Code Refactoring](https://i.skobk.in/i/refactoring.gif)
@ -0,0 +10,4 @@
final class Version20230720115905 extends AbstractMigration
{
public function getDescription(): string
Collaborator

Do you need it here if it's empty?

Do you need it here if it's empty?
skobkin marked this conversation as resolved
@ -0,0 +23,4 @@
public function down(Schema $schema): void
{
$this->addSql('CREATE SCHEMA public');
Collaborator

This shouldn't be here.

This shouldn't be here.
@ -0,0 +26,4 @@
$pasteData = $form->getData();
$secret = null;
if ($pasteData->private) {
Collaborator

This still could be done in the entity itself.

This still could be done in the entity itself.
skobkin marked this conversation as resolved
@ -0,0 +30,4 @@
$secret = hash('sha1', random_bytes(25));
}
$paste = new Paste(
Collaborator

I still suggest to make Paste::fromFormData().

I still suggest to make `Paste::fromFormData()`.
skobkin marked this conversation as resolved
@ -0,0 +36,4 @@
$pasteData->description,
$pasteData->filename,
$pasteData->author,
new \DateTimeImmutable(),
Collaborator

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.
skobkin marked this conversation as resolved
@ -0,0 +39,4 @@
new \DateTimeImmutable(),
$pasteData->expirationDate,
$request->getClientIp(),
$secret
Collaborator
  • 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.
skobkin marked this conversation as resolved
@ -0,0 +11,4 @@
{
#[Assert\NotBlank]
public string $text;
#[Assert\Type(\boolean::class)]
Collaborator

Huh?

Huh?
skobkin marked this conversation as resolved
@ -0,0 +17,4 @@
public ?string $description = null;
public ?string $filename = null;
#[Assert\NotBlank]
public string $author = 'anonymous';
Collaborator

Why string and Assert\NotBlank?

Why `string` and `Assert\NotBlank`?
skobkin marked this conversation as resolved
@ -0,0 +21,4 @@
#[Assert\Type(\DateTimeImmutable::class)]
public ?\DateTimeImmutable $expirationDate;
public function __construct(?Paste $paste=null)
Collaborator

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.
skobkin marked this conversation as resolved
@ -0,0 +24,4 @@
public readonly ?string $description,
#[ORM\Column(length: 128, nullable: true)]
public readonly ?string $filename,
#[ORM\Column(length: 128, nullable: false)]
Collaborator

Why not nullable?

Why not nullable?
skobkin marked this conversation as resolved
@ -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)]
Collaborator

IPv6?

IPv6?
skobkin marked this conversation as resolved
@ -0,0 +28,4 @@
)
->add('description', TextType::class, ['required' => false])
->add('text', TextareaType::class)
->add('author', TextType::class, ['attr' => ['maxlength' =>128]])
Collaborator

required => false?

`required => false`?
skobkin marked this conversation as resolved
@ -0,0 +18,4 @@
{
$entityManager = $this->getEntityManager();
$entityManager->persist($paste);
$entityManager->flush();
Collaborator

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.
skobkin marked this conversation as resolved
Miroslavsckaya added 4 commits 2023-07-22 17:37:22 +00:00
skobkin approved these changes 2023-07-22 18:03:04 +00:00
skobkin left a comment
Collaborator

Closed old threads, added a couple of comments.

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();
Collaborator

Do you need IP in PasteData?

Do you need IP in `PasteData`?
skobkin marked this conversation as resolved
@ -0,0 +19,4 @@
#[Assert\NotBlank]
public string $author = 'anonymous';
#[Assert\Type(\DateTimeImmutable::class)]
public ?\DateTimeImmutable $expirationDate;
Collaborator

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.
skobkin marked this conversation as resolved
@ -0,0 +12,4 @@
#[ORM\Id]
#[ORM\GeneratedValue]
#[ORM\Column]
public readonly int $id;
Collaborator

Why not also constructor promotion though?

Why not also constructor promotion though?
@ -0,0 +14,4 @@
parent::__construct($registry, Paste::class);
}
public function save(Paste $paste, bool $flush=false): void
Collaborator

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.
skobkin marked this conversation as resolved
Miroslavsckaya added 6 commits 2023-07-23 11:50:46 +00:00
Miroslavsckaya added 1 commit 2023-07-23 12:23:40 +00:00
Miroslavsckaya added 1 commit 2023-08-06 09:50:49 +00:00
skobkin requested changes 2023-08-06 11:01:58 +00:00
skobkin left a comment
Collaborator

Good start, but there are things to improve and other things to do.

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",
Collaborator

Why so specific?

Why so specific?
@ -0,0 +46,4 @@
return $this->render('show_paste.html.twig', [
'form' => $form,
'highlighted_text' => CodeHighlighter::highlight($pasteData->language, $pasteData->text)
Collaborator

I'd suggest you implementing Twig extension wih filter and function to use highlighter in the template.

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();
Collaborator

Why not inject into constructor?

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>';
Collaborator

Why do you use HTML here and not in the template? Check the comment I left in PasterController::showPaste().

Why do you use HTML here and not in the template? Check the comment I left in `PasterController::showPaste()`.
Collaborator

Also naming here is not good. You have $highlighted and $highlighted_text.

  • $highlighted_text is not in $camelCase
  • Both variables contain text, both contain code. If it shouldn't be removed, I'd say to think a bit more on naming.
Also naming here is not good. You have `$highlighted` and `$highlighted_text`. - `$highlighted_text` is not in `$camelCase` - Both variables contain text, both contain code. If it shouldn't be removed, I'd say to think a bit more on naming.
@ -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>';
Collaborator

Again why HTML here?

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">
Collaborator

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.

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>
Collaborator

Same here.

Same here.
@ -0,0 +1,6 @@
{% extends "base.html.twig" %}
Collaborator

Why double quotes again?

Why double quotes again?
Collaborator

BTW, if you're including it in the show_paste.html.twig, then you don't need to extend base.html.twig here.

BTW, if you're including it in the `show_paste.html.twig`, then you don't need to extend `base.html.twig` here.
@ -0,0 +5,4 @@
{% endblock %}
{% block content %}
<ul class="nav nav-tabs" id="myTab" role="tablist">
Collaborator

Do not forget to name classes and ID's properly.

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>
Collaborator

profile-tab?

`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 }}
Collaborator

As I said above, better implement Twig extension and use it here.

As I said above, better implement Twig extension and use it here.
skobkin reviewed 2023-08-06 11:03:34 +00:00
@ -0,0 +37,4 @@
]);
}
#[Route('/{id}/{secret}', name: 'showpaste')]
Collaborator

show_paste at least.

`show_paste` at least.
skobkin reviewed 2023-08-06 11:10:15 +00:00
@ -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,
Collaborator

Why 128?

Why 128?
skobkin reviewed 2023-08-06 11:13:10 +00:00
@ -0,0 +1,26 @@
<?php
declare(strict_types = 1);
namespace App\Entity;
Collaborator

Why Entity?
Are you storing it in the database? Does it have an identity?

Why `Entity`? Are you storing it in the database? Does it have an identity?
skobkin reviewed 2023-08-06 11:14:46 +00:00
@ -0,0 +18,4 @@
public function down(Schema $schema): void
{
$this->addSql('CREATE SCHEMA public');
Collaborator

This shouldn't be here.

This shouldn't be here.
skobkin reviewed 2023-08-06 11:17:44 +00:00
@ -0,0 +35,4 @@
public readonly ?string $secret,
) {}
public static function fromFormDataAndIp(PasteFormData $pasteFormData, $ip): Paste
Collaborator

You can use self as return type hint too.

You can use `self` as return type hint too.
skobkin reviewed 2023-08-06 11:19:24 +00:00
@ -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]])
Collaborator

Why 128?

Why 128?
This pull request is marked as a work in progress.
View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.
git fetch -u origin feature_paste:feature_paste
git checkout feature_paste

Merge

Merge the changes and update on Forgejo.
git checkout master
git merge --no-ff feature_paste
git checkout master
git merge --ff-only feature_paste
git checkout feature_paste
git rebase master
git checkout master
git merge --no-ff feature_paste
git checkout master
git merge --squash feature_paste
git checkout master
git merge --ff-only feature_paste
git checkout master
git merge feature_paste
git push origin master
Sign in to join this conversation.
No reviewers
No milestone
No project
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference: Miroslavsckaya/copypaste#1
No description provided.