- Development - lists.drupal.org

[drupal-devel] [task] Extend db_query()
by killes 30 Aug '05

30 Aug '05

Issue status update for http://drupal.org/node/17656 Post a follow up: http://drupal.org/project/comments/add/17656 Project: Drupal Version: cvs Component: database system Category: tasks Priority: normal Assigned to: killes(a)www.drop.org Reported by: killes(a)www.drop.org Updated by: killes(a)www.drop.org Status: patch (code needs review) Attachment: http://drupal.org/files/issues/node_save_0.patch (3.65 KB) Here is an updated patch that takes a lot of the code I added to node_save out again (approx 20 loc). it also fixes a docs glitch in that function. Needs testing and is to be used with the patch from http://drupal.org/node/17656#comment-39222 killes(a)www.drop.org Previous comments: ------------------------------------------------------------------------ Mon, 21 Feb 2005 12:48:30 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/db-query.patch (2.16 KB) We should make our database abstraction layer more robust and ensure that module authors can use it without string manipulations inside the query. Several queries use implode() to get their arguments into the query. This is undesirable as we rely on the module author to check the keys and values of such arrays for exploitation attempts. I have created the attached patch which shouldbe able to allow us to not use implode anymore. A minor problem is that all inserted values will be treated as strings. This might be a problem with PostgreSQL at least. However, the same strategy is already used in Drupal core without any complaints I know of. Summary: This patch will alow us to simplify some code in node.module, user.module, taxonomy.module and probably others. ------------------------------------------------------------------------ Mon, 21 Feb 2005 17:03:58 +0000 : killes(a)www.drop.org It's a patch. ------------------------------------------------------------------------ Mon, 21 Feb 2005 17:19:13 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/db-query_0.patch (2.07 KB) Squeezed out two lines of code after consultation with Karoly. Adds only 10 loc (plus some docs). ------------------------------------------------------------------------ Mon, 21 Feb 2005 17:23:08 +0000 : chx Do I need to say +1? ------------------------------------------------------------------------ Thu, 03 Mar 2005 00:15:10 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/db-query_1.patch (2.69 KB) After some discussion with Adrian at Drupal Con we found out that we do not know why node_save currently works with pgsql. It currently assumes that all db columns are strings. It seems to work but we should not rely on it. Here is a patch that checks for the type of field that is inserted. It needs testing. ------------------------------------------------------------------------ Tue, 26 Jul 2005 01:17:04 +0000 : drumm +1 for making this into an API. I've seen too many hacked together query builders in Drupal and Contrib. I have not tested. ------------------------------------------------------------------------ Wed, 27 Jul 2005 11:30:29 +0000 : Bèr Kessels untested. a big +1 for the feature ------------------------------------------------------------------------ Wed, 27 Jul 2005 11:41:52 +0000 : killes(a)www.drop.org the patch still applies. the new patch here updates node_save to use it. Untested. ------------------------------------------------------------------------ Wed, 27 Jul 2005 11:42:24 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/node-%a.patch (926 bytes) the patch still applies. the new patch here updates node_save to use it. Untested. ------------------------------------------------------------------------ Wed, 27 Jul 2005 11:45:17 +0000 : Cvbge "After some discussion with Adrian at Drupal Con we found out that we do not know why node_save currently works with pgsql. " Well, it does not work. Real life exampless of not-working include flexinode (my experience) and forum module (as someone reported). Probably also others. The bug occurs when a normal user (but with necessarily rights) adds a node and he has no controls (the 'moderated', 'sticky', 'published' etc). The, in node_load() $node->sticky, $node->moderated (and maybe others) are set to FALSE (or TRUE, but in this case it works). When doing printf("%s", FALSE) the FALSE is change to empty string. The sticky and moderated db fields are numeric and postgresql do not accept '' (empty string) as a value of integer type. The result is for example such error: warning: pg_query(): Query failed: ERROR: invalid input syntax for integer: "" in ..../includes/database.pgsql.inc on line 45. user error: query: INSERT INTO node (title, uid, type, teaser, status, moderate, promote, sticky, body, comment, created, changed, nid) VALUES('xx', '2', 'flexinode-1', '<div class="flexinode-body flexinode-1"><div class="flexinode-image-3"><div class="form-item"> <label>Zdjęcie:</label><br /> <img alt="xx" src="..../pliki/" /><br />Get original file (28KB) [1] </div> </div></div>', '1', '', '1', '', '<div class="flexinode-body flexinode-1"><div class="flexinode-image-3"><div class="form-item"> <label>Zdjęcie:</label><br /> <img alt="xx" src="..../pliki/" /><br />Get original file (28KB) [2] </div> < in ..../includes/database.pgsql.inc on line 62. [1] http://drupal.org/..../pliki//tmp/male.jpg [2] http://drupal.org/..../pliki//tmp/male.jpg ------------------------------------------------------------------------ Wed, 27 Jul 2005 13:03:14 +0000 : Cvbge Attachment: http://drupal.org/files/issues/node.module_5.diff (500 bytes) Here's a quick fix for 4.6 ------------------------------------------------------------------------ Wed, 27 Jul 2005 13:10:54 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/node-a.patch (1.8 KB) the last patch wasn't good. ------------------------------------------------------------------------ Mon, 08 Aug 2005 08:19:14 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/db_api_taxonomy.patch (3.5 KB) Here is an additional patch for taxonomy.module it removes the two custom functions that are used for inserts and updates. Needs testing. @Cvbge: shoduld we have a better test than just is_numeric in _db_argument_type to fix the pgsql problems? We could also try to pass $value by reference and cast it to int. this function is only used for updates/inserts so we can afford an additional check. ------------------------------------------------------------------------ Mon, 08 Aug 2005 08:37:25 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/db_query.patch (2.74 KB) Here's a revised version fo the original patch. _db_argument_type now gets the $value argument by reference and we check for bools too. numerics and bools are cast to int. ------------------------------------------------------------------------ Mon, 08 Aug 2005 10:34:36 +0000 : Bèr Kessels I cannot say anything about performance for I do not know how to benchmark this. But I tried it, And rewrote some queries in a custom module, to use it, and like it a lot. The code becomes cleaner, and better readable. But above all, i see this idea going into a very interesting and good direction: that of 'more' query builders in core'. a +1! ------------------------------------------------------------------------ Mon, 08 Aug 2005 10:51:14 +0000 : Cvbge I'll be talking about postgres db only. The patch won't work as expected in some cases. Let's say we have a text/char/other character column and we want to insert a text that looks like a number. The _db_argument_type() will recognize the value as a numeric/number etc and will use $key = %d format. Even not considering printf() numeric conversion this can lead to incorrect value inserted into db. Examples of such problematic texts are: 01 (leading 0), 2e2 (scientific notation), probably also text with trailing/leading white spaces. When inserting such data into db without quotes the postgres db will think this is a number and will change 010 to 10, 2e2 to 200 . Here is an example: piotr=> CREATE TABLE t(c CHAR(10)); piotr=> INSERT INTO t VALUES (01); piotr=> INSERT INTO t VALUES (2e2); piotr=> INSERT INTO t VALUES (-01); piotr=> INSERT INTO t VALUES (000); piotr=> SELECT * from t; c ------------ 1 200 -1 0 (4 rows) I think it's safe to use '%s' everywhere. I could not find it writtent directly in postgres documentation (although I remember reading it somewhere some time ago). Postgres will convert the string to correct integer/numeric/bool value. ------------------------------------------------------------------------ Sat, 13 Aug 2005 01:14:41 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/db_query_0.patch (2.69 KB) Ok, the patch now does only distinguish between integerss and the rest. This is needed to decide whether we need quotes. ------------------------------------------------------------------------ Sat, 13 Aug 2005 09:40:59 +0000 : Cvbge I'm sorry, but I see no reason for this integer check. Can you explain why there is a need to differentiate between integers and other types? The only problem I see is with converting bool(FALSE). This value is converted to empty string ('') and when trying to insert it into numeric, float, bool, date (probably any column that is not text type) etc you get error. One solution would be to check for bool(FALSE) and convert it to 0. This would work for numeric-like and bool fields. But it would not work for DATA columns (would produce error). Also, entering '0' into text column might not be what the author wanted. But this is more a problem with the coder - if you have integer/date etc field, insert integer/date/etc data type, not bool! Side note: the integer-version, the one without '', will be used only for integers (i.e. ..., -2, -1, 0, 1, 2, ... see http://php.net/manual/en/language.types.integer.php) ------------------------------------------------------------------------ Sat, 13 Aug 2005 09:43:19 +0000 : Cvbge "DATA" should be "DATE" ------------------------------------------------------------------------ Sun, 14 Aug 2005 00:53:34 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/db_query_1.patch (2.71 KB) Ok, another attempt. There is indeed no real reason to use %d for integers, they work fine as strings and are converted by sprintf to strings independend of %d or %s. But it is the Right Thing(tm), so I kept it. Booleans, however, need the %d formatter in order to be converted to 0 and 1. TRUE to 1 always works, but FALSE to 0 only works with %d. All tests on php 4.3.x. ------------------------------------------------------------------------ Thu, 18 Aug 2005 21:46:04 +0000 : Cvbge The part I was objecting previously looks ok now. I haven't tested the code though. I still think it'd be nice to change php NULL to real SQL NULLs, but I don't have how to do it (at least not with current approach). The best would be if there was (s)printf flag that would just ignore it's argument... ------------------------------------------------------------------------ Sat, 27 Aug 2005 19:36:04 +0000 : Thomas Ilsche Attachment: http://drupal.org/files/issues/node_save.patch (907 bytes) +1, tested Runs nicely but maybe NULL and floats might need special treatment. I have attached an INCOMPLETE patch for node_save using the new feature that also fixes the following issue.

1 0

[drupal-devel] Node revisions hit core!
by Dries Buytaert 30 Aug '05

30 Aug '05

Hello world, for your information, thanks to Gerhard's hard work, the node revision patch hit core! Yay. More information at http://drupal.org/node/7582 and http://drupal.org/ cvs?commit=18899. Upgrade instructions will be made available shortly. For those who maintain a node module, three recent changes are likely to affect your module: 1. The introduction of _node_info(), node_get_name(), node_get_base () and the removal of some node system functions. (chx: will you provide documentation and upgrade notes?) 2. Changes to the taxonomy_save_*() functions. (chx: will you provide documentation and upgrade notes?) 3. The node revision changes I just committed (Gerhard: will you provide documentation and upgrade notes?) We don't anticipate more changes to the node system, except for those introduced by the form API. -- Dries Buytaert :: http://www.buytaert.net/

5 5

[drupal-devel] [feature] Replace core archive.module w/ codemonkeyx archive.module
by Morbus Iff 30 Aug '05

30 Aug '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: Morbus Iff Status: patch (code needs review) dtan: I'm pretty sure I know what this is - I'll address it either later today or tomorrow. Morbus Iff 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)

1 0

[drupal-devel] [feature] Display comments on own tab like wikipedia (patch attached)
by Bèr Kessels 30 Aug '05

30 Aug '05

Issue status update for http://drupal.org/node/24804 Post a follow up: http://drupal.org/project/comments/add/24804 Project: Drupal Version: cvs Component: comment.module Category: feature requests Priority: normal Assigned to: chx Reported by: moshe weitzman Updated by: Bèr Kessels Status: patch (code needs review) sorry, with "Another idea: put this in..." I meant to say "Another idea: put the settings for tabs vs threads ..." Bèr Kessels Previous comments: ------------------------------------------------------------------------ Sat, 11 Jun 2005 20:57:10 +0000 : moshe weitzman Attached please find a demonstration patch which moves comment display to its own tab on the node view page. Admin may choose this new view or old view. The patch needs work still: - comment control form redirects to the node page after submit. Should be comment page - same for comment posting form - probably more I hope someone gets inspired to finish this patch. I won't revisit this for a while. ------------------------------------------------------------------------ Sat, 11 Jun 2005 21:00:56 +0000 : moshe weitzman Attachment: http://drupal.org/files/issues/discuss.patch (3.87 KB) with attachment this time ------------------------------------------------------------------------ Sat, 11 Jun 2005 23:24:12 +0000 : moshe weitzman oh yeah - also on the TODO list is to comment URLs to always point to comment.module and never to node/x#comment-cid or node/x#comment-new and so on. then comment.module can work out whether to show you the discuss tab or the traditional view. ------------------------------------------------------------------------ Sun, 12 Jun 2005 00:56:02 +0000 : Boris Mann I like the concept. Would want this to be part of workflow settings, so that comment-tab could be enabled/disabled per content type. ------------------------------------------------------------------------ Sun, 12 Jun 2005 02:20:42 +0000 : Jaza I like the idea - big +1. This is a must for sites that have long articles and/or many comments - viewing the node is enough bandwidth as it is, without all the comments getting displayed as well. Also improves usability, IMO. Would also like to see pagination of the comments tab (I'm sure you've thought of this already, Moshe). Something similar to A List Apart [1]'s system, e.g. node/x/discuss/2 (page 2 of that node's comments). Or, if the existing pager system demands it, node/x/discuss?page=2. [1] http://www.alistapart.com/ ------------------------------------------------------------------------ Tue, 30 Aug 2005 00:41:42 +0000 : chx Attachment: http://drupal.org/files/issues/discuss_0.patch (3.87 KB) Let's discuss! Like do we want to have a reply form on the node display even if the comments are on the discuss page? ------------------------------------------------------------------------ Tue, 30 Aug 2005 00:50:13 +0000 : chx Attachment: http://drupal.org/files/issues/discuss_1.patch (3.58 KB) I have uploaded moshe's patch. Let's try again. ------------------------------------------------------------------------ Tue, 30 Aug 2005 11:22:18 +0000 : Bèr Kessels I like this approach a lot. However, as i use drupal also for blogging, would dislike my bog to behave the same. Thus there must be some -easy- way to get around this. µ The problem lies mainly in the fact that tabs are not administratable in Drupals menu admin. And I think it is a very bad idea to add a config option where to place the comments. We should go for one way, teh default way, and offer a menu-admin of some sort to change this, for bloggers and slashdot alikes. How to achieve this? I see two options, but I really hope people see more then one.: - make local-tasks appear in the menu admin (gets a huge +1 from me) It's a separate issue, though. - add a config option to the comment admin to allow comments either as tab or as thread under the post. IMO that config option should not set a variable, that gets checked every hook_menu call, but it should alter the menu-entry in the database for the tabs Its possible, I did that before. But, as said, I hope others have better ideas. for I really would love this feature to get in. But I am afraid a huge lot of people will move away from drupal if we allow tabs only. ------------------------------------------------------------------------ Tue, 30 Aug 2005 14:05:59 +0000 : chx No, not tabs only. I know the drupal_gotos are giving you this impression, but will fix in the evening. Also, there is only a variable_get for node/nid pages and then again, variable_get is double cached, it's very fast. ------------------------------------------------------------------------ Tue, 30 Aug 2005 16:19:22 +0000 : Bèr Kessels I know variable is fast, but I was referring to: && variable_get('comment_task', FALSE) that is evaluated every node view. As well as the setting in the comment options. I am no fan of such options; However, it seems in this case that setting is sortof required, unfortunately. So unless others come up with better ideas, just ignore my previous comment/post. Another idea: put this in hook_nodeapi() $op == "settings". that way this setting can be used per type. I can imagine that this makes no sense for forums and blog entries, yet is very usefull for the books on that same site.

1 0

[drupal-devel] [feature] Display comments on own tab like wikipedia (patch attached)
by Bèr Kessels 30 Aug '05

30 Aug '05

Issue status update for http://drupal.org/node/24804 Post a follow up: http://drupal.org/project/comments/add/24804 Project: Drupal Version: cvs Component: comment.module Category: feature requests Priority: normal Assigned to: chx Reported by: moshe weitzman Updated by: Bèr Kessels Status: patch (code needs review) I know variable is fast, but I was referring to: && variable_get('comment_task', FALSE) that is evaluated every node view. As well as the setting in the comment options. I am no fan of such options; However, it seems in this case that setting is sortof required, unfortunately. So unless others come up with better ideas, just ignore my previous comment/post. Another idea: put this in hook_nodeapi() $op == "settings". that way this setting can be used per type. I can imagine that this makes no sense for forums and blog entries, yet is very usefull for the books on that same site. Bèr Kessels Previous comments: ------------------------------------------------------------------------ Sat, 11 Jun 2005 20:57:10 +0000 : moshe weitzman Attached please find a demonstration patch which moves comment display to its own tab on the node view page. Admin may choose this new view or old view. The patch needs work still: - comment control form redirects to the node page after submit. Should be comment page - same for comment posting form - probably more I hope someone gets inspired to finish this patch. I won't revisit this for a while. ------------------------------------------------------------------------ Sat, 11 Jun 2005 21:00:56 +0000 : moshe weitzman Attachment: http://drupal.org/files/issues/discuss.patch (3.87 KB) with attachment this time ------------------------------------------------------------------------ Sat, 11 Jun 2005 23:24:12 +0000 : moshe weitzman oh yeah - also on the TODO list is to comment URLs to always point to comment.module and never to node/x#comment-cid or node/x#comment-new and so on. then comment.module can work out whether to show you the discuss tab or the traditional view. ------------------------------------------------------------------------ Sun, 12 Jun 2005 00:56:02 +0000 : Boris Mann I like the concept. Would want this to be part of workflow settings, so that comment-tab could be enabled/disabled per content type. ------------------------------------------------------------------------ Sun, 12 Jun 2005 02:20:42 +0000 : Jaza I like the idea - big +1. This is a must for sites that have long articles and/or many comments - viewing the node is enough bandwidth as it is, without all the comments getting displayed as well. Also improves usability, IMO. Would also like to see pagination of the comments tab (I'm sure you've thought of this already, Moshe). Something similar to A List Apart [1]'s system, e.g. node/x/discuss/2 (page 2 of that node's comments). Or, if the existing pager system demands it, node/x/discuss?page=2. [1] http://www.alistapart.com/ ------------------------------------------------------------------------ Tue, 30 Aug 2005 00:41:42 +0000 : chx Attachment: http://drupal.org/files/issues/discuss_0.patch (3.87 KB) Let's discuss! Like do we want to have a reply form on the node display even if the comments are on the discuss page? ------------------------------------------------------------------------ Tue, 30 Aug 2005 00:50:13 +0000 : chx Attachment: http://drupal.org/files/issues/discuss_1.patch (3.58 KB) I have uploaded moshe's patch. Let's try again. ------------------------------------------------------------------------ Tue, 30 Aug 2005 11:22:18 +0000 : Bèr Kessels I like this approach a lot. However, as i use drupal also for blogging, would dislike my bog to behave the same. Thus there must be some -easy- way to get around this. µ The problem lies mainly in the fact that tabs are not administratable in Drupals menu admin. And I think it is a very bad idea to add a config option where to place the comments. We should go for one way, teh default way, and offer a menu-admin of some sort to change this, for bloggers and slashdot alikes. How to achieve this? I see two options, but I really hope people see more then one.: - make local-tasks appear in the menu admin (gets a huge +1 from me) It's a separate issue, though. - add a config option to the comment admin to allow comments either as tab or as thread under the post. IMO that config option should not set a variable, that gets checked every hook_menu call, but it should alter the menu-entry in the database for the tabs Its possible, I did that before. But, as said, I hope others have better ideas. for I really would love this feature to get in. But I am afraid a huge lot of people will move away from drupal if we allow tabs only. ------------------------------------------------------------------------ Tue, 30 Aug 2005 14:05:59 +0000 : chx No, not tabs only. I know the drupal_gotos are giving you this impression, but will fix in the evening. Also, there is only a variable_get for node/nid pages and then again, variable_get is double cached, it's very fast.

1 0

[drupal-devel] [bug] Problems with PostgreSQL and GROUP BY statements
by Dries 30 Aug '05

30 Aug '05

