Development

Download

development@drupal.org

September 2005

78 participants
615 discussions

[drupal-devel] [task] Improvments to Filter UI
by Bèr Kessels 14 Sep '05

14 Sep '05

Issue status update for http://drupal.org/node/27364 Post a follow up: http://drupal.org/project/comments/add/27364 Project: Drupal Version: cvs Component: filter.module Category: tasks Priority: normal Assigned to: Bèr Kessels Reported by: Bèr Kessels Updated by: Bèr Kessels -Status: patch (code needs work) +Status: patch (ready to be committed) Moshe. I agreed with you, and was against the name change. In this patch at least. However, it slipped in. Stetting code to be committed for it has gone trough so many review cycles that this patch has MUCH better code than the rest of the filter module, which REALLY needs a big overhaul :) Bèr Kessels Previous comments: ------------------------------------------------------------------------ Sun, 24 Jul 2005 09:30:54 +0000 : Bèr Kessels This is a mockup for the Filter UI. Currently it is soo complex, that I dare to call it broken. Please comment on this, for I will not ake any patches, when the general idea is disliked. The idea is t split filters and input formats better. Filters are filters, defined in modules. Input formats are bundles of filters. This is how we have it ATM, but the interface fails to communicate that. I tried to develop a consistent (with the rest of Drupal) interfce that makes it clearer what is what. The menu is as follows: * administer ** .... ** settings *** input formats *** filters ** ... ------------------------------------------------------------------------ Sun, 24 Jul 2005 09:37:10 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/input-formats_list1.png (16.34 KB) * administer ** .... ** settings *** input formats >> see attachement 1 *** filters ** ------------------------------------------------------------------------ Sun, 24 Jul 2005 09:39:02 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/input-formats_add.png (23.42 KB) * administer ** .... ** settings *** input formats >> see attachement 2, tab 2 *** filters ** Note: The default checkbox lmay seem a bit odd. But checking it will remove the "default status" from the current "default" one and make this one "default". The help text, and a drupal_set_message should explain this. ------------------------------------------------------------------------ Sun, 24 Jul 2005 09:40:08 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/input-formats_edit_format.png (22.17 KB) * administer ** .... ** settings *** input formats >> clicking a 'configure' link in the table of attachement 1 *** filters ** ------------------------------------------------------------------------ Sun, 24 Jul 2005 09:40:55 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/input-filters_list.png (36.24 KB) * administer ** .... ** settings *** input formats *** filters >> see attachement 4 ** ------------------------------------------------------------------------ Sun, 24 Jul 2005 09:41:40 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/input-formats_edit.png (22.63 KB) * administer ** .... ** settings *** input formats *** filters >> clicking a 'configure' link in the table of attachement 4 ** ------------------------------------------------------------------------ Mon, 25 Jul 2005 14:46:27 +0000 : Bèr Kessels IMO this makes the interface easier. But other disagree. And because it also removes one feature: (being able to use E.g. HTML in different input formats, with different settings) I'll simply wont fix this :( ------------------------------------------------------------------------ Thu, 28 Jul 2005 13:36:12 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy.patch (10.28 KB) I decided to go for it anyway. Be it in a slightly different way then my initial mockups. The difference is that I left the filters where they are, hidden beneath the formats. So, here is the patch that makes the filter UI more consistent with screens like blocs, menus, vocabularies et al also nice to note is that: 93 lines are added, 104 are removed. So netto we have less code :) (cvs diff filter.module | grep ^+ | wc -l) ------------------------------------------------------------------------ Thu, 28 Jul 2005 13:37:02 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/input-formats_list_html.png (44.64 KB) Here is how the listing now looks. ------------------------------------------------------------------------ Thu, 28 Jul 2005 13:37:57 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/input-formats_edit_format_html.png (113.71 KB) And the "configure" screen. The 'add' screen looks exactly the same, only with the default values pre-filled. ------------------------------------------------------------------------ Thu, 28 Jul 2005 13:39:19 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/input-formats_edit_format_too_long.png (121.82 KB) Oh, And here is a really nice example of why the current screen is no good. And here I "only" have four roles. I run a site with 12 roles, imagine this screen then! ------------------------------------------------------------------------ Thu, 28 Jul 2005 13:48:35 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy_HEAD.patch (10.24 KB) And here is a pathc for HEAD (I forgot that I developed this on a 4.6.2 codebase. Sorry) ------------------------------------------------------------------------ Thu, 28 Jul 2005 13:55:25 +0000 : stefan nagtegaal I like this very, very much! This is - indeed - much better than the current UI. This always fits, and makes the life of people who use a fixed width theme much nicer.. Ber, FYI (you probably know this too), 'admin/access' does also break fixed width themes due to the fact that it expands the width of the table when more roles are being added.. Good catch! I like it much more this way.. +1 from me... ------------------------------------------------------------------------ Fri, 29 Jul 2005 07:27:34 +0000 : Dries The code style needs work (spaces, tabs, 'as' -> 'AS'), the code comments need work, and debug statements should be removed. Screenshots look good to me. db_query('UPDATE {filter_formats} SET cache = %d, name = \'%s\', roles = \'%s\' WHERE format = %d', (int)$cache, $name, $roles, $format); can be written better/shorter quote-wise. ------------------------------------------------------------------------ Fri, 29 Jul 2005 09:04:16 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy_HEAD_0.patch (10.37 KB) Dries, thanks a lot for the review. OI fixed your comments. Or so I hope. code-style.pl was of no use (can that *please* be removed from core?) because it chokes on the help texts. It also pointed out a lot of old formatting isues, whic, when fixed wuold flood this patch with unrelated fixes. I tried to narow my style-searching to the areas I touched. And I made the comments more verbous. The query is fixed. it saves no characters, but looks better now. Thanks for the tip. The print_r..... blush. thats what I get for not running my default grep -r print_r on my code after finishing up... I could not find a lot of spaces and tabs. So it lmight be that i missed one. I triple checked it though. ------------------------------------------------------------------------ Fri, 29 Jul 2005 09:32:21 +0000 : Steven Moving the role settings inward does fix the wide-table problem, but it makes the filter configuration much more opaque. We could perhaps fix this by showing the roles as a comma-separate list in the input formats table. Such a list can wrap and will not stretch the table. Other than that: * Having the instructions for the Roles and Filters form_group() at the bottom is a bit weird. The distance is too big. * The table that was used for toggling filters on/off was much more compact before, and IMO a lot clearer. Why change to a flat list with the description staggered below each item? ------------------------------------------------------------------------ Fri, 29 Jul 2005 10:31:30 +0000 : Bèr Kessels consistancy, consistancy and more consistancy. There really is nothing worse that having a different 'concept' of UI for every thing in Drupal. We are going in the right direction, with the blocks being editable objects, menus acting like that, taxonomy, users, etc.. I wanted to make this act similar. I am not at all happy with the fact tah the list is still a form. But we need this, for setting the default. A big -1 for a comma-separate list. That is extremely hackish, even worse useabilitywise and just silly * we should avoid typing when not needed. * we should avoid errors on input, when they can be avoided (a typo is made very easy, I know all about it) Form set errors won't fix that usability -wise. I agree that you sort-of loose the overview. But IMO that is a minor loss comared to what we gain, usability-wise. The wtoo-wide was onlywhat triggered me. But do not think that I am noly making this patch to fix that issue. This patch is there to improve usability, through consistancy. o make drupal behave the same in most places in admin. If you know one screen, you know most of them. We really should move away from custom-hacks for every page, because that is useability rule #1: consistancy. Stadardisation, even if sometimes a custom hack might make things easier in that single case, it will reduce your overall usability so much that nearly ever it is worth that hack. That select-boxes instead of the table issue: consistancy. As well that it completely broke fluid CSS layouts, my solution is more consistant, and uses forms the way they are meant. If we cannot use $descritption in checkboxes they should be removed. but IMO it feels much more consistant. Tables are for tabular data, and this is simply a list of items, with additional data. And i do not really get what you mean with "too far down". Do you mean that you would like to see the order of the groups different? ------------------------------------------------------------------------ Fri, 29 Jul 2005 13:31:24 +0000 : Dries I haven't tested Ber's patch yet but looking at the screenshots, it like what I see. I remember being confused by the current filter UI, and often, I still am. ------------------------------------------------------------------------ Fri, 29 Jul 2005 19:53:26 +0000 : moshe weitzman I dislike the recent movement away from overview pages. I think we can still provide and overview while allowing editing to happen in a dedicated form. An example in the wrong direction is 'content types' admin. It is impossible to get an overview, and the listing page is worthless. I agree with Steven on this one. It is true that consistency is desirable. But consistent crap is not. ------------------------------------------------------------------------ Fri, 29 Jul 2005 20:02:39 +0000 : killes(a)www.drop.org I agree with Moshe. Getting a quick overview about what is set how is essential for an admin. ------------------------------------------------------------------------ Sat, 30 Jul 2005 00:27:15 +0000 : Robrecht Jacques Overall I like the screenshots. Did not test the patch. Bèr, what I think Steven (#15) meant by "showing the roles as a comma-separate list in the input formats table" is that as an overview page it would be better if you added a list (not an input box!) to screenshot "input-formats_list1.png" (comment #1). Add a column that reads "Roles" (or "Enabled for") and then for each row a comma seperated list of the roles that can use this input format (or maybe even better a HTML list). You would still need to click "configure" to _change_ the roles. The "Default" text (not a radio button) you already show in the "Options" column is something similar: it immediately shows that this input format is the default without you having to go to the "configure" page. Why not do the same for "roles"? If this is what Steven meant, then I tend to agree with him. And to help moshe and killes: maybe you could even add a column that _lists_ the used input filters. Again, only a list, not a way to change it. That's what the "operations" column is for. A table like that will "fluid" better than what we have now, still it is a _complete_ overview of the settings. I could have misunderstand someone... I have the tendency to do that... ------------------------------------------------------------------------ Mon, 22 Aug 2005 08:21:32 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_interface_improvements.patch (14.52 KB) by popular demand: a comma separated list. ------------------------------------------------------------------------ Mon, 22 Aug 2005 08:24:31 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/format_now_contains_roles.png (22.09 KB) and a screenshot ------------------------------------------------------------------------ Mon, 22 Aug 2005 18:39:44 +0000 : Dries Please check your use of print theme('page', $output). Normally, you just return $output. ------------------------------------------------------------------------ Mon, 29 Aug 2005 18:54:28 +0000 : Bèr Kessels All instances of theme('page',...) in filter.module are now changed to return ... ; furthermore, the patch is the same. ------------------------------------------------------------------------ Mon, 29 Aug 2005 20:04:02 +0000 : Uwe Hermann Bèr, I think you forgot to attach your updated patch. ------------------------------------------------------------------------ Mon, 29 Aug 2005 20:27:39 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_interface_improvements_2.patch (13.52 KB) my email client has this nice warning system for attachements. Maybe drupal should search for the word attachement too, and when no att. found give an error ;). Or maybe i should just learn to pay attention. Guess that is easier. Anyway, here it is. ------------------------------------------------------------------------ Thu, 08 Sep 2005 20:22:54 +0000 : Dries I wanted to apply this patch but: 1. I can't change the default input format. 'Save'-button doesn't work. 2. I got confused by the fact I can't change the roles of the default filter. I think the form group description should explain this. 3. form_group(t(filter)) should be form_group(t('filter'), ...). ------------------------------------------------------------------------ Thu, 08 Sep 2005 20:51:24 +0000 : m3avrck Found a few more issues: If I go into 'Configure' for 'PHP Code' and click the 'Configure' tab at the top and then click 'list filters' link in the text, I get this warning with PHP5 warning: Missing argument 1 for filter_admin_format() in \drupal\modules\filter.module on line 378. Also, it is sort of confusing as to what the 'List' tab implies. For example, if I click on 'input formats' in the admin menu, I get a list of the current filters. When I click 'configure' for one of those, I see the options for that filter. However, it says 'list' in one of the tab, which doesn't really mean much. I thought that was the list of filters, not the options for that filter. That should be cleaned up. Also, there is no way to get back to the full list of filters unless you click on 'input formats' in the admin menu. A setting/link to fix this would be great. Also, where is the link to add filters??? I think the options/layout/tabs should mimic of that of admin > users. So on admin > input formats, it has the tabs 'list', 'add format' ... very clear and consistent. On a configure page for a filter, it should have 'list', 'view', 'configure', 'rearrange' ... this would make it easier to navigate and get back to the original list of filters. Make sense? I think this and Dries' comments put this patch over the top :) ------------------------------------------------------------------------ Fri, 09 Sep 2005 01:48:55 +0000 : tag In my initial battle to figure out this module's (quite nice) functionality, what tripped me up the most was actually the nomenclature. The way I keep it straight in my head now is to translate "input format" to "filter set". If you view those (the current) screens with this in mind, I think it's a lot easier to understand what's what. It sort of describes this in the help text, but the terms "filter" and "input format" don't really convey the relationship between the two -- there's no way to intuit that an "input format" is a collection of "filters". Not to my mind at least... and well, we know nobody reads instructions... Are there any technicalities that make renaming "input format" to "filter set" not accurate? Or does someone have a better term/terms to better show the relationship of these elements? (I'm aware of filters in servlet architectures but I don't think there's much of a collision with that usage). Anyhow, I guess this doesn't quite address the UI controls and/or layout, but would still help a lot. ------------------------------------------------------------------------ Fri, 09 Sep 2005 06:35:34 +0000 : Bèr Kessels I prefer filter set. In fact: I like it a lot. Anyone else ideas on this? ------------------------------------------------------------------------ Fri, 09 Sep 2005 10:18:18 +0000 : Dries I too had problems with the terminology; it took me months to get used to 'input formats' versus 'filters'. Using 'filter sets' might improve the situation -- especially from an administrator's point of view when you have to wrap your head around the UI/difference. I'm not native English, but 'filter set' sounds more explanatory, yet slightly more geeky so I'm not sure if it flies for users who don't need to understand the underlying concepts (eg. under the node and comment submission forms). ------------------------------------------------------------------------ Fri, 09 Sep 2005 11:00:05 +0000 : Bèr Kessels a heads up: I ma redoing the patch. But decided no to change the name yet. That is a too big change. IT should be in a separate issue, IMO. ------------------------------------------------------------------------ Fri, 09 Sep 2005 15:27:43 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy_2.patch (13.57 KB) * Bugs as reported by dries fixed. * The [add] tab not appearing is due to your menu caching, refresh that please. * Fixed an additional bug: Some agents do not send TRUE for disabled checkboxen. Also in current filter admin. and people: we have a problem. Deleting a filter trows errors, due to the revisions changes. that happens in filter module now, but also after this patch. I have too little knowledge of the filter system to fix that, and IMO that is a separate issue. It should be fixed right after this patch is committed. ------------------------------------------------------------------------ Fri, 09 Sep 2005 15:57:18 +0000 : Bèr Kessels FYI: the delete bug is waiting here: http://drupal.org/node/30781 ------------------------------------------------------------------------ Fri, 09 Sep 2005 18:18:17 +0000 : m3avrck Get this warning in PHP5: Warning: Missing argument 1 for filter_admin_format() in \modules\filter.module on line 379 I have traced this error back to line 240. Why are there no callback arguments like above on line 235? Looks like a source of a problem/bug here so needs to be addressed, not sure exactly how to fix it myself (otherwise I would). Should be easy it seems. Also, for a quick and easy usability improvement, on line 239, change t('list') to t('view') ... makes the menus less confusing. After those fixes, looks like this patch is ready to be committed :) ------------------------------------------------------------------------ Fri, 09 Sep 2005 18:23:27 +0000 : m3avrck Also, this dialog is a bit confusing: "Choose which roles may use this filter format You are editing the default format. For the default format, all roles must be enabled. Therefore you cannot change them. " Maybe make it so that first line doesn't appear there on that page? ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:35:50 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy_2_0.patch (13.59 KB) changed that line of help. Can this please still be taken in consideration before the freeze? ------------------------------------------------------------------------ Tue, 13 Sep 2005 18:29:28 +0000 : m3avrck Can't apply patch, getting error: Patch malformed at line 287 ------------------------------------------------------------------------ Tue, 13 Sep 2005 19:10:07 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_interface_improvements_2_0.patch (14.19 KB) strange. the dowloaded one breaks too, yet the one i uploaded stll works. anyway, rerolled ------------------------------------------------------------------------ Tue, 13 Sep 2005 19:25:25 +0000 : Dries IMO, the form function needs more work. You need to group the form construction and the form saving. Right now, when you don't provide a title, you are given an error and an emptied form. This may be a left-over from the original code though but it would be cool if you could tidy this up. The form_group() description still need a trailing point. There is a broken link in the following help text on the 'add input format'-page: /If you notice some filters are causing conflicts in the output, you can [rearrange] them./. ------------------------------------------------------------------------ Tue, 13 Sep 2005 19:30:12 +0000 : Bèr Kessels dont have time anymore. sorry. ------------------------------------------------------------------------ Tue, 13 Sep 2005 20:50:34 +0000 : m3avrck new patch soon! ------------------------------------------------------------------------ Tue, 13 Sep 2005 22:10:25 +0000 : m3avrck Attachment: http://drupal.org/files/issues/filter.module_10.patch (20.27 KB) Ok here's a new patch. Lot's of fixes in this one (all of the ones from above except the one noted below), including support for the new revisions patch. Additionally, 'input formats' has been renamed to 'input filters'. After debate on IRC, and the above comments in this post, it seems that 'input formats' is somewhat misleading and doesn't make sense. 'input filters' clears this up and makes sense to both native and non-native English speakers (as tested in IRC anywho ;)). This also makes more sense since Drupal refers to everything as 'filters' and not 'formats'. However, there is one issue still left unresolved: If you create a new input filter, and don't specify a title, page reloads clearing all filled in fields and prompts the error message. Additionally, a blank filter is created. Wanted to get this patch posted, I'll look at it later today but if someone can get this fixed easily, please do, thanks! ------------------------------------------------------------------------ Tue, 13 Sep 2005 22:38:05 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_interface_improvements_3.patch (21.49 KB) this patch should fix the issue m3 rasies abuot amepty formats being created. Yes, i use a simple drupal_goto, in case of a for error. And no, it is not very easy to get the old posted data back. ------------------------------------------------------------------------ Tue, 13 Sep 2005 23:44:26 +0000 : moshe weitzman the most visible place for this 'input format' term is on the node form. thats where ordinary users see this stuff. in that context, i don't think 'input filters is an improvement over input formats. the question we are asking our users is 'what is the format of your post'?, not what filters do you want applied ... just my .02. sorry i wasn't in IRC when this came up. this need not hold up the whole patch.

1 0

[drupal-devel] [feature] Replace core archive.module w/ codemonkeyx archive.module
by Bèr Kessels 14 Sep '05

14 Sep '05

Issue status update for http://drupal.org/node/29676 Post a follow up: http://drupal.org/project/comments/add/29676 Project: Drupal Version: cvs Component: archive.module Category: feature requests Priority: normal Assigned to: Morbus Iff Reported by: Morbus Iff Updated by: Bèr Kessels Status: patch (code needs work) Dries, I can see your point. The navigation interfce feels odd. But after some time I found this UI to be VERY good. it just feels odd because you are used to a calendar, yet this really is a better interface for browsing dates. It requires less clicks, offers all possible options in one glance etc. If we don't go for this archive module. Then please pretty please, remove at least the other one from core. It is completely useless as Boris states Better that people must use contribs than something that makes us look like we code crappy modules for core. Bèr Kessels Previous comments: ------------------------------------------------------------------------ Thu, 25 Aug 2005 21:08:49 +0000 : Morbus Iff Over at http://drupal.org/node/8287, Berkes mentions that the core archive.module was considered being removed, per a discussion at the Drupal Sprint. Kjartan also mentions he would "love to have the archive module improved in general." In chatting with chx about this, he mentioned codemonkeyx's rewrite sitting in contrib/modules/archive/. I'll be doing some work with the archive.module over the next few days, and will be basing my changes around codemonkeyx's version, and making it compatible with HEAD. This general Issue is to move codemonkeyx's version into HEAD as a replacement to the existing archive.module. An unknown version of his replacement can be seen at http://www.codemonkeyx.net/archive. I'll be running a live HEAD version soon as well. These patches were made during the customization of Drupal by http://www.NHPR.org. In loving support of open source software, http://www.NHPR.org will continue to contribute patches they feel the community will benefit from. Questions about this patch should be directed to morbus(a)disobey.com. ------------------------------------------------------------------------ Fri, 26 Aug 2005 19:45:59 +0000 : Morbus Iff Attachment: http://drupal.org/files/issues_2 (9.56 KB) As an example of a very early revision, see the attached, with the following changes from the current contrib CVS: * removed the year offset from theme_archive_navigation_years, which controlled how many year links to show at once in the top nav. For those with sites with more than five years, they'll WANT people to notice that they have five years, not to have to click on the earliest date and then have their expectations changed. * made the "created > $date" in archive_buildQuery "created >= $date" instead, to allow posts that were created at exactly midnight that day (like me, by design). * since there's no block, I made the menu item visible upon first load. this menu item is given "access content" permissions. More to come, including doxygen and gmt considerations. ------------------------------------------------------------------------ Fri, 26 Aug 2005 19:47:41 +0000 : Morbus Iff Might as well start getting a review of it so I can fix 'em as they come in. ------------------------------------------------------------------------ Fri, 26 Aug 2005 19:56:49 +0000 : Tobias Maier cant you provide a patch file? thanks for your work ------------------------------------------------------------------------ Fri, 26 Aug 2005 20:04:01 +0000 : Morbus Iff The codemonkeyx version is a complete rewrite of the core archive.module. If I were to create a patch file against core, every line would be deleted, and every line would be new. Once I finish my revisions to codemonkey's version, I'll post the final version here, as well as a patch against his current CVS. ------------------------------------------------------------------------ Fri, 26 Aug 2005 20:09:13 +0000 : Tobias Maier ok, thanks again :D ------------------------------------------------------------------------ Mon, 29 Aug 2005 13:41:43 +0000 : Junyor +1 for this change. The archive.module in core is dead. ------------------------------------------------------------------------ Mon, 29 Aug 2005 16:14:30 +0000 : adrian What is the progress on this morbus ? ------------------------------------------------------------------------ Mon, 29 Aug 2005 16:29:13 +0000 : Morbus Iff Adrian - I'll be attaching a new version either later today or tomorrow, with a CHANGELOG. I'll also be running a live version of it over on NHPR.org for people to play with. The three major things I'm worried about right now are a) doxygen, b) variable/function naming, c) GMT considerations. After those, I'll be exploring a patch for my own needs: the ability to get archives for particular term. ------------------------------------------------------------------------ Mon, 29 Aug 2005 18:53:34 +0000 : Morbus Iff Attachment: http://drupal.org/files/issues/archive.module (9.93 KB) Here's the latest, with the following changelog: * reordered some routines to be a little more workflowish. * renamed archive_buildQuery to archive_build_query. * general whitespace and formatting cleanup. * HEADish update: returning $output, not page templating it. * removed the reference of &$ad in archive_build_query. * test for the existence of arg(#)'s before validating them. * archive_validSomething changed to archive_valid_something. * removed unused vars: cur_date, cur_date_end. * renamed archive_buildURL to archive_build_url. * removed the HTML whitespace from the theming. * twiddled a lot of quotes and apostraphes. * removed 'future' CSS class. ill-defined. * reordered/renamed the CSS classes. ------------------------------------------------------------------------ Mon, 29 Aug 2005 18:54:08 +0000 : Morbus Iff Attachment: http://drupal.org/files/issues/_p_29676_archive_css.patch (1.56 KB) And the drupal.css patch. ------------------------------------------------------------------------ Mon, 29 Aug 2005 18:56:29 +0000 : Morbus Iff This version of the module is currently running live at http://www.nhpr.org/archive/. ------------------------------------------------------------------------ Mon, 29 Aug 2005 19:59:57 +0000 : Tobias Maier if i click for example on 2003 it would be good if this would go to january or december and marks them that this one will be shown now as you can see it if you click on january 2003. it has to select * on the first: the first month of writing * on the last: the last month of writing * on every else: january I hope you can understand what I mean... greets tobias ------------------------------------------------------------------------ Mon, 29 Aug 2005 20:17:40 +0000 : Morbus Iff Attachment: http://drupal.org/files/issues/archive_0.module (11.28 KB) Alright. I've attached another new version that adds a new feature that wasn't part of the original codemonkeyx CVS, but was chatted about on the devel list back in April. If this particular feature has bad code or needs heavy refactoring, certainly consider ONLY the version in comment #9 (and the matching drupal.css patch in #10). This new version supports dated archives based on taxonomy tids. It was a quick addition which NHPR.org needed (for the date nav; the normal tid archive pager wasn't strong enough for our needs). Since it was a quick addition, it supports only ONE tid at a time - the 'and/or' syntax for the taxonomy.module was not brought over. If that syntax was desired, it'd make more sense to create some sort of API for archive.module so that other nodes can take advantage of the dated nav in their normal pages (like node types, users, forums, etc.) The added code supports term matches at any granularity: # all node types that match tid 15000 ('The Front Porch') http://www.nhpr.org/archive/term/15000 # all 2005 node types that match tid 20 ('Health') http://www.nhpr.org/archive/2005/term/20 # all March, 2003 node types that match tid 9 ('Education') http://www.nhpr.org/archive/2003/3/term/9 # all July 11, 2003 node types that match tid 49 ('Economy') http://morbus.totalnetnh.net/nhpr/archive/2002/7/11/term/49 ------------------------------------------------------------------------ Mon, 29 Aug 2005 20:27:26 +0000 : Tobias Maier what does this mean? "Story Archives of 'archives' " on http://www.nhpr.org/archive/term/15000 should this maybe named? "Archive of 'The Front Porch'" if I go to http://www.nhpr.org/archive/term/20 I can only read "archives"... why is there a difference? - I never tested this module on my test site, because I'm not at home - ------------------------------------------------------------------------ Mon, 29 Aug 2005 20:29:34 +0000 : Morbus Iff Tobias: that wouldn't be possible, at least not accurately. The new archive.module supports browsing by year, month, and day, as you know. archive/2005 loads up all the data from a particular year and starts creating a pager out of it. Consider if you have 3 posts in December, and 15 posts in November. It wouldn't be "right" to highlight December because the pager display for the entire year would also include some of November's entries (since 3 is less than the pager increment). Likewise, if we ONLY showed the items from December, then we wouldn't have a "pager by year" feature, only a "pager by month (defaulting to December when none is selected)" feature. ------------------------------------------------------------------------ Mon, 29 Aug 2005 20:30:41 +0000 : Morbus Iff Tobias: regarding #14, that's an artifact of the templates that I'm using for NHPR, and has nothing to do with archive.module itself (in fact, once the anonymous cache expires, you'll see that little oopsie go away). ------------------------------------------------------------------------ Mon, 29 Aug 2005 20:31:47 +0000 : Tobias Maier I can see your right :) I hope it comes in HEAD before tomorrow :D ------------------------------------------------------------------------ Tue, 30 Aug 2005 00:20:12 +0000 : dtan I apologize if this is already a known issue. http://www.nhpr.org/archive/2005/9 does not create a link for september 1st, even though there are 2 nodes listed (http://www.nhpr.org/archive/2005/9/1) ------------------------------------------------------------------------ Tue, 30 Aug 2005 17:11:21 +0000 : Morbus Iff dtan: I'm pretty sure I know what this is - I'll address it either later today or tomorrow. ------------------------------------------------------------------------ Tue, 30 Aug 2005 20:49:58 +0000 : Morbus Iff Attachment: http://drupal.org/files/issues/archive_1.module (11.53 KB) Alright, I've attached a new archive.module - this is the version WITH term filtering enabled. I can make one without terms if necessary - otherwise, I'll just work from this one for now. This version fixes the bug that dtan saw, as a well as a bunch of other off-by-one errors. Of primary importance, however, is that all mktime's that mattered have been switched to gmmktime, which was one of the oft-reported Issues with the old archive.module. I want to eyeball them all again and make sure they're right though. The URLs from #13 are still operational and the CSS from #10 is still required. ------------------------------------------------------------------------ Wed, 31 Aug 2005 00:02:06 +0000 : Morbus Iff In testing with a few people in #drupal, we've discovered a much bigger problem, which affects this rewritten module as well as the current core archive.module. In a nutshell, the node.created time is stored with time(). PHP's time() bases itself on the server time, NOT on GMT. Thus, for archive.module to work correctly, it must ALWAYS use mktime (relative to server time) and never consider the $user->timezone (relative to GMT). For archive.module, this would cause dates to always be considered via server time, which isn't good, but is better than the craziness going on now. Alternatively, we could try to convert server time to GMT first, and then work with that. The proper solution is to fix node.module to use gmmktime without any arguments for node.created, then have an update path that modifies all node .created and .modified values to GMT, not server time. ------------------------------------------------------------------------ Wed, 31 Aug 2005 05:53:51 +0000 : Junyor Since that is also a problem with the old archive.module, I don't see why it should stop this from getting into core. ------------------------------------------------------------------------ Wed, 31 Aug 2005 06:37:28 +0000 : stefan nagtegaal "Since that is also a problem with the old archive.module, I don't see why it should stop this from getting into core. " Well, I think it's better to only accept the best code rather than accepting bugs getting in core. ------------------------------------------------------------------------ Wed, 31 Aug 2005 07:05:51 +0000 : Kobus I am with Junyor on this. If this can be fixed, great, but if not, it's not a train smash, as the old one exhibits the same problem. I say add the new archive module, if there are no other ciritical bugs with it. It is much more robust and usable than the old one. We desperately need a new archive module. I couldn't find any other bugs while testing with the links Morbus posted. I don't have a HEAD installation anywhere, so can't test it locally at this moment. Regards, Kobus ------------------------------------------------------------------------ Wed, 31 Aug 2005 07:07:49 +0000 : Kobus BTW, code freeze means no new code added, right? Can't this module put in Core as is for the code freeze and the bug sorted out before the official release? Or is that just mean of me to suggest that? Kobus ------------------------------------------------------------------------ Wed, 31 Aug 2005 07:13:09 +0000 : Junyor Stefan: The bug is already in core, since node.module is in core. Archive.module (old and new!) just shows that bug. ------------------------------------------------------------------------ Wed, 31 Aug 2005 07:20:35 +0000 : Tobias Maier I want to have it in core for 4.7, too!!! ------------------------------------------------------------------------ Wed, 31 Aug 2005 07:25:24 +0000 : stefan nagtegaal Junyor: I know that.. Though I think it's not good to accept code which we are aware of that it has bugs in it.. Offcourse this is very double, because drupal contains (probably) a lot of bugs, only they weren't spotted yet.. But, accepting code which has bugs in unacceptable imo.. For example, the node revisions patch had almost 40 reviews/rewritings from Gerhard and several others before it was accepted to be in core.. If we do not allow bugs to go into core, we don't have to bughunt and fix later which is a good thing.. So, imo we should first sort out the problem, then discuss what the best way is to fix the problem, and after that Squeeze that moron! ;-) ------------------------------------------------------------------------ Wed, 31 Aug 2005 09:11:06 +0000 : adrian I vote we include this module, and open up a new issue for the bug. It's not archive's fault that this occurs, it is just showing the data it has access to. The bug already exists in core too. ------------------------------------------------------------------------ Wed, 31 Aug 2005 13:50:59 +0000 : Morbus Iff Attachment: http://drupal.org/files/issues/archive_2.module (10.63 KB) Attached is a new, and most likely final, version of the archive.module. I've removed all user->timezone references - all date determinations are based on server time, which is also the time used in the "created" column of the node table. This is "more accurate" than what the core archive.module currently does (which'll always be wrong because it's treating server time as GMT, which isn't always the case). When node.module does start saving times as GMT properly, archive.module will have be to be tweaked with timezones and blah blah blah. I did fiddle around with determining the server offset in an attempt to get to a GMT base, but I didn't have much luck with that. The NHPR.org URLs above are still valid for testing. I need testers and reviewers. ------------------------------------------------------------------------ Wed, 31 Aug 2005 14:55:16 +0000 : m3avrck I get these two PHP errors when I have *no* content in my Drupal install (just did a clean install to test it): warning: mktime() [<a href='function.mktime'>function.mktime</a>]: Windows does not support negative values for this function in websites\drupal_cvs\drupal\modules\archive.module on line 274. warning: date() [<a href='function.date'>function.date</a>]: Windows does not support dates prior to midnight (00:00:00), January 1, 1970 in websites\drupal_cvs\drupal\modules\archive.module on line 274. Once I add content, these go away. Need some better checks to make sure if there is *no* content you don't get weird errors and what not :) ------------------------------------------------------------------------ Wed, 31 Aug 2005 14:55:45 +0000 : Morbus Iff Attachment: http://drupal.org/files/issues/_p_29676_archive_css_0.patch (1.56 KB) Updated drupal.css patch for HEAD. ------------------------------------------------------------------------ Wed, 31 Aug 2005 15:26:01 +0000 : Morbus Iff Attachment: http://drupal.org/files/issues/archive_3.module (10.75 KB) Attached is the complete module, fixed for m3avrck's #31. ------------------------------------------------------------------------ Wed, 31 Aug 2005 17:30:36 +0000 : m3avrck Reviewed patch and further test, running great over here! Definetly +1 for this one! ------------------------------------------------------------------------ Wed, 31 Aug 2005 20:03:36 +0000 : Morbus Iff Note: The NHPR folks preferred their archives to be sorted from earliest to latest (SQL ASC vs. DESC) for month/day listings, so no longer use the NHPR.org URLs above as representative of the module itself. ------------------------------------------------------------------------ Wed, 31 Aug 2005 21:26:37 +0000 : Morbus Iff Attachment: http://drupal.org/files/issues/drupal.css (10.35 KB) CVS updated for HEAD again. ------------------------------------------------------------------------ Wed, 31 Aug 2005 21:27:00 +0000 : Morbus Iff Attachment: http://drupal.org/files/issues/_p_29676_archive_css_1.patch (1.56 KB) GRRR. ------------------------------------------------------------------------ Thu, 08 Sep 2005 20:18:48 +0000 : m3avrck Attachment: http://drupal.org/files/issues/archive_0.patch (20.72 KB) Rerolled patch against head. Also created one patch that fixes both archive.module and drupal.css so it's a clean and simple process now :) ------------------------------------------------------------------------ Thu, 08 Sep 2005 20:23:55 +0000 : m3avrck Just retested with the latest patch I made, applies cleanly against HEAD and *everything* works great and looks great too. No errors of any kind, even works on PHP 5.0.5 with no call by reference errors, :D Let's get this baby in! ------------------------------------------------------------------------ Thu, 08 Sep 2005 21:26:06 +0000 : Souvent22 +1. I like the new patch, makes things easier to patch. Very slick. I like the break down of years, to months, to days. Very nice. Ready to go I say. ------------------------------------------------------------------------ Mon, 12 Sep 2005 18:55:56 +0000 : m3avrck Bump to the top, freeze coming soon!!! ------------------------------------------------------------------------ Tue, 13 Sep 2005 18:05:21 +0000 : Cvbge So, pgsql does not have FROM_UNIXTIME nor MONTH functions... I'd prefer, if it is possibile, to change the query to not use this mysql-specific functions, then writing pgsql specific equivalents... ------------------------------------------------------------------------ Tue, 13 Sep 2005 18:39:20 +0000 : Dries It might be me, but visually, I like the little calendar block better. Though, this new (slightly ugly) widget is easier to navigate. I'd like to postpone this patch until after 4.7. While nice, I'm not convinced this is the right approach. Like, adding the term-magic to the archive module is more of a hack than anything else. What's next, adding a uid-check so you can filter by user ID? Or a type check so you can filter by node type? Ultimately this should be merged into the tracker module, or better yet, into a generic "query string to node query"-mechanism. (I've outlined this elsewhere in the issues.) There is also a bunch of very minor code issues that need to be looked at. ------------------------------------------------------------------------ Tue, 13 Sep 2005 19:06:57 +0000 : Morbus Iff Attachment: http://drupal.org/files/issues/archive_4.module (11.18 KB) I've attached an updated version, which includes gmmktime + user->timezones, as well as changing all date()'s to format_date()'s. As for nodetypes/users, actually, yeah, NHPR.org is interested in those filter functionalities too (and actually wanted them a few weeks ago). Haven't yet addressed the PSQL stuff from Cvbge. ------------------------------------------------------------------------ Tue, 13 Sep 2005 19:09:46 +0000 : Morbus Iff I'm not entirely convinced that this should be part of tracker.module - the tracker module is to "track changes" or to "track updates". That's an entire different problem than archived data, which probably hasn't been changed for months or even years. I'd hate to see the tracker view applied to an archive system - I could care less about the titles of a blog post (which are usually piss-poor), especially when most *entire* blog posts are small enough to fit into the teaser that the archive.module shows. ------------------------------------------------------------------------ Wed, 14 Sep 2005 05:45:57 +0000 : Boris Mann Dries, I agree with Morbus. I can see what you're thinking with tracker (I think) -- have it handle read/unread for all sorts of stuff, in which case you'll want all sorts of filters. Archive is the place where all content can be navigated primarily by date. Let's commit this rather than live with the current completely useless archive.module for another entire release cycle.

1 0

[drupal-devel] [feature] Replace core archive.module w/ codemonkeyx archive.module
by Boris Mann 14 Sep '05

14 Sep '05

