Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

simple comment system #700

Closed
digitalinferno opened this issue May 13, 2024 · 7 comments
Closed

simple comment system #700

digitalinferno opened this issue May 13, 2024 · 7 comments

Comments

@digitalinferno
Copy link

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?

This comment was marked as outdated.

@dkyme
Copy link

dkyme commented May 22, 2024

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

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
Copy link
Author

digitalinferno commented May 23, 2024

@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
Copy link
Collaborator

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
Copy link
Author

digitalinferno commented May 23, 2024

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).

Copy link

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! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants