September 2005 - Development - lists.drupal.org

[drupal-devel] [feature] Hooks for comment enhancements
by Eaton 18 Sep '05

18 Sep '05

Issue status update for http://drupal.org/node/28255 Post a follow up: http://drupal.org/project/comments/add/28255 Project: Drupal Version: cvs Component: comment.module Category: feature requests Priority: normal Assigned to: Anonymous Reported by: Eaton Updated by: Eaton Status: patch (code needs review) Attachment: http://drupal.org/files/issues/comment.module_10.patch (2.05 KB) I've been working on a couple modules that modify and enhance the commenting system (file attachments in comments, and custom comment validation for specific nodetypes). Another patch (http://drupal.org/node/14708) would add limited form and validation customizatoin, but this one adds nodeapi-style form_pre, form_post, validate, and view hooks. It's my first patch offered to the Drupal core, hope it's useful. Eaton

11 34

[drupal-devel] [task] Introduce theme wrapper, as discussed on the Sprint
by Bèr Kessels 18 Sep '05

18 Sep '05

Issue status update for http://drupal.org/node/23584 Project: Drupal Version: cvs Component: theme system Category: tasks Priority: normal Assigned to: Bèr Kessels Reported by: Bèr Kessels Updated by: Bèr Kessels Status: patch Attachment: http://drupal.org/files/issues/theme_wrapper.patch (1.23 KB) I finally got around this. On the useability sprint inAntwerp we agreed that theme_box is: 1) not flexible, 2) underused 3) very powerfull for themers, consistancy and developers if it has enough information. This patch introduces such a function, called theme_wrapper. Theme('wrapper') should be called EVERYWHERE where we now have hardcoded spans and divs, only to be able to add a class or ID to a (part of -) a page. >From the notes: " ** HTML themed wrapper ** Currently, we use a theme_box to wrap parts of pages in HTML. This should be extended, but most of, all, get better CSS support. Thus a wrapper should be introduced, one that should replace theme_box. This theme_wrapper can and/or will get an ID and a class. * Both class and ID should be passed as elements, so that it is up to the developers to pqss correct classes and Ids; * Both the ID and class are required. (?) * Discuss whether or not they are required. **List of classes.** When passing a class, the developer should be encourqged to choose an existing class, instead of inventing new ones for every wrapper. This will be achieved by a list of standard classes, that will be available in drupal.css. Think of classes such as “3-col-left”, “3-col-center” and “3-col-right”, for wrappers that will make content in a 3 columned way. This could be used, for example in a directory.module to present three directories next to one another. TODO: Create a standard set of classes, that can, and should be, (re-) used by developers. ** Ids** Ids should be unique on a page, thus the developer must make sure, he or she does not use an ID that is already existing on the page where his or her HTML will be rendered. But just like with the classees, we should avoid a wildgrowth of ids. Thus, good guidelines, including lists of examples, lust be written. TODO: Create guidelines for ID-ing elements in Drupal. " Bèr Kessels

8 40

[drupal-devel] [bug] Move node revision information for files to a separate table
by Junyor 18 Sep '05

18 Sep '05

Issue status update for http://drupal.org/node/31354 Post a follow up: http://drupal.org/project/comments/add/31354 Project: Drupal Version: cvs Component: file system Category: bug reports Priority: normal Assigned to: Anonymous Reported by: Souvent22 Updated by: Junyor -Status: patch (code needs work) +Status: patch (code needs review) Attachment: http://drupal.org/files/issues/drupal-head.file-revisions2.junyor.patch (8.55 KB) Updated patch: - cleaned up errors in database.* - cleaned up errors in update.inc I tested adding files, listing and unlisting files, deleting files, deleting revisions, deleting nodes, adding files only to certain revisions, etc. Deleting revisions does not delete the files, but as far as I can tell, that's because of http://drupal.org/node/31323. Junyor Previous comments: ------------------------------------------------------------------------ Fri, 16 Sep 2005 00:55:42 +0000 : Souvent22 I think that the files table has too much redundat data in it since we now have the new revisions system. I think another table to link revisions to files would be more appropriate. ------------------------------------------------------------------------ Fri, 16 Sep 2005 14:17:55 +0000 : Junyor I agree. NID and VID should be removed, making a new table containing FID, NID, and VID with all three as keys. ------------------------------------------------------------------------ Sun, 18 Sep 2005 09:56:50 +0000 : Junyor Attachment: http://drupal.org/files/issues/drupal-head.file-revisions.junyor.patch (8.27 KB) Here's a patch with the proposed changes. This is completely untested. I don't know SQL very well, so I'm sure I've done something wrong. I'll hopefully be able to do testing later today.

1 0

[drupal-devel] [bug] Node.module does not fire event when a revision is deleted.
by Souvent22 18 Sep '05

18 Sep '05

Issue status update for http://drupal.org/node/31323 Post a follow up: http://drupal.org/project/comments/add/31323 Project: Drupal Version: cvs Component: node.module Category: bug reports Priority: critical Assigned to: Anonymous Reported by: Souvent22 Updated by: Souvent22 Status: patch (code needs review) Attachment: http://drupal.org/files/issues/node_revisions.patch (1.39 KB) When you delete a revision, nothing is fired to tell other modules that a revision of that node has been deleted. This patch fires a _invoke_nodeapi" so that other modules will know, and can catch the event that a revision of a node has been deleted, and then take the correct action. This is needed for other modules such as 'upload.module', which keeps files for each revision. This patch also creates a watchdog entry when a revision is deleted. I'm sure the wording of the watch dog entry, if it should be watched, _invoke vs another paramenter, etc. will be argued, but I wanted to get this up, so I can hear the arguments, and get this quared away and patched it. I think this is really important. Thanks. Souvent22

5 9

[drupal-devel] [feature] Track guest paths
by chx 18 Sep '05

18 Sep '05

Issue status update for http://drupal.org/node/31449 Post a follow up: http://drupal.org/project/comments/add/31449 Project: Drupal Version: cvs Component: statistics.module Category: feature requests Priority: critical Assigned to: chx Reported by: chx Updated by: chx Status: patch (code needs review) Attachment: http://drupal.org/files/issues/anon_paths.patch (1.04 KB) For the first time I needed to do something with drupal statistics and I was shocked to see that session ID is not stored so I can't track my guests. I guess this affects most sites as the typical site has magnitudes more guests than logged in users. update_sql is not here, if you say this is a go, I'll write it. chx

6 13

[drupal-devel] [bug] Forum table: vid gone AWOL
by Cvbge 18 Sep '05

18 Sep '05

Issue status update for http://drupal.org/node/31343 Post a follow up: http://drupal.org/project/comments/add/31343 Project: Drupal Version: cvs Component: forum.module Category: bug reports Priority: critical Assigned to: Souvent22 Reported by: robertDouglass Updated by: Cvbge Status: patch (ready to be committed) Looks good for me too. Cvbge Previous comments: ------------------------------------------------------------------------ Thu, 15 Sep 2005 21:02:54 +0000 : robertDouglass The function forum_load consists entirely of this query: function forum_load($node) { $forum = db_fetch_object(db_query('SELECT * FROM {forum} WHERE vid = %d', $node->vid)); return $forum; } The forum table, however, has no vid column. What should it be? WHERE nid = %d ? I haven't been following the changes to forum.module closely, so I don't know the rest of the issues involved, but the module doesn't work as is. ------------------------------------------------------------------------ Fri, 16 Sep 2005 17:20:49 +0000 : syllance There's also en error when simply adding forum topic : user error: Unknown column 'vid' in 'field list' query: INSERT INTO forum (nid, vid, tid) VALUES (6, 6, 2) in /drupal/head/database.mysql.inc on line 99. Sounds like the removed vid field in forum table is still used in forum.module queries. The forum topic node is inserted, but not linked with forum due to this error, so that's blocking the use of forums. I've quickly searched but could not find any patch for this. I'll double check and try to make one if needed. ------------------------------------------------------------------------ Fri, 16 Sep 2005 17:55:55 +0000 : Souvent22 Attachment: http://drupal.org/files/issues/ref_error_0.patch (4.64 KB) Yes, vid should be the second column, just as the query says, INSERT INTO....(nid, vid,tid).... Seems you have the code for this, but you havne't updated your database with update.inc, i'm not sure if there is an update to the forum table in update.inc..... Well, nevermind, a quick check shows that in updates.inc, it does alter your tables for you and fix the problem, however, the current schema in database.mysql and database.pgsql does *not*. Here's a patch that fixes that. So in short, form.module has been updated to use revisions, however, you're missing a vid field in your table. ------------------------------------------------------------------------ Fri, 16 Sep 2005 17:58:04 +0000 : Souvent22 Update status; anyone wanna take a look? ------------------------------------------------------------------------ Fri, 16 Sep 2005 18:01:31 +0000 : Souvent22 Attachment: http://drupal.org/files/issues/database_forum.patch (1.27 KB) Crap, wrong patch. :). Too many things at once, sheesh. Get stable already Drupal! :). Anyway, here's the right one....I aplogize, ignore the other ones. ------------------------------------------------------------------------ Fri, 16 Sep 2005 18:06:57 +0000 : Souvent22 Just to clairfy, update_146() in updates.inc does correctly update the forum table and add the vid column. However, the databaswe creation scripts (database.mysql, database.pgsql) do NOT have the correct new schema for the forum table. ------------------------------------------------------------------------ Sat, 17 Sep 2005 01:14:28 +0000 : syllance I was surprised to find such an issue, even in head :) And i was right. I've been reading the node revisions changes, and just thought the database script were not updated yet. I simply did not thought to check the update script. That will be in my check list now, as everything works fine after running the update. Thanks for your quick answer, and thanks again to everyone making drupal such a nice thing :) ------------------------------------------------------------------------ Sat, 17 Sep 2005 10:11:26 +0000 : robertDouglass If all that is missing is the vid column from the database creation scripts, this patch should be a no brainer. Here is the pertinent SQL patch for those on the list that don't want to look for the patch: CREATE TABLE forum ( nid integer NOT NULL default '0', + vid integer NOT NULL default '0', tid integer NOT NULL default '0', shadow integer NOT NULL default '0', PRIMARY KEY (nid) ); CREATE INDEX forum_tid_idx ON forum(tid); +CREATE INDEX forum_vid_idx ON forum(vid); CREATE TABLE forum ( nid int(10) unsigned NOT NULL default '0', + vid int(10) unsigned NOT NULL default '0', tid int(10) unsigned NOT NULL default '0', PRIMARY KEY (nid), + KEY vid (vid), KEY tid (tid) ) TYPE=MyISAM; It looks good to me. ------------------------------------------------------------------------ Sat, 17 Sep 2005 11:58:00 +0000 : syllance i've quickly checked on my test sandbox, and there's a "duplicate" entry error when creating a forum topic revision. the update is to add the vid field, create a primary key on it, and make nid a non-primary key. i'll try to provide updated patches later today, but mysql changes should something like : CREATE TABLE forum ( nid int(10) unsigned NOT NULL default '0', + vid int(10) unsigned NOT NULL default '0', tid int(10) unsigned NOT NULL default '0', - PRIMARY KEY (nid), + KEY (nid), + PRIMARY KEY vid (vid), KEY tid (tid) ) TYPE=MyISAM; (manual patch :p, i'll do the thing later) while talking database, do you know if there's any specific reason why all drupal tables are forced MyISAM type ? ------------------------------------------------------------------------ Sat, 17 Sep 2005 16:27:24 +0000 : m3avrck Code looks good to me. ------------------------------------------------------------------------ Sun, 18 Sep 2005 11:27:33 +0000 : Dries There is no patch? ------------------------------------------------------------------------ Sun, 18 Sep 2005 11:36:40 +0000 : robertDouglass yes there is: http://drupal.org/files/issues/ref_error_0.patch ------------------------------------------------------------------------ Sun, 18 Sep 2005 11:37:48 +0000 : robertDouglass Oops: http://drupal.org/files/issues/database_forum.patch ------------------------------------------------------------------------ Sun, 18 Sep 2005 13:33:44 +0000 : m3avrck Yes patch is in comment #4 and as I said above, code looks correct to me.

1 0

[drupal-devel] [bug] Make node_save() recursion-aware
by jvandyk 18 Sep '05

18 Sep '05

Issue status update for http://drupal.org/node/27007 Post a follow up: http://drupal.org/project/comments/add/27007 Project: Drupal Version: cvs Component: node system Category: bug reports Priority: normal Assigned to: jvandyk Reported by: jvandyk Updated by: jvandyk Status: patch Attachment: http://drupal.org/files/issues/node_serialized2x.patch (664 bytes) node_save() saves a node, then calls insert or update hooks. These hooks may run functions that, in turn, call node_save(). This all works fine except that if a revision is being created, the revision gets serialized twice -- once on the original node_save() call and once on the callback's node_save() call. The attached two-line patch simply inserts a boolean variable called rev_serialized to keep track of whether the serialization has already been done. I'd like to see cvs and 4.6 patched. This patch is necessary so I can release actions and workflow for 4.6. Background: http://drupal.org/node/24326 jvandyk

15 26

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

18 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) I have written a function called hash_is_duplicate() that uses a serialized array stored in the variable table to validate that the token is not duplicated for the n last tokens. (n is a constant, defined as 256 in my patch). It is relatively simple, and avoids the need to introduce a new database table. However, left to do is to teach it the difference between a preview and a submit (ie, multiple previews are okay, but only one submit is okay), I'll try and post a finished patch soon. An alternative idea is to simply remember the last token used (rather than remembering n tokens). At this time, spammers simply submit the same thing over and over as fast as they can. But they're not stupid, and they'd quickly learn to switch back and forth between two different spams. > For the contact module, maybe it is better to use the recipient's e-mail > address to calculate the token. Like that, each personal contact form > would have a unique token, making it slightly more difficult to reuse tokens. That is what it currently does, on one of the forms. On the other, the information is dynamic so not a good token. The only way we can use dynamic data as tokens is to first force previews. > Another solution might be IP- or session-based tokens. Do spammers post > from a single IP, or do they come from different IPs (e.g. using hacked > computers). If you use the poster's IP to calculate the token, tokens become > more dynamic. Very good idea, and a simple change. From what I've observed, spammers use a wide range of IPs. > For sake of simplicity, I didn't commit the cron-based private key regenerator. Please reconsider. There are dozens of md5 brute force crackers freely available. They would have to be customized to easily crack our tokens, but as all data but the private key is freely available the customization is simple. Thus, a smart spammer could brute force crack a private key with relative ease and a little patience. An alternative approach would be to double the length of the private key, which would at least make this harder. Thoughts? 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. ------------------------------------------------------------------------ Wed, 14 Sep 2005 03:58:35 +0000 : Jeremy(a)kerneltrap.org 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. ------------------------------------------------------------------------ Sun, 18 Sep 2005 11:51:00 +0000 : Dries For the contact module, maybe it is better to use the recipient's e-mail address to calculate the token. Like that, each personal contact form would have a unique token, making it slightly more difficult to reuse tokens. Another solution might be IP- or session-based tokens. Do spammers post from a single IP, or do they come from different IPs (e.g. using hacked computers). If you use the poster's IP to calculate the token, tokens become more dynamic. ------------------------------------------------------------------------ Sun, 18 Sep 2005 11:53:50 +0000 : Dries I committed the current implementation of this patch, though I'd like us to think some more about how we can make reusing tokens more difficult without introducing complex logic. For sake of simplicity, I didn't commit the cron-based private key regenerator.

1 0

[drupal-devel] [bug] Forum table: vid gone AWOL
by m3avrck 18 Sep '05

18 Sep '05

Issue status update for http://drupal.org/node/31343 Post a follow up: http://drupal.org/project/comments/add/31343 Project: Drupal Version: cvs Component: forum.module Category: bug reports Priority: critical Assigned to: Souvent22 Reported by: robertDouglass Updated by: m3avrck -Status: patch (code needs work) +Status: patch (ready to be committed) Yes patch is in comment #4 and as I said above, code looks correct to me. m3avrck Previous comments: ------------------------------------------------------------------------ Thu, 15 Sep 2005 21:02:54 +0000 : robertDouglass The function forum_load consists entirely of this query: function forum_load($node) { $forum = db_fetch_object(db_query('SELECT * FROM {forum} WHERE vid = %d', $node->vid)); return $forum; } The forum table, however, has no vid column. What should it be? WHERE nid = %d ? I haven't been following the changes to forum.module closely, so I don't know the rest of the issues involved, but the module doesn't work as is. ------------------------------------------------------------------------ Fri, 16 Sep 2005 17:20:49 +0000 : syllance There's also en error when simply adding forum topic : user error: Unknown column 'vid' in 'field list' query: INSERT INTO forum (nid, vid, tid) VALUES (6, 6, 2) in /drupal/head/database.mysql.inc on line 99. Sounds like the removed vid field in forum table is still used in forum.module queries. The forum topic node is inserted, but not linked with forum due to this error, so that's blocking the use of forums. I've quickly searched but could not find any patch for this. I'll double check and try to make one if needed. ------------------------------------------------------------------------ Fri, 16 Sep 2005 17:55:55 +0000 : Souvent22 Attachment: http://drupal.org/files/issues/ref_error_0.patch (4.64 KB) Yes, vid should be the second column, just as the query says, INSERT INTO....(nid, vid,tid).... Seems you have the code for this, but you havne't updated your database with update.inc, i'm not sure if there is an update to the forum table in update.inc..... Well, nevermind, a quick check shows that in updates.inc, it does alter your tables for you and fix the problem, however, the current schema in database.mysql and database.pgsql does *not*. Here's a patch that fixes that. So in short, form.module has been updated to use revisions, however, you're missing a vid field in your table. ------------------------------------------------------------------------ Fri, 16 Sep 2005 17:58:04 +0000 : Souvent22 Update status; anyone wanna take a look? ------------------------------------------------------------------------ Fri, 16 Sep 2005 18:01:31 +0000 : Souvent22 Attachment: http://drupal.org/files/issues/database_forum.patch (1.27 KB) Crap, wrong patch. :). Too many things at once, sheesh. Get stable already Drupal! :). Anyway, here's the right one....I aplogize, ignore the other ones. ------------------------------------------------------------------------ Fri, 16 Sep 2005 18:06:57 +0000 : Souvent22 Just to clairfy, update_146() in updates.inc does correctly update the forum table and add the vid column. However, the databaswe creation scripts (database.mysql, database.pgsql) do NOT have the correct new schema for the forum table. ------------------------------------------------------------------------ Sat, 17 Sep 2005 01:14:28 +0000 : syllance I was surprised to find such an issue, even in head :) And i was right. I've been reading the node revisions changes, and just thought the database script were not updated yet. I simply did not thought to check the update script. That will be in my check list now, as everything works fine after running the update. Thanks for your quick answer, and thanks again to everyone making drupal such a nice thing :) ------------------------------------------------------------------------ Sat, 17 Sep 2005 10:11:26 +0000 : robertDouglass If all that is missing is the vid column from the database creation scripts, this patch should be a no brainer. Here is the pertinent SQL patch for those on the list that don't want to look for the patch: CREATE TABLE forum ( nid integer NOT NULL default '0', + vid integer NOT NULL default '0', tid integer NOT NULL default '0', shadow integer NOT NULL default '0', PRIMARY KEY (nid) ); CREATE INDEX forum_tid_idx ON forum(tid); +CREATE INDEX forum_vid_idx ON forum(vid); CREATE TABLE forum ( nid int(10) unsigned NOT NULL default '0', + vid int(10) unsigned NOT NULL default '0', tid int(10) unsigned NOT NULL default '0', PRIMARY KEY (nid), + KEY vid (vid), KEY tid (tid) ) TYPE=MyISAM; It looks good to me. ------------------------------------------------------------------------ Sat, 17 Sep 2005 11:58:00 +0000 : syllance i've quickly checked on my test sandbox, and there's a "duplicate" entry error when creating a forum topic revision. the update is to add the vid field, create a primary key on it, and make nid a non-primary key. i'll try to provide updated patches later today, but mysql changes should something like : CREATE TABLE forum ( nid int(10) unsigned NOT NULL default '0', + vid int(10) unsigned NOT NULL default '0', tid int(10) unsigned NOT NULL default '0', - PRIMARY KEY (nid), + KEY (nid), + PRIMARY KEY vid (vid), KEY tid (tid) ) TYPE=MyISAM; (manual patch :p, i'll do the thing later) while talking database, do you know if there's any specific reason why all drupal tables are forced MyISAM type ? ------------------------------------------------------------------------ Sat, 17 Sep 2005 16:27:24 +0000 : m3avrck Code looks good to me. ------------------------------------------------------------------------ Sun, 18 Sep 2005 11:27:33 +0000 : Dries There is no patch? ------------------------------------------------------------------------ Sun, 18 Sep 2005 11:36:40 +0000 : robertDouglass yes there is: http://drupal.org/files/issues/ref_error_0.patch ------------------------------------------------------------------------ Sun, 18 Sep 2005 11:37:48 +0000 : robertDouglass Oops: http://drupal.org/files/issues/database_forum.patch

1 0

[drupal-devel] [feature] Administration Control Panel suggestion
by der 18 Sep '05

18 Sep '05

Issue status update for http://drupal.org/node/31326 Post a follow up: http://drupal.org/project/comments/add/31326 Project: Drupal Version: cvs Component: system.module Category: feature requests Priority: normal Assigned to: Anonymous Reported by: der Updated by: der Status: patch (code needs work) Attachment: http://drupal.org/files/issues/control_panel.patch (5.61 KB) Here's a new patch with the code reworked per chx's comments with the exception of his objection to my use of drupal_goto function. chx, I'm probably missing something really obvious but I'm not understanding your suggestion. Are you suggesting I use some menu function to do the redirect? If so, I could not find one that suits my needs. Or are you suggesting to use the drupal_goto function in a different manner than I am using it. Like I said, I'm probably missing something really obvious here but for the life of me I don't know what it is. Any further advice would be greatly appreciated. The offending code below: <?php function _system_adminpage () { drupal_goto($path = variable_get('site_adminpage', 'admin/controlpanel'), $query = NULL, $fragment = NULL); } ?> der Previous comments: ------------------------------------------------------------------------ Thu, 15 Sep 2005 17:50:52 +0000 : der Attachment: http://drupal.org/files/issues/system_9.patch (4.27 KB) New feature suggestion: I would like to suggest a couple of enhancments to administration pages. First, add a new general settings option to allow the user to specify the admin home page. Currently the watchdog log view is the default home page. Second, add a new control panel page that can be used as the default admin start page. I've attached a suggested patch to the CVS head version of the system.module that implements these two enhancements. The patch does the following. Adds a new "default admin page" text field to the general settings group (just after the "default front page"). It also changes the admin menu entry to call a new function that examines the default "admin page" variable and redirects to the appropriate path. For the control panel, the patch adds a new function (system_admin_controlpanel). This function basically looks for the admin entry in the menu structure and then iterates through the child menu items and builds a control panel page using the path and title information returned from the menu structure. For the control panel icons it looks for an icon file in the misc directory that matches the name of the menu item (e.g. user.png, access.png etc) If it doesn't find an icon it uses a default. I've also included some 48/48 icons that I borrowed from my usr/share/icon/gnome directory of my local Linux box. I believe these are gnome GPL'ed icons but any 48/48 icons could be used. This patch allows users who prefer the current functionality to keep it as their default. It provides the option for others to make any admin page they want to be their admin "home page" including using a graphical control panel. Module developers that add new admin menu options can add their own custom icons for the control panel but they are not "required" to do so as a default will be used if they don't provide one. ------------------------------------------------------------------------ Thu, 15 Sep 2005 17:52:15 +0000 : der Attachment: http://drupal.org/files/issues/drupal_controlpanel_icons.zip (39.09 KB) 48x48 icons attached ------------------------------------------------------------------------ Fri, 16 Sep 2005 14:30:16 +0000 : der Attachment: http://drupal.org/files/issues/system_2_0.patch (4.5 KB) updated patch. slightly changed the formatting of the control panel page ------------------------------------------------------------------------ Sat, 17 Sep 2005 00:37:24 +0000 : moggy I quite like this. It certainly goes some way to making Drupal easier on the eyes for first time users. I noticed there's a lot of hard coded style. Would this not be better in a stylesheet and the code just containing classes and ids? Also would a dropdown box of possible admin pages be better than trying to remember how to spell controlpanel (something I'm having trouble with tonight ;-) ) ------------------------------------------------------------------------ Sat, 17 Sep 2005 13:02:08 +0000 : syllance Nice job :) the default admin page is a nice feature, and the controlpanel is a very good idea. This goes in the good direction to make Drupal more user friendly. i agree with the dropdown menu and stylesheet additions, but i already really appreciate the current version. hope this will go into core soon, as it really makes the first admin pages contact better :) i don't mind scrolling through the nav menu and i rarely goes into admin without checking the logs, but that definitely will help me converting my wife's site to drupal :p thanks ! ------------------------------------------------------------------------ Sat, 17 Sep 2005 16:09:43 +0000 : der I'll move the styles off to a style sheet but I'm uncertain about whether or not make make the default admin page a dropdown. It's an easy change and it eliminates any typos by the user but it also restricts the user to a current visible admin menu item. If by chance someone wanted to write their own admin start page (ie their own control panel) they would have to hack the core system.module to do it. Anyone else want to weigh in with their preference? Also, anyone up for creating drupalized versions of the control panel icons? ------------------------------------------------------------------------ Sat, 17 Sep 2005 16:16:15 +0000 : chx Maybe just because I created it, I like my control panel module better. ------------------------------------------------------------------------ Sat, 17 Sep 2005 16:25:30 +0000 : der If I would have known there was a module I wouldn't have written this patch although I do think this would be good for core Drupal as the default. Is your modules a recent creation? I don't see it on drupal.org nor could I find in the contribs section of CVS (either the modules or sandbox sections). I'm I missing something? Is it called something different? ------------------------------------------------------------------------ Sat, 17 Sep 2005 18:20:15 +0000 : syllance chx control panel is located in his sandbox (cp.module), and thanks to him for letting us know there's one :) it's working fine (tested on HEAD) and provide interesting concept for navigating through the admin, instead of just add a frontend. the settings part is the better one, listing all elements in the page makes quickly forget the standard menu. der's one looks nice and provide a more immediate access to admin pages, but the standard menu is still mandatory to access some elements (dba or store settings for example, still needs to be accessed via left menu). to be honest, i like them both :) being able to change the default admin page is very nice, and icons make admin looks a lot better, raising the Woman Acceptance Factor by a huge amount (my wife loves it :). but i also really like the more in depth admin navigation offered by chx module. mixing both on my head setup gives a nice result, so may be it would be a good idea to join forces and mix the 2 panels. chx, is your module somewhat official and scheduled to hit the core ? ------------------------------------------------------------------------ Sat, 17 Sep 2005 19:24:11 +0000 : Amazon Hi, I asked Karoly to create the control panel module. It important that the control panel be tied into the navigation menu block so that it does not create inconsistent navigation. The goal is to start a user experience grouping exercise to help the community categorize administration tasks. The first thing that has to happen is that administration must be considered a seperate situation from creating personal content in Drupal. To support this seperation of personal tasks and administration tasks we broke out administration to have a separate theme in the CivicSpace theme. Through research interviews into Drupal administration we need to discover what the goals and tasks of administrators are. Some early feedback is that site developers need modules to be evaluated on Drupal.org. They have also indicated they need better and consistent administration help ,with incontext list, which we have been adding. We have also heard administrators want a quick overview. Any change to provide a control panel like overview must have a dashboard like overview. You should assume that we will identify a list of 5-7 top goals for administrators. Once those those 5-7 top goals are identified we need to ensure administrators can acommplish tasks to achieve those goals. Some of those tasks will completed by clicking through to icons or admin interface links. Sub-goals will be accomplished by providing effective navigation. We need to consider 4 types of navigation to be added to the control panel: Global navigation, local navigation, contextual navigation, and situational navigation. Some of this navigation can be accomplished through a theme or blocks. Some will need to be in the page and some need to be added in otherways, such as interaction techniques. Keep all this in mind when creating a control panel solution. It also must respond to the fact that every site will have different permissions set and different modules. This is a complex problem and it's going to research and experimentation to get it right. But this is a big step in the right direction. Cheers, Kieran ------------------------------------------------------------------------ Sat, 17 Sep 2005 19:47:55 +0000 : der Attachment: http://drupal.org/files/issues/system.module_11.patch (5.61 KB) This has sparked a lot of good discussion. I've taken a look at cp.module and I think it really is complimentary with my proposed patch. I've taken my patch one step further and provided the settings icons in a collapsable group below the main control panel. This would allow access to all admin functions without referring to the traditional menu. Syllance, do you think this addresses the gap you saw in my solution? ------------------------------------------------------------------------ Sat, 17 Sep 2005 20:33:10 +0000 : der Kieran, I'm glad to see some serious thought going into the user experience for site administrators. I think separating the user and admin sections of the site is critical. It will not only allow extensive usability work to be done for site admins it will also make it much easier for theme creators as they will be able to focus on the end user experience. I like the admin theme for civicspace. My only dislike about the approach is that it's template driven which causes a sites template to be larger and more complex due to handling all the logic for user and admin themes. I think a better approach may be to use a module such as the sections module. This allows you to have smaller templates focused on user or admin themes. I agree with your point about making sure the control panel ties into them menu system. My approach uses the existing menu structure so modules are added dynamically and all permissions are enforced. ------------------------------------------------------------------------ Sat, 17 Sep 2005 22:21:05 +0000 : chx <?php function _system_adminpage () { drupal_goto($path = variable_get('site_adminpage', 'admin/controlpanel'), $query = NULL, $fragment = NULL); } ?> no way. Use menu !$may_cache and set the path based on the variable. Adding in-line style elements is also a no-go. The code style is not kept. Never a space between full stop and apostrophe, always otherwise. It's not $menuvisible but $menu_visible I must be blind (it's 0:17am) but // Build the settings section of the control panel does not seem to differ from the previous section. A function may be appropriate. ------------------------------------------------------------------------ Sat, 17 Sep 2005 22:39:40 +0000 : der Thanks for the code feedback. I'm assuming the extracted style info would get patched to drupal.css? Your right about the duplicate code. A function would make more sense, I'll make the change. I'm not sure about the comment to use "$may_cache" I've never used it before. What's it's purpose? How should I use it in this situation. I'll dig around and see if I can figure it out but I thought you might be able to give me a little insight. Thanks! ------------------------------------------------------------------------ Sun, 18 Sep 2005 10:41:00 +0000 : syllance der, i've checked the new version of your control panel, and it does simplify access to the settings menu (although with a complex setup, the site config collapsable menu goes a lot more down than the menu, but this a page and not a small menu so its still better) however, there other case settings like, such as the ecommerce modules, which provides a more complex menu tree (ex : store->settings->payment->adjustments, each with their own admin pages). these are not taken into account by your panel, leaving the menu mandatory. chx control panel handles this nicely with page navigation. going through the store menu with chx panel is a real pleasure while its a real pain with the menu. So i think the best would be to mix the two panels, chx one handling page navigation, and yours providing a frontend for basic admin pages, and collapsable site config items. instead of linking to the standard admin pages, icons might link to chx nav pages where there are sub entries. i'm running both today, and this already makes the admin really better, but i'm often switching from der panel to chx one (for store), so mixing both would be, for me, a very nice solution :p thanks for the good work :)

1 0