Issue status update for http://drupal.org/node/29676 Post a follow up: http://drupal.org/project/comments/add/29676 Project: Drupal Version: cvs Component: archive.module Category: feature requests Priority: normal Assigned to: Morbus Iff Reported by: Morbus Iff Updated by: Boris Mann Status: patch (code needs work) Dries, I agree with Morbus. I can see what you're thinking with tracker (I think) -- have it handle read/unread for all sorts of stuff, in which case you'll want all sorts of filters. Archive is the place where all content can be navigated primarily by date. Let's commit this rather than live with the current completely useless archive.module for another entire release cycle. Boris Mann Previous comments: ------------------------------------------------------------------------ Thu, 25 Aug 2005 21:08:49 +0000 : Morbus Iff Over at http://drupal.org/node/8287, Berkes mentions that the core archive.module was considered being removed, per a discussion at the Drupal Sprint. Kjartan also mentions he would "love to have the archive module improved in general." In chatting with chx about this, he mentioned codemonkeyx's rewrite sitting in contrib/modules/archive/. I'll be doing some work with the archive.module over the next few days, and will be basing my changes around codemonkeyx's version, and making it compatible with HEAD. This general Issue is to move codemonkeyx's version into HEAD as a replacement to the existing archive.module. An unknown version of his replacement can be seen at http://www.codemonkeyx.net/archive. I'll be running a live HEAD version soon as well. These patches were made during the customization of Drupal by http://www.NHPR.org. In loving support of open source software, http://www.NHPR.org will continue to contribute patches they feel the community will benefit from. Questions about this patch should be directed to morbus(a)disobey.com. ------------------------------------------------------------------------ Fri, 26 Aug 2005 19:45:59 +0000 : Morbus Iff Attachment: http://drupal.org/files/issues_2 (9.56 KB) As an example of a very early revision, see the attached, with the following changes from the current contrib CVS: * removed the year offset from theme_archive_navigation_years, which controlled how many year links to show at once in the top nav. For those with sites with more than five years, they'll WANT people to notice that they have five years, not to have to click on the earliest date and then have their expectations changed. * made the "created > $date" in archive_buildQuery "created >= $date" instead, to allow posts that were created at exactly midnight that day (like me, by design). * since there's no block, I made the menu item visible upon first load. this menu item is given "access content" permissions. More to come, including doxygen and gmt considerations. ------------------------------------------------------------------------ Fri, 26 Aug 2005 19:47:41 +0000 : Morbus Iff Might as well start getting a review of it so I can fix 'em as they come in. ------------------------------------------------------------------------ Fri, 26 Aug 2005 19:56:49 +0000 : Tobias Maier cant you provide a patch file? thanks for your work ------------------------------------------------------------------------ Fri, 26 Aug 2005 20:04:01 +0000 : Morbus Iff The codemonkeyx version is a complete rewrite of the core archive.module. If I were to create a patch file against core, every line would be deleted, and every line would be new. Once I finish my revisions to codemonkey's version, I'll post the final version here, as well as a patch against his current CVS. ------------------------------------------------------------------------ Fri, 26 Aug 2005 20:09:13 +0000 : Tobias Maier ok, thanks again :D ------------------------------------------------------------------------ Mon, 29 Aug 2005 13:41:43 +0000 : Junyor +1 for this change. The archive.module in core is dead. ------------------------------------------------------------------------ Mon, 29 Aug 2005 16:14:30 +0000 : adrian What is the progress on this morbus ? ------------------------------------------------------------------------ Mon, 29 Aug 2005 16:29:13 +0000 : Morbus Iff Adrian - I'll be attaching a new version either later today or tomorrow, with a CHANGELOG. I'll also be running a live version of it over on NHPR.org for people to play with. The three major things I'm worried about right now are a) doxygen, b) variable/function naming, c) GMT considerations. After those, I'll be exploring a patch for my own needs: the ability to get archives for particular term. ------------------------------------------------------------------------ Mon, 29 Aug 2005 18:53:34 +0000 : Morbus Iff Attachment: http://drupal.org/files/issues/archive.module (9.93 KB) Here's the latest, with the following changelog: * reordered some routines to be a little more workflowish. * renamed archive_buildQuery to archive_build_query. * general whitespace and formatting cleanup. * HEADish update: returning $output, not page templating it. * removed the reference of &$ad in archive_build_query. * test for the existence of arg(#)'s before validating them. * archive_validSomething changed to archive_valid_something. * removed unused vars: cur_date, cur_date_end. * renamed archive_buildURL to archive_build_url. * removed the HTML whitespace from the theming. * twiddled a lot of quotes and apostraphes. * removed 'future' CSS class. ill-defined. * reordered/renamed the CSS classes. ------------------------------------------------------------------------ Mon, 29 Aug 2005 18:54:08 +0000 : Morbus Iff Attachment: http://drupal.org/files/issues/_p_29676_archive_css.patch (1.56 KB) And the drupal.css patch. ------------------------------------------------------------------------ Mon, 29 Aug 2005 18:56:29 +0000 : Morbus Iff This version of the module is currently running live at http://www.nhpr.org/archive/. ------------------------------------------------------------------------ Mon, 29 Aug 2005 19:59:57 +0000 : Tobias Maier if i click for example on 2003 it would be good if this would go to january or december and marks them that this one will be shown now as you can see it if you click on january 2003. it has to select * on the first: the first month of writing * on the last: the last month of writing * on every else: january I hope you can understand what I mean... greets tobias ------------------------------------------------------------------------ Mon, 29 Aug 2005 20:17:40 +0000 : Morbus Iff Attachment: http://drupal.org/files/issues/archive_0.module (11.28 KB) Alright. I've attached another new version that adds a new feature that wasn't part of the original codemonkeyx CVS, but was chatted about on the devel list back in April. If this particular feature has bad code or needs heavy refactoring, certainly consider ONLY the version in comment #9 (and the matching drupal.css patch in #10). This new version supports dated archives based on taxonomy tids. It was a quick addition which NHPR.org needed (for the date nav; the normal tid archive pager wasn't strong enough for our needs). Since it was a quick addition, it supports only ONE tid at a time - the 'and/or' syntax for the taxonomy.module was not brought over. If that syntax was desired, it'd make more sense to create some sort of API for archive.module so that other nodes can take advantage of the dated nav in their normal pages (like node types, users, forums, etc.) The added code supports term matches at any granularity: # all node types that match tid 15000 ('The Front Porch') http://www.nhpr.org/archive/term/15000 # all 2005 node types that match tid 20 ('Health') http://www.nhpr.org/archive/2005/term/20 # all March, 2003 node types that match tid 9 ('Education') http://www.nhpr.org/archive/2003/3/term/9 # all July 11, 2003 node types that match tid 49 ('Economy') http://morbus.totalnetnh.net/nhpr/archive/2002/7/11/term/49 ------------------------------------------------------------------------ Mon, 29 Aug 2005 20:27:26 +0000 : Tobias Maier what does this mean? "Story Archives of 'archives' " on http://www.nhpr.org/archive/term/15000 should this maybe named? "Archive of 'The Front Porch'" if I go to http://www.nhpr.org/archive/term/20 I can only read "archives"... why is there a difference? - I never tested this module on my test site, because I'm not at home - ------------------------------------------------------------------------ Mon, 29 Aug 2005 20:29:34 +0000 : Morbus Iff Tobias: that wouldn't be possible, at least not accurately. The new archive.module supports browsing by year, month, and day, as you know. archive/2005 loads up all the data from a particular year and starts creating a pager out of it. Consider if you have 3 posts in December, and 15 posts in November. It wouldn't be "right" to highlight December because the pager display for the entire year would also include some of November's entries (since 3 is less than the pager increment). Likewise, if we ONLY showed the items from December, then we wouldn't have a "pager by year" feature, only a "pager by month (defaulting to December when none is selected)" feature. ------------------------------------------------------------------------ Mon, 29 Aug 2005 20:30:41 +0000 : Morbus Iff Tobias: regarding #14, that's an artifact of the templates that I'm using for NHPR, and has nothing to do with archive.module itself (in fact, once the anonymous cache expires, you'll see that little oopsie go away). ------------------------------------------------------------------------ Mon, 29 Aug 2005 20:31:47 +0000 : Tobias Maier I can see your right :) I hope it comes in HEAD before tomorrow :D ------------------------------------------------------------------------ Tue, 30 Aug 2005 00:20:12 +0000 : dtan I apologize if this is already a known issue. http://www.nhpr.org/archive/2005/9 does not create a link for september 1st, even though there are 2 nodes listed (http://www.nhpr.org/archive/2005/9/1) ------------------------------------------------------------------------ Tue, 30 Aug 2005 17:11:21 +0000 : Morbus Iff dtan: I'm pretty sure I know what this is - I'll address it either later today or tomorrow. ------------------------------------------------------------------------ Tue, 30 Aug 2005 20:49:58 +0000 : Morbus Iff Attachment: http://drupal.org/files/issues/archive_1.module (11.53 KB) Alright, I've attached a new archive.module - this is the version WITH term filtering enabled. I can make one without terms if necessary - otherwise, I'll just work from this one for now. This version fixes the bug that dtan saw, as a well as a bunch of other off-by-one errors. Of primary importance, however, is that all mktime's that mattered have been switched to gmmktime, which was one of the oft-reported Issues with the old archive.module. I want to eyeball them all again and make sure they're right though. The URLs from #13 are still operational and the CSS from #10 is still required. ------------------------------------------------------------------------ Wed, 31 Aug 2005 00:02:06 +0000 : Morbus Iff In testing with a few people in #drupal, we've discovered a much bigger problem, which affects this rewritten module as well as the current core archive.module. In a nutshell, the node.created time is stored with time(). PHP's time() bases itself on the server time, NOT on GMT. Thus, for archive.module to work correctly, it must ALWAYS use mktime (relative to server time) and never consider the $user->timezone (relative to GMT). For archive.module, this would cause dates to always be considered via server time, which isn't good, but is better than the craziness going on now. Alternatively, we could try to convert server time to GMT first, and then work with that. The proper solution is to fix node.module to use gmmktime without any arguments for node.created, then have an update path that modifies all node .created and .modified values to GMT, not server time. ------------------------------------------------------------------------ Wed, 31 Aug 2005 05:53:51 +0000 : Junyor Since that is also a problem with the old archive.module, I don't see why it should stop this from getting into core. ------------------------------------------------------------------------ Wed, 31 Aug 2005 06:37:28 +0000 : stefan nagtegaal "Since that is also a problem with the old archive.module, I don't see why it should stop this from getting into core. " Well, I think it's better to only accept the best code rather than accepting bugs getting in core. ------------------------------------------------------------------------ Wed, 31 Aug 2005 07:05:51 +0000 : Kobus I am with Junyor on this. If this can be fixed, great, but if not, it's not a train smash, as the old one exhibits the same problem. I say add the new archive module, if there are no other ciritical bugs with it. It is much more robust and usable than the old one. We desperately need a new archive module. I couldn't find any other bugs while testing with the links Morbus posted. I don't have a HEAD installation anywhere, so can't test it locally at this moment. Regards, Kobus ------------------------------------------------------------------------ Wed, 31 Aug 2005 07:07:49 +0000 : Kobus BTW, code freeze means no new code added, right? Can't this module put in Core as is for the code freeze and the bug sorted out before the official release? Or is that just mean of me to suggest that? Kobus ------------------------------------------------------------------------ Wed, 31 Aug 2005 07:13:09 +0000 : Junyor Stefan: The bug is already in core, since node.module is in core. Archive.module (old and new!) just shows that bug. ------------------------------------------------------------------------ Wed, 31 Aug 2005 07:20:35 +0000 : Tobias Maier I want to have it in core for 4.7, too!!! ------------------------------------------------------------------------ Wed, 31 Aug 2005 07:25:24 +0000 : stefan nagtegaal Junyor: I know that.. Though I think it's not good to accept code which we are aware of that it has bugs in it.. Offcourse this is very double, because drupal contains (probably) a lot of bugs, only they weren't spotted yet.. But, accepting code which has bugs in unacceptable imo.. For example, the node revisions patch had almost 40 reviews/rewritings from Gerhard and several others before it was accepted to be in core.. If we do not allow bugs to go into core, we don't have to bughunt and fix later which is a good thing.. So, imo we should first sort out the problem, then discuss what the best way is to fix the problem, and after that Squeeze that moron! ;-) ------------------------------------------------------------------------ Wed, 31 Aug 2005 09:11:06 +0000 : adrian I vote we include this module, and open up a new issue for the bug. It's not archive's fault that this occurs, it is just showing the data it has access to. The bug already exists in core too. ------------------------------------------------------------------------ Wed, 31 Aug 2005 13:50:59 +0000 : Morbus Iff Attachment: http://drupal.org/files/issues/archive_2.module (10.63 KB) Attached is a new, and most likely final, version of the archive.module. I've removed all user->timezone references - all date determinations are based on server time, which is also the time used in the "created" column of the node table. This is "more accurate" than what the core archive.module currently does (which'll always be wrong because it's treating server time as GMT, which isn't always the case). When node.module does start saving times as GMT properly, archive.module will have be to be tweaked with timezones and blah blah blah. I did fiddle around with determining the server offset in an attempt to get to a GMT base, but I didn't have much luck with that. The NHPR.org URLs above are still valid for testing. I need testers and reviewers. ------------------------------------------------------------------------ Wed, 31 Aug 2005 14:55:16 +0000 : m3avrck I get these two PHP errors when I have *no* content in my Drupal install (just did a clean install to test it): warning: mktime() [<a href='function.mktime'>function.mktime</a>]: Windows does not support negative values for this function in websites\drupal_cvs\drupal\modules\archive.module on line 274. warning: date() [<a href='function.date'>function.date</a>]: Windows does not support dates prior to midnight (00:00:00), January 1, 1970 in websites\drupal_cvs\drupal\modules\archive.module on line 274. Once I add content, these go away. Need some better checks to make sure if there is *no* content you don't get weird errors and what not :) ------------------------------------------------------------------------ Wed, 31 Aug 2005 14:55:45 +0000 : Morbus Iff Attachment: http://drupal.org/files/issues/_p_29676_archive_css_0.patch (1.56 KB) Updated drupal.css patch for HEAD. ------------------------------------------------------------------------ Wed, 31 Aug 2005 15:26:01 +0000 : Morbus Iff Attachment: http://drupal.org/files/issues/archive_3.module (10.75 KB) Attached is the complete module, fixed for m3avrck's #31. ------------------------------------------------------------------------ Wed, 31 Aug 2005 17:30:36 +0000 : m3avrck Reviewed patch and further test, running great over here! Definetly +1 for this one! ------------------------------------------------------------------------ Wed, 31 Aug 2005 20:03:36 +0000 : Morbus Iff Note: The NHPR folks preferred their archives to be sorted from earliest to latest (SQL ASC vs. DESC) for month/day listings, so no longer use the NHPR.org URLs above as representative of the module itself. ------------------------------------------------------------------------ Wed, 31 Aug 2005 21:26:37 +0000 : Morbus Iff Attachment: http://drupal.org/files/issues/drupal.css (10.35 KB) CVS updated for HEAD again. ------------------------------------------------------------------------ Wed, 31 Aug 2005 21:27:00 +0000 : Morbus Iff Attachment: http://drupal.org/files/issues/_p_29676_archive_css_1.patch (1.56 KB) GRRR. ------------------------------------------------------------------------ Thu, 08 Sep 2005 20:18:48 +0000 : m3avrck Attachment: http://drupal.org/files/issues/archive_0.patch (20.72 KB) Rerolled patch against head. Also created one patch that fixes both archive.module and drupal.css so it's a clean and simple process now :) ------------------------------------------------------------------------ Thu, 08 Sep 2005 20:23:55 +0000 : m3avrck Just retested with the latest patch I made, applies cleanly against HEAD and *everything* works great and looks great too. No errors of any kind, even works on PHP 5.0.5 with no call by reference errors, :D Let's get this baby in! ------------------------------------------------------------------------ Thu, 08 Sep 2005 21:26:06 +0000 : Souvent22 +1. I like the new patch, makes things easier to patch. Very slick. I like the break down of years, to months, to days. Very nice. Ready to go I say. ------------------------------------------------------------------------ Mon, 12 Sep 2005 18:55:56 +0000 : m3avrck Bump to the top, freeze coming soon!!! ------------------------------------------------------------------------ Tue, 13 Sep 2005 18:05:21 +0000 : Cvbge So, pgsql does not have FROM_UNIXTIME nor MONTH functions... I'd prefer, if it is possibile, to change the query to not use this mysql-specific functions, then writing pgsql specific equivalents... ------------------------------------------------------------------------ Tue, 13 Sep 2005 18:39:20 +0000 : Dries It might be me, but visually, I like the little calendar block better. Though, this new (slightly ugly) widget is easier to navigate. I'd like to postpone this patch until after 4.7. While nice, I'm not convinced this is the right approach. Like, adding the term-magic to the archive module is more of a hack than anything else. What's next, adding a uid-check so you can filter by user ID? Or a type check so you can filter by node type? Ultimately this should be merged into the tracker module, or better yet, into a generic "query string to node query"-mechanism. (I've outlined this elsewhere in the issues.) There is also a bunch of very minor code issues that need to be looked at. ------------------------------------------------------------------------ Tue, 13 Sep 2005 19:06:57 +0000 : Morbus Iff Attachment: http://drupal.org/files/issues/archive_4.module (11.18 KB) I've attached an updated version, which includes gmmktime + user->timezones, as well as changing all date()'s to format_date()'s. As for nodetypes/users, actually, yeah, NHPR.org is interested in those filter functionalities too (and actually wanted them a few weeks ago). Haven't yet addressed the PSQL stuff from Cvbge. ------------------------------------------------------------------------ Tue, 13 Sep 2005 19:09:46 +0000 : Morbus Iff I'm not entirely convinced that this should be part of tracker.module - the tracker module is to "track changes" or to "track updates". That's an entire different problem than archived data, which probably hasn't been changed for months or even years. I'd hate to see the tracker view applied to an archive system - I could care less about the titles of a blog post (which are usually piss-poor), especially when most *entire* blog posts are small enough to fit into the teaser that the archive.module shows.

1 0

[drupal-devel] help
by "Ялдыгин В.Б." 14 Sep '05

14 Sep '05

>Send drupal-devel mailing list submissions to > drupal-devel(a)drupal.org > >To subscribe or unsubscribe via the World Wide Web, visit > http://lists.drupal.org/listinfo/drupal-devel >or, via email, send a message with subject or body 'help' to > drupal-devel-request(a)drupal.org > >You can reach the person managing the list at > drupal-devel-owner(a)drupal.org > >When replying, please edit your Subject line so it is more specific >than "Re: Contents of drupal-devel digest..." > > >Today's Topics: > > 1. [bug] comment preview "Required" is easily bypassed (Jeremy) > 2. [bug] comment preview "Required" is easily bypassed (Jeremy) > > >---------------------------------------------------------------------- > >Message: 1 >Date: Tue, 13 Sep 2005 20:16:21 -0700 >From: Jeremy <drupal-devel(a)drupal.org> >Subject: [drupal-devel] [bug] comment preview "Required" is easily > bypassed >To: drupal-devel(a)drupal.org >Message-ID: <type=project&nid=28420&cid=44266&host=(a)drupal.org> >Content-Type: text/plain; charset=UTF-8; format=flowed > >Issue status update for >http://drupal.org/node/28420 >Post a follow up: >http://drupal.org/project/comments/add/28420 > > Project: Drupal > Version: cvs > Component: comment.module > Category: bug reports > Priority: normal > Assigned to: Jeremy(a)kerneltrap.org > Reported by: Jeremy(a)kerneltrap.org > Updated by: Jeremy(a)kerneltrap.org > Status: patch (code needs review) > Attachment: http://drupal.org/files/issues/form_validate-2.patch (1.1 KB) > >This optional patch is intended to be applied after my >form_validation.patch attached to #20 above. It makes the >drupal_private_key more secure by regenerating it every 24 hours. >Without this patch, it would be possible for a spammer to use >brute-force to learn a site's private key by reading the code and >observing the token generated for a given form. With this patch, brute >force is still possible, but with this patch it would have to be done >every 24 hours. > > >It is possible for a form to be generated prior to a rekey, and then to >be validated after a rekey. In this event, the key will fail validation >and the user will see a message telling them something like "Validation >error, please try again. If this error persists, please contact the >site administrator." When they try pressing "submit" again it will >work fine. > > >I kept this patch separate as it adds complexity that may not be >desired in core. Personally I think it is important and should be >merged along with the first patch. I added it to the system module as >it seemed the most logical place, and is a "required" module that can >not be disabled. > > > > >Jeremy(a)kerneltrap.org > > > >Previous comments: >------------------------------------------------------------------------ > >Mon, 08 Aug 2005 01:55:34 +0000 : Jeremy(a)kerneltrap.org > >Setting "Preview comment" to "Required" does not strictly require that >the comment be previewed first. This is being abused by spammers to >quickly and efficiently post spam comments. > > >I discovered this after I added a new feature to my new spam module [1] >to auto-blacklist spammer IP addresses, allowing me to block comment >spammers when they preview a comment and thus preventing them from ever >inserting their spam into my database. I configured my comment module >to "require" comment previews, and yet found that the comments were >slipping past my filter. I finally realized what the spammer is doing >is setting $_POST['op'] to 'Post comment', effectively bypassing the >preview phase. > > >I'm currently looking for a clean solution to this. At the moment the >only idea I have is to generate a token at the preview phase, and >validate the token at the post phase. Unfortunately the token would >have to be stored in the databse between the preview and the post, >which adds overhead. > > >Alternatively, I've considered using a time-based hash which would >constantly update depending on the time of day. This could easily be >validated without storing anything in the database. If too long has >gone between the preview and the post, an additional preview step would >be required... The down side here is that the time-based hash would be >publically available, and thus the spammer could easily duplicate it in >their script. A private key could solve for that, but increases the >complexity as it adds a configuration step. > > >I have the feeling I'm missing a simpler, cleaner solution. >Suggestions? >[1] http://kerneltrap.org/jeremy/drupal/spam/ > > > > >------------------------------------------------------------------------ > >Mon, 08 Aug 2005 02:26:21 +0000 : moshe weitzman > >even if you get this fixed, won't these bots just add a preview step? > > >this 'preview required' feature is designed to maintain high quality >submissions by forcing users to proof read. it isn;t designed for >security. > > >i think you want to hook into comment_validate(). just add a hook here >- there is already a hook_comment() waiting for you to add an >operation. > > > > >------------------------------------------------------------------------ > >Mon, 08 Aug 2005 08:49:30 +0000 : Eaton > >I posted a patch a few days ago (http://drupal.org/node/28255) that adds >validation and form construction hooks for comments. It's similar to the >one that the captcha module uses, though it adds comment form_pre and >form_post hooks instead of a single comment form hook. > > > > >------------------------------------------------------------------------ > >Mon, 08 Aug 2005 13:30:34 +0000 : Jeremy(a)kerneltrap.org > >Attachment: http://drupal.org/files/issues/comment.module_11.patch (2.5 KB) > >> even if you get this fixed, won't these bots just add a preview step? > > >Eventually, yes, but it drastically changes their ability to fling spam >at a site. As is, they simply have a script that shoots data out at >high speed without having to wait for messages to return from the >server. It is the server that is doing all the work, thus making it >simple for a spammer to DoS a site. > > >If "preview required" really meant "preview required", they would be >forced to first automate clicking "preview", and then wait for a >response before clicking "submit". This requires more resources on >their side, and allows us to add delays after clicking "preview" (if we >detect that they are a spammer) further using their resources. > > >> this 'preview required' feature is designed to maintain high quality >> submissions by forcing users to proof read. it isn;t designed for >security. > > >Regardless of the intention, I was misled to believe that configuring >my site to require previews would require that all comments were first >previewed. As a site administrator, I would prefer to know that >"required" really means "required". > > >> i think you want to hook into comment_validate(). just add a hook >here - >> there is already a hook_comment() waiting for you to add an >operation. > > >Yes and no. Ultimately yes that will work and will allow my spam >module to prevent the spam from ever being posted. But it still leaves >the greatest burden on the web server, instead of on the spammer. The >spammer can still use a very simple script that only pushes data, and >thus can generate spam at an unbelievable rapid rate. > > >Here is an example patch to enforce "preview required". It's one idea, >I'm sure there are better ones. > > > > >------------------------------------------------------------------------ > >Mon, 08 Aug 2005 14:01:38 +0000 : Jeremy(a)kerneltrap.org > >Attachment: http://drupal.org/files/issues/comment.module_12.patch (1.4 KB) > >Here's a second version of the patch that doesn't require any manual >configuration. > > > > >------------------------------------------------------------------------ > >Mon, 08 Aug 2005 16:27:27 +0000 : Jose A Reyero > >I like this idea, and the patch looks good > > >Still, I think it misses something, like some timestamp related hash, >because once you get the hash code you can post multiple comments with >that. > > >Another problem I can think of is, what happens when a cron run happens >between the preview and the post?? I'm afraid comments would get lost > > >For this second problem, I think a key generated only once after module >activation could do. About the first one....mmm... I'll sit down for a >while and think..... > > > > >------------------------------------------------------------------------ > >Tue, 09 Aug 2005 12:27:20 +0000 : Jeremy(a)kerneltrap.org > >Attachment: http://drupal.org/files/issues/comment.module_13.patch (1.33 KB) > >> I think it misses something, like some timestamp related hash, because >> once you get the hash code you can post multiple comments with that. > > >Using a timestamp will mean that the comment form "expires". That is, >if you wait too long to preview your comment, it will generate an error >when you try to post. > > >Yes, technically a spammer could post one real comment, then based on >what was in the session from that they could post the same identical >comment over and over, so long as it was attached to the same node. >But this is not what they do, they try and spread their spam throughout >your webpage. Furthermore, the spam module is perfectly capable of >detecting and preventing this. > > >> Another problem I can think of is, what happens when a cron run >happens >> between the preview and the post?? I'm afraid comments would get lost > > >The key is only generated once, that's what the first test is about. >In any case, in the unlikely event that the key were to change between >preview and post they would simply have to post a second time. > > >My earlier patch wasn't quite right, I was testing the token in the >wrong place. This patch fixes that. > > >BTW: This is beneficial for maintaining high quality submissions too, >as prior to this change someone could: > 1) enter a comment > 2) press preview > 3) completely change their comment (introducing a mistake) > 4) press submit and the comment (mistake and all) would go into the >database unpreviewed > > >After this change: > 1) enter a comment > 2) press preview > 3) completely change their comment (introducing a mistake) > 4) press submit and they get an error because they didn't preview >their changes - forcing them to preview once after any change > > > > >------------------------------------------------------------------------ > >Wed, 10 Aug 2005 03:50:33 +0000 : Jeremy(a)kerneltrap.org > >FWIW: I've been getting slammed by spam attacks this whole week. >Installing this patch has made a huge difference. Well over 100 spam >attempts per minute (sometimes two and three times that) and I hardly >notice the spammer, whereas before it was choking my database. > > >(Granted, the spammer has not yet upgraded his script to first preview, >then submit. But even if he did it wouldn't help him as testing has >verified that the new spam module would prevent the comments from ever >getting to the database.) > > >Additionally, user and anonymous (nonspam) comments continue to show up >at a normal rate. > > > > >------------------------------------------------------------------------ > >Tue, 16 Aug 2005 14:08:04 +0000 : Jeremy(a)kerneltrap.org > >I would love to see _any_ discussion on this. Drupal is currently too >easy to spam, with little effort on the spammer's side, and lots of >resources wasted on the Drupal side. A patch like this will greatly >increase the spammer's burden, and make it possible to effectively >block even the most aggressive spammer attacks. > > > > >------------------------------------------------------------------------ > >Wed, 17 Aug 2005 16:24:04 +0000 : Jose A Reyero > >Well, this patch is definitely better than what we have, and would save >some spam for sure. > > >But maybe keeping track, at the session level, of generated hashes for >a user, and then removing them when the comment is sent, could do the >work. > > >This way we can forget about previewing comments or not, and also the >"permission" to post the comment would expire when the session expires. >Any randomly generated value could do for this, no need for complex >hashes, but having nid and pid in the hash would add some extra >security. > > > > >------------------------------------------------------------------------ > >Wed, 17 Aug 2005 19:58:02 +0000 : breyten > >Jeremy, a big +1 on the idea, but why not generate the private key when >it is actually needed (Ie, when displaying the comment form), instead >of wasting a _cron() hook on it? > > > > >------------------------------------------------------------------------ > >Thu, 18 Aug 2005 03:20:02 +0000 : Jeremy(a)kerneltrap.org > >> Well, this patch is definitely better than what we have, and would >save some spam for sure. > > >It is continuing to work very well on my site, which seems to be under >nearly perpetual spam attacks from multiple sources. > > >> But maybe keeping track, at the session level, of generated hashes >for a user, and then >> removing them when the comment is sent, could do the work. > > >The catch is: the key has to be something unique to the server, not >guessable or learnable from the outside Simply storing the hash data >in the session alone is not enough, as then the spammer could create >any random data and store it in the session. > > >That said, the hash could be generated off something other than the >text of the comment as it is now, so that a preview is not required. >I'll look at doing something like that and submit another patch. > > >> This way we can forget about previewing comments or not, and also the >"permission" to >> post the comment would expire when the session expires. Any randomly >generated value >> could do for this, no need for complex hashes, but having nid and pid >in the hash would >> add some extra security. > > >nid and pid alone are worthless, as they are easy to learn. The pid >can always be 0 (spam is rarely attached to a pre-existing comment). >The nid is obtained in the path of where the spam is being posted. > > >The solution is a "private-key", which is what my patch adds. Then >sure, hash the private key plus the nid and the pid, and you've got >enough protection to prevent most spammers. To make it even more >secure, automatic rey-keying could be easily accomplished. > > > > >------------------------------------------------------------------------ > >Thu, 18 Aug 2005 04:09:10 +0000 : Jeremy(a)kerneltrap.org > >Attachment: http://drupal.org/files/issues/comment.module_15.patch (1.18 KB) > >The attached patch: > 1) gets rid of the _cron() hook > 2) no longer requires that comments be previewed > > >Prior to this patch, comment spammers were able to send data to a >Drupal server acting as though they'd filled out a comment form and >pressed submit. As they didn't actually use the form, they could >submit spam comments at an obscene rate. > > >With this patch, comment spammers will have to actually load the form, >enter text, and press submit. Yes, that can still be automated, but it >takes much more work and slows them down, as they have to wait for the >entry form to load each time. > > >Unfortunately a spammer could manually submit one comment, then re-use >that same session info over and over to attach repeated spam comments >to the same node. Such an attempt would be detected and blocked by the >spam module if enabled, but again such a session re-use attack could be >done without loading the form each time. Fortunately there is much >less gain for a spammer to submit 100 spam comments on the same page, >versus submitting 100 spam comments each on a different page as they do >now. > > >Ideas to improve upon this concept include: > - re-key every day or week, changing the private key regularly to be >sure it couldn't ever be permanently cracked > - add a key table, and generate a unique key for every comment form. >essentially, upon comment form creation generate a random key which is >stored both in a database table and in the session. when a comment is >submited, look for the key from the session in the database table, if a >match is found delete it from the database table and post the comment. >this would prevent session re-use, but adds overhead. i don't know if >it's worth it, perhaps as an external module if the hooks were >available. > > > > >------------------------------------------------------------------------ > >Fri, 19 Aug 2005 18:55:11 +0000 : drumm > > ><?php >form_set_error('error',В═t('ValidationВ═error,В═pleaseВ═beВ═sureВ═cookiesВ═areВ═enabledВ═onВ═yourВ═browser.')); >?> > > > > >form_set_error [2]()'s fist argument should be the name of a form >field, not 'error.' Using (..., 'error') would be better in this case. > > >And the actual message needs work. Since this is a hidden field I don't >think it has anything to do with cookies. >[2] http://drupaldocs.org/api/head/function/form_set_error > > > > >------------------------------------------------------------------------ > >Fri, 19 Aug 2005 18:56:41 +0000 : drumm > >The unclosed link in my last update was supposed to say >drupal_set_message(..., 'error') > > > > >------------------------------------------------------------------------ > >Sat, 20 Aug 2005 16:00:15 +0000 : Jeremy(a)kerneltrap.org > >Attachment: http://drupal.org/files/issues/comment.module_16.patch (1.11 KB) > >drupal_set_message(..., 'error') isn't sufficient to prevent the comment >from being posted. I have instead updated the patch to set the error on >the hidden 'token' form field. > > >I have updated the message to read: >"Unable to validate your comment, please try again. If this error >persists, please contact the site administrator." > > >If you don't like the error message, better suggestions are welcome. > > > > >------------------------------------------------------------------------ > >Fri, 09 Sep 2005 03:16:06 +0000 : Jeremy(a)kerneltrap.org > >Any feedback on this patch? I have been running it on my website for a >couple of weeks, and it has completely stopped the most persistent >auto-spam scripts that had been posting poker type comments constantly >to my site. > > > > >------------------------------------------------------------------------ > >Sat, 10 Sep 2005 18:12:15 +0000 : Zed Pobre > >This patch is against HEAD? It doesn't want to apply to my 2.6.3 >comment.module. > > > > >------------------------------------------------------------------------ > >Sun, 11 Sep 2005 18:09:08 +0000 : Abalieno > >It's is for cvs but I'm trying to manually apply it to 4.6.3. > > >Will comment later to tell how it went. > > > > >------------------------------------------------------------------------ > >Mon, 12 Sep 2005 19:49:17 +0000 : Abalieno > >Well, it worked. > > >No spam at all in more than a day. I don't know if other users are >having problem but this patch broke the tool the spammer was using. > > >:) > > > > >------------------------------------------------------------------------ > >Wed, 14 Sep 2005 02:47:40 +0000 : Jeremy(a)kerneltrap.org > >Attachment: http://drupal.org/files/issues/form_validation.patch (2.72 KB) > >Here's a completely rewritten version of the patch, based on an email >discussion with Dries. This provides a more generic interface that can >be used to validate other form submissions, not just comments. The >patch introduces two new functions, form_token() and form_validate(). >The first function uses a private key and a public key to set a token >in a hidden field. The second function validates the token. The patch >also updates the comment module, demonstrating how these new functions >are used. > > >More information as to how the patch works can be found in the comments >that are within the code. > > >Based on my own experiences on kerneltrap.org, this patch blocks 99% of >the current comment spammers. > > > > > > >------------------------------ > >Message: 2 >Date: Tue, 13 Sep 2005 20:58:35 -0700 >From: Jeremy <drupal-devel(a)drupal.org> >Subject: [drupal-devel] [bug] comment preview "Required" is easily > bypassed >To: drupal-devel(a)drupal.org >Message-ID: <type=project&nid=28420&cid=44270&host=(a)drupal.org> >Content-Type: text/plain; charset=UTF-8; format=flowed > >Issue status update for >http://drupal.org/node/28420 >Post a follow up: >http://drupal.org/project/comments/add/28420 > > Project: Drupal > Version: cvs > Component: comment.module > Category: bug reports > Priority: normal > Assigned to: Jeremy(a)kerneltrap.org > Reported by: Jeremy(a)kerneltrap.org > Updated by: Jeremy(a)kerneltrap.org > Status: patch (code needs review) > Attachment: http://drupal.org/files/issues/contact.module_8.patch (1.44 KB) > >This third patch updates the contact.module to use the new form token >validation functions. > > >Note that this is not yet a perfect solution. Someone wanting to spam >the contact form could manually fill it out once to obtain a valid >token. They could then use that token to spam the contact form >repeatedly, so long as they don't change the fields that were used to >generate the token. > > >The solution is to keep a history of recently validated tokens. Each >time a token is validated, make sure it was not already recently >validated -- if it was, don't allow it a second time. I would be happy >to provide patches for this if it is deemed necessary, and these first >patches are merged. It would require a new database table, and a >db_query to validate the token. Ideas for alternative solutions are >welcome. > > > > >Jeremy(a)kerneltrap.org > > > >Previous comments: >------------------------------------------------------------------------ > >Mon, 08 Aug 2005 01:55:34 +0000 : Jeremy(a)kerneltrap.org > >Setting "Preview comment" to "Required" does not strictly require that >the comment be previewed first. This is being abused by spammers to >quickly and efficiently post spam comments. > > >I discovered this after I added a new feature to my new spam module [1] >to auto-blacklist spammer IP addresses, allowing me to block comment >spammers when they preview a comment and thus preventing them from ever >inserting their spam into my database. I configured my comment module >to "require" comment previews, and yet found that the comments were >slipping past my filter. I finally realized what the spammer is doing >is setting $_POST['op'] to 'Post comment', effectively bypassing the >preview phase. > > >I'm currently looking for a clean solution to this. At the moment the >only idea I have is to generate a token at the preview phase, and >validate the token at the post phase. Unfortunately the token would >have to be stored in the databse between the preview and the post, >which adds overhead. > > >Alternatively, I've considered using a time-based hash which would >constantly update depending on the time of day. This could easily be >validated without storing anything in the database. If too long has >gone between the preview and the post, an additional preview step would >be required... The down side here is that the time-based hash would be >publically available, and thus the spammer could easily duplicate it in >their script. A private key could solve for that, but increases the >complexity as it adds a configuration step. > > >I have the feeling I'm missing a simpler, cleaner solution. >Suggestions? >[1] http://kerneltrap.org/jeremy/drupal/spam/ > > > > >------------------------------------------------------------------------ > >Mon, 08 Aug 2005 02:26:21 +0000 : moshe weitzman > >even if you get this fixed, won't these bots just add a preview step? > > >this 'preview required' feature is designed to maintain high quality >submissions by forcing users to proof read. it isn;t designed for >security. > > >i think you want to hook into comment_validate(). just add a hook here >- there is already a hook_comment() waiting for you to add an >operation. > > > > >------------------------------------------------------------------------ > >Mon, 08 Aug 2005 08:49:30 +0000 : Eaton > >I posted a patch a few days ago (http://drupal.org/node/28255) that adds >validation and form construction hooks for comments. It's similar to the >one that the captcha module uses, though it adds comment form_pre and >form_post hooks instead of a single comment form hook. > > > > >------------------------------------------------------------------------ > >Mon, 08 Aug 2005 13:30:34 +0000 : Jeremy(a)kerneltrap.org > >Attachment: http://drupal.org/files/issues/comment.module_11.patch (2.5 KB) > >> even if you get this fixed, won't these bots just add a preview step? > > >Eventually, yes, but it drastically changes their ability to fling spam >at a site. As is, they simply have a script that shoots data out at >high speed without having to wait for messages to return from the >server. It is the server that is doing all the work, thus making it >simple for a spammer to DoS a site. > > >If "preview required" really meant "preview required", they would be >forced to first automate clicking "preview", and then wait for a >response before clicking "submit". This requires more resources on >their side, and allows us to add delays after clicking "preview" (if we >detect that they are a spammer) further using their resources. > > >> this 'preview required' feature is designed to maintain high quality >> submissions by forcing users to proof read. it isn;t designed for >security. > > >Regardless of the intention, I was misled to believe that configuring >my site to require previews would require that all comments were first >previewed. As a site administrator, I would prefer to know that >"required" really means "required". > > >> i think you want to hook into comment_validate(). just add a hook >here - >> there is already a hook_comment() waiting for you to add an >operation. > > >Yes and no. Ultimately yes that will work and will allow my spam >module to prevent the spam from ever being posted. But it still leaves >the greatest burden on the web server, instead of on the spammer. The >spammer can still use a very simple script that only pushes data, and >thus can generate spam at an unbelievable rapid rate. > > >Here is an example patch to enforce "preview required". It's one idea, >I'm sure there are better ones. > > > > >------------------------------------------------------------------------ > >Mon, 08 Aug 2005 14:01:38 +0000 : Jeremy(a)kerneltrap.org > >Attachment: http://drupal.org/files/issues/comment.module_12.patch (1.4 KB) > >Here's a second version of the patch that doesn't require any manual >configuration. > > > > >------------------------------------------------------------------------ > >Mon, 08 Aug 2005 16:27:27 +0000 : Jose A Reyero > >I like this idea, and the patch looks good > > >Still, I think it misses something, like some timestamp related hash, >because once you get the hash code you can post multiple comments with >that. > > >Another problem I can think of is, what happens when a cron run happens >between the preview and the post?? I'm afraid comments would get lost > > >For this second problem, I think a key generated only once after module >activation could do. About the first one....mmm... I'll sit down for a >while and think..... > > > > >------------------------------------------------------------------------ > >Tue, 09 Aug 2005 12:27:20 +0000 : Jeremy(a)kerneltrap.org > >Attachment: http://drupal.org/files/issues/comment.module_13.patch (1.33 KB) > >> I think it misses something, like some timestamp related hash, because >> once you get the hash code you can post multiple comments with that. > > >Using a timestamp will mean that the comment form "expires". That is, >if you wait too long to preview your comment, it will generate an error >when you try to post. > > >Yes, technically a spammer could post one real comment, then based on >what was in the session from that they could post the same identical >comment over and over, so long as it was attached to the same node. >But this is not what they do, they try and spread their spam throughout >your webpage. Furthermore, the spam module is perfectly capable of >detecting and preventing this. > > >> Another problem I can think of is, what happens when a cron run >happens >> between the preview and the post?? I'm afraid comments would get lost > > >The key is only generated once, that's what the first test is about. >In any case, in the unlikely event that the key were to change between >preview and post they would simply have to post a second time. > > >My earlier patch wasn't quite right, I was testing the token in the >wrong place. This patch fixes that. > > >BTW: This is beneficial for maintaining high quality submissions too, >as prior to this change someone could: > 1) enter a comment > 2) press preview > 3) completely change their comment (introducing a mistake) > 4) press submit and the comment (mistake and all) would go into the >database unpreviewed > > >After this change: > 1) enter a comment > 2) press preview > 3) completely change their comment (introducing a mistake) > 4) press submit and they get an error because they didn't preview >their changes - forcing them to preview once after any change > > > > >------------------------------------------------------------------------ > >Wed, 10 Aug 2005 03:50:33 +0000 : Jeremy(a)kerneltrap.org > >FWIW: I've been getting slammed by spam attacks this whole week. >Installing this patch has made a huge difference. Well over 100 spam >attempts per minute (sometimes two and three times that) and I hardly >notice the spammer, whereas before it was choking my database. > > >(Granted, the spammer has not yet upgraded his script to first preview, >then submit. But even if he did it wouldn't help him as testing has >verified that the new spam module would prevent the comments from ever >getting to the database.) > > >Additionally, user and anonymous (nonspam) comments continue to show up >at a normal rate. > > > > >------------------------------------------------------------------------ > >Tue, 16 Aug 2005 14:08:04 +0000 : Jeremy(a)kerneltrap.org > >I would love to see _any_ discussion on this. Drupal is currently too >easy to spam, with little effort on the spammer's side, and lots of >resources wasted on the Drupal side. A patch like this will greatly >increase the spammer's burden, and make it possible to effectively >block even the most aggressive spammer attacks. > > > > >------------------------------------------------------------------------ > >Wed, 17 Aug 2005 16:24:04 +0000 : Jose A Reyero > >Well, this patch is definitely better than what we have, and would save >some spam for sure. > > >But maybe keeping track, at the session level, of generated hashes for >a user, and then removing them when the comment is sent, could do the >work. > > >This way we can forget about previewing comments or not, and also the >"permission" to post the comment would expire when the session expires. >Any randomly generated value could do for this, no need for complex >hashes, but having nid and pid in the hash would add some extra >security. > > > > >------------------------------------------------------------------------ > >Wed, 17 Aug 2005 19:58:02 +0000 : breyten > >Jeremy, a big +1 on the idea, but why not generate the private key when >it is actually needed (Ie, when displaying the comment form), instead >of wasting a _cron() hook on it? > > > > >------------------------------------------------------------------------ > >Thu, 18 Aug 2005 03:20:02 +0000 : Jeremy(a)kerneltrap.org > >> Well, this patch is definitely better than what we have, and would >save some spam for sure. > > >It is continuing to work very well on my site, which seems to be under >nearly perpetual spam attacks from multiple sources. > > >> But maybe keeping track, at the session level, of generated hashes >for a user, and then >> removing them when the comment is sent, could do the work. > > >The catch is: the key has to be something unique to the server, not >guessable or learnable from the outside Simply storing the hash data >in the session alone is not enough, as then the spammer could create >any random data and store it in the session. > > >That said, the hash could be generated off something other than the >text of the comment as it is now, so that a preview is not required. >I'll look at doing something like that and submit another patch. > > >> This way we can forget about previewing comments or not, and also the >"permission" to >> post the comment would expire when the session expires. Any randomly >generated value >> could do for this, no need for complex hashes, but having nid and pid >in the hash would >> add some extra security. > > >nid and pid alone are worthless, as they are easy to learn. The pid >can always be 0 (spam is rarely attached to a pre-existing comment). >The nid is obtained in the path of where the spam is being posted. > > >The solution is a "private-key", which is what my patch adds. Then >sure, hash the private key plus the nid and the pid, and you've got >enough protection to prevent most spammers. To make it even more >secure, automatic rey-keying could be easily accomplished. > > > > >------------------------------------------------------------------------ > >Thu, 18 Aug 2005 04:09:10 +0000 : Jeremy(a)kerneltrap.org > >Attachment: http://drupal.org/files/issues/comment.module_15.patch (1.18 KB) > >The attached patch: > 1) gets rid of the _cron() hook > 2) no longer requires that comments be previewed > > >Prior to this patch, comment spammers were able to send data to a >Drupal server acting as though they'd filled out a comment form and >pressed submit. As they didn't actually use the form, they could >submit spam comments at an obscene rate. > > >With this patch, comment spammers will have to actually load the form, >enter text, and press submit. Yes, that can still be automated, but it >takes much more work and slows them down, as they have to wait for the >entry form to load each time. > > >Unfortunately a spammer could manually submit one comment, then re-use >that same session info over and over to attach repeated spam comments >to the same node. Such an attempt would be detected and blocked by the >spam module if enabled, but again such a session re-use attack could be >done without loading the form each time. Fortunately there is much >less gain for a spammer to submit 100 spam comments on the same page, >versus submitting 100 spam comments each on a different page as they do >now. > > >Ideas to improve upon this concept include: > - re-key every day or week, changing the private key regularly to be >sure it couldn't ever be permanently cracked > - add a key table, and generate a unique key for every comment form. >essentially, upon comment form creation generate a random key which is >stored both in a database table and in the session. when a comment is >submited, look for the key from the session in the database table, if a >match is found delete it from the database table and post the comment. >this would prevent session re-use, but adds overhead. i don't know if >it's worth it, perhaps as an external module if the hooks were >available. > > > > >------------------------------------------------------------------------ > >Fri, 19 Aug 2005 18:55:11 +0000 : drumm > > ><?php >form_set_error('error',В═t('ValidationВ═error,В═pleaseВ═beВ═sureВ═cookiesВ═areВ═enabledВ═onВ═yourВ═browser.')); >?> > > > > >form_set_error [2]()'s fist argument should be the name of a form >field, not 'error.' Using (..., 'error') would be better in this case. > > >And the actual message needs work. Since this is a hidden field I don't >think it has anything to do with cookies. >[2] http://drupaldocs.org/api/head/function/form_set_error > > > > >------------------------------------------------------------------------ > >Fri, 19 Aug 2005 18:56:41 +0000 : drumm > >The unclosed link in my last update was supposed to say >drupal_set_message(..., 'error') > > > > >------------------------------------------------------------------------ > >Sat, 20 Aug 2005 16:00:15 +0000 : Jeremy(a)kerneltrap.org > >Attachment: http://drupal.org/files/issues/comment.module_16.patch (1.11 KB) > >drupal_set_message(..., 'error') isn't sufficient to prevent the comment >from being posted. I have instead updated the patch to set the error on >the hidden 'token' form field. > > >I have updated the message to read: >"Unable to validate your comment, please try again. If this error >persists, please contact the site administrator." > > >If you don't like the error message, better suggestions are welcome. > > > > >------------------------------------------------------------------------ > >Fri, 09 Sep 2005 03:16:06 +0000 : Jeremy(a)kerneltrap.org > >Any feedback on this patch? I have been running it on my website for a >couple of weeks, and it has completely stopped the most persistent >auto-spam scripts that had been posting poker type comments constantly >to my site. > > > > >------------------------------------------------------------------------ > >Sat, 10 Sep 2005 18:12:15 +0000 : Zed Pobre > >This patch is against HEAD? It doesn't want to apply to my 2.6.3 >comment.module. > > > > >------------------------------------------------------------------------ > >Sun, 11 Sep 2005 18:09:08 +0000 : Abalieno > >It's is for cvs but I'm trying to manually apply it to 4.6.3. > > >Will comment later to tell how it went. > > > > >------------------------------------------------------------------------ > >Mon, 12 Sep 2005 19:49:17 +0000 : Abalieno > >Well, it worked. > > >No spam at all in more than a day. I don't know if other users are >having problem but this patch broke the tool the spammer was using. > > >:) > > > > >------------------------------------------------------------------------ > >Wed, 14 Sep 2005 02:47:40 +0000 : Jeremy(a)kerneltrap.org > >Attachment: http://drupal.org/files/issues/form_validation.patch (2.72 KB) > >Here's a completely rewritten version of the patch, based on an email >discussion with Dries. This provides a more generic interface that can >be used to validate other form submissions, not just comments. The >patch introduces two new functions, form_token() and form_validate(). >The first function uses a private key and a public key to set a token >in a hidden field. The second function validates the token. The patch >also updates the comment module, demonstrating how these new functions >are used. > > >More information as to how the patch works can be found in the comments >that are within the code. > > >Based on my own experiences on kerneltrap.org, this patch blocks 99% of >the current comment spammers. > > > > >------------------------------------------------------------------------ > >Wed, 14 Sep 2005 03:16:21 +0000 : Jeremy(a)kerneltrap.org > >Attachment: http://drupal.org/files/issues/form_validate-2.patch (1.1 KB) > >This optional patch is intended to be applied after my >form_validation.patch attached to #20 above. It makes the >drupal_private_key more secure by regenerating it every 24 hours. >Without this patch, it would be possible for a spammer to use >brute-force to learn a site's private key by reading the code and >observing the token generated for a given form. With this patch, brute >force is still possible, but with this patch it would have to be done >every 24 hours. > > >It is possible for a form to be generated prior to a rekey, and then to >be validated after a rekey. In this event, the key will fail validation >and the user will see a message telling them something like "Validation >error, please try again. If this error persists, please contact the >site administrator." When they try pressing "submit" again it will >work fine. > > >I kept this patch separate as it adds complexity that may not be >desired in core. Personally I think it is important and should be >merged along with the first patch. I added it to the system module as >it seemed the most logical place, and is a "required" module that can >not be disabled. > > > > > > >------------------------------ -- Сегодня удачный день, чтобы завести почту на Яндексе http://mail.yandex.ru

1 0

[drupal-devel] [bug] comment preview "Required" is easily bypassed
by Jeremy 14 Sep '05

14 Sep '05

Issue status update for http://drupal.org/node/28420 Post a follow up: http://drupal.org/project/comments/add/28420 Project: Drupal Version: cvs Component: comment.module Category: bug reports Priority: normal Assigned to: Jeremy(a)kerneltrap.org Reported by: Jeremy(a)kerneltrap.org Updated by: Jeremy(a)kerneltrap.org Status: patch (code needs review) Attachment: http://drupal.org/files/issues/contact.module_8.patch (1.44 KB) This third patch updates the contact.module to use the new form token validation functions. Note that this is not yet a perfect solution. Someone wanting to spam the contact form could manually fill it out once to obtain a valid token. They could then use that token to spam the contact form repeatedly, so long as they don't change the fields that were used to generate the token. The solution is to keep a history of recently validated tokens. Each time a token is validated, make sure it was not already recently validated -- if it was, don't allow it a second time. I would be happy to provide patches for this if it is deemed necessary, and these first patches are merged. It would require a new database table, and a db_query to validate the token. Ideas for alternative solutions are welcome. Jeremy(a)kerneltrap.org Previous comments: ------------------------------------------------------------------------ Mon, 08 Aug 2005 01:55:34 +0000 : Jeremy(a)kerneltrap.org Setting "Preview comment" to "Required" does not strictly require that the comment be previewed first. This is being abused by spammers to quickly and efficiently post spam comments. I discovered this after I added a new feature to my new spam module [1] to auto-blacklist spammer IP addresses, allowing me to block comment spammers when they preview a comment and thus preventing them from ever inserting their spam into my database. I configured my comment module to "require" comment previews, and yet found that the comments were slipping past my filter. I finally realized what the spammer is doing is setting $_POST['op'] to 'Post comment', effectively bypassing the preview phase. I'm currently looking for a clean solution to this. At the moment the only idea I have is to generate a token at the preview phase, and validate the token at the post phase. Unfortunately the token would have to be stored in the databse between the preview and the post, which adds overhead. Alternatively, I've considered using a time-based hash which would constantly update depending on the time of day. This could easily be validated without storing anything in the database. If too long has gone between the preview and the post, an additional preview step would be required... The down side here is that the time-based hash would be publically available, and thus the spammer could easily duplicate it in their script. A private key could solve for that, but increases the complexity as it adds a configuration step. I have the feeling I'm missing a simpler, cleaner solution. Suggestions? [1] http://kerneltrap.org/jeremy/drupal/spam/ ------------------------------------------------------------------------ Mon, 08 Aug 2005 02:26:21 +0000 : moshe weitzman even if you get this fixed, won't these bots just add a preview step? this 'preview required' feature is designed to maintain high quality submissions by forcing users to proof read. it isn;t designed for security. i think you want to hook into comment_validate(). just add a hook here - there is already a hook_comment() waiting for you to add an operation. ------------------------------------------------------------------------ Mon, 08 Aug 2005 08:49:30 +0000 : Eaton I posted a patch a few days ago (http://drupal.org/node/28255) that adds validation and form construction hooks for comments. It's similar to the one that the captcha module uses, though it adds comment form_pre and form_post hooks instead of a single comment form hook. ------------------------------------------------------------------------ Mon, 08 Aug 2005 13:30:34 +0000 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/comment.module_11.patch (2.5 KB) > even if you get this fixed, won't these bots just add a preview step? Eventually, yes, but it drastically changes their ability to fling spam at a site. As is, they simply have a script that shoots data out at high speed without having to wait for messages to return from the server. It is the server that is doing all the work, thus making it simple for a spammer to DoS a site. If "preview required" really meant "preview required", they would be forced to first automate clicking "preview", and then wait for a response before clicking "submit". This requires more resources on their side, and allows us to add delays after clicking "preview" (if we detect that they are a spammer) further using their resources. > this 'preview required' feature is designed to maintain high quality > submissions by forcing users to proof read. it isn;t designed for security. Regardless of the intention, I was misled to believe that configuring my site to require previews would require that all comments were first previewed. As a site administrator, I would prefer to know that "required" really means "required". > i think you want to hook into comment_validate(). just add a hook here - > there is already a hook_comment() waiting for you to add an operation. Yes and no. Ultimately yes that will work and will allow my spam module to prevent the spam from ever being posted. But it still leaves the greatest burden on the web server, instead of on the spammer. The spammer can still use a very simple script that only pushes data, and thus can generate spam at an unbelievable rapid rate. Here is an example patch to enforce "preview required". It's one idea, I'm sure there are better ones. ------------------------------------------------------------------------ Mon, 08 Aug 2005 14:01:38 +0000 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/comment.module_12.patch (1.4 KB) Here's a second version of the patch that doesn't require any manual configuration. ------------------------------------------------------------------------ Mon, 08 Aug 2005 16:27:27 +0000 : Jose A Reyero I like this idea, and the patch looks good Still, I think it misses something, like some timestamp related hash, because once you get the hash code you can post multiple comments with that. Another problem I can think of is, what happens when a cron run happens between the preview and the post?? I'm afraid comments would get lost For this second problem, I think a key generated only once after module activation could do. About the first one....mmm... I'll sit down for a while and think..... ------------------------------------------------------------------------ Tue, 09 Aug 2005 12:27:20 +0000 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/comment.module_13.patch (1.33 KB) > I think it misses something, like some timestamp related hash, because > once you get the hash code you can post multiple comments with that. Using a timestamp will mean that the comment form "expires". That is, if you wait too long to preview your comment, it will generate an error when you try to post. Yes, technically a spammer could post one real comment, then based on what was in the session from that they could post the same identical comment over and over, so long as it was attached to the same node. But this is not what they do, they try and spread their spam throughout your webpage. Furthermore, the spam module is perfectly capable of detecting and preventing this. > Another problem I can think of is, what happens when a cron run happens > between the preview and the post?? I'm afraid comments would get lost The key is only generated once, that's what the first test is about. In any case, in the unlikely event that the key were to change between preview and post they would simply have to post a second time. My earlier patch wasn't quite right, I was testing the token in the wrong place. This patch fixes that. BTW: This is beneficial for maintaining high quality submissions too, as prior to this change someone could: 1) enter a comment 2) press preview 3) completely change their comment (introducing a mistake) 4) press submit and the comment (mistake and all) would go into the database unpreviewed After this change: 1) enter a comment 2) press preview 3) completely change their comment (introducing a mistake) 4) press submit and they get an error because they didn't preview their changes - forcing them to preview once after any change ------------------------------------------------------------------------ Wed, 10 Aug 2005 03:50:33 +0000 : Jeremy(a)kerneltrap.org FWIW: I've been getting slammed by spam attacks this whole week. Installing this patch has made a huge difference. Well over 100 spam attempts per minute (sometimes two and three times that) and I hardly notice the spammer, whereas before it was choking my database. (Granted, the spammer has not yet upgraded his script to first preview, then submit. But even if he did it wouldn't help him as testing has verified that the new spam module would prevent the comments from ever getting to the database.) Additionally, user and anonymous (nonspam) comments continue to show up at a normal rate. ------------------------------------------------------------------------ Tue, 16 Aug 2005 14:08:04 +0000 : Jeremy(a)kerneltrap.org I would love to see _any_ discussion on this. Drupal is currently too easy to spam, with little effort on the spammer's side, and lots of resources wasted on the Drupal side. A patch like this will greatly increase the spammer's burden, and make it possible to effectively block even the most aggressive spammer attacks. ------------------------------------------------------------------------ Wed, 17 Aug 2005 16:24:04 +0000 : Jose A Reyero Well, this patch is definitely better than what we have, and would save some spam for sure. But maybe keeping track, at the session level, of generated hashes for a user, and then removing them when the comment is sent, could do the work. This way we can forget about previewing comments or not, and also the "permission" to post the comment would expire when the session expires. Any randomly generated value could do for this, no need for complex hashes, but having nid and pid in the hash would add some extra security. ------------------------------------------------------------------------ Wed, 17 Aug 2005 19:58:02 +0000 : breyten Jeremy, a big +1 on the idea, but why not generate the private key when it is actually needed (Ie, when displaying the comment form), instead of wasting a _cron() hook on it? ------------------------------------------------------------------------ Thu, 18 Aug 2005 03:20:02 +0000 : Jeremy(a)kerneltrap.org > Well, this patch is definitely better than what we have, and would save some spam for sure. It is continuing to work very well on my site, which seems to be under nearly perpetual spam attacks from multiple sources. > But maybe keeping track, at the session level, of generated hashes for a user, and then > removing them when the comment is sent, could do the work. The catch is: the key has to be something unique to the server, not guessable or learnable from the outside Simply storing the hash data in the session alone is not enough, as then the spammer could create any random data and store it in the session. That said, the hash could be generated off something other than the text of the comment as it is now, so that a preview is not required. I'll look at doing something like that and submit another patch. > This way we can forget about previewing comments or not, and also the "permission" to > post the comment would expire when the session expires. Any randomly generated value > could do for this, no need for complex hashes, but having nid and pid in the hash would > add some extra security. nid and pid alone are worthless, as they are easy to learn. The pid can always be 0 (spam is rarely attached to a pre-existing comment). The nid is obtained in the path of where the spam is being posted. The solution is a "private-key", which is what my patch adds. Then sure, hash the private key plus the nid and the pid, and you've got enough protection to prevent most spammers. To make it even more secure, automatic rey-keying could be easily accomplished. ------------------------------------------------------------------------ Thu, 18 Aug 2005 04:09:10 +0000 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/comment.module_15.patch (1.18 KB) The attached patch: 1) gets rid of the _cron() hook 2) no longer requires that comments be previewed Prior to this patch, comment spammers were able to send data to a Drupal server acting as though they'd filled out a comment form and pressed submit. As they didn't actually use the form, they could submit spam comments at an obscene rate. With this patch, comment spammers will have to actually load the form, enter text, and press submit. Yes, that can still be automated, but it takes much more work and slows them down, as they have to wait for the entry form to load each time. Unfortunately a spammer could manually submit one comment, then re-use that same session info over and over to attach repeated spam comments to the same node. Such an attempt would be detected and blocked by the spam module if enabled, but again such a session re-use attack could be done without loading the form each time. Fortunately there is much less gain for a spammer to submit 100 spam comments on the same page, versus submitting 100 spam comments each on a different page as they do now. Ideas to improve upon this concept include: - re-key every day or week, changing the private key regularly to be sure it couldn't ever be permanently cracked - add a key table, and generate a unique key for every comment form. essentially, upon comment form creation generate a random key which is stored both in a database table and in the session. when a comment is submited, look for the key from the session in the database table, if a match is found delete it from the database table and post the comment. this would prevent session re-use, but adds overhead. i don't know if it's worth it, perhaps as an external module if the hooks were available. ------------------------------------------------------------------------ Fri, 19 Aug 2005 18:55:11 +0000 : drumm <?php form_set_error('error', t('Validation error, please be sure cookies are enabled on your browser.')); ?> form_set_error [2]()'s fist argument should be the name of a form field, not 'error.' Using (..., 'error') would be better in this case. And the actual message needs work. Since this is a hidden field I don't think it has anything to do with cookies. [2] http://drupaldocs.org/api/head/function/form_set_error ------------------------------------------------------------------------ Fri, 19 Aug 2005 18:56:41 +0000 : drumm The unclosed link in my last update was supposed to say drupal_set_message(..., 'error') ------------------------------------------------------------------------ Sat, 20 Aug 2005 16:00:15 +0000 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/comment.module_16.patch (1.11 KB) drupal_set_message(..., 'error') isn't sufficient to prevent the comment from being posted. I have instead updated the patch to set the error on the hidden 'token' form field. I have updated the message to read: "Unable to validate your comment, please try again. If this error persists, please contact the site administrator." If you don't like the error message, better suggestions are welcome. ------------------------------------------------------------------------ Fri, 09 Sep 2005 03:16:06 +0000 : Jeremy(a)kerneltrap.org Any feedback on this patch? I have been running it on my website for a couple of weeks, and it has completely stopped the most persistent auto-spam scripts that had been posting poker type comments constantly to my site. ------------------------------------------------------------------------ Sat, 10 Sep 2005 18:12:15 +0000 : Zed Pobre This patch is against HEAD? It doesn't want to apply to my 2.6.3 comment.module. ------------------------------------------------------------------------ Sun, 11 Sep 2005 18:09:08 +0000 : Abalieno It's is for cvs but I'm trying to manually apply it to 4.6.3. Will comment later to tell how it went. ------------------------------------------------------------------------ Mon, 12 Sep 2005 19:49:17 +0000 : Abalieno Well, it worked. No spam at all in more than a day. I don't know if other users are having problem but this patch broke the tool the spammer was using. :) ------------------------------------------------------------------------ Wed, 14 Sep 2005 02:47:40 +0000 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/form_validation.patch (2.72 KB) Here's a completely rewritten version of the patch, based on an email discussion with Dries. This provides a more generic interface that can be used to validate other form submissions, not just comments. The patch introduces two new functions, form_token() and form_validate(). The first function uses a private key and a public key to set a token in a hidden field. The second function validates the token. The patch also updates the comment module, demonstrating how these new functions are used. More information as to how the patch works can be found in the comments that are within the code. Based on my own experiences on kerneltrap.org, this patch blocks 99% of the current comment spammers. ------------------------------------------------------------------------ Wed, 14 Sep 2005 03:16:21 +0000 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/form_validate-2.patch (1.1 KB) This optional patch is intended to be applied after my form_validation.patch attached to #20 above. It makes the drupal_private_key more secure by regenerating it every 24 hours. Without this patch, it would be possible for a spammer to use brute-force to learn a site's private key by reading the code and observing the token generated for a given form. With this patch, brute force is still possible, but with this patch it would have to be done every 24 hours. It is possible for a form to be generated prior to a rekey, and then to be validated after a rekey. In this event, the key will fail validation and the user will see a message telling them something like "Validation error, please try again. If this error persists, please contact the site administrator." When they try pressing "submit" again it will work fine. I kept this patch separate as it adds complexity that may not be desired in core. Personally I think it is important and should be merged along with the first patch. I added it to the system module as it seemed the most logical place, and is a "required" module that can not be disabled.

1 0

[drupal-devel] [bug] comment preview "Required" is easily bypassed
by Jeremy 14 Sep '05

14 Sep '05

Issue status update for http://drupal.org/node/28420 Post a follow up: http://drupal.org/project/comments/add/28420 Project: Drupal Version: cvs Component: comment.module Category: bug reports Priority: normal Assigned to: Jeremy(a)kerneltrap.org Reported by: Jeremy(a)kerneltrap.org Updated by: Jeremy(a)kerneltrap.org Status: patch (code needs review) Attachment: http://drupal.org/files/issues/form_validate-2.patch (1.1 KB) This optional patch is intended to be applied after my form_validation.patch attached to #20 above. It makes the drupal_private_key more secure by regenerating it every 24 hours. Without this patch, it would be possible for a spammer to use brute-force to learn a site's private key by reading the code and observing the token generated for a given form. With this patch, brute force is still possible, but with this patch it would have to be done every 24 hours. It is possible for a form to be generated prior to a rekey, and then to be validated after a rekey. In this event, the key will fail validation and the user will see a message telling them something like "Validation error, please try again. If this error persists, please contact the site administrator." When they try pressing "submit" again it will work fine. I kept this patch separate as it adds complexity that may not be desired in core. Personally I think it is important and should be merged along with the first patch. I added it to the system module as it seemed the most logical place, and is a "required" module that can not be disabled. Jeremy(a)kerneltrap.org Previous comments: ------------------------------------------------------------------------ Mon, 08 Aug 2005 01:55:34 +0000 : Jeremy(a)kerneltrap.org Setting "Preview comment" to "Required" does not strictly require that the comment be previewed first. This is being abused by spammers to quickly and efficiently post spam comments. I discovered this after I added a new feature to my new spam module [1] to auto-blacklist spammer IP addresses, allowing me to block comment spammers when they preview a comment and thus preventing them from ever inserting their spam into my database. I configured my comment module to "require" comment previews, and yet found that the comments were slipping past my filter. I finally realized what the spammer is doing is setting $_POST['op'] to 'Post comment', effectively bypassing the preview phase. I'm currently looking for a clean solution to this. At the moment the only idea I have is to generate a token at the preview phase, and validate the token at the post phase. Unfortunately the token would have to be stored in the databse between the preview and the post, which adds overhead. Alternatively, I've considered using a time-based hash which would constantly update depending on the time of day. This could easily be validated without storing anything in the database. If too long has gone between the preview and the post, an additional preview step would be required... The down side here is that the time-based hash would be publically available, and thus the spammer could easily duplicate it in their script. A private key could solve for that, but increases the complexity as it adds a configuration step. I have the feeling I'm missing a simpler, cleaner solution. Suggestions? [1] http://kerneltrap.org/jeremy/drupal/spam/ ------------------------------------------------------------------------ Mon, 08 Aug 2005 02:26:21 +0000 : moshe weitzman even if you get this fixed, won't these bots just add a preview step? this 'preview required' feature is designed to maintain high quality submissions by forcing users to proof read. it isn;t designed for security. i think you want to hook into comment_validate(). just add a hook here - there is already a hook_comment() waiting for you to add an operation. ------------------------------------------------------------------------ Mon, 08 Aug 2005 08:49:30 +0000 : Eaton I posted a patch a few days ago (http://drupal.org/node/28255) that adds validation and form construction hooks for comments. It's similar to the one that the captcha module uses, though it adds comment form_pre and form_post hooks instead of a single comment form hook. ------------------------------------------------------------------------ Mon, 08 Aug 2005 13:30:34 +0000 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/comment.module_11.patch (2.5 KB) > even if you get this fixed, won't these bots just add a preview step? Eventually, yes, but it drastically changes their ability to fling spam at a site. As is, they simply have a script that shoots data out at high speed without having to wait for messages to return from the server. It is the server that is doing all the work, thus making it simple for a spammer to DoS a site. If "preview required" really meant "preview required", they would be forced to first automate clicking "preview", and then wait for a response before clicking "submit". This requires more resources on their side, and allows us to add delays after clicking "preview" (if we detect that they are a spammer) further using their resources. > this 'preview required' feature is designed to maintain high quality > submissions by forcing users to proof read. it isn;t designed for security. Regardless of the intention, I was misled to believe that configuring my site to require previews would require that all comments were first previewed. As a site administrator, I would prefer to know that "required" really means "required". > i think you want to hook into comment_validate(). just add a hook here - > there is already a hook_comment() waiting for you to add an operation. Yes and no. Ultimately yes that will work and will allow my spam module to prevent the spam from ever being posted. But it still leaves the greatest burden on the web server, instead of on the spammer. The spammer can still use a very simple script that only pushes data, and thus can generate spam at an unbelievable rapid rate. Here is an example patch to enforce "preview required". It's one idea, I'm sure there are better ones. ------------------------------------------------------------------------ Mon, 08 Aug 2005 14:01:38 +0000 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/comment.module_12.patch (1.4 KB) Here's a second version of the patch that doesn't require any manual configuration. ------------------------------------------------------------------------ Mon, 08 Aug 2005 16:27:27 +0000 : Jose A Reyero I like this idea, and the patch looks good Still, I think it misses something, like some timestamp related hash, because once you get the hash code you can post multiple comments with that. Another problem I can think of is, what happens when a cron run happens between the preview and the post?? I'm afraid comments would get lost For this second problem, I think a key generated only once after module activation could do. About the first one....mmm... I'll sit down for a while and think..... ------------------------------------------------------------------------ Tue, 09 Aug 2005 12:27:20 +0000 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/comment.module_13.patch (1.33 KB) > I think it misses something, like some timestamp related hash, because > once you get the hash code you can post multiple comments with that. Using a timestamp will mean that the comment form "expires". That is, if you wait too long to preview your comment, it will generate an error when you try to post. Yes, technically a spammer could post one real comment, then based on what was in the session from that they could post the same identical comment over and over, so long as it was attached to the same node. But this is not what they do, they try and spread their spam throughout your webpage. Furthermore, the spam module is perfectly capable of detecting and preventing this. > Another problem I can think of is, what happens when a cron run happens > between the preview and the post?? I'm afraid comments would get lost The key is only generated once, that's what the first test is about. In any case, in the unlikely event that the key were to change between preview and post they would simply have to post a second time. My earlier patch wasn't quite right, I was testing the token in the wrong place. This patch fixes that. BTW: This is beneficial for maintaining high quality submissions too, as prior to this change someone could: 1) enter a comment 2) press preview 3) completely change their comment (introducing a mistake) 4) press submit and the comment (mistake and all) would go into the database unpreviewed After this change: 1) enter a comment 2) press preview 3) completely change their comment (introducing a mistake) 4) press submit and they get an error because they didn't preview their changes - forcing them to preview once after any change ------------------------------------------------------------------------ Wed, 10 Aug 2005 03:50:33 +0000 : Jeremy(a)kerneltrap.org FWIW: I've been getting slammed by spam attacks this whole week. Installing this patch has made a huge difference. Well over 100 spam attempts per minute (sometimes two and three times that) and I hardly notice the spammer, whereas before it was choking my database. (Granted, the spammer has not yet upgraded his script to first preview, then submit. But even if he did it wouldn't help him as testing has verified that the new spam module would prevent the comments from ever getting to the database.) Additionally, user and anonymous (nonspam) comments continue to show up at a normal rate. ------------------------------------------------------------------------ Tue, 16 Aug 2005 14:08:04 +0000 : Jeremy(a)kerneltrap.org I would love to see _any_ discussion on this. Drupal is currently too easy to spam, with little effort on the spammer's side, and lots of resources wasted on the Drupal side. A patch like this will greatly increase the spammer's burden, and make it possible to effectively block even the most aggressive spammer attacks. ------------------------------------------------------------------------ Wed, 17 Aug 2005 16:24:04 +0000 : Jose A Reyero Well, this patch is definitely better than what we have, and would save some spam for sure. But maybe keeping track, at the session level, of generated hashes for a user, and then removing them when the comment is sent, could do the work. This way we can forget about previewing comments or not, and also the "permission" to post the comment would expire when the session expires. Any randomly generated value could do for this, no need for complex hashes, but having nid and pid in the hash would add some extra security. ------------------------------------------------------------------------ Wed, 17 Aug 2005 19:58:02 +0000 : breyten Jeremy, a big +1 on the idea, but why not generate the private key when it is actually needed (Ie, when displaying the comment form), instead of wasting a _cron() hook on it? ------------------------------------------------------------------------ Thu, 18 Aug 2005 03:20:02 +0000 : Jeremy(a)kerneltrap.org > Well, this patch is definitely better than what we have, and would save some spam for sure. It is continuing to work very well on my site, which seems to be under nearly perpetual spam attacks from multiple sources. > But maybe keeping track, at the session level, of generated hashes for a user, and then > removing them when the comment is sent, could do the work. The catch is: the key has to be something unique to the server, not guessable or learnable from the outside Simply storing the hash data in the session alone is not enough, as then the spammer could create any random data and store it in the session. That said, the hash could be generated off something other than the text of the comment as it is now, so that a preview is not required. I'll look at doing something like that and submit another patch. > This way we can forget about previewing comments or not, and also the "permission" to > post the comment would expire when the session expires. Any randomly generated value > could do for this, no need for complex hashes, but having nid and pid in the hash would > add some extra security. nid and pid alone are worthless, as they are easy to learn. The pid can always be 0 (spam is rarely attached to a pre-existing comment). The nid is obtained in the path of where the spam is being posted. The solution is a "private-key", which is what my patch adds. Then sure, hash the private key plus the nid and the pid, and you've got enough protection to prevent most spammers. To make it even more secure, automatic rey-keying could be easily accomplished. ------------------------------------------------------------------------ Thu, 18 Aug 2005 04:09:10 +0000 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/comment.module_15.patch (1.18 KB) The attached patch: 1) gets rid of the _cron() hook 2) no longer requires that comments be previewed Prior to this patch, comment spammers were able to send data to a Drupal server acting as though they'd filled out a comment form and pressed submit. As they didn't actually use the form, they could submit spam comments at an obscene rate. With this patch, comment spammers will have to actually load the form, enter text, and press submit. Yes, that can still be automated, but it takes much more work and slows them down, as they have to wait for the entry form to load each time. Unfortunately a spammer could manually submit one comment, then re-use that same session info over and over to attach repeated spam comments to the same node. Such an attempt would be detected and blocked by the spam module if enabled, but again such a session re-use attack could be done without loading the form each time. Fortunately there is much less gain for a spammer to submit 100 spam comments on the same page, versus submitting 100 spam comments each on a different page as they do now. Ideas to improve upon this concept include: - re-key every day or week, changing the private key regularly to be sure it couldn't ever be permanently cracked - add a key table, and generate a unique key for every comment form. essentially, upon comment form creation generate a random key which is stored both in a database table and in the session. when a comment is submited, look for the key from the session in the database table, if a match is found delete it from the database table and post the comment. this would prevent session re-use, but adds overhead. i don't know if it's worth it, perhaps as an external module if the hooks were available. ------------------------------------------------------------------------ Fri, 19 Aug 2005 18:55:11 +0000 : drumm <?php form_set_error('error', t('Validation error, please be sure cookies are enabled on your browser.')); ?> form_set_error [2]()'s fist argument should be the name of a form field, not 'error.' Using (..., 'error') would be better in this case. And the actual message needs work. Since this is a hidden field I don't think it has anything to do with cookies. [2] http://drupaldocs.org/api/head/function/form_set_error ------------------------------------------------------------------------ Fri, 19 Aug 2005 18:56:41 +0000 : drumm The unclosed link in my last update was supposed to say drupal_set_message(..., 'error') ------------------------------------------------------------------------ Sat, 20 Aug 2005 16:00:15 +0000 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/comment.module_16.patch (1.11 KB) drupal_set_message(..., 'error') isn't sufficient to prevent the comment from being posted. I have instead updated the patch to set the error on the hidden 'token' form field. I have updated the message to read: "Unable to validate your comment, please try again. If this error persists, please contact the site administrator." If you don't like the error message, better suggestions are welcome. ------------------------------------------------------------------------ Fri, 09 Sep 2005 03:16:06 +0000 : Jeremy(a)kerneltrap.org Any feedback on this patch? I have been running it on my website for a couple of weeks, and it has completely stopped the most persistent auto-spam scripts that had been posting poker type comments constantly to my site. ------------------------------------------------------------------------ Sat, 10 Sep 2005 18:12:15 +0000 : Zed Pobre This patch is against HEAD? It doesn't want to apply to my 2.6.3 comment.module. ------------------------------------------------------------------------ Sun, 11 Sep 2005 18:09:08 +0000 : Abalieno It's is for cvs but I'm trying to manually apply it to 4.6.3. Will comment later to tell how it went. ------------------------------------------------------------------------ Mon, 12 Sep 2005 19:49:17 +0000 : Abalieno Well, it worked. No spam at all in more than a day. I don't know if other users are having problem but this patch broke the tool the spammer was using. :) ------------------------------------------------------------------------ Wed, 14 Sep 2005 02:47:40 +0000 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/form_validation.patch (2.72 KB) Here's a completely rewritten version of the patch, based on an email discussion with Dries. This provides a more generic interface that can be used to validate other form submissions, not just comments. The patch introduces two new functions, form_token() and form_validate(). The first function uses a private key and a public key to set a token in a hidden field. The second function validates the token. The patch also updates the comment module, demonstrating how these new functions are used. More information as to how the patch works can be found in the comments that are within the code. Based on my own experiences on kerneltrap.org, this patch blocks 99% of the current comment spammers.

1 0

[drupal-devel] [bug] comment preview "Required" is easily bypassed
by Jeremy 14 Sep '05

14 Sep '05

Issue status update for http://drupal.org/node/28420 Post a follow up: http://drupal.org/project/comments/add/28420 Project: Drupal Version: cvs Component: comment.module Category: bug reports Priority: normal Assigned to: Jeremy(a)kerneltrap.org Reported by: Jeremy(a)kerneltrap.org Updated by: Jeremy(a)kerneltrap.org Status: patch (code needs review) Attachment: http://drupal.org/files/issues/form_validation.patch (2.72 KB) Here's a completely rewritten version of the patch, based on an email discussion with Dries. This provides a more generic interface that can be used to validate other form submissions, not just comments. The patch introduces two new functions, form_token() and form_validate(). The first function uses a private key and a public key to set a token in a hidden field. The second function validates the token. The patch also updates the comment module, demonstrating how these new functions are used. More information as to how the patch works can be found in the comments that are within the code. Based on my own experiences on kerneltrap.org, this patch blocks 99% of the current comment spammers. Jeremy(a)kerneltrap.org Previous comments: ------------------------------------------------------------------------ Mon, 08 Aug 2005 01:55:34 +0000 : Jeremy(a)kerneltrap.org Setting "Preview comment" to "Required" does not strictly require that the comment be previewed first. This is being abused by spammers to quickly and efficiently post spam comments. I discovered this after I added a new feature to my new spam module [1] to auto-blacklist spammer IP addresses, allowing me to block comment spammers when they preview a comment and thus preventing them from ever inserting their spam into my database. I configured my comment module to "require" comment previews, and yet found that the comments were slipping past my filter. I finally realized what the spammer is doing is setting $_POST['op'] to 'Post comment', effectively bypassing the preview phase. I'm currently looking for a clean solution to this. At the moment the only idea I have is to generate a token at the preview phase, and validate the token at the post phase. Unfortunately the token would have to be stored in the databse between the preview and the post, which adds overhead. Alternatively, I've considered using a time-based hash which would constantly update depending on the time of day. This could easily be validated without storing anything in the database. If too long has gone between the preview and the post, an additional preview step would be required... The down side here is that the time-based hash would be publically available, and thus the spammer could easily duplicate it in their script. A private key could solve for that, but increases the complexity as it adds a configuration step. I have the feeling I'm missing a simpler, cleaner solution. Suggestions? [1] http://kerneltrap.org/jeremy/drupal/spam/ ------------------------------------------------------------------------ Mon, 08 Aug 2005 02:26:21 +0000 : moshe weitzman even if you get this fixed, won't these bots just add a preview step? this 'preview required' feature is designed to maintain high quality submissions by forcing users to proof read. it isn;t designed for security. i think you want to hook into comment_validate(). just add a hook here - there is already a hook_comment() waiting for you to add an operation. ------------------------------------------------------------------------ Mon, 08 Aug 2005 08:49:30 +0000 : Eaton I posted a patch a few days ago (http://drupal.org/node/28255) that adds validation and form construction hooks for comments. It's similar to the one that the captcha module uses, though it adds comment form_pre and form_post hooks instead of a single comment form hook. ------------------------------------------------------------------------ Mon, 08 Aug 2005 13:30:34 +0000 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/comment.module_11.patch (2.5 KB) > even if you get this fixed, won't these bots just add a preview step? Eventually, yes, but it drastically changes their ability to fling spam at a site. As is, they simply have a script that shoots data out at high speed without having to wait for messages to return from the server. It is the server that is doing all the work, thus making it simple for a spammer to DoS a site. If "preview required" really meant "preview required", they would be forced to first automate clicking "preview", and then wait for a response before clicking "submit". This requires more resources on their side, and allows us to add delays after clicking "preview" (if we detect that they are a spammer) further using their resources. > this 'preview required' feature is designed to maintain high quality > submissions by forcing users to proof read. it isn;t designed for security. Regardless of the intention, I was misled to believe that configuring my site to require previews would require that all comments were first previewed. As a site administrator, I would prefer to know that "required" really means "required". > i think you want to hook into comment_validate(). just add a hook here - > there is already a hook_comment() waiting for you to add an operation. Yes and no. Ultimately yes that will work and will allow my spam module to prevent the spam from ever being posted. But it still leaves the greatest burden on the web server, instead of on the spammer. The spammer can still use a very simple script that only pushes data, and thus can generate spam at an unbelievable rapid rate. Here is an example patch to enforce "preview required". It's one idea, I'm sure there are better ones. ------------------------------------------------------------------------ Mon, 08 Aug 2005 14:01:38 +0000 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/comment.module_12.patch (1.4 KB) Here's a second version of the patch that doesn't require any manual configuration. ------------------------------------------------------------------------ Mon, 08 Aug 2005 16:27:27 +0000 : Jose A Reyero I like this idea, and the patch looks good Still, I think it misses something, like some timestamp related hash, because once you get the hash code you can post multiple comments with that. Another problem I can think of is, what happens when a cron run happens between the preview and the post?? I'm afraid comments would get lost For this second problem, I think a key generated only once after module activation could do. About the first one....mmm... I'll sit down for a while and think..... ------------------------------------------------------------------------ Tue, 09 Aug 2005 12:27:20 +0000 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/comment.module_13.patch (1.33 KB) > I think it misses something, like some timestamp related hash, because > once you get the hash code you can post multiple comments with that. Using a timestamp will mean that the comment form "expires". That is, if you wait too long to preview your comment, it will generate an error when you try to post. Yes, technically a spammer could post one real comment, then based on what was in the session from that they could post the same identical comment over and over, so long as it was attached to the same node. But this is not what they do, they try and spread their spam throughout your webpage. Furthermore, the spam module is perfectly capable of detecting and preventing this. > Another problem I can think of is, what happens when a cron run happens > between the preview and the post?? I'm afraid comments would get lost The key is only generated once, that's what the first test is about. In any case, in the unlikely event that the key were to change between preview and post they would simply have to post a second time. My earlier patch wasn't quite right, I was testing the token in the wrong place. This patch fixes that. BTW: This is beneficial for maintaining high quality submissions too, as prior to this change someone could: 1) enter a comment 2) press preview 3) completely change their comment (introducing a mistake) 4) press submit and the comment (mistake and all) would go into the database unpreviewed After this change: 1) enter a comment 2) press preview 3) completely change their comment (introducing a mistake) 4) press submit and they get an error because they didn't preview their changes - forcing them to preview once after any change ------------------------------------------------------------------------ Wed, 10 Aug 2005 03:50:33 +0000 : Jeremy(a)kerneltrap.org FWIW: I've been getting slammed by spam attacks this whole week. Installing this patch has made a huge difference. Well over 100 spam attempts per minute (sometimes two and three times that) and I hardly notice the spammer, whereas before it was choking my database. (Granted, the spammer has not yet upgraded his script to first preview, then submit. But even if he did it wouldn't help him as testing has verified that the new spam module would prevent the comments from ever getting to the database.) Additionally, user and anonymous (nonspam) comments continue to show up at a normal rate. ------------------------------------------------------------------------ Tue, 16 Aug 2005 14:08:04 +0000 : Jeremy(a)kerneltrap.org I would love to see _any_ discussion on this. Drupal is currently too easy to spam, with little effort on the spammer's side, and lots of resources wasted on the Drupal side. A patch like this will greatly increase the spammer's burden, and make it possible to effectively block even the most aggressive spammer attacks. ------------------------------------------------------------------------ Wed, 17 Aug 2005 16:24:04 +0000 : Jose A Reyero Well, this patch is definitely better than what we have, and would save some spam for sure. But maybe keeping track, at the session level, of generated hashes for a user, and then removing them when the comment is sent, could do the work. This way we can forget about previewing comments or not, and also the "permission" to post the comment would expire when the session expires. Any randomly generated value could do for this, no need for complex hashes, but having nid and pid in the hash would add some extra security. ------------------------------------------------------------------------ Wed, 17 Aug 2005 19:58:02 +0000 : breyten Jeremy, a big +1 on the idea, but why not generate the private key when it is actually needed (Ie, when displaying the comment form), instead of wasting a _cron() hook on it? ------------------------------------------------------------------------ Thu, 18 Aug 2005 03:20:02 +0000 : Jeremy(a)kerneltrap.org > Well, this patch is definitely better than what we have, and would save some spam for sure. It is continuing to work very well on my site, which seems to be under nearly perpetual spam attacks from multiple sources. > But maybe keeping track, at the session level, of generated hashes for a user, and then > removing them when the comment is sent, could do the work. The catch is: the key has to be something unique to the server, not guessable or learnable from the outside Simply storing the hash data in the session alone is not enough, as then the spammer could create any random data and store it in the session. That said, the hash could be generated off something other than the text of the comment as it is now, so that a preview is not required. I'll look at doing something like that and submit another patch. > This way we can forget about previewing comments or not, and also the "permission" to > post the comment would expire when the session expires. Any randomly generated value > could do for this, no need for complex hashes, but having nid and pid in the hash would > add some extra security. nid and pid alone are worthless, as they are easy to learn. The pid can always be 0 (spam is rarely attached to a pre-existing comment). The nid is obtained in the path of where the spam is being posted. The solution is a "private-key", which is what my patch adds. Then sure, hash the private key plus the nid and the pid, and you've got enough protection to prevent most spammers. To make it even more secure, automatic rey-keying could be easily accomplished. ------------------------------------------------------------------------ Thu, 18 Aug 2005 04:09:10 +0000 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/comment.module_15.patch (1.18 KB) The attached patch: 1) gets rid of the _cron() hook 2) no longer requires that comments be previewed Prior to this patch, comment spammers were able to send data to a Drupal server acting as though they'd filled out a comment form and pressed submit. As they didn't actually use the form, they could submit spam comments at an obscene rate. With this patch, comment spammers will have to actually load the form, enter text, and press submit. Yes, that can still be automated, but it takes much more work and slows them down, as they have to wait for the entry form to load each time. Unfortunately a spammer could manually submit one comment, then re-use that same session info over and over to attach repeated spam comments to the same node. Such an attempt would be detected and blocked by the spam module if enabled, but again such a session re-use attack could be done without loading the form each time. Fortunately there is much less gain for a spammer to submit 100 spam comments on the same page, versus submitting 100 spam comments each on a different page as they do now. Ideas to improve upon this concept include: - re-key every day or week, changing the private key regularly to be sure it couldn't ever be permanently cracked - add a key table, and generate a unique key for every comment form. essentially, upon comment form creation generate a random key which is stored both in a database table and in the session. when a comment is submited, look for the key from the session in the database table, if a match is found delete it from the database table and post the comment. this would prevent session re-use, but adds overhead. i don't know if it's worth it, perhaps as an external module if the hooks were available. ------------------------------------------------------------------------ Fri, 19 Aug 2005 18:55:11 +0000 : drumm <?php form_set_error('error', t('Validation error, please be sure cookies are enabled on your browser.')); ?> form_set_error [2]()'s fist argument should be the name of a form field, not 'error.' Using (..., 'error') would be better in this case. And the actual message needs work. Since this is a hidden field I don't think it has anything to do with cookies. [2] http://drupaldocs.org/api/head/function/form_set_error ------------------------------------------------------------------------ Fri, 19 Aug 2005 18:56:41 +0000 : drumm The unclosed link in my last update was supposed to say drupal_set_message(..., 'error') ------------------------------------------------------------------------ Sat, 20 Aug 2005 16:00:15 +0000 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/comment.module_16.patch (1.11 KB) drupal_set_message(..., 'error') isn't sufficient to prevent the comment from being posted. I have instead updated the patch to set the error on the hidden 'token' form field. I have updated the message to read: "Unable to validate your comment, please try again. If this error persists, please contact the site administrator." If you don't like the error message, better suggestions are welcome. ------------------------------------------------------------------------ Fri, 09 Sep 2005 03:16:06 +0000 : Jeremy(a)kerneltrap.org Any feedback on this patch? I have been running it on my website for a couple of weeks, and it has completely stopped the most persistent auto-spam scripts that had been posting poker type comments constantly to my site. ------------------------------------------------------------------------ Sat, 10 Sep 2005 18:12:15 +0000 : Zed Pobre This patch is against HEAD? It doesn't want to apply to my 2.6.3 comment.module. ------------------------------------------------------------------------ Sun, 11 Sep 2005 18:09:08 +0000 : Abalieno It's is for cvs but I'm trying to manually apply it to 4.6.3. Will comment later to tell how it went. ------------------------------------------------------------------------ Mon, 12 Sep 2005 19:49:17 +0000 : Abalieno Well, it worked. No spam at all in more than a day. I don't know if other users are having problem but this patch broke the tool the spammer was using. :)

1 0

Re: [drupal-devel] Mail.inc
by Pat Collins 14 Sep '05

14 Sep '05

On Mon, 12 Sep 2005 22:06:29 +0200, Robert Douglass <rob(a)robshouse.net> wrote : > Is anyone actually working on mail.inc and if so, how far along are you? > I've got one working. Still doing some unit testing. I also have to read the contribution documentation a bit more closely so I don't sound like an idiot and make it easy if it is decided to be included. Pat

1 0

[drupal-devel] [task] Improvments to Filter UI
by moshe weitzman 14 Sep '05

14 Sep '05

Issue status update for http://drupal.org/node/27364 Post a follow up: http://drupal.org/project/comments/add/27364 Project: Drupal Version: cvs Component: filter.module Category: tasks Priority: normal Assigned to: Bèr Kessels Reported by: Bèr Kessels Updated by: moshe weitzman Status: patch (code needs work) the most visible place for this 'input format' term is on the node form. thats where ordinary users see this stuff. in that context, i don't think 'input filters is an improvement over input formats. the question we are asking our users is 'what is the format of your post'?, not what filters do you want applied ... just my .02. sorry i wasn't in IRC when this came up. this need not hold up the whole patch. moshe weitzman Previous comments: ------------------------------------------------------------------------ Sun, 24 Jul 2005 09:30:54 +0000 : Bèr Kessels This is a mockup for the Filter UI. Currently it is soo complex, that I dare to call it broken. Please comment on this, for I will not ake any patches, when the general idea is disliked. The idea is t split filters and input formats better. Filters are filters, defined in modules. Input formats are bundles of filters. This is how we have it ATM, but the interface fails to communicate that. I tried to develop a consistent (with the rest of Drupal) interfce that makes it clearer what is what. The menu is as follows: * administer ** .... ** settings *** input formats *** filters ** ... ------------------------------------------------------------------------ Sun, 24 Jul 2005 09:37:10 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/input-formats_list1.png (16.34 KB) * administer ** .... ** settings *** input formats >> see attachement 1 *** filters ** ------------------------------------------------------------------------ Sun, 24 Jul 2005 09:39:02 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/input-formats_add.png (23.42 KB) * administer ** .... ** settings *** input formats >> see attachement 2, tab 2 *** filters ** Note: The default checkbox lmay seem a bit odd. But checking it will remove the "default status" from the current "default" one and make this one "default". The help text, and a drupal_set_message should explain this. ------------------------------------------------------------------------ Sun, 24 Jul 2005 09:40:08 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/input-formats_edit_format.png (22.17 KB) * administer ** .... ** settings *** input formats >> clicking a 'configure' link in the table of attachement 1 *** filters ** ------------------------------------------------------------------------ Sun, 24 Jul 2005 09:40:55 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/input-filters_list.png (36.24 KB) * administer ** .... ** settings *** input formats *** filters >> see attachement 4 ** ------------------------------------------------------------------------ Sun, 24 Jul 2005 09:41:40 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/input-formats_edit.png (22.63 KB) * administer ** .... ** settings *** input formats *** filters >> clicking a 'configure' link in the table of attachement 4 ** ------------------------------------------------------------------------ Mon, 25 Jul 2005 14:46:27 +0000 : Bèr Kessels IMO this makes the interface easier. But other disagree. And because it also removes one feature: (being able to use E.g. HTML in different input formats, with different settings) I'll simply wont fix this :( ------------------------------------------------------------------------ Thu, 28 Jul 2005 13:36:12 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy.patch (10.28 KB) I decided to go for it anyway. Be it in a slightly different way then my initial mockups. The difference is that I left the filters where they are, hidden beneath the formats. So, here is the patch that makes the filter UI more consistent with screens like blocs, menus, vocabularies et al also nice to note is that: 93 lines are added, 104 are removed. So netto we have less code :) (cvs diff filter.module | grep ^+ | wc -l) ------------------------------------------------------------------------ Thu, 28 Jul 2005 13:37:02 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/input-formats_list_html.png (44.64 KB) Here is how the listing now looks. ------------------------------------------------------------------------ Thu, 28 Jul 2005 13:37:57 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/input-formats_edit_format_html.png (113.71 KB) And the "configure" screen. The 'add' screen looks exactly the same, only with the default values pre-filled. ------------------------------------------------------------------------ Thu, 28 Jul 2005 13:39:19 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/input-formats_edit_format_too_long.png (121.82 KB) Oh, And here is a really nice example of why the current screen is no good. And here I "only" have four roles. I run a site with 12 roles, imagine this screen then! ------------------------------------------------------------------------ Thu, 28 Jul 2005 13:48:35 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy_HEAD.patch (10.24 KB) And here is a pathc for HEAD (I forgot that I developed this on a 4.6.2 codebase. Sorry) ------------------------------------------------------------------------ Thu, 28 Jul 2005 13:55:25 +0000 : stefan nagtegaal I like this very, very much! This is - indeed - much better than the current UI. This always fits, and makes the life of people who use a fixed width theme much nicer.. Ber, FYI (you probably know this too), 'admin/access' does also break fixed width themes due to the fact that it expands the width of the table when more roles are being added.. Good catch! I like it much more this way.. +1 from me... ------------------------------------------------------------------------ Fri, 29 Jul 2005 07:27:34 +0000 : Dries The code style needs work (spaces, tabs, 'as' -> 'AS'), the code comments need work, and debug statements should be removed. Screenshots look good to me. db_query('UPDATE {filter_formats} SET cache = %d, name = \'%s\', roles = \'%s\' WHERE format = %d', (int)$cache, $name, $roles, $format); can be written better/shorter quote-wise. ------------------------------------------------------------------------ Fri, 29 Jul 2005 09:04:16 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy_HEAD_0.patch (10.37 KB) Dries, thanks a lot for the review. OI fixed your comments. Or so I hope. code-style.pl was of no use (can that *please* be removed from core?) because it chokes on the help texts. It also pointed out a lot of old formatting isues, whic, when fixed wuold flood this patch with unrelated fixes. I tried to narow my style-searching to the areas I touched. And I made the comments more verbous. The query is fixed. it saves no characters, but looks better now. Thanks for the tip. The print_r..... blush. thats what I get for not running my default grep -r print_r on my code after finishing up... I could not find a lot of spaces and tabs. So it lmight be that i missed one. I triple checked it though. ------------------------------------------------------------------------ Fri, 29 Jul 2005 09:32:21 +0000 : Steven Moving the role settings inward does fix the wide-table problem, but it makes the filter configuration much more opaque. We could perhaps fix this by showing the roles as a comma-separate list in the input formats table. Such a list can wrap and will not stretch the table. Other than that: * Having the instructions for the Roles and Filters form_group() at the bottom is a bit weird. The distance is too big. * The table that was used for toggling filters on/off was much more compact before, and IMO a lot clearer. Why change to a flat list with the description staggered below each item? ------------------------------------------------------------------------ Fri, 29 Jul 2005 10:31:30 +0000 : Bèr Kessels consistancy, consistancy and more consistancy. There really is nothing worse that having a different 'concept' of UI for every thing in Drupal. We are going in the right direction, with the blocks being editable objects, menus acting like that, taxonomy, users, etc.. I wanted to make this act similar. I am not at all happy with the fact tah the list is still a form. But we need this, for setting the default. A big -1 for a comma-separate list. That is extremely hackish, even worse useabilitywise and just silly * we should avoid typing when not needed. * we should avoid errors on input, when they can be avoided (a typo is made very easy, I know all about it) Form set errors won't fix that usability -wise. I agree that you sort-of loose the overview. But IMO that is a minor loss comared to what we gain, usability-wise. The wtoo-wide was onlywhat triggered me. But do not think that I am noly making this patch to fix that issue. This patch is there to improve usability, through consistancy. o make drupal behave the same in most places in admin. If you know one screen, you know most of them. We really should move away from custom-hacks for every page, because that is useability rule #1: consistancy. Stadardisation, even if sometimes a custom hack might make things easier in that single case, it will reduce your overall usability so much that nearly ever it is worth that hack. That select-boxes instead of the table issue: consistancy. As well that it completely broke fluid CSS layouts, my solution is more consistant, and uses forms the way they are meant. If we cannot use $descritption in checkboxes they should be removed. but IMO it feels much more consistant. Tables are for tabular data, and this is simply a list of items, with additional data. And i do not really get what you mean with "too far down". Do you mean that you would like to see the order of the groups different? ------------------------------------------------------------------------ Fri, 29 Jul 2005 13:31:24 +0000 : Dries I haven't tested Ber's patch yet but looking at the screenshots, it like what I see. I remember being confused by the current filter UI, and often, I still am. ------------------------------------------------------------------------ Fri, 29 Jul 2005 19:53:26 +0000 : moshe weitzman I dislike the recent movement away from overview pages. I think we can still provide and overview while allowing editing to happen in a dedicated form. An example in the wrong direction is 'content types' admin. It is impossible to get an overview, and the listing page is worthless. I agree with Steven on this one. It is true that consistency is desirable. But consistent crap is not. ------------------------------------------------------------------------ Fri, 29 Jul 2005 20:02:39 +0000 : killes(a)www.drop.org I agree with Moshe. Getting a quick overview about what is set how is essential for an admin. ------------------------------------------------------------------------ Sat, 30 Jul 2005 00:27:15 +0000 : Robrecht Jacques Overall I like the screenshots. Did not test the patch. Bèr, what I think Steven (#15) meant by "showing the roles as a comma-separate list in the input formats table" is that as an overview page it would be better if you added a list (not an input box!) to screenshot "input-formats_list1.png" (comment #1). Add a column that reads "Roles" (or "Enabled for") and then for each row a comma seperated list of the roles that can use this input format (or maybe even better a HTML list). You would still need to click "configure" to _change_ the roles. The "Default" text (not a radio button) you already show in the "Options" column is something similar: it immediately shows that this input format is the default without you having to go to the "configure" page. Why not do the same for "roles"? If this is what Steven meant, then I tend to agree with him. And to help moshe and killes: maybe you could even add a column that _lists_ the used input filters. Again, only a list, not a way to change it. That's what the "operations" column is for. A table like that will "fluid" better than what we have now, still it is a _complete_ overview of the settings. I could have misunderstand someone... I have the tendency to do that... ------------------------------------------------------------------------ Mon, 22 Aug 2005 08:21:32 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_interface_improvements.patch (14.52 KB) by popular demand: a comma separated list. ------------------------------------------------------------------------ Mon, 22 Aug 2005 08:24:31 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/format_now_contains_roles.png (22.09 KB) and a screenshot ------------------------------------------------------------------------ Mon, 22 Aug 2005 18:39:44 +0000 : Dries Please check your use of print theme('page', $output). Normally, you just return $output. ------------------------------------------------------------------------ Mon, 29 Aug 2005 18:54:28 +0000 : Bèr Kessels All instances of theme('page',...) in filter.module are now changed to return ... ; furthermore, the patch is the same. ------------------------------------------------------------------------ Mon, 29 Aug 2005 20:04:02 +0000 : Uwe Hermann Bèr, I think you forgot to attach your updated patch. ------------------------------------------------------------------------ Mon, 29 Aug 2005 20:27:39 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_interface_improvements_2.patch (13.52 KB) my email client has this nice warning system for attachements. Maybe drupal should search for the word attachement too, and when no att. found give an error ;). Or maybe i should just learn to pay attention. Guess that is easier. Anyway, here it is. ------------------------------------------------------------------------ Thu, 08 Sep 2005 20:22:54 +0000 : Dries I wanted to apply this patch but: 1. I can't change the default input format. 'Save'-button doesn't work. 2. I got confused by the fact I can't change the roles of the default filter. I think the form group description should explain this. 3. form_group(t(filter)) should be form_group(t('filter'), ...). ------------------------------------------------------------------------ Thu, 08 Sep 2005 20:51:24 +0000 : m3avrck Found a few more issues: If I go into 'Configure' for 'PHP Code' and click the 'Configure' tab at the top and then click 'list filters' link in the text, I get this warning with PHP5 warning: Missing argument 1 for filter_admin_format() in \drupal\modules\filter.module on line 378. Also, it is sort of confusing as to what the 'List' tab implies. For example, if I click on 'input formats' in the admin menu, I get a list of the current filters. When I click 'configure' for one of those, I see the options for that filter. However, it says 'list' in one of the tab, which doesn't really mean much. I thought that was the list of filters, not the options for that filter. That should be cleaned up. Also, there is no way to get back to the full list of filters unless you click on 'input formats' in the admin menu. A setting/link to fix this would be great. Also, where is the link to add filters??? I think the options/layout/tabs should mimic of that of admin > users. So on admin > input formats, it has the tabs 'list', 'add format' ... very clear and consistent. On a configure page for a filter, it should have 'list', 'view', 'configure', 'rearrange' ... this would make it easier to navigate and get back to the original list of filters. Make sense? I think this and Dries' comments put this patch over the top :) ------------------------------------------------------------------------ Fri, 09 Sep 2005 01:48:55 +0000 : tag In my initial battle to figure out this module's (quite nice) functionality, what tripped me up the most was actually the nomenclature. The way I keep it straight in my head now is to translate "input format" to "filter set". If you view those (the current) screens with this in mind, I think it's a lot easier to understand what's what. It sort of describes this in the help text, but the terms "filter" and "input format" don't really convey the relationship between the two -- there's no way to intuit that an "input format" is a collection of "filters". Not to my mind at least... and well, we know nobody reads instructions... Are there any technicalities that make renaming "input format" to "filter set" not accurate? Or does someone have a better term/terms to better show the relationship of these elements? (I'm aware of filters in servlet architectures but I don't think there's much of a collision with that usage). Anyhow, I guess this doesn't quite address the UI controls and/or layout, but would still help a lot. ------------------------------------------------------------------------ Fri, 09 Sep 2005 06:35:34 +0000 : Bèr Kessels I prefer filter set. In fact: I like it a lot. Anyone else ideas on this? ------------------------------------------------------------------------ Fri, 09 Sep 2005 10:18:18 +0000 : Dries I too had problems with the terminology; it took me months to get used to 'input formats' versus 'filters'. Using 'filter sets' might improve the situation -- especially from an administrator's point of view when you have to wrap your head around the UI/difference. I'm not native English, but 'filter set' sounds more explanatory, yet slightly more geeky so I'm not sure if it flies for users who don't need to understand the underlying concepts (eg. under the node and comment submission forms). ------------------------------------------------------------------------ Fri, 09 Sep 2005 11:00:05 +0000 : Bèr Kessels a heads up: I ma redoing the patch. But decided no to change the name yet. That is a too big change. IT should be in a separate issue, IMO. ------------------------------------------------------------------------ Fri, 09 Sep 2005 15:27:43 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy_2.patch (13.57 KB) * Bugs as reported by dries fixed. * The [add] tab not appearing is due to your menu caching, refresh that please. * Fixed an additional bug: Some agents do not send TRUE for disabled checkboxen. Also in current filter admin. and people: we have a problem. Deleting a filter trows errors, due to the revisions changes. that happens in filter module now, but also after this patch. I have too little knowledge of the filter system to fix that, and IMO that is a separate issue. It should be fixed right after this patch is committed. ------------------------------------------------------------------------ Fri, 09 Sep 2005 15:57:18 +0000 : Bèr Kessels FYI: the delete bug is waiting here: http://drupal.org/node/30781 ------------------------------------------------------------------------ Fri, 09 Sep 2005 18:18:17 +0000 : m3avrck Get this warning in PHP5: Warning: Missing argument 1 for filter_admin_format() in \modules\filter.module on line 379 I have traced this error back to line 240. Why are there no callback arguments like above on line 235? Looks like a source of a problem/bug here so needs to be addressed, not sure exactly how to fix it myself (otherwise I would). Should be easy it seems. Also, for a quick and easy usability improvement, on line 239, change t('list') to t('view') ... makes the menus less confusing. After those fixes, looks like this patch is ready to be committed :) ------------------------------------------------------------------------ Fri, 09 Sep 2005 18:23:27 +0000 : m3avrck Also, this dialog is a bit confusing: "Choose which roles may use this filter format You are editing the default format. For the default format, all roles must be enabled. Therefore you cannot change them. " Maybe make it so that first line doesn't appear there on that page? ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:35:50 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy_2_0.patch (13.59 KB) changed that line of help. Can this please still be taken in consideration before the freeze? ------------------------------------------------------------------------ Tue, 13 Sep 2005 18:29:28 +0000 : m3avrck Can't apply patch, getting error: Patch malformed at line 287 ------------------------------------------------------------------------ Tue, 13 Sep 2005 19:10:07 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_interface_improvements_2_0.patch (14.19 KB) strange. the dowloaded one breaks too, yet the one i uploaded stll works. anyway, rerolled ------------------------------------------------------------------------ Tue, 13 Sep 2005 19:25:25 +0000 : Dries IMO, the form function needs more work. You need to group the form construction and the form saving. Right now, when you don't provide a title, you are given an error and an emptied form. This may be a left-over from the original code though but it would be cool if you could tidy this up. The form_group() description still need a trailing point. There is a broken link in the following help text on the 'add input format'-page: /If you notice some filters are causing conflicts in the output, you can [rearrange] them./. ------------------------------------------------------------------------ Tue, 13 Sep 2005 19:30:12 +0000 : Bèr Kessels dont have time anymore. sorry. ------------------------------------------------------------------------ Tue, 13 Sep 2005 20:50:34 +0000 : m3avrck new patch soon! ------------------------------------------------------------------------ Tue, 13 Sep 2005 22:10:25 +0000 : m3avrck Attachment: http://drupal.org/files/issues/filter.module_10.patch (20.27 KB) Ok here's a new patch. Lot's of fixes in this one (all of the ones from above except the one noted below), including support for the new revisions patch. Additionally, 'input formats' has been renamed to 'input filters'. After debate on IRC, and the above comments in this post, it seems that 'input formats' is somewhat misleading and doesn't make sense. 'input filters' clears this up and makes sense to both native and non-native English speakers (as tested in IRC anywho ;)). This also makes more sense since Drupal refers to everything as 'filters' and not 'formats'. However, there is one issue still left unresolved: If you create a new input filter, and don't specify a title, page reloads clearing all filled in fields and prompts the error message. Additionally, a blank filter is created. Wanted to get this patch posted, I'll look at it later today but if someone can get this fixed easily, please do, thanks! ------------------------------------------------------------------------ Tue, 13 Sep 2005 22:38:05 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_interface_improvements_3.patch (21.49 KB) this patch should fix the issue m3 rasies abuot amepty formats being created. Yes, i use a simple drupal_goto, in case of a for error. And no, it is not very easy to get the old posted data back.

1 0

[drupal-devel] [task] Improvments to Filter UI
by Bèr Kessels 14 Sep '05

14 Sep '05

Issue status update for http://drupal.org/node/27364 Post a follow up: http://drupal.org/project/comments/add/27364 Project: Drupal Version: cvs Component: filter.module Category: tasks Priority: normal Assigned to: Bèr Kessels Reported by: Bèr Kessels Updated by: Bèr Kessels Status: patch (code needs work) Attachment: http://drupal.org/files/issues/filter_interface_improvements_3.patch (21.49 KB) this patch should fix the issue m3 rasies abuot amepty formats being created. Yes, i use a simple drupal_goto, in case of a for error. And no, it is not very easy to get the old posted data back. Bèr Kessels Previous comments: ------------------------------------------------------------------------ Sun, 24 Jul 2005 09:30:54 +0000 : Bèr Kessels This is a mockup for the Filter UI. Currently it is soo complex, that I dare to call it broken. Please comment on this, for I will not ake any patches, when the general idea is disliked. The idea is t split filters and input formats better. Filters are filters, defined in modules. Input formats are bundles of filters. This is how we have it ATM, but the interface fails to communicate that. I tried to develop a consistent (with the rest of Drupal) interfce that makes it clearer what is what. The menu is as follows: * administer ** .... ** settings *** input formats *** filters ** ... ------------------------------------------------------------------------ Sun, 24 Jul 2005 09:37:10 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/input-formats_list1.png (16.34 KB) * administer ** .... ** settings *** input formats >> see attachement 1 *** filters ** ------------------------------------------------------------------------ Sun, 24 Jul 2005 09:39:02 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/input-formats_add.png (23.42 KB) * administer ** .... ** settings *** input formats >> see attachement 2, tab 2 *** filters ** Note: The default checkbox lmay seem a bit odd. But checking it will remove the "default status" from the current "default" one and make this one "default". The help text, and a drupal_set_message should explain this. ------------------------------------------------------------------------ Sun, 24 Jul 2005 09:40:08 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/input-formats_edit_format.png (22.17 KB) * administer ** .... ** settings *** input formats >> clicking a 'configure' link in the table of attachement 1 *** filters ** ------------------------------------------------------------------------ Sun, 24 Jul 2005 09:40:55 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/input-filters_list.png (36.24 KB) * administer ** .... ** settings *** input formats *** filters >> see attachement 4 ** ------------------------------------------------------------------------ Sun, 24 Jul 2005 09:41:40 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/input-formats_edit.png (22.63 KB) * administer ** .... ** settings *** input formats *** filters >> clicking a 'configure' link in the table of attachement 4 ** ------------------------------------------------------------------------ Mon, 25 Jul 2005 14:46:27 +0000 : Bèr Kessels IMO this makes the interface easier. But other disagree. And because it also removes one feature: (being able to use E.g. HTML in different input formats, with different settings) I'll simply wont fix this :( ------------------------------------------------------------------------ Thu, 28 Jul 2005 13:36:12 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy.patch (10.28 KB) I decided to go for it anyway. Be it in a slightly different way then my initial mockups. The difference is that I left the filters where they are, hidden beneath the formats. So, here is the patch that makes the filter UI more consistent with screens like blocs, menus, vocabularies et al also nice to note is that: 93 lines are added, 104 are removed. So netto we have less code :) (cvs diff filter.module | grep ^+ | wc -l) ------------------------------------------------------------------------ Thu, 28 Jul 2005 13:37:02 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/input-formats_list_html.png (44.64 KB) Here is how the listing now looks. ------------------------------------------------------------------------ Thu, 28 Jul 2005 13:37:57 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/input-formats_edit_format_html.png (113.71 KB) And the "configure" screen. The 'add' screen looks exactly the same, only with the default values pre-filled. ------------------------------------------------------------------------ Thu, 28 Jul 2005 13:39:19 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/input-formats_edit_format_too_long.png (121.82 KB) Oh, And here is a really nice example of why the current screen is no good. And here I "only" have four roles. I run a site with 12 roles, imagine this screen then! ------------------------------------------------------------------------ Thu, 28 Jul 2005 13:48:35 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy_HEAD.patch (10.24 KB) And here is a pathc for HEAD (I forgot that I developed this on a 4.6.2 codebase. Sorry) ------------------------------------------------------------------------ Thu, 28 Jul 2005 13:55:25 +0000 : stefan nagtegaal I like this very, very much! This is - indeed - much better than the current UI. This always fits, and makes the life of people who use a fixed width theme much nicer.. Ber, FYI (you probably know this too), 'admin/access' does also break fixed width themes due to the fact that it expands the width of the table when more roles are being added.. Good catch! I like it much more this way.. +1 from me... ------------------------------------------------------------------------ Fri, 29 Jul 2005 07:27:34 +0000 : Dries The code style needs work (spaces, tabs, 'as' -> 'AS'), the code comments need work, and debug statements should be removed. Screenshots look good to me. db_query('UPDATE {filter_formats} SET cache = %d, name = \'%s\', roles = \'%s\' WHERE format = %d', (int)$cache, $name, $roles, $format); can be written better/shorter quote-wise. ------------------------------------------------------------------------ Fri, 29 Jul 2005 09:04:16 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy_HEAD_0.patch (10.37 KB) Dries, thanks a lot for the review. OI fixed your comments. Or so I hope. code-style.pl was of no use (can that *please* be removed from core?) because it chokes on the help texts. It also pointed out a lot of old formatting isues, whic, when fixed wuold flood this patch with unrelated fixes. I tried to narow my style-searching to the areas I touched. And I made the comments more verbous. The query is fixed. it saves no characters, but looks better now. Thanks for the tip. The print_r..... blush. thats what I get for not running my default grep -r print_r on my code after finishing up... I could not find a lot of spaces and tabs. So it lmight be that i missed one. I triple checked it though. ------------------------------------------------------------------------ Fri, 29 Jul 2005 09:32:21 +0000 : Steven Moving the role settings inward does fix the wide-table problem, but it makes the filter configuration much more opaque. We could perhaps fix this by showing the roles as a comma-separate list in the input formats table. Such a list can wrap and will not stretch the table. Other than that: * Having the instructions for the Roles and Filters form_group() at the bottom is a bit weird. The distance is too big. * The table that was used for toggling filters on/off was much more compact before, and IMO a lot clearer. Why change to a flat list with the description staggered below each item? ------------------------------------------------------------------------ Fri, 29 Jul 2005 10:31:30 +0000 : Bèr Kessels consistancy, consistancy and more consistancy. There really is nothing worse that having a different 'concept' of UI for every thing in Drupal. We are going in the right direction, with the blocks being editable objects, menus acting like that, taxonomy, users, etc.. I wanted to make this act similar. I am not at all happy with the fact tah the list is still a form. But we need this, for setting the default. A big -1 for a comma-separate list. That is extremely hackish, even worse useabilitywise and just silly * we should avoid typing when not needed. * we should avoid errors on input, when they can be avoided (a typo is made very easy, I know all about it) Form set errors won't fix that usability -wise. I agree that you sort-of loose the overview. But IMO that is a minor loss comared to what we gain, usability-wise. The wtoo-wide was onlywhat triggered me. But do not think that I am noly making this patch to fix that issue. This patch is there to improve usability, through consistancy. o make drupal behave the same in most places in admin. If you know one screen, you know most of them. We really should move away from custom-hacks for every page, because that is useability rule #1: consistancy. Stadardisation, even if sometimes a custom hack might make things easier in that single case, it will reduce your overall usability so much that nearly ever it is worth that hack. That select-boxes instead of the table issue: consistancy. As well that it completely broke fluid CSS layouts, my solution is more consistant, and uses forms the way they are meant. If we cannot use $descritption in checkboxes they should be removed. but IMO it feels much more consistant. Tables are for tabular data, and this is simply a list of items, with additional data. And i do not really get what you mean with "too far down". Do you mean that you would like to see the order of the groups different? ------------------------------------------------------------------------ Fri, 29 Jul 2005 13:31:24 +0000 : Dries I haven't tested Ber's patch yet but looking at the screenshots, it like what I see. I remember being confused by the current filter UI, and often, I still am. ------------------------------------------------------------------------ Fri, 29 Jul 2005 19:53:26 +0000 : moshe weitzman I dislike the recent movement away from overview pages. I think we can still provide and overview while allowing editing to happen in a dedicated form. An example in the wrong direction is 'content types' admin. It is impossible to get an overview, and the listing page is worthless. I agree with Steven on this one. It is true that consistency is desirable. But consistent crap is not. ------------------------------------------------------------------------ Fri, 29 Jul 2005 20:02:39 +0000 : killes(a)www.drop.org I agree with Moshe. Getting a quick overview about what is set how is essential for an admin. ------------------------------------------------------------------------ Sat, 30 Jul 2005 00:27:15 +0000 : Robrecht Jacques Overall I like the screenshots. Did not test the patch. Bèr, what I think Steven (#15) meant by "showing the roles as a comma-separate list in the input formats table" is that as an overview page it would be better if you added a list (not an input box!) to screenshot "input-formats_list1.png" (comment #1). Add a column that reads "Roles" (or "Enabled for") and then for each row a comma seperated list of the roles that can use this input format (or maybe even better a HTML list). You would still need to click "configure" to _change_ the roles. The "Default" text (not a radio button) you already show in the "Options" column is something similar: it immediately shows that this input format is the default without you having to go to the "configure" page. Why not do the same for "roles"? If this is what Steven meant, then I tend to agree with him. And to help moshe and killes: maybe you could even add a column that _lists_ the used input filters. Again, only a list, not a way to change it. That's what the "operations" column is for. A table like that will "fluid" better than what we have now, still it is a _complete_ overview of the settings. I could have misunderstand someone... I have the tendency to do that... ------------------------------------------------------------------------ Mon, 22 Aug 2005 08:21:32 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_interface_improvements.patch (14.52 KB) by popular demand: a comma separated list. ------------------------------------------------------------------------ Mon, 22 Aug 2005 08:24:31 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/format_now_contains_roles.png (22.09 KB) and a screenshot ------------------------------------------------------------------------ Mon, 22 Aug 2005 18:39:44 +0000 : Dries Please check your use of print theme('page', $output). Normally, you just return $output. ------------------------------------------------------------------------ Mon, 29 Aug 2005 18:54:28 +0000 : Bèr Kessels All instances of theme('page',...) in filter.module are now changed to return ... ; furthermore, the patch is the same. ------------------------------------------------------------------------ Mon, 29 Aug 2005 20:04:02 +0000 : Uwe Hermann Bèr, I think you forgot to attach your updated patch. ------------------------------------------------------------------------ Mon, 29 Aug 2005 20:27:39 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_interface_improvements_2.patch (13.52 KB) my email client has this nice warning system for attachements. Maybe drupal should search for the word attachement too, and when no att. found give an error ;). Or maybe i should just learn to pay attention. Guess that is easier. Anyway, here it is. ------------------------------------------------------------------------ Thu, 08 Sep 2005 20:22:54 +0000 : Dries I wanted to apply this patch but: 1. I can't change the default input format. 'Save'-button doesn't work. 2. I got confused by the fact I can't change the roles of the default filter. I think the form group description should explain this. 3. form_group(t(filter)) should be form_group(t('filter'), ...). ------------------------------------------------------------------------ Thu, 08 Sep 2005 20:51:24 +0000 : m3avrck Found a few more issues: If I go into 'Configure' for 'PHP Code' and click the 'Configure' tab at the top and then click 'list filters' link in the text, I get this warning with PHP5 warning: Missing argument 1 for filter_admin_format() in \drupal\modules\filter.module on line 378. Also, it is sort of confusing as to what the 'List' tab implies. For example, if I click on 'input formats' in the admin menu, I get a list of the current filters. When I click 'configure' for one of those, I see the options for that filter. However, it says 'list' in one of the tab, which doesn't really mean much. I thought that was the list of filters, not the options for that filter. That should be cleaned up. Also, there is no way to get back to the full list of filters unless you click on 'input formats' in the admin menu. A setting/link to fix this would be great. Also, where is the link to add filters??? I think the options/layout/tabs should mimic of that of admin > users. So on admin > input formats, it has the tabs 'list', 'add format' ... very clear and consistent. On a configure page for a filter, it should have 'list', 'view', 'configure', 'rearrange' ... this would make it easier to navigate and get back to the original list of filters. Make sense? I think this and Dries' comments put this patch over the top :) ------------------------------------------------------------------------ Fri, 09 Sep 2005 01:48:55 +0000 : tag In my initial battle to figure out this module's (quite nice) functionality, what tripped me up the most was actually the nomenclature. The way I keep it straight in my head now is to translate "input format" to "filter set". If you view those (the current) screens with this in mind, I think it's a lot easier to understand what's what. It sort of describes this in the help text, but the terms "filter" and "input format" don't really convey the relationship between the two -- there's no way to intuit that an "input format" is a collection of "filters". Not to my mind at least... and well, we know nobody reads instructions... Are there any technicalities that make renaming "input format" to "filter set" not accurate? Or does someone have a better term/terms to better show the relationship of these elements? (I'm aware of filters in servlet architectures but I don't think there's much of a collision with that usage). Anyhow, I guess this doesn't quite address the UI controls and/or layout, but would still help a lot. ------------------------------------------------------------------------ Fri, 09 Sep 2005 06:35:34 +0000 : Bèr Kessels I prefer filter set. In fact: I like it a lot. Anyone else ideas on this? ------------------------------------------------------------------------ Fri, 09 Sep 2005 10:18:18 +0000 : Dries I too had problems with the terminology; it took me months to get used to 'input formats' versus 'filters'. Using 'filter sets' might improve the situation -- especially from an administrator's point of view when you have to wrap your head around the UI/difference. I'm not native English, but 'filter set' sounds more explanatory, yet slightly more geeky so I'm not sure if it flies for users who don't need to understand the underlying concepts (eg. under the node and comment submission forms). ------------------------------------------------------------------------ Fri, 09 Sep 2005 11:00:05 +0000 : Bèr Kessels a heads up: I ma redoing the patch. But decided no to change the name yet. That is a too big change. IT should be in a separate issue, IMO. ------------------------------------------------------------------------ Fri, 09 Sep 2005 15:27:43 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy_2.patch (13.57 KB) * Bugs as reported by dries fixed. * The [add] tab not appearing is due to your menu caching, refresh that please. * Fixed an additional bug: Some agents do not send TRUE for disabled checkboxen. Also in current filter admin. and people: we have a problem. Deleting a filter trows errors, due to the revisions changes. that happens in filter module now, but also after this patch. I have too little knowledge of the filter system to fix that, and IMO that is a separate issue. It should be fixed right after this patch is committed. ------------------------------------------------------------------------ Fri, 09 Sep 2005 15:57:18 +0000 : Bèr Kessels FYI: the delete bug is waiting here: http://drupal.org/node/30781 ------------------------------------------------------------------------ Fri, 09 Sep 2005 18:18:17 +0000 : m3avrck Get this warning in PHP5: Warning: Missing argument 1 for filter_admin_format() in \modules\filter.module on line 379 I have traced this error back to line 240. Why are there no callback arguments like above on line 235? Looks like a source of a problem/bug here so needs to be addressed, not sure exactly how to fix it myself (otherwise I would). Should be easy it seems. Also, for a quick and easy usability improvement, on line 239, change t('list') to t('view') ... makes the menus less confusing. After those fixes, looks like this patch is ready to be committed :) ------------------------------------------------------------------------ Fri, 09 Sep 2005 18:23:27 +0000 : m3avrck Also, this dialog is a bit confusing: "Choose which roles may use this filter format You are editing the default format. For the default format, all roles must be enabled. Therefore you cannot change them. " Maybe make it so that first line doesn't appear there on that page? ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:35:50 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_module_UI_consistancy_2_0.patch (13.59 KB) changed that line of help. Can this please still be taken in consideration before the freeze? ------------------------------------------------------------------------ Tue, 13 Sep 2005 18:29:28 +0000 : m3avrck Can't apply patch, getting error: Patch malformed at line 287 ------------------------------------------------------------------------ Tue, 13 Sep 2005 19:10:07 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/filter_interface_improvements_2_0.patch (14.19 KB) strange. the dowloaded one breaks too, yet the one i uploaded stll works. anyway, rerolled ------------------------------------------------------------------------ Tue, 13 Sep 2005 19:25:25 +0000 : Dries IMO, the form function needs more work. You need to group the form construction and the form saving. Right now, when you don't provide a title, you are given an error and an emptied form. This may be a left-over from the original code though but it would be cool if you could tidy this up. The form_group() description still need a trailing point. There is a broken link in the following help text on the 'add input format'-page: /If you notice some filters are causing conflicts in the output, you can [rearrange] them./. ------------------------------------------------------------------------ Tue, 13 Sep 2005 19:30:12 +0000 : Bèr Kessels dont have time anymore. sorry. ------------------------------------------------------------------------ Tue, 13 Sep 2005 20:50:34 +0000 : m3avrck new patch soon! ------------------------------------------------------------------------ Tue, 13 Sep 2005 22:10:25 +0000 : m3avrck Attachment: http://drupal.org/files/issues/filter.module_10.patch (20.27 KB) Ok here's a new patch. Lot's of fixes in this one (all of the ones from above except the one noted below), including support for the new revisions patch. Additionally, 'input formats' has been renamed to 'input filters'. After debate on IRC, and the above comments in this post, it seems that 'input formats' is somewhat misleading and doesn't make sense. 'input filters' clears this up and makes sense to both native and non-native English speakers (as tested in IRC anywho ;)). This also makes more sense since Drupal refers to everything as 'filters' and not 'formats'. However, there is one issue still left unresolved: If you create a new input filter, and don't specify a title, page reloads clearing all filled in fields and prompts the error message. Additionally, a blank filter is created. Wanted to get this patch posted, I'll look at it later today but if someone can get this fixed easily, please do, thanks!

1 0

← Newer
1
...
28
29
30
31
32
33
34
...
62
Older →

Jump to page: