picocms / Pico

Pico is a stupidly simple, blazing fast, flat file CMS.
http://picocms.org/
MIT License
3.81k stars 616 forks source link

simple comment system #700

Closed digitalinferno closed 4 weeks ago

digitalinferno commented 1 month ago

I would implement a simple comment system. The concept is to add _comment-datenumber.md files inside a directory named page.id.

So, for example, if I have a page file "my-post.md", the directory structure would be:

my-post.md
my-post/_comment-020202020.md
my-post/_comment-030303030.md

I think some plugin editor can edit these files, and I can use Pico parser for read/write tasks. My plugin reads all _comment-*.md files and returns an array for Twig.

The downside is the number of files. I think a blog can easily accumulate various comments for each post, which could be an issue for Pico's performance (in long term).

So I think $skipPage help here:

public function onSinglePageLoading($id, &$skipPage)
{
    if (strpos($id, '_comment-') === 0) {
       $skipPage = true;
    }
}   

Are there any blunders to correct?

github-actions[bot] commented 1 month ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in two days if no further activity occurs. Thank you for your contributions! :+1:

dkyme commented 1 month ago

Hmm... not answering your question, but you know that there's already a comment plug-in? https://github.com/push-eax/pico-comments Maybe the code will help you solving your problem.

PhrozenByte commented 1 month ago

The downside is the number of files. I think a blog can easily accumulate various comments for each post, which could be an issue for Pico's performance (in long term).

So I think $skipPage help here:

Yes, that's a good point. Having a lot of pages has a noticeable negative performance impact on Pico, so keeping the total number of pages down is a good thing. Please note that even though $skipPage at least prevents Pico from parsing the page, there still is some performance impact just for page discovery. Thus you might want to think about whether a single file per page containing all comments of that page might be enough. However, please note that this is no definitive answer, both approaches have advantages and disadvantages, they are both reasonable IMO. Besides, your code looks good and should work as expected.

digitalinferno commented 1 month ago

@dkyme thanks for the hint. Everything can be made to work, but I want to learn the correct (Pico) way :)

So, as in many other projects, like the same pico-comments plugin, login plugin, etc., is it reasonable to move out of the content folder and create your own data structure? On paper, I don't like this solution, for the sake of cleanliness, ease of backup, and a touch of my own personal closed-mindedness. Perhaps for this type of application, it's better to use a different extension, like .yaml? They are YAML data, but they don't need to be manipulated like a regular .md page, given that they extend it... but they can remain in the content directory. Does that make sense?

In all functions that require knowing the ID (like this plugin, but also, for example, the one on statistics), is it correct to use $this->pico->is404Content() to avoid error from $this->getPico()->getPageId($this->getPico()->getRequestFile() ?? '')?

@PhrozenByte Naturally, thanks as always for your availability; maybe these are obvious questions.

PhrozenByte commented 1 month ago

So, as in many other projects, like the same pico-comments plugin, login plugin, etc., is it reasonable to move out of the content folder and create your own data structure?

It has both advantages and disadvantages. The biggest advantage is that such extra structure doesn't cause Pico to prematurely load the contents. The biggest disadvantage is that you have to implement the page parsing logic yourself (even though Pico provides methods for that, so that it's no big deal in the end). There's no "true" answer. However, if you choose to create your own structure, I'd always recommend moving such data into its own directory, i.e. not relying on just different file extensions.

In all functions that require knowing the ID (like this plugin, but also, for example, the one on statistics), is it correct to use $this->pico->is404Content() to avoid error from $this->getPico()->getPageId($this->getPico()->getRequestFile() ?? '')?

Pico::getPageId() will work just fine for non-existing pages, it just won't work for files outside of Pico's content dir or files with different file extensions. Generally I'd advise against using some "path / page ID magic" to determine whether one can add comments to a page - just add a meta header (like comments: true) to the page's YAML Front Matter and use that. If Pico::getPageId() returns nothing, just forcefully disable the comments system.

digitalinferno commented 1 month ago

i.e. not relying on just different file extensions.

Why avoid something like this?

my-post.md
my-post/_comment-020202020.yaml
my-post/_comment-030303030.yaml

At this point, the holy grail for performance is to use YAML inside the MD file (in theory, we don't have many concurrent write operations).

Pico::getPageId() will work just fine for non-existing pages

If I request the abot page instead of about (bad url, typo, or so on) , my page ID is abot and I have a function readComment('id') which returns an error. Instead, if previously it's a 404 error, I skip the read comments. I use comments: true to enable writing (so I can disable writing, but still read comments).

github-actions[bot] commented 1 month ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in two days if no further activity occurs. Thank you for your contributions! :+1: