August 2005 - Development - lists.drupal.org

[drupal-devel] [bug] [PGSQL] node_save, design flaw
by Thomas Ilsche 27 Aug '05

27 Aug '05

Issue status update for http://drupal.org/node/29818 Post a follow up: http://drupal.org/project/comments/add/29818 Project: Drupal Version: cvs Component: node.module Category: bug reports Priority: critical Assigned to: Anonymous Reported by: Thomas Ilsche Updated by: Thomas Ilsche Status: patch (code needs work) Attachment: http://drupal.org/files/issues/pgsql_node_save.patch (764 bytes) on pgsql node_save produces errors when saving certain fields like the boolean sticky. FALSE as string returns as '' instead of 0 as it should be in db - resulting in this error: warning: pg_query() [<a href='/manual/en/function.pg-query.html'>function.pg-query.html</a>]: Query failed: ERROR: invalid input syntax for integer: "" in D:\amp\htdocs\projekte\drupal_head\includes\database.pgsql.inc on line 76. user error: query: INSERT INTO node (title, body, format, uid, type, teaser, status, moderate, promote, sticky, comment, created, changed, nid) VALUES('!SimpleTest test node! simpletest_xwn9wlV2_S', '!SimpleTest test body! simpletest_oiZZeQw0LqeHVhfrk0twJ_RYNOQbbB5p simpletest_WU5E8tu0dmtvwg_1RN7B9byh5OuFw02u', '1', '12', 'page', '!SimpleTest test body! simpletest_oiZZeQw0LqeHVhfrk0twJ_RYNOQbbB5p simpletest_WU5E8tu0dmtvwg_1RN7B9byh5OuFw02u', '1', '', '1', '', '2', '1125165026', '1125165026', '5') in D:\amp\htdocs\projekte\drupal_head\includes\database.pgsql.inc on line 93. I consider this a quite major design flaw as the looped general mixed->string conversion strongly relies on the forgiving string->mixed conversions of MySQL. However I have crafted a fix that solves the issue for me when creating pages, maybe needs more work or even redesign. Thats the curse of dynamic typing! Thomas Ilsche

1 1

[drupal-devel] [task] Extend db_query()
by Thomas Ilsche 27 Aug '05

27 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: Thomas Ilsche Status: patch (code needs review) 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. Thomas Ilsche 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...

1 0

[drupal-devel] [bug] PG_QUERY ERROR - LOCK TABLES - IMPLEMENTATION FAULT.
by Thomas Ilsche 27 Aug '05

27 Aug '05

Issue status update for http://drupal.org/node/22911 Post a follow up: http://drupal.org/project/comments/add/22911 Project: Drupal Version: cvs Component: database system Category: bug reports Priority: critical Assigned to: Anonymous Reported by: kinai Updated by: Thomas Ilsche Status: patch (ready to be committed) +1 tested the patch on pgsql, runs as expected. Thomas Ilsche Previous comments: ------------------------------------------------------------------------ Sun, 15 May 2005 14:34:48 +0000 : kinai I get this error : Warning: pg_query() [function.pg-query]: Query failed: ERROR: syntax error on or near "cache" at character 13 in /var/www/drupal/includes/database.pgsql.inc on line 45 Fatal error: ERROR: syntax error on or near "cache" at character 13 query: LOCK TABLES cache WRITE in /var/www/drupal/includes/database.pgsql.inc on line 62 Someone could solve this problem ? Thanks kinaï ------------------------------------------------------------------------ Sun, 15 May 2005 15:19:26 +0000 : kinai The SQL Command : LOCK TABLES like used in bootstrap.inc is only specified for MySQL, not for PostgreSQL. The UNLOCK TABLES SQL command is not implemented in PostgreSQL. Someone could solve this problem ? Thanks, Kinaï ------------------------------------------------------------------------ Sat, 28 May 2005 23:34:03 +0000 : bd_csmc this issue still exists, i've verified it on a fresh cvs install w/ pgsql 8 ------------------------------------------------------------------------ Sat, 28 May 2005 23:39:52 +0000 : bd_csmc i fixed the problem in my installation, by changing the syntax to: LOCK TABLE {cache} it's table, not tables. and i dropped the 'write' bit. furthermore, pgsql does not support the UNLOCK command, so i had to comment the unlock tables command. unless there are more lock/unlock commands elsewhere in the code, this works fine for me. according to pgsql docs, UNLOCK is automatically performed at transaction end, which is why there is no such command. ------------------------------------------------------------------------ Fri, 19 Aug 2005 11:29:25 +0000 : Cvbge Attachment: http://drupal.org/files/issues/locks.diff (2.91 KB) The patch adds two functions to lock and unlock tables. ------------------------------------------------------------------------ Tue, 23 Aug 2005 12:57:43 +0000 : Cvbge Attachment: http://drupal.org/files/issues/locks_0.diff (2.8 KB) Updated, without unnecessary changes. ------------------------------------------------------------------------ Tue, 23 Aug 2005 13:07:18 +0000 : chx Cvbge says he tested pgsql, I applied to a mysql one, saved system settings and looks OK. ------------------------------------------------------------------------ Tue, 23 Aug 2005 14:31:53 +0000 : Cvbge Attachment: http://drupal.org/files/issues/locks_1.diff (2.81 KB) Worked, yet not worked ... This version does not use <?php db_query("{$table}" ) ?> syntax. I promise I won't do that again! ------------------------------------------------------------------------ Thu, 25 Aug 2005 21:10:07 +0000 : Dries MySQL is happy with the singular 'LOCK TABLE' (rather than 'LOCK TABLES') which should work with both MySQL and PostgreSQL. Committed that changes to CVS. ------------------------------------------------------------------------ Thu, 25 Aug 2005 21:43:09 +0000 : Cvbge Please rollback your patch and either a) use syntax LOCK TABLE name (dropping S and WRITE, if you say that mysql support "LOCK") b) apply my patch. Postgres does not support WRITE part (as well as S part). Also locking won't work without starting transaction. You can do LOCK TABLE name only, but this lock is too excessive and won't allow even SELECTs on locked table. ------------------------------------------------------------------------ Fri, 26 Aug 2005 10:15:15 +0000 : Cvbge Attachment: http://drupal.org/files/issues/drupal-head-lock.diff (2.82 KB) Attached updated patch for current head.

1 0

[drupal-devel] [feature] Add support for generating an RSS 0.92 feed of news aggregator items
by Uwe Hermann 27 Aug '05

27 Aug '05

Issue status update for http://drupal.org/node/9167 Post a follow up: http://drupal.org/project/comments/add/9167 Project: Drupal Version: cvs Component: aggregator.module Category: feature requests Priority: normal -Assigned to: Anonymous +Assigned to: Uwe Hermann Reported by: blake7 Updated by: Uwe Hermann -Status: patch (code needs work) +Status: patch (code needs review) Attachment: http://drupal.org/files/issues/aggregator_rss_0.patch (4.77 KB) OK, updated. Uwe Hermann Previous comments: ------------------------------------------------------------------------ Fri, 09 Jul 2004 04:11:13 +0000 : blake7 Attachment: http://drupal.org/files/issues/aggregator.module_0.patch (2.96 KB) The attached patch adds support for generating an RSS 0.92 feed of the default news aggregator page (the output of aggregator_page_last()). This allows re-use of the Drupal aggregator in other RSS readers like Trillian. The patch consists of the aggregator_page_rss() function which builds the XML output needed. It uses the existing RSS building functions already present to construct the feed. Note the comment about how many items are in the feed. In Drupal 4.4.x, there was a variable "aggregator_page_limit" which represented how many items were returned for both the HTML and RSS versions. It defaulted to 75 items. In Drupal CVS, all the HTML output is now paged through a hard-coded limit of 20 items per page. A limit of 20 seems small for the RSS feed, so I reverted to the old limit of 75. Would it be better to add a new variable like "aggregator_rss_limit"? Also, there are 2 places that the callback to aggregator_page_rss are installed. One is in _aggregator_page_list right before the pager output and shows up as the XML icon (like how the OPML feeds are linked now). The other is in aggregator_menu where I try and install a menu itme called "RSS feed". For some reason, the menu itme is not displaying itself though I believe I am inserting it properly. Feel free to contact me if there are any questions. Thanks! ------------------------------------------------------------------------ Fri, 09 Jul 2004 17:29:48 +0000 : blake7 Attachment: http://drupal.org/files/issues/aggregator.module_1.patch (2.54 KB) Fixed the navigation menu problems mentioned above. Please consider this patch instead of the first one. ------------------------------------------------------------------------ Fri, 09 Jul 2004 18:09:54 +0000 : drumm When this functionality is added it should be able to generate feeds for each aggregator category as well. ------------------------------------------------------------------------ Sat, 10 Jul 2004 01:22:31 +0000 : blake7 Attachment: http://drupal.org/files/issues/aggregator.module_2.patch (3.25 KB) Done. The attached patch supports RSS feeds of each aggregator category through the XML icon at the bottom exactly like the main aggregator page. ------------------------------------------------------------------------ Sat, 10 Jul 2004 12:01:05 +0000 : Dries 1. $base_url . "/aggregator" . $url is incorrect. All URLs must be constructed using url() as it is aware of clean URLs and path aliasing. 2. Emitting 75 RSS items is quite much (uncommon) and can easily turn into a bandwidth bottleneck. Can we add one global setting that controls how many entries our feed should have (not just the newly introduced feeds)? To 'export' your content in case you wish to migrate to another tool, a special value 'all' (say '1000000') might be handy. ------------------------------------------------------------------------ Fri, 17 Sep 2004 08:19:27 +0000 : blake7 Attachment: http://drupal.org/files/issues/aggregator.module_3.patch (4.47 KB) 1. Using url() now. 2. Added the presistent variable aggregator_rss_limit which controls how many RSS items are generated. A form_select edit control to control the new variable, with all defined as 1000000, was added to the configure tab. I am not sure what you mean by "our feeds" versus "newly introduced feeds" in your comment. Also, even though the code-freeze has taken place, do you think it would be possible to get this into Drupal 4.5? The code change is very minimal and I believe the ability to share aggregated feeds is a great feature. ------------------------------------------------------------------------ Fri, 17 Sep 2004 18:07:05 +0000 : Boris Mann +1 I actually assumed (there was much discussion?) that the re-exporting of aggregator items on a per-bundle basis was going to be enabled for 4.5. Let's get this in before the code freeze -- this would be a major feature announcement for 4.5, and could perform the basis for other great functionality for 4.6 (e.g. per category OPML file generation). ------------------------------------------------------------------------ Fri, 22 Oct 2004 18:20:21 +0000 : blake7 Now that 4.5 has released. Can this go into CVS/HEAD? Is there any other changes to the patch that need to be made by me to make that happen? Thanks. ------------------------------------------------------------------------ Sat, 23 Oct 2004 18:05:33 +0000 : Dries I'll look at it again. Does the patch add XML-icons or how do people find out about the RSS feeds? Can't we make the URLs a bit more intuitive? For example by making it so that you merely have to append '/rss.xml' to the aggregator module's URLs: foo.com/aggregator/rss.xml foo.com/aggregator/categories/1/rss.xmlIt is certainly not a requirement, just a thought that might be worth considering. ------------------------------------------------------------------------ Sat, 23 Oct 2004 18:15:58 +0000 : Dries I'll look at it again. Does the patch add XML-icons or how do people find out about the RSS feeds? Can't we make the URLs a bit more intuitive? For example by making it so that you merely have to append '/rss.xml' to the aggregator module's URLs: foo.com/aggregator/rss.xml foo.com/aggregator/categories/1/rss.xmlIt is certainly not a requirement, just a thought that might be worth considering. ------------------------------------------------------------------------ Mon, 25 Oct 2004 10:50:27 +0000 : Bèr Kessels cross linking to a [feature] that talks about such a unified rss link: http://drupal.org/node/11937 ------------------------------------------------------------------------ Wed, 09 Feb 2005 20:17:38 +0000 : sethcohn Bump. Neat feature, would be nice for 4.6 What's the progress on making it happen? ------------------------------------------------------------------------ Wed, 09 Feb 2005 20:43:59 +0000 : Boris Mann Here's another bump to this. I would say since it missed 4.5 (and was ready at the time) we get this in to 4.6. Should mesh nicely with walkah's RSS updates. Dries: there is an XML icon at the bottom of each category page, plus the HTML header can include an alternate link to the feed. I vote against any sort of "rss.xml", people can alias if they wish. (ideally we could get per-category OPML links as well, but I'll leave that for another feature request) ------------------------------------------------------------------------ Thu, 10 Feb 2005 14:58:47 +0000 : Bèr Kessels A request for unification of feeds [1] is waiting in the queue already. [1] http://drupal.org/node/11937 ------------------------------------------------------------------------ Thu, 10 Feb 2005 15:23:47 +0000 : Boris Mann This patch is not about "feed unification", it adds RSS feeds for category bundles...which have been asked about (and coded for, here in this patch) since 4.4. ------------------------------------------------------------------------ Mon, 28 Feb 2005 22:37:21 +0000 : playdreamer Thrilled I found this as its the one killer feature that the aggregator appeared to be missing. One point I notice though is that the title of the generated feed is 'drupal', when presumably it should pick up something along the lines of 'sitename news aggregator'. ------------------------------------------------------------------------ Tue, 01 Mar 2005 20:42:20 +0000 : Dries In addition to the other suggestions, this patch should not use arg() and should make the hardcoded string stranslatable. ------------------------------------------------------------------------ Fri, 11 Mar 2005 19:00:53 +0000 : deskpops I'm trying to integrate Blake7's RSS creation code into my aggregator.module, but I don't think I have the experience to do it properly. Has anyone created a complete aggregator.module that has this already in it that I can just use to replace my current aggregator module? (I'm running 4.5.2) Thanks in advance, Jeff ------------------------------------------------------------------------ Tue, 12 Apr 2005 23:06:58 +0000 : Richard Eriksson Attachment: http://drupal.org/files/issues/aggregator.module.bundle-feeds.patch.txt (4.53 KB) The attached patch adds a bugfix in the title (changes it from a hard-coded 'drupal' to a variable_get('site_name', 'drupal') and makes the link element inside the channel a fully-qualifed URL using base_url + '/' then the path of the aggregator. (It doesn't incorporate, however, Dries suggestions for removing the args and translating the hard-coded strings.) This patch is based on the above patches. Suggestions for future patches: +1 on Dries' suggestions, and add a link element in the HTML header for RSS autodiscovery. ------------------------------------------------------------------------ Wed, 08 Jun 2005 19:02:55 +0000 : Michael Acord(a)www.simplemoneymanagement.info Any idea how to get this to apply to drupal 4.6.1? The first chunk does not patch, but chunks 2-4 do. ------------------------------------------------------------------------ Sat, 20 Aug 2005 02:00:22 +0000 : Uwe Hermann Attachment: http://drupal.org/files/issues/aggregator_feed.patch (4.79 KB) Here's an update for the patch from #18, rerolled for current HEAD. Please test. ------------------------------------------------------------------------ Mon, 22 Aug 2005 20:59:09 +0000 : Dries This patch needs some love: - Use %d and %s directives in SQL queries. - Remove the RSS items configuration option. We don't have such setting for node feeds either so this looks unbalanced. It would be nice to have some global RSS feed settings though, to control how many items are syndicated, whether descriptions are to be provided, etc. - The strings can not be translated. ------------------------------------------------------------------------ Sat, 27 Aug 2005 14:18:59 +0000 : Uwe Hermann Attachment: http://drupal.org/files/issues/aggregator_rss.patch (4.72 KB) Updated patch for HEAD. Hardcodes the number of feed items to 15 (as with the normal Drupal feed) for now, I'll create another patch which introduces site-wide feed customization options later. SQL queries use %d and %s now. Replaced " with ' where possible. ------------------------------------------------------------------------ Sat, 27 Aug 2005 15:43:42 +0000 : Dries Still not good enough: - That t(' for ') needs to be fixed ... translators won't be able to do anything with it. They won't spot the spaces either. At the very least make it '%foo for %bar' or something, or just change the code so there is nothing to translate. - $item->title . t(' [').$item->ftitle.t(']') is hairy; impossible to translate and ugly to look at in your aggregator. I'd go with just $item->title. - . The leading space in t(' aggregator') needs fixing too ... - t('Drupal aggregator RSS feed') . $title is plain wrong: no space between feed and $title. Joe Average doesn't and shouldn't know what Drupal is.

1 0

[drupal-devel] [feature] Error message when pg_sql module not enabled.
by Thomas Ilsche 27 Aug '05

27 Aug '05

Issue status update for http://drupal.org/node/29808 Post a follow up: http://drupal.org/project/comments/add/29808 Project: Drupal Version: cvs Component: postgresql database Category: feature requests Priority: minor Assigned to: Thomas Ilsche Reported by: Thomas Ilsche Updated by: Thomas Ilsche Status: patch (code needs review) Attachment: http://drupal.org/files/issues/pg_error.patch (999 bytes) I just started playing with the pgsql db and encountered a no-error-empty page because i forgot to restart apache after enabling the php_pgsql module. No error is shown because the pg_connect is @error @squelched. I have added a simple error page for that case. I think even for stupidy, wich really is the reason for this, an error should be displayed. Please review, especially my bad english... Thomas Ilsche

1 0

[drupal-devel] [feature] Add support for generating an RSS 0.92 feed of news aggregator items
by Dries 27 Aug '05

27 Aug '05

Issue status update for http://drupal.org/node/9167 Post a follow up: http://drupal.org/project/comments/add/9167 Project: Drupal Version: cvs Component: aggregator.module Category: feature requests Priority: normal Assigned to: Anonymous Reported by: blake7 Updated by: Dries -Status: patch (code needs review) +Status: patch (code needs work) Still not good enough: - That t(' for ') needs to be fixed ... translators won't be able to do anything with it. They won't spot the spaces either. At the very least make it '%foo for %bar' or something, or just change the code so there is nothing to translate. - $item->title . t(' [').$item->ftitle.t(']') is hairy; impossible to translate and ugly to look at in your aggregator. I'd go with just $item->title. - . The leading space in t(' aggregator') needs fixing too ... - t('Drupal aggregator RSS feed') . $title is plain wrong: no space between feed and $title. Joe Average doesn't and shouldn't know what Drupal is. Dries Previous comments: ------------------------------------------------------------------------ Fri, 09 Jul 2004 04:11:13 +0000 : blake7 Attachment: http://drupal.org/files/issues/aggregator.module_0.patch (2.96 KB) The attached patch adds support for generating an RSS 0.92 feed of the default news aggregator page (the output of aggregator_page_last()). This allows re-use of the Drupal aggregator in other RSS readers like Trillian. The patch consists of the aggregator_page_rss() function which builds the XML output needed. It uses the existing RSS building functions already present to construct the feed. Note the comment about how many items are in the feed. In Drupal 4.4.x, there was a variable "aggregator_page_limit" which represented how many items were returned for both the HTML and RSS versions. It defaulted to 75 items. In Drupal CVS, all the HTML output is now paged through a hard-coded limit of 20 items per page. A limit of 20 seems small for the RSS feed, so I reverted to the old limit of 75. Would it be better to add a new variable like "aggregator_rss_limit"? Also, there are 2 places that the callback to aggregator_page_rss are installed. One is in _aggregator_page_list right before the pager output and shows up as the XML icon (like how the OPML feeds are linked now). The other is in aggregator_menu where I try and install a menu itme called "RSS feed". For some reason, the menu itme is not displaying itself though I believe I am inserting it properly. Feel free to contact me if there are any questions. Thanks! ------------------------------------------------------------------------ Fri, 09 Jul 2004 17:29:48 +0000 : blake7 Attachment: http://drupal.org/files/issues/aggregator.module_1.patch (2.54 KB) Fixed the navigation menu problems mentioned above. Please consider this patch instead of the first one. ------------------------------------------------------------------------ Fri, 09 Jul 2004 18:09:54 +0000 : drumm When this functionality is added it should be able to generate feeds for each aggregator category as well. ------------------------------------------------------------------------ Sat, 10 Jul 2004 01:22:31 +0000 : blake7 Attachment: http://drupal.org/files/issues/aggregator.module_2.patch (3.25 KB) Done. The attached patch supports RSS feeds of each aggregator category through the XML icon at the bottom exactly like the main aggregator page. ------------------------------------------------------------------------ Sat, 10 Jul 2004 12:01:05 +0000 : Dries 1. $base_url . "/aggregator" . $url is incorrect. All URLs must be constructed using url() as it is aware of clean URLs and path aliasing. 2. Emitting 75 RSS items is quite much (uncommon) and can easily turn into a bandwidth bottleneck. Can we add one global setting that controls how many entries our feed should have (not just the newly introduced feeds)? To 'export' your content in case you wish to migrate to another tool, a special value 'all' (say '1000000') might be handy. ------------------------------------------------------------------------ Fri, 17 Sep 2004 08:19:27 +0000 : blake7 Attachment: http://drupal.org/files/issues/aggregator.module_3.patch (4.47 KB) 1. Using url() now. 2. Added the presistent variable aggregator_rss_limit which controls how many RSS items are generated. A form_select edit control to control the new variable, with all defined as 1000000, was added to the configure tab. I am not sure what you mean by "our feeds" versus "newly introduced feeds" in your comment. Also, even though the code-freeze has taken place, do you think it would be possible to get this into Drupal 4.5? The code change is very minimal and I believe the ability to share aggregated feeds is a great feature. ------------------------------------------------------------------------ Fri, 17 Sep 2004 18:07:05 +0000 : Boris Mann +1 I actually assumed (there was much discussion?) that the re-exporting of aggregator items on a per-bundle basis was going to be enabled for 4.5. Let's get this in before the code freeze -- this would be a major feature announcement for 4.5, and could perform the basis for other great functionality for 4.6 (e.g. per category OPML file generation). ------------------------------------------------------------------------ Fri, 22 Oct 2004 18:20:21 +0000 : blake7 Now that 4.5 has released. Can this go into CVS/HEAD? Is there any other changes to the patch that need to be made by me to make that happen? Thanks. ------------------------------------------------------------------------ Sat, 23 Oct 2004 18:05:33 +0000 : Dries I'll look at it again. Does the patch add XML-icons or how do people find out about the RSS feeds? Can't we make the URLs a bit more intuitive? For example by making it so that you merely have to append '/rss.xml' to the aggregator module's URLs: foo.com/aggregator/rss.xml foo.com/aggregator/categories/1/rss.xmlIt is certainly not a requirement, just a thought that might be worth considering. ------------------------------------------------------------------------ Sat, 23 Oct 2004 18:15:58 +0000 : Dries I'll look at it again. Does the patch add XML-icons or how do people find out about the RSS feeds? Can't we make the URLs a bit more intuitive? For example by making it so that you merely have to append '/rss.xml' to the aggregator module's URLs: foo.com/aggregator/rss.xml foo.com/aggregator/categories/1/rss.xmlIt is certainly not a requirement, just a thought that might be worth considering. ------------------------------------------------------------------------ Mon, 25 Oct 2004 10:50:27 +0000 : Bèr Kessels cross linking to a [feature] that talks about such a unified rss link: http://drupal.org/node/11937 ------------------------------------------------------------------------ Wed, 09 Feb 2005 20:17:38 +0000 : sethcohn Bump. Neat feature, would be nice for 4.6 What's the progress on making it happen? ------------------------------------------------------------------------ Wed, 09 Feb 2005 20:43:59 +0000 : Boris Mann Here's another bump to this. I would say since it missed 4.5 (and was ready at the time) we get this in to 4.6. Should mesh nicely with walkah's RSS updates. Dries: there is an XML icon at the bottom of each category page, plus the HTML header can include an alternate link to the feed. I vote against any sort of "rss.xml", people can alias if they wish. (ideally we could get per-category OPML links as well, but I'll leave that for another feature request) ------------------------------------------------------------------------ Thu, 10 Feb 2005 14:58:47 +0000 : Bèr Kessels A request for unification of feeds [1] is waiting in the queue already. [1] http://drupal.org/node/11937 ------------------------------------------------------------------------ Thu, 10 Feb 2005 15:23:47 +0000 : Boris Mann This patch is not about "feed unification", it adds RSS feeds for category bundles...which have been asked about (and coded for, here in this patch) since 4.4. ------------------------------------------------------------------------ Mon, 28 Feb 2005 22:37:21 +0000 : playdreamer Thrilled I found this as its the one killer feature that the aggregator appeared to be missing. One point I notice though is that the title of the generated feed is 'drupal', when presumably it should pick up something along the lines of 'sitename news aggregator'. ------------------------------------------------------------------------ Tue, 01 Mar 2005 20:42:20 +0000 : Dries In addition to the other suggestions, this patch should not use arg() and should make the hardcoded string stranslatable. ------------------------------------------------------------------------ Fri, 11 Mar 2005 19:00:53 +0000 : deskpops I'm trying to integrate Blake7's RSS creation code into my aggregator.module, but I don't think I have the experience to do it properly. Has anyone created a complete aggregator.module that has this already in it that I can just use to replace my current aggregator module? (I'm running 4.5.2) Thanks in advance, Jeff ------------------------------------------------------------------------ Tue, 12 Apr 2005 23:06:58 +0000 : Richard Eriksson Attachment: http://drupal.org/files/issues/aggregator.module.bundle-feeds.patch.txt (4.53 KB) The attached patch adds a bugfix in the title (changes it from a hard-coded 'drupal' to a variable_get('site_name', 'drupal') and makes the link element inside the channel a fully-qualifed URL using base_url + '/' then the path of the aggregator. (It doesn't incorporate, however, Dries suggestions for removing the args and translating the hard-coded strings.) This patch is based on the above patches. Suggestions for future patches: +1 on Dries' suggestions, and add a link element in the HTML header for RSS autodiscovery. ------------------------------------------------------------------------ Wed, 08 Jun 2005 19:02:55 +0000 : Michael Acord(a)www.simplemoneymanagement.info Any idea how to get this to apply to drupal 4.6.1? The first chunk does not patch, but chunks 2-4 do. ------------------------------------------------------------------------ Sat, 20 Aug 2005 02:00:22 +0000 : Uwe Hermann Attachment: http://drupal.org/files/issues/aggregator_feed.patch (4.79 KB) Here's an update for the patch from #18, rerolled for current HEAD. Please test. ------------------------------------------------------------------------ Mon, 22 Aug 2005 20:59:09 +0000 : Dries This patch needs some love: - Use %d and %s directives in SQL queries. - Remove the RSS items configuration option. We don't have such setting for node feeds either so this looks unbalanced. It would be nice to have some global RSS feed settings though, to control how many items are syndicated, whether descriptions are to be provided, etc. - The strings can not be translated. ------------------------------------------------------------------------ Sat, 27 Aug 2005 14:18:59 +0000 : Uwe Hermann Attachment: http://drupal.org/files/issues/aggregator_rss.patch (4.72 KB) Updated patch for HEAD. Hardcodes the number of feed items to 15 (as with the normal Drupal feed) for now, I'll create another patch which introduces site-wide feed customization options later. SQL queries use %d and %s now. Replaced " with ' where possible.

1 0

[drupal-devel] Movable Type
by Dries Buytaert 27 Aug '05

27 Aug '05

Movable Type 3.2 has been released. Might be a good source for inspiration: http://www.cmswire.com/cms/micro-cms/movable-type-32-hits-the- street-000651.php -- Dries Buytaert :: http://www.buytaert.net/

4 3

[drupal-devel] [feature] Add support for generating an RSS 0.92 feed of news aggregator items
by Uwe Hermann 27 Aug '05

27 Aug '05

Issue status update for http://drupal.org/node/9167 Post a follow up: http://drupal.org/project/comments/add/9167 Project: Drupal Version: cvs Component: aggregator.module Category: feature requests Priority: normal Assigned to: Anonymous Reported by: blake7 Updated by: Uwe Hermann -Status: patch (code needs work) +Status: patch (code needs review) Attachment: http://drupal.org/files/issues/aggregator_rss.patch (4.72 KB) Updated patch for HEAD. Hardcodes the number of feed items to 15 (as with the normal Drupal feed) for now, I'll create another patch which introduces site-wide feed customization options later. SQL queries use %d and %s now. Replaced " with ' where possible. Uwe Hermann Previous comments: ------------------------------------------------------------------------ Fri, 09 Jul 2004 04:11:13 +0000 : blake7 Attachment: http://drupal.org/files/issues/aggregator.module_0.patch (2.96 KB) The attached patch adds support for generating an RSS 0.92 feed of the default news aggregator page (the output of aggregator_page_last()). This allows re-use of the Drupal aggregator in other RSS readers like Trillian. The patch consists of the aggregator_page_rss() function which builds the XML output needed. It uses the existing RSS building functions already present to construct the feed. Note the comment about how many items are in the feed. In Drupal 4.4.x, there was a variable "aggregator_page_limit" which represented how many items were returned for both the HTML and RSS versions. It defaulted to 75 items. In Drupal CVS, all the HTML output is now paged through a hard-coded limit of 20 items per page. A limit of 20 seems small for the RSS feed, so I reverted to the old limit of 75. Would it be better to add a new variable like "aggregator_rss_limit"? Also, there are 2 places that the callback to aggregator_page_rss are installed. One is in _aggregator_page_list right before the pager output and shows up as the XML icon (like how the OPML feeds are linked now). The other is in aggregator_menu where I try and install a menu itme called "RSS feed". For some reason, the menu itme is not displaying itself though I believe I am inserting it properly. Feel free to contact me if there are any questions. Thanks! ------------------------------------------------------------------------ Fri, 09 Jul 2004 17:29:48 +0000 : blake7 Attachment: http://drupal.org/files/issues/aggregator.module_1.patch (2.54 KB) Fixed the navigation menu problems mentioned above. Please consider this patch instead of the first one. ------------------------------------------------------------------------ Fri, 09 Jul 2004 18:09:54 +0000 : drumm When this functionality is added it should be able to generate feeds for each aggregator category as well. ------------------------------------------------------------------------ Sat, 10 Jul 2004 01:22:31 +0000 : blake7 Attachment: http://drupal.org/files/issues/aggregator.module_2.patch (3.25 KB) Done. The attached patch supports RSS feeds of each aggregator category through the XML icon at the bottom exactly like the main aggregator page. ------------------------------------------------------------------------ Sat, 10 Jul 2004 12:01:05 +0000 : Dries 1. $base_url . "/aggregator" . $url is incorrect. All URLs must be constructed using url() as it is aware of clean URLs and path aliasing. 2. Emitting 75 RSS items is quite much (uncommon) and can easily turn into a bandwidth bottleneck. Can we add one global setting that controls how many entries our feed should have (not just the newly introduced feeds)? To 'export' your content in case you wish to migrate to another tool, a special value 'all' (say '1000000') might be handy. ------------------------------------------------------------------------ Fri, 17 Sep 2004 08:19:27 +0000 : blake7 Attachment: http://drupal.org/files/issues/aggregator.module_3.patch (4.47 KB) 1. Using url() now. 2. Added the presistent variable aggregator_rss_limit which controls how many RSS items are generated. A form_select edit control to control the new variable, with all defined as 1000000, was added to the configure tab. I am not sure what you mean by "our feeds" versus "newly introduced feeds" in your comment. Also, even though the code-freeze has taken place, do you think it would be possible to get this into Drupal 4.5? The code change is very minimal and I believe the ability to share aggregated feeds is a great feature. ------------------------------------------------------------------------ Fri, 17 Sep 2004 18:07:05 +0000 : Boris Mann +1 I actually assumed (there was much discussion?) that the re-exporting of aggregator items on a per-bundle basis was going to be enabled for 4.5. Let's get this in before the code freeze -- this would be a major feature announcement for 4.5, and could perform the basis for other great functionality for 4.6 (e.g. per category OPML file generation). ------------------------------------------------------------------------ Fri, 22 Oct 2004 18:20:21 +0000 : blake7 Now that 4.5 has released. Can this go into CVS/HEAD? Is there any other changes to the patch that need to be made by me to make that happen? Thanks. ------------------------------------------------------------------------ Sat, 23 Oct 2004 18:05:33 +0000 : Dries I'll look at it again. Does the patch add XML-icons or how do people find out about the RSS feeds? Can't we make the URLs a bit more intuitive? For example by making it so that you merely have to append '/rss.xml' to the aggregator module's URLs: foo.com/aggregator/rss.xml foo.com/aggregator/categories/1/rss.xmlIt is certainly not a requirement, just a thought that might be worth considering. ------------------------------------------------------------------------ Sat, 23 Oct 2004 18:15:58 +0000 : Dries I'll look at it again. Does the patch add XML-icons or how do people find out about the RSS feeds? Can't we make the URLs a bit more intuitive? For example by making it so that you merely have to append '/rss.xml' to the aggregator module's URLs: foo.com/aggregator/rss.xml foo.com/aggregator/categories/1/rss.xmlIt is certainly not a requirement, just a thought that might be worth considering. ------------------------------------------------------------------------ Mon, 25 Oct 2004 10:50:27 +0000 : Bèr Kessels cross linking to a [feature] that talks about such a unified rss link: http://drupal.org/node/11937 ------------------------------------------------------------------------ Wed, 09 Feb 2005 20:17:38 +0000 : sethcohn Bump. Neat feature, would be nice for 4.6 What's the progress on making it happen? ------------------------------------------------------------------------ Wed, 09 Feb 2005 20:43:59 +0000 : Boris Mann Here's another bump to this. I would say since it missed 4.5 (and was ready at the time) we get this in to 4.6. Should mesh nicely with walkah's RSS updates. Dries: there is an XML icon at the bottom of each category page, plus the HTML header can include an alternate link to the feed. I vote against any sort of "rss.xml", people can alias if they wish. (ideally we could get per-category OPML links as well, but I'll leave that for another feature request) ------------------------------------------------------------------------ Thu, 10 Feb 2005 14:58:47 +0000 : Bèr Kessels A request for unification of feeds [1] is waiting in the queue already. [1] http://drupal.org/node/11937 ------------------------------------------------------------------------ Thu, 10 Feb 2005 15:23:47 +0000 : Boris Mann This patch is not about "feed unification", it adds RSS feeds for category bundles...which have been asked about (and coded for, here in this patch) since 4.4. ------------------------------------------------------------------------ Mon, 28 Feb 2005 22:37:21 +0000 : playdreamer Thrilled I found this as its the one killer feature that the aggregator appeared to be missing. One point I notice though is that the title of the generated feed is 'drupal', when presumably it should pick up something along the lines of 'sitename news aggregator'. ------------------------------------------------------------------------ Tue, 01 Mar 2005 20:42:20 +0000 : Dries In addition to the other suggestions, this patch should not use arg() and should make the hardcoded string stranslatable. ------------------------------------------------------------------------ Fri, 11 Mar 2005 19:00:53 +0000 : deskpops I'm trying to integrate Blake7's RSS creation code into my aggregator.module, but I don't think I have the experience to do it properly. Has anyone created a complete aggregator.module that has this already in it that I can just use to replace my current aggregator module? (I'm running 4.5.2) Thanks in advance, Jeff ------------------------------------------------------------------------ Tue, 12 Apr 2005 23:06:58 +0000 : Richard Eriksson Attachment: http://drupal.org/files/issues/aggregator.module.bundle-feeds.patch.txt (4.53 KB) The attached patch adds a bugfix in the title (changes it from a hard-coded 'drupal' to a variable_get('site_name', 'drupal') and makes the link element inside the channel a fully-qualifed URL using base_url + '/' then the path of the aggregator. (It doesn't incorporate, however, Dries suggestions for removing the args and translating the hard-coded strings.) This patch is based on the above patches. Suggestions for future patches: +1 on Dries' suggestions, and add a link element in the HTML header for RSS autodiscovery. ------------------------------------------------------------------------ Wed, 08 Jun 2005 19:02:55 +0000 : Michael Acord(a)www.simplemoneymanagement.info Any idea how to get this to apply to drupal 4.6.1? The first chunk does not patch, but chunks 2-4 do. ------------------------------------------------------------------------ Sat, 20 Aug 2005 02:00:22 +0000 : Uwe Hermann Attachment: http://drupal.org/files/issues/aggregator_feed.patch (4.79 KB) Here's an update for the patch from #18, rerolled for current HEAD. Please test. ------------------------------------------------------------------------ Mon, 22 Aug 2005 20:59:09 +0000 : Dries This patch needs some love: - Use %d and %s directives in SQL queries. - Remove the RSS items configuration option. We don't have such setting for node feeds either so this looks unbalanced. It would be nice to have some global RSS feed settings though, to control how many items are syndicated, whether descriptions are to be provided, etc. - The strings can not be translated.

1 0

[drupal-devel] [bug] function format_date uses gmdate() incorrectly
by thehunmonkgroup 27 Aug '05

27 Aug '05

Issue status update for http://drupal.org/node/29681 Post a follow up: http://drupal.org/project/comments/add/29681 Project: Drupal Version: cvs Component: base system Category: bug reports Priority: normal Assigned to: thehunmonkgroup Reported by: thehunmonkgroup Updated by: thehunmonkgroup -Status: active +Status: patch (code needs review) switching to patch status thehunmonkgroup Previous comments: ------------------------------------------------------------------------ Thu, 25 Aug 2005 22:04:51 +0000 : thehunmonkgroup Attachment: http://drupal.org/files/issues/format_date.patch (817 bytes) function format_date in common.inc is using gmdate() incorrectly. the function is trying to format a local timestamp, and GMT conversion is incorrect in this case. date() should be used instead. bug also exists in 4.6, and this patch should apply there, too. haven't checked the earlier versions of drupal...

1 0

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

26 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) 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)?* 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.

1 0