Issue status update for http://drupal.org/node/20255 Post a follow up: http://drupal.org/project/comments/add/20255 Project: Drupal Version: cvs Component: throttle.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: Anonymous Updated by: Dries Status: patch (ready to be committed) Committed to HEAD. Thanks. Dries Previous comments: ------------------------------------------------------------------------ Sun, 10 Apr 2005 09:05:22 +0000 : Anonymous I set up Drupal from CVS yesterday, using PostgreSQL 8.0 as database. viewing pages as anonymous user often generates the following warning: warning: pg_query(): Query failed: ERROR: column "sessions.timestamp" must appear in the GROUP BY clause or be used in an aggregate function in /www/htdocs/includes/database.pgsql.inc on line 45.always followed by: user error: query: SELECT COUNT(DISTINCT(uid)) AS count FROM sessions WHERE timestamp >= 1113122734 AND uid != 0 GROUP BY uid ORDER BY timestamp DESC in /www/htdocs/includes/database.pgsql.inc on line 62. ------------------------------------------------------------------------ Sun, 10 Apr 2005 18:18:17 +0000 : Anonymous okay, after some time playing around in drupal, I could near the problem down a bit: the message appeared about every 10th time... and my auto-throttle-probability was set to 10%. Setting the probability to 50%, and the mentioned warnings came with that percentage. so the error seems to be in the throttle module. anyway, looking through the support issues, similar warnings ("xyz must appear in the GROUP BY") happen in several places when pgsql is used. Maybe the problem could be solved somewhere general instead of in each module, or there should be at least a note regarding this on the module developers handbook ( http://drupal.org/node/1395 ), as I assume this is some MySQL-specific notation. ------------------------------------------------------------------------ Thu, 14 Apr 2005 13:37:34 +0000 : r11r At my work, we ran into the same problem when doing a migration from Oracle to Postgres. The problem, I think, is that Postgres orders its results after the select statement not before so that it's impossible for it to order the results on a column you didn't select. I'm told we mostly ended up rewriting the queries to avoid this problem but given the information below, there might be a way around it without changing the queries too much. Here are a couple of links potentially relating to this same error: http://archives.postgresql.org/pgsql-general/2004-02/msg01199.php [1] http://www.postgresql.org/docs/7.4/static/queries-select-lists.html#QUERIES… [2] [1] http://archives.postgresql.org/pgsql-general/2004-02/msg01199.php [2] http://www.postgresql.org/docs/7.4/static/queries-select-lists.html#QUERIES… ------------------------------------------------------------------------ Fri, 15 Apr 2005 13:16:14 +0000 : adrian We always change the queries. From the link you posted : The DISTINCT ON clause is not part of the SQL standard and is sometimes considered bad style because of the potentially indeterminate nature of its results. With judicious use of GROUP BY and subqueries in FROM the construct can be avoided, but it is often the most convenient alternative. Adding the fields in the GROUP BY is the only way to have the sql work for both mysql and postgres. I'm gonna make a patch for this now. ------------------------------------------------------------------------ Mon, 22 Aug 2005 14:13:12 +0000 : Uwe Hermann Adrian, any updates on this? ------------------------------------------------------------------------ Mon, 29 Aug 2005 14:56:29 +0000 : Cvbge Attachment: http://drupal.org/files/issues/drupal-head-throttle-20255.diff (856 bytes) Attached patch should fix the error. I've removed the ORDER BY (sorting does not have any impact on COUNT) and GROUP BY (we already have DISTINCT). Please test. ------------------------------------------------------------------------ Mon, 29 Aug 2005 21:08:08 +0000 : Cvbge I believe there's a problem with throttling by registered users. The code from throttle.module: ------------------------------------------------------------------------ Mon, 29 Aug 2005 21:12:49 +0000 : Cvbge Currently throttling by registered users does not work. I've explained it nicely and project.module or drupal have eaten my explanation without any warning. The attached post fixes postgres' GROUP BY problem and also fixes throttling problem. ------------------------------------------------------------------------ Mon, 29 Aug 2005 21:22:06 +0000 : Cvbge There. I've even tested the patch and it works. +1 ;) ------------------------------------------------------------------------ Mon, 29 Aug 2005 22:52:02 +0000 : Jeremy(a)kerneltrap.org Agreed, the GROUP BY and ORDER BY are both unnecessary. Please commit.

1 0

[drupal-devel] [wanted] file system improvements
by Dries Buytaert 30 Aug '05

30 Aug '05

Anyone willing to improve file.inc, and the way we deal with private and public downloads (and node-level permissions)? -- Dries Buytaert :: http://www.buytaert.net/

4 3

[drupal-devel] [bug] Users with 'maintain books' or 'edit own book pages' permission cannot delete book pages
by noid 30 Aug '05

30 Aug '05

Issue status update for http://drupal.org/node/21559 Post a follow up: http://drupal.org/project/comments/add/21559 Project: Drupal Version: cvs Component: book.module Category: bug reports Priority: normal Assigned to: Anonymous Reported by: clydefrog Updated by: noid Status: patch (code needs review) My bad. *"Maintain books" works after all.* Tested the permission again and found out that a user with the aforementioned access can edit some of the nodes after all, and these nodes were created after a certain date. So I looked into the modules I installed after that date, and, to cut to the chase, found out that the culprit was TinyMCE. Well, indirectly, that is. :) Turned out I was using a new customized input format -- per advice of TinyMCE install instructions -- and the older nodes that were created with the default filtered HTML format couldn't be edited. So all I have to do now is change the input format of these older nodes to the new input format I set for TinyMCE. :) At any rate, perhaps someone can figure out why the new format made the book module act that way. :) noid Previous comments: ------------------------------------------------------------------------ Fri, 29 Apr 2005 04:32:19 +0000 : clydefrog Attachment: http://drupal.org/files/issues/delete_own_book_pages.patch (626 bytes) AFAICT, only 'administer nodes' permission allows deletion of book pages. This patch allows users with 'maintain books' or 'edit own book pages' permission to delete book pages. The logic doesn't check if the node has updates pending. Should it? ------------------------------------------------------------------------ Fri, 29 Apr 2005 14:10:22 +0000 : clydefrog It's a patch, duh. ------------------------------------------------------------------------ Fri, 29 Apr 2005 14:21:27 +0000 : rivena If maintain books is a permission that overlaps other permissions, it should say so. For example, /maintain books (create, edit, and delete all book pages)/. While I agree that someone who has been given maintainer status should be able to delete pages, I wonder if 'delete own pages' should be made into a seperate permission. I could see a situation where I would not want someone to be able to delete their own pages. Do these permissions affect things that are not book pages but have been put into books? Anisa. It's /Golden Week/! ------------------------------------------------------------------------ Sat, 30 Apr 2005 03:43:17 +0000 : clydefrog I think it's clear that "maintain books" implies the ability to edit and delete book pages. No other node module provides a "delete own " permission, so it would be inconsistent for book.module to provide it. Can you provide some examples to justify the inconsistency? These permissions do not affect nodes which aren't book pages but are in a book. Those nodes are controlled by their respective type-specific permissions, but only users with "maintain books" permission can add or remove those nodes from books. I believe this is the correct behavior. ------------------------------------------------------------------------ Sat, 30 Apr 2005 11:03:47 +0000 : rivena Hmm... But, you just said that until this patch, it *didn't* include the delete pages permission. Right? So, now you are changing it's behavior, but leaving the wording the same. How will people know? Or does it not matter, because you presume they didn't really know it didn't include delete in the first place? I don't know that I can defend my idea of having a seperate delete own pages permission against a standard of consistency. If all the other edit own permissions include the delete pages permission, then I have no quibbles. :) Except it would be more userfriendly if I knew what the permissions meant before I granted them to someone, without having to look at the code. But I do realize that is completely out of the scope of the patch. :) Where do all these little changes *go*, anyway? The changelog just says things like... refractored the search module. Anisa. ------------------------------------------------------------------------ Sat, 30 Apr 2005 16:04:14 +0000 : clydefrog I believe that "maintain books" permission implies the ability to delete book pages, so this patch fixes that bug. Other node modules include deletion in the "edit own" permission. For example, page.module: <?php if ($op == 'update' || $op == 'delete') { if (user_access('edit own pages') && ($user->uid == $node->uid)) { return TRUE; } } ?> I agree that it would be better if permissions were clearer about what they implied. There was a discussion started by killes [1] a while ago: "One problem with Drupal is the way it handles user permissions. The permissions are attached to the user object but nobody really knows what they do. To find out what a particular permission allows you to do, you often need to have a look at the code. " I don't know if it ever lead to any code. I think the changelog is meant to list the changes since the previous version without too much technical detail. If you want to see every last change, take a look at the cvs messages [2]. [1] http://lists.drupal.org/archives/drupal-devel/2005-04/msg00704.html [2] http://drupal.org/project/cvs/3060 ------------------------------------------------------------------------ Mon, 23 May 2005 17:33:18 +0000 : rivena I've been looking at the book module permissions recently, and did a bit of testing. I wanted to know what was the minimum permissions needed to create a new book. I assumed based on this discussion, and a similar discussion on the drupal support list that this was 'maintain books' and 'administer nodes'. I ran into a problem, then added 'access nodes' to this. So now my user has no other permissions other than 'maintain books', 'administer nodes' and 'access nodes'. You'd think this is more than sufficient to do something simple like create a new book, but in fact, with just these permissions, you do not have access to create a book page. You can edit other pages, and move them into a new book. Once you have create pages permission, you appear to finally have all the permissions needed, edit own pages is not needed. So, since, the point of this patch was that not having delete permission was odd, might I suggest adding create permission to maintain books? ;p Anisa. ------------------------------------------------------------------------ Mon, 08 Aug 2005 10:37:49 +0000 : Bèr Kessels I would say this is a "by design". Maybe we need to rename the two book permissions into: "create books" and "edit own book pages". None of the other nodes can be deleted by the roles with "edit own Foo" permissions, nor should the books. ------------------------------------------------------------------------ Mon, 08 Aug 2005 16:59:30 +0000 : clydefrog Thanks for your review, Ber. story.module [3] allows users with "edit own stories" permission to delete stories. <?php if ($op == 'update' || $op == 'delete') { if (user_access('edit own stories') && ($user->uid == $node->uid)) { return TRUE; } } ?> Therefore it is not inconsistent for the same behavior to apply to book nodes. If "maintain books" really means "create books", then the name should be changed and existing code in book.module needs to be updated. However, I think "maintain books" should allow more than just that. In my situation, I want to be able to give someone the permissions to create books and to create, edit, and delete all book pages. With my patch, "maintain books" and "create book pages" does that. [3] http://cvs.drupal.org/viewcvs/drupal/drupal/modules/story.module?rev=1.167&… ------------------------------------------------------------------------ Mon, 08 Aug 2005 19:16:35 +0000 : Bèr Kessels What i see from story, that is specific to story.module too. We should really settle for a consistent behaviour for *all* nodes, one that is "enforced" from node.module. These things should really not be decided per node-type and per-module. Thats a recipe for a mess. ------------------------------------------------------------------------ Mon, 08 Aug 2005 19:25:27 +0000 : Eric Scouten I agree that the current system is a mess. I disagree that there should be one consistent policy enforced by node.module. Sites may have quite legitimate reasons to have different policies for different node types. ------------------------------------------------------------------------ Mon, 08 Aug 2005 20:35:59 +0000 : Bèr Kessels What i was aiming at is to let node module take care of the following: * edit $node_type * administer $node_type * delete $node_type (but would that not better be in 'administer';: users who can administer can delete) * create $node_type That way the modules need not cre about this. ut they /can/ take care of it, if they wish, and add more rules and permissions. ------------------------------------------------------------------------ Mon, 08 Aug 2005 20:36:32 +0000 : Bèr Kessels oh, and -forgot- : * view $node_type ------------------------------------------------------------------------ Mon, 08 Aug 2005 20:49:31 +0000 : Eric Scouten Thanks for the clarification, Bèr. I was afraid that you meant that there would be a single policy applied to *all* node types, not a policy *per* node type. Now I think we're in complete agreement on this subject. ------------------------------------------------------------------------ Mon, 08 Aug 2005 20:51:13 +0000 : puregin I think it's important to keep a clean separation between content and structure (e.g., what's inside of the node, versus how the node is associated with other nodes) I think that there will likely be other explicit structures in Drupal over time, so these concerns affect more than just book module. Deleting a node from a book is potentially a problem, since this operation affects the structure of a book. We also need to think about other interactions with structure - for example, what happens if someone unpublished a node in a book? Should all children also be unpublished? Should the node contents be hidden, while children remain available in navigation? ------------------------------------------------------------------------ Mon, 08 Aug 2005 21:08:01 +0000 : Bèr Kessels puregrin. I think you are gettnig OT here, Thought it is a very valid (and problematic) issue, It really belongs in a new thread. ------------------------------------------------------------------------ Mon, 08 Aug 2005 22:57:34 +0000 : puregin What I was saying is that whether a user should be able to delete a node from a structure should really be enforced by the module which is responsible for the structure. For example, imagining a simple linear list structure - let's say for a "wish list" - composed of nodes owned by a bunch of different users. In this example, a user could delete his own node with no problem. For the book module, a users node may have children authored by other users. Letting an arbitrary user delete his own node would be, I think, a mistake, since the children would then be orphaned. I suppose one could imagine other modules which rely on a hierarchy, which might have a different policy (e.g., automatically attach children to the parent of the deleted node, if such a node exists). Perhaps eventually two 'tree-based' structure modules might inherit from a tree structure module, each enforcing separate policies regarding node operations. The point is, that one should not have to modify node module when a new structure is defined. Node module should not have to know about how composites of nodes work. ------------------------------------------------------------------------ Tue, 09 Aug 2005 00:45:07 +0000 : Boris Mann puregin: I was talking about this today, so I created a task [4] to remind us to build a generalized node relationship module/schema. [4] http://drupal.org/node/28480 ------------------------------------------------------------------------ Fri, 12 Aug 2005 00:42:58 +0000 : Crell /I think it's clear that "maintain books" implies the ability to edit and delete book pages./ I'm afraid I have to disagree with clydefrog on this one. The words "administer" and "maintain" and "manage" don't give any firm indication of what they mean, in and of themselves. They could vary depending on the context. Some nodes are deleteable in the fist place, some aren't. In that case, "maintain" means two different things, as would anything else you put in there unqualified. This isn't just a problem with the books module, but in general. It is not intuitively obvious what "administer nodes" or "administer X" means. That's a problem, especially when, as I said, it can vary from one node type to another. I see, generally, the following types of operations on nodes: - Create - Change Ownership - View Anyone's - View Own - Edit Anyone's - Edit Own - Delete Anyone's - Delete Own Which of those is covered by "administer" or "maintain"? And what if I want to allow someone to, say, edit their own nodes, but not delete them? Or allow a given class of administrator to edit anyone's nodes but not delete any but their own? I'd actually be much in favor of more fine-grained control over nodes access rights, both for the greater power it offers and for the clearer UI, system-wide. Whether that's best done via a central node API system or via a "recommended permission breakdown" I don't know, and will leave that to those who have spent more time with core than I have. :-) I do believe it's a good idea, however, both for greater flexibility and greater learnability. ------------------------------------------------------------------------ Fri, 26 Aug 2005 21:23:27 +0000 : noid Just to clarify things -- based on tests I did with the book module -- (the following hold true for regular users with no "administer nodes" permission) *1) the "edit own book pages" does not give the user the option to delete his own pages.* and *2) "maintain books" doesn't do anything. * This is the state of things right now, right? If this is so, may I suggest that the initial fix be simple in the meantime which should include: *1) "edit own book pages" means the user /can also delete his own pages./* *2) "maintain books" means the user /can edit (but not delete) pages created by other users./* ******************** I've also noticed that "create book pages" (or "edit own book pages" and "maintain books" for that matter) doesn't allow the user to create a top-level page -- there's no option for it in the pulldown. Can this be fixed so that * "create book pages" allow the user to create a top-level page (that is start his own book)?*

1 0

[drupal-devel] Processed: drupal security bugs
by owner＠bugs.debian.org 30 Aug '05

30 Aug '05

Processing commands for control(a)bugs.debian.org: > close 323347 4.5.5-1 Bug#323347: Another XMLRPC issue in drupal 'close' is deprecated; see http://www.debian.org/Bugs/Developer#closing. Bug marked as fixed in version 4.5.5-1, send any further explanations to Moritz Muehlenhoff <jmm(a)inutil.org> > close 323578 4.5.5-1 Bug#323578: drupal: DRUPAL-SA-2005-004 'close' is deprecated; see http://www.debian.org/Bugs/Developer#closing. Bug marked as fixed in version 4.5.5-1, send any further explanations to serrador(a)hispafuentes.com > -- Stopping processing here. Please contact me if you need assistance. Debian bug tracking system administrator (administrator, Debian Bugs database)

1 0

[drupal-devel] [feature] Display comments on own tab like wikipedia (patch attached)
by chx 30 Aug '05

30 Aug '05

Issue status update for http://drupal.org/node/24804 Post a follow up: http://drupal.org/project/comments/add/24804 Project: Drupal Version: cvs Component: comment.module Category: feature requests Priority: normal Assigned to: chx Reported by: moshe weitzman Updated by: chx Status: patch (code needs review) No, not tabs only. I know the drupal_gotos are giving you this impression, but will fix in the evening. Also, there is only a variable_get for node/nid pages and then again, variable_get is double cached, it's very fast. chx Previous comments: ------------------------------------------------------------------------ Sat, 11 Jun 2005 20:57:10 +0000 : moshe weitzman Attached please find a demonstration patch which moves comment display to its own tab on the node view page. Admin may choose this new view or old view. The patch needs work still: - comment control form redirects to the node page after submit. Should be comment page - same for comment posting form - probably more I hope someone gets inspired to finish this patch. I won't revisit this for a while. ------------------------------------------------------------------------ Sat, 11 Jun 2005 21:00:56 +0000 : moshe weitzman Attachment: http://drupal.org/files/issues/discuss.patch (3.87 KB) with attachment this time ------------------------------------------------------------------------ Sat, 11 Jun 2005 23:24:12 +0000 : moshe weitzman oh yeah - also on the TODO list is to comment URLs to always point to comment.module and never to node/x#comment-cid or node/x#comment-new and so on. then comment.module can work out whether to show you the discuss tab or the traditional view. ------------------------------------------------------------------------ Sun, 12 Jun 2005 00:56:02 +0000 : Boris Mann I like the concept. Would want this to be part of workflow settings, so that comment-tab could be enabled/disabled per content type. ------------------------------------------------------------------------ Sun, 12 Jun 2005 02:20:42 +0000 : Jaza I like the idea - big +1. This is a must for sites that have long articles and/or many comments - viewing the node is enough bandwidth as it is, without all the comments getting displayed as well. Also improves usability, IMO. Would also like to see pagination of the comments tab (I'm sure you've thought of this already, Moshe). Something similar to A List Apart [1]'s system, e.g. node/x/discuss/2 (page 2 of that node's comments). Or, if the existing pager system demands it, node/x/discuss?page=2. [1] http://www.alistapart.com/ ------------------------------------------------------------------------ Tue, 30 Aug 2005 00:41:42 +0000 : chx Attachment: http://drupal.org/files/issues/discuss_0.patch (3.87 KB) Let's discuss! Like do we want to have a reply form on the node display even if the comments are on the discuss page? ------------------------------------------------------------------------ Tue, 30 Aug 2005 00:50:13 +0000 : chx Attachment: http://drupal.org/files/issues/discuss_1.patch (3.58 KB) I have uploaded moshe's patch. Let's try again. ------------------------------------------------------------------------ Tue, 30 Aug 2005 11:22:18 +0000 : Bèr Kessels I like this approach a lot. However, as i use drupal also for blogging, would dislike my bog to behave the same. Thus there must be some -easy- way to get around this. µ The problem lies mainly in the fact that tabs are not administratable in Drupals menu admin. And I think it is a very bad idea to add a config option where to place the comments. We should go for one way, teh default way, and offer a menu-admin of some sort to change this, for bloggers and slashdot alikes. How to achieve this? I see two options, but I really hope people see more then one.: - make local-tasks appear in the menu admin (gets a huge +1 from me) It's a separate issue, though. - add a config option to the comment admin to allow comments either as tab or as thread under the post. IMO that config option should not set a variable, that gets checked every hook_menu call, but it should alter the menu-entry in the database for the tabs Its possible, I did that before. But, as said, I hope others have better ideas. for I really would love this feature to get in. But I am afraid a huge lot of people will move away from drupal if we allow tabs only.

1 0