Posted by earnie on May 1, 2009 at 6:31pm
5 followers
| Project: | XML sitemap |
| Version: | 6.x-1.x-dev |
| Component: | xmlsitemap_node.module |
| Category: | task |
| Priority: | critical |
| Assigned: | earnie |
| Status: | closed (fixed) |
Issue Summary
I don't know why others didn't have this issue but @mdlueck in #379854: The site map is not being populated had such a simple dataset that it triggered it for him.
Comments
#1
Here's the patch.
#2
Patch doesn't look right. Shouldn't it return a disabled link if the post threshold is greating that posts?
#3
You want to disable the link if the user has posted more than his share for the sitemap. @mdlueck has 4 nodes in his DB and a post threshold of 100. Only when the number of posts are greater than 100 should the link be disabled from the sitemap.
This change
<?php$posts_threshold = variable_get('xmlsitemap_node_posts_count_'. $node->uid, variable_get('xmlsitemap_node_posts_count', 100));
$posts = (integer) db_result(db_query("SELECT COUNT(nid) FROM {node} WHERE uid = %d", $node->uid));
- if (!user_access('by-pass the authored nodes check', $user) && $posts_threshold > $posts) {
+ if (!user_access('by-pass the authored nodes check', $user) && ($posts > $posts_threshold)) {
return XMLSITEMAP_LINK_DISABLED;
}
if ($node->status) {
?>
will return XMLSITEMAP_LINK_DISABLED if the users posts are greater than the threshold which is what is wanted.
#4
The note on the project page says "The latest 6.x-1.x-dev commits added a setting, which can be set globally or for each user, to limit the nodes included in the sitemap to the ones authored by the users who created 100 nodes (by default); change that setting according to the trust level you have for the users of your web site."
I take this to mean that any nodes written by users that have written less than 100 nodes, then that node should not be excluded in the sitemap. Then a user that comes to your site and just creates one node and never does anything else wouldn't have their content listed when regular authors should. Did this feature change or am I just not getting it (could very well be a possibility)?
#5
The idea of the setting is that the content created by a user is inserted in the site map if the number of posts made from the user is greater than a limit that can be general, or set for each user.
The code I wrote is correct.
!user_access('by-pass the authored nodes check', $user)will returnTRUEif the user doesn't have the permission to by-pass the check;$posts_threshold > $postswill beTRUEonly when the number of posts made from the user is less than the limit.#6
Then we need to change the default limit to zero. Having a limit of 100 for any user OOB before they see data is wrong.
#7
Maybe we should have this set to '0' by default? That way new users who are testing the module will see all the content in the sitemap that they are expecting. Users that need this setting will be free to adjust it as they need.
#8
Cross-posted, but I have a patch ready for review. :)
#9
And the settings on the users page is the wrong place IMO. I missed it totally. Maybe both but definitely should be at admin/settings/xmlsitemap. Throwing settings to other pages is asking for problems.
#10
Thanks, Dave.
#11
BTW, I love code/peer review. :)
#12
Hee hee hee,.. You have had a busy day both of you. Thanks much!
Please say when I should install the -dev build. I will watch this thread for the "when". Thanks!
#13
I am changing the report data to the correct values.
#14
I am not sure there is a default value that is valid for all cases.
If you set that setting to a value too little, then you will start to see links to spam nodes that then will be removed from the content, causing some search engines to complain that the link causes a 404 error (we all know which search engine does that :-)); if you use a too high value, then the content will not appear in the site map in quick times.
Actually, the last sentence is not correct. There is a permission that allows some roles to by-pass the check on the number of posts made; as the permission is automatically given to the first user, the test of the site map should not give problems. The testing of the site map is normally done on a test site; for a testing site is still possible to change the global setting, and not touch the setting for each single users.
#15
I understand. I can live with a default value of 5 and moving the settings form to the admin/settings/xmlsitemap page. It is really hidden on the admin/settings/users page since it falls below the email blobs and one doesn't expect to find settings for one module scattered in the settings of other modules.
#16
I'm sorry but I just don't disagree. A default value of 1 makes the most sense to me as this should operate as simply as possible and not making things more complicated for users. Users coming and testing the 6.x-1.x-dev version will keep creating support requests since their content doesn't show up, or they won't understand why it's all of a sudden showing up now that they've authored 5 or more nodes.
I agree with earnie that moving the post threshold setting to the admin/settings/xmlsitemap page is the first step. But I believe that the default needs to be 1. I can't make my case for it any more than that, so I'll leave it at that. :)
#17
@#15: Actually, that page already contains settings coming from other modules.
@#16: Settings that are specific for a user are usually in the user profile page; the options in the project settings page are general.
The posts threshold is thought also to avoid the problem that a search engine (but maybe the problem is common to other search engines) has; Google Webmaster Tools (which is the only search engine that I know it has the problem) has the habit to warn the user when it finds a link that returns a 404 error. It doesn't matter if the link has been removed in the last site map it accessed; it will keep to emit the warning until the site administrator will say to Google Webmaster Tools it must remove that link from its internal database.
#18
The default value for the posts threshold should be a value that would not allow a spammer to submit spam content, which is then included in the site map.
The suggested value of 5 would be a number of posts that a spammer would easily reach in a day; therefore, his posts would be included in the site map, and the site administrator should say to Google Webmasters Tools that such pages don't exist anymore (once he removed the spam nodes).
I think it would be better if the code would be put in a module that the user can optionally enable, if he really needs it. To set the default posts threshold to 1, when it is possible to assign a permission to the rules that don't need that check, doesn't make sense (IMO).
#19
I think 2 out of 3 wins in contests. I disagree Kiam, if your running a site where SPAM is present you're already looking for the options to stop the SPAM. Preventing users from getting their data OOB isn't correct.
#20
The option is not just for spam, actually.
If the option would even be used just for spam control, setting the default value to 1 would be like disabling the feature. Nobody is looking for an option that would help with the spam in a project like XML Sitemap, and at the time the administrator would notice that an option in XML Sitemap can help with spam, the spam content would be already be in the site map.
Rather than using a default value of 1, why don't we remove the feature (which seems the implicit solution that is not going to be said)?
#21
I've copied the form settings from the form_user_admin_settings_alter to the form_xmlsitemap_settings_alter in the xmlsitemap_node module and combined it with Dave's patch. Note that I left the original alter code in place changing the default value.
#22
Oops!
#23
I am not sure the access check is correct in the module settings page; I am referring to the line
<?php'#access' => user_access('override profile settings') || user_access('administer users')
?>
I still think that the best thing to do is to remove the feature from the module; it should be in a module that is enabled from the user. In this way, he will know that the site map is not being populated because he enabled a module that purposely delays the moment a new link is inserted in the site map; there would not be any report about the site map not being populated, and the feature would be used in the right way.
#24
#25
Ack!!!
Not for 1.x! Especially if it is in 5.2!
#26
<?php'#access' => user_access('override profile settings') || user_access('administer users')
?>
What else should it be?
#27
Well, it should be completely removed.
#28
What is the problem? It can be removed from both the branches. Actually, I don't remember I implemented the same feature for the Drupal 5 branch.
I still have to understand the reason to implement a feature that is then not enabled, or that uses some default values that don't allow it to correctly work.
What I am saying is that we should remove the feature from xmlsitemap_node.module; it can be implemented in a different module that would be enabled from who really needs such feature. It doesn't even need to be a module part of this project too.
As far as I can see, nobody is using this feature, if not to change the default value to something that disable the check on the number of the posts made from user. Being this the case, the feature can be removed.
#29
Scope creep. I have no problem with the idea, the timing of the idea doesn't fit the release schedule. However, if the option isn't available in a 5.x release currently then yes it is the best solution.
#30
We simply need to remove the feature; if we will create the module as I described it, it's not said that it must be included in the main project, and actually I consider the best solution to release that module separately from XML Sitemap.
#31
@#20 Kiam@avpnet.org:
A bit different than this features intended purpose... but I have been concerned with auto resubmit upon a node being created / changed. I would rather never get a nastygram that soandsosite.com is resubmitting the sitemap too many times.
Perhaps as a module setting (not a per-user setting) a value of "submit the site map right away after n commits" or else it would update the sitemap, just not notify the search engines. Have that value tracked on a per-UID basis, but allow it to be set globally and not on a per UID basis.
But it should always update the sitemap, just maybe not always auto-notify that an update occurred. Always updating would have never caused this problem.
#32
Kiam, you've convinced me. This patch needs tested but thought I would throw the initial take out for review and comment.
@mdlueck: You're off topic for this issue.
#33
Thumbs up for the patch.
I know I should not consider the patch reviewed and tested by the community (as somebody would tell me), but I change the status so earnie knows the patch has been reviewed by at least a maintainer.
#34
#35
Automatically closed -- issue fixed for 2 weeks with no activity.