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

Update valine comment system version #345

Merged
merged 4 commits into from Jul 14, 2018
Merged

Update valine comment system version #345

merged 4 commits into from Jul 14, 2018

Conversation

xCss
Copy link
Contributor

@xCss xCss commented Jul 7, 2018

PR Checklist

Please check if your PR fulfills the following requirements:

  • The commit message follows our guidelines.
  • Tests for the changes have been added (for bug fixes / features).
    • Muse | Mist have been tested.
    • Pisces | Gemini have been tested.
  • Docs have been added / updated (for bug fixes / features).

PR Type

What kind of change does this PR introduce?

  • Bugfix.
  • Feature.
  • Code style update (formatting, local variables).
  • Refactoring (no functional changes, no api changes).
  • Build related changes.
  • CI related changes.
  • Documentation content changes.
  • Other... Please describe:

What is the current behavior?

Issue Number(s): N/A

What is the new behavior?

Description about this pull, in several words...

  • Screens with this changes: N/A
  • Link to demo site with this changes: N/A

How to use?

In NexT _config.yml:

valine:
    ...
    visitor: false # Article reading statistic https://valine.js.org/visitor.html
...

Does this PR introduce a breaking change?

  • Yes.
  • No.

@sli1989
Copy link
Collaborator

sli1989 commented Jul 7, 2018

Can this share the same class Counter in former LeanCloud visitor count. If this do, the change maybe more smooth.

@xCss
Copy link
Contributor Author

xCss commented Jul 7, 2018

@sli1989
Yes, now it's the previous Counter class in former LeanCloud visitor count

The premise is the same application that used to be used by Valine (the same appid and appkey).

@sli1989
Copy link
Collaborator

sli1989 commented Jul 8, 2018

everything is fine. but seems if the view=0, there is blank view instead of 0 in meta. after clicking the post, the xid item was created, then the counter increase.

qq 20180708143549

@xCss
Copy link
Contributor Author

xCss commented Jul 8, 2018

@sli1989xid is currently a redundant field.
And your blog is http://saili.science/2018/06/24/cost-function/
I see the statistics on your blog are normal.

@sli1989
Copy link
Collaborator

sli1989 commented Jul 8, 2018

that's because i clicked the post. I mean the view=0 situation.

@xCss
Copy link
Contributor Author

xCss commented Jul 8, 2018

@sli1989
For view=0 or no view record, there is fault tolerance, All set to 0

@ivan-nginx
Copy link
Member

@xCss So, Valine will count visitors? It's current visitors counting (like was in HyperComments) or total counting for post (this function already exists in NexT, right?)

@xCss
Copy link
Contributor Author

xCss commented Jul 8, 2018

@ivan-nginx Yes, Valine has been able to count visitors, But this feature is optional.

The original NexT already contains this method, I think we need to give users more choices.

@ivan-nginx
Copy link
Member

@xCss so, for count visitors option user need to be loggen in Valine? I mean how about LC Security Plugin?

@xCss
Copy link
Contributor Author

xCss commented Jul 8, 2018

@ivan-nginx Sorry, Valine can't call _config.yml, but I think Valine can solve the security problem with the leancloud cloud engine. Follow these steps.

@xCss
Copy link
Contributor Author

xCss commented Jul 8, 2018

@ivan-nginx And the count visitors of the Valine is a private method, no external calls are provided.

@sli1989
Copy link
Collaborator

sli1989 commented Jul 8, 2018

For view=0 or no view record, there is fault tolerance, All set to 0

That's the problem, the view of new post is blank not the '0'...

@xCss
Copy link
Contributor Author

xCss commented Jul 8, 2018

@sli1989 , Sorry, the bug has been reproduced, this is my logical error.
I'll fix it in the next version of Valine.
Thx.

@sli1989
Copy link
Collaborator

sli1989 commented Jul 8, 2018

for count visitors option user need to be logged in Valine?

@ivan-nginx For now, it's the same function for leancloud counter without security setting. don't need to be logged. the same appid and appkey, the same steps to add Counter.

hexo-theme-next/_config.yml

Lines 635 to 645 in ec18888

# Show number of visitors to each article.
# You can visit https://leancloud.cn get AppID and AppKey.
leancloud_visitors:
enable: false
app_id: #<app_id>
app_key: #<app_key>
# Dependencies: https://github.com/theme-next/hexo-leancloud-counter-security
# If you don't care about security in lc counter and just want to use it directly
# (without hexo-leancloud-counter-security plugin), set the `security` to `false`.
security: true
betterPerformance: false

@xCss thanks. But for next user not use the integrated leancloud_visitors, we need to add some instructions about the steps to add class Counter alone.

@xCss
Copy link
Contributor Author

xCss commented Jul 8, 2018

@sli1989 In fact, the number of visits in the Valine will automatically create the class, and you don't need to manually create the Counter (of course you can create it manually)

There are no conflicts between the two methods, and leancoud_visitors have higher priority:
#diff-d4f96cf2cea92b9d7c8d95bb034e2eb2

- {% if theme.leancloud_visitors.enable %}
+ {% if theme.leancloud_visitors.enable and !theme.valine.visitor %}

@sli1989
Copy link
Collaborator

sli1989 commented Jul 8, 2018

Oh, U mean valine visitor automatically creates the class Counter? Well, that's great.

@ivan-nginx
Copy link
Member

@xCss @sli1989 i mean need to join this 2 counts in one, without confusing user. Docs or instructions maybe?

@sli1989
Copy link
Collaborator

sli1989 commented Jul 8, 2018

i mean need to join this 2 counts in one

Maybe not, it's the way provided by valine comments. We cannot force users to use valine. But some intructions needed to be added in valine setting section in _config.yml or doc about valine comment in HELP.


By the way, there seems a conflict about the sdk version (reinitiating problem with two version) when use leancoud_visitors and valine at the same time.

@xCss
Copy link
Contributor Author

xCss commented Jul 8, 2018

@sli1989 @ivan-nginx Don't force users to choose. Other comment systems still need leancloud_visitors, and the count visitors in the Valine can be used as an alternative.

In fact, the Valine uses the same class Counter as leancloud_visitor, except that the Valine adds a redundant field xid, the other is exactly the same.

@ivan-nginx
Copy link
Member

ivan-nginx commented Jul 8, 2018

I mean that's cases:

  1. If user use Disqus system and use internal LC (with or without LC Sec plugin), it's okay.
  2. If user want to replace Disqus with Valine, but he still use internal LC (with or without LC Sec plugin), he can not turn on Count in Valine.
  3. Same as above, but if he turn on Valine count then what? Will use Valine's counter with or without LC Sec plugin?

@sli1989
Copy link
Collaborator

sli1989 commented Jul 8, 2018

If user want to replace Disqus with Valine, but he still use internal LC (with or without LC Sec plugin), he can not turn on Count in Valine.
Same as above, but if he turn on Valine count then what?

There seems a conflict about the sdk version (reinitiating problem with two version, don't how to solve) when using internal leancoud_visitors and valine comment at the same time. So we maybe suggest users turn off the internal leancoud_visitors when using valine comment.

<script src="//cdn1.lncld.net/static/js/3.0.4/av-min.js"></script>

<script src="https://cdn1.lncld.net/static/js/av-core-mini-0.6.4.js"></script>

As said before, there are no conflicts between Counter class introduced by the two methods. This can be seen as redundant.

Will use Valine's counter with or without LC Sec plugin?

Need @xCss to develop maybe.

@ivan-nginx
Copy link
Member

Need @xCss to develop maybe.

i mean need to join this 2 counts in one, without confusing user

If it's possible, yes.

@sli1989 sli1989 added this to the v6.4.0 milestone Jul 12, 2018
@sli1989 sli1989 merged commit 7d17575 into theme-next:master Jul 14, 2018
@LEAFERx
Copy link
Contributor

LEAFERx commented Sep 24, 2018

And the count visitors of the Valine is a private method, no external calls are provided.

@xCss
But AppKey and AppId are still available for attackers. Attackers can init another AV object and modify Counter class, and maybe even contents of comments. It still need further ACL configuration. The LC Sec plugin makes the process of adding page item into db locally so that attackers cannot add their data by AV object. And we still need Leanengine to ensure the count number been updated in the right way, which is +1 every visit, since the ACL to anaymous must be CAN UPDATE to let the count number grows. Maybe Valine comment doesn't have these problems by right ACL choices, but counter system should have I think.

Jona-lee pushed a commit to Jona-lee/hexo-theme-next that referenced this pull request Oct 10, 2018
* Update valine comment system version

* Update valine visitor annotation

* Update valine instruction
tongluyang pushed a commit to tongluyang/hexo-theme-next that referenced this pull request Nov 19, 2019
* Update valine comment system version

* Update valine visitor annotation

* Update valine instruction
@stevenjoezhang
Copy link
Contributor

stevenjoezhang commented Nov 26, 2019

Docs updated in 8c1b655
See also https://github.com/theme-next/hexo-theme-next/pull/926/files

GitHub
PR Checklist Please check if your PR fulfills the following requirements:

The commit message follows our guidelines.
Tests for the changes was maked (for bug fixes / features).

Muse | Mist ha...

@sli1989 sli1989 mentioned this pull request Jan 15, 2020
15 tasks
@theme-next theme-next deleted a comment from 241zhiyuan Jan 22, 2020
@theme-next theme-next locked and limited conversation to collaborators Jan 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants