- Development - lists.drupal.org

[drupal-devel] [task] Collapsible settings for all settings pages?
by m3avrck 13 Sep '05

13 Sep '05

Issue status update for http://drupal.org/node/30038 Post a follow up: http://drupal.org/project/comments/add/30038 Project: Drupal Version: cvs Component: base system Category: tasks Priority: normal Assigned to: Anonymous Reported by: m3avrck Updated by: m3avrck -Status: active +Status: patch (ready to be committed) Attachment: http://drupal.org/files/issues/drupal_17.patch (9.17 KB) Rerolled against HEAD. Since it seems the FORM api is going to go in *after* the freeze, this should go in now, so before any of the forms are updated to the new API (so this change is accounted for later instead of having to do another patch :)) m3avrck Previous comments: ------------------------------------------------------------------------ Tue, 30 Aug 2005 17:37:22 +0000 : m3avrck Is there a reason collapsible fieldsets is used *only* on the main settings page? What about the users setting page? That is really long and would benefit. And the uploads setting page, as you add more roles, that page can get really long too. I think we should add this feature to all standard Drupal settings pages that are long like the settings page... it really helps and would make things look more consistent, instead of "why the heck is this on one page and not them all?". Need to get this into the next release as well to be consistent with the approach of this new feature. ------------------------------------------------------------------------ Tue, 30 Aug 2005 20:38:54 +0000 : m3avrck Attachment: http://drupal.org/files/issues/modules_0.patch (9.17 KB) I've gone through all of the settings pages for the included default Drupal modules and have applied the collapsible form fix. This really does make a *huge* difference... it makes the Settings interface more consistent and greatly improves readability. Let's get this one in! :) ------------------------------------------------------------------------ Tue, 30 Aug 2005 21:32:11 +0000 : Souvent22 Installed, and used the patched. Worked great. Much needed. :). +1 ------------------------------------------------------------------------ Tue, 30 Aug 2005 21:54:42 +0000 : chx -1 and +1 at the same time. +1 but only after the forms API is in -- it'd be better if we won't break the fastly evolving patch. ------------------------------------------------------------------------ Tue, 30 Aug 2005 22:28:25 +0000 : m3avrck Beat me to the post :) I just was reviewing the Forms API patch and realized this one is no longer applicable if that goes through. chx, can you include the fixes in this patch with the forms api? it just makes a few extra menus 'collapsible' which with the new API, requires even less code change, haha!

1 0

[drupal-devel] [bug] node revisions should only be viewable by admins
by killes 13 Sep '05

13 Sep '05

Issue status update for http://drupal.org/node/30098 Post a follow up: http://drupal.org/project/comments/add/30098 Project: Drupal Version: cvs Component: node system Category: bug reports Priority: normal Assigned to: Anonymous 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_rev.patch (1.27 KB) With or without the revisions patch, every user who can access content can access old revisions. I think this is a bug because why would you need to see old revisions if you cannot change them or make them the current revision? The attached patch fixes this and lets only users with "administer nodes" permission see old revs as you need this permission to change revisiosn to be the current one. One might make a case for introducing new "view revisions" and "set revisions" perms. You woud need those if you wanted to mimic a wiki with Drupal. killes(a)www.drop.org

9 25

[drupal-devel] [task] Extend db_query()
by killes 13 Sep '05

13 Sep '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/db_query_3.patch (2.85 KB) Kjartan prefers this version. I also added some comment. 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. ------------------------------------------------------------------------ Tue, 30 Aug 2005 18:13:48 +0000 : killes(a)www.drop.org 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 ------------------------------------------------------------------------ Sun, 04 Sep 2005 18:13:36 +0000 : Cvbge A remark about NULL values: in this patch they are treated as integers and thus allways changed to '0'. In existing code the '' was INSERTed into text-like columns (%s) if didn't check if the variable was set (or did not care). If they now use %a and still do not check for NULLs the '0' will be inserted which migh or might not create problems (it won't matter for if()s, but maybe for other uses?) ------------------------------------------------------------------------ Sun, 04 Sep 2005 19:50:58 +0000 : killes(a)www.drop.org Cvbge: AFAIK we don't use NULL values inside the DB in Drupal core. ------------------------------------------------------------------------ Sun, 04 Sep 2005 20:56:41 +0000 : Cvbge You're probably right, then 1. not checking if a variable is set is a bug (and I've already filled a bug for one of such bugs, can't find it - sticky, moderated etc. fields were not set at all when not selected when submitting a post) 2. There's already a lot of DEFAULT NULL in the database schema... ------------------------------------------------------------------------ Fri, 09 Sep 2005 15:03:09 +0000 : Cvbge Hello, NULLs again. Previously I wrote that NULLs are treated as integers and passed with %d and converted to 0. This is of course not true [were I sick when writting it?]. They are treated as strings and converted to '' (empty string) This will create similar problems as FALSE. Maybe NULL should be treated as %d (and converted to 0) after all? ------------------------------------------------------------------------ Tue, 13 Sep 2005 16:06:30 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/db_query_2.patch (2.72 KB) Ok, I've included is_null() in the condition. ------------------------------------------------------------------------ Tue, 13 Sep 2005 16:31:55 +0000 : Cvbge ok, last thing (really ;)): maybe add some information to docs saying that null is converted to '0' (not to '') ? :)

1 0

[drupal-devel] [task] Extend db_query()
by Cvbge 13 Sep '05

13 Sep '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: Cvbge Status: patch (code needs review) ok, last thing (really ;)): maybe add some information to docs saying that null is converted to '0' (not to '') ? :) Cvbge 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. ------------------------------------------------------------------------ Tue, 30 Aug 2005 18:13:48 +0000 : killes(a)www.drop.org 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 ------------------------------------------------------------------------ Sun, 04 Sep 2005 18:13:36 +0000 : Cvbge A remark about NULL values: in this patch they are treated as integers and thus allways changed to '0'. In existing code the '' was INSERTed into text-like columns (%s) if didn't check if the variable was set (or did not care). If they now use %a and still do not check for NULLs the '0' will be inserted which migh or might not create problems (it won't matter for if()s, but maybe for other uses?) ------------------------------------------------------------------------ Sun, 04 Sep 2005 19:50:58 +0000 : killes(a)www.drop.org Cvbge: AFAIK we don't use NULL values inside the DB in Drupal core. ------------------------------------------------------------------------ Sun, 04 Sep 2005 20:56:41 +0000 : Cvbge You're probably right, then 1. not checking if a variable is set is a bug (and I've already filled a bug for one of such bugs, can't find it - sticky, moderated etc. fields were not set at all when not selected when submitting a post) 2. There's already a lot of DEFAULT NULL in the database schema... ------------------------------------------------------------------------ Fri, 09 Sep 2005 15:03:09 +0000 : Cvbge Hello, NULLs again. Previously I wrote that NULLs are treated as integers and passed with %d and converted to 0. This is of course not true [were I sick when writting it?]. They are treated as strings and converted to '' (empty string) This will create similar problems as FALSE. Maybe NULL should be treated as %d (and converted to 0) after all? ------------------------------------------------------------------------ Tue, 13 Sep 2005 16:06:30 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/db_query_2.patch (2.72 KB) Ok, I've included is_null() in the condition.

1 0

[drupal-devel] [bug] Can't delete subjects with '&' in title
by Cvbge 13 Sep '05

13 Sep '05

Issue status update for http://drupal.org/node/27140 Post a follow up: http://drupal.org/project/comments/add/27140 Project: Drupal Version: cvs Component: contact.module Category: bug reports Priority: normal Assigned to: Souvent22 Reported by: m3avrck Updated by: Cvbge -Status: patch (ready to be committed) +Status: patch (code needs work) Attachment: http://drupal.org/files/issues/drupal-head-contact-27140_0.diff (6.81 KB) It's case 'pgsql' not case "pg_sql" Also, I've noticed now that you use db_next_id(). First, the format is db_next_id('{tablename}_fieldname'), so in this case it should be db_next_id('{contact}_cid') Second, using db_next_id() with postgres needs a sequence defined. Also update path needs changes in case there are already some contacs in the table - the new column cid needs to be updated and sequences set. I've fixed this [for postgres] and attached new patch. I'm not sure how mysql behaves when adding a primary key and there is an existing data, buy you probably have to take care of it (if mysql allows you to add a column that is a primary key and allows it to have nulls then... well, I don't want to swear ;)) I've tested the code and it seems to run ok. Cvbge Previous comments: ------------------------------------------------------------------------ Wed, 20 Jul 2005 14:57:03 +0000 : m3avrck Using the site-wide contact forms, any subjects with an '&' cannot be deleted. Clicking delete for these subjects brings up the expected action flow, however, confirming the delete does nothing. Subject still exists. ------------------------------------------------------------------------ Mon, 25 Jul 2005 13:45:38 +0000 : m3avrck It appears this issue might be linked with this one: http://drupal.org/node/23685 A problem with mod_rewrite and not the actual contact module. ------------------------------------------------------------------------ Wed, 31 Aug 2005 16:51:56 +0000 : Souvent22 Attachment: http://drupal.org/files/issues/contact.module_6.patch (4.7 KB) Redid how contacts work. It now references contacts by ID instead of their category as the unique identifier. It also allows for contacts to have weights, so that one may control where the contact will appear in the listing. ------------------------------------------------------------------------ Wed, 31 Aug 2005 16:53:01 +0000 : Souvent22 Attachment: http://drupal.org/files/issues/updates.inc (27.23 KB) This is the update.inc for the database cahnges that goes along with this patch. ------------------------------------------------------------------------ Wed, 31 Aug 2005 18:31:37 +0000 : Souvent22 Attachment: http://drupal.org/files/issues/contact.module_7.patch (4.5 KB) Updated patch. ------------------------------------------------------------------------ Wed, 31 Aug 2005 18:31:55 +0000 : Souvent22 Attachment: http://drupal.org/files/issues/updates.inc_4.patch (1.29 KB) Updated database patch. ------------------------------------------------------------------------ Sat, 03 Sep 2005 16:17:21 +0000 : Cvbge Hi, 0. You should keep all changes in one file, i.e. pake one patch, not patch for every file 1. You should include changes for database/database.{mysql,pgsql} 2. IIRC auto_increment was depreciated, you should use db_next_id() (but I see auto_increment in cvs...) 3. You use "WHERE cid = '%s'", but cid is integer so this should probably be %d (although this probably would not break anything..) ------------------------------------------------------------------------ Wed, 07 Sep 2005 17:31:21 +0000 : Souvent22 Attachment: http://drupal.org/files/issues/contact_full.patch (6.38 KB) Ok, I've taken your suggestions in, and i think I have it this time. Let me know if this is a correct format for a patch. This only fixes the bug of not being able to delete special characters. ------------------------------------------------------------------------ Wed, 07 Sep 2005 17:37:30 +0000 : m3avrck Check your database updates, the UNIQUE fields don't look right syntactically. ------------------------------------------------------------------------ Wed, 07 Sep 2005 18:23:07 +0000 : Souvent22 Attachment: http://drupal.org/files/issues/contact_full_0.patch (6.25 KB) Ok, i apologize for the last upload. Pleasle ignore. I send the wrong file. :-[. Anyway, here's a tested, tried, and correct patch. ------------------------------------------------------------------------ Wed, 07 Sep 2005 18:26:33 +0000 : m3avrck +1 patch works great! Latest HEAD version applied with update.php works great, modifies database correctly for MySQL (and PostgreSQL looks correct as well) and contacts can now be deleted if they have '&' in the subject. Also, dumped entire database and created from modified database.mysql and everything works great. I'd say this patch is ready to be committed! ------------------------------------------------------------------------ Wed, 07 Sep 2005 21:44:47 +0000 : Cvbge Hi, patch does not applies to currect cvs anymore. For the database.pgsql you need to remove (11) from int(11) and also first "category" from UNIQUE line, so it looks like this: CREATE TABLE contact ( cid int NOT NULL, category varchar(255) NOT NULL default '', recipients text NOT NULL default '', reply text NOT NULL default '', UNIQUE (category), PRIMARY KEY (cid) ); The updates are also not compatibile with postgres, but postgres upgrade path is completly broken in cvs and I'll fix it later, so do not pay attention to it. ------------------------------------------------------------------------ Thu, 08 Sep 2005 13:05:54 +0000 : Souvent22 Hello. Thanks for the feedback. I'm in the middle of fixing it, but i have a question. You'll have to excuse my ignorance with PostGres, I'm not as familiar with it as I am MySQL. With that said, I have a question..... I am trying to make my update where one does not lose data. However, in PostGres, I have to make a seperate "temp" table before hand and then copy the date over the the new table (cause i don't know the exact name of the constraint to drop e.g. the primary key). Basically I'm asking if there is a better suggestion in which I doint have to use a temp table, and i could just manipulate the original table? Thanks. ------------------------------------------------------------------------ Thu, 08 Sep 2005 13:12:35 +0000 : Souvent22 Also, I created a temp name for my temp table. However, for prefix purposes, should I enclose it? Example: $temp_name = rand(); $res = db_query("SELECT * FROM $temp_name"); or $temp_name = rand(); $res = db_query("SELECT * FROM {$temp_name}"); Thanks. ------------------------------------------------------------------------ Thu, 08 Sep 2005 13:55:40 +0000 : Souvent22 Clarification: $temp_name = rand(); db_query("CREATE TABLE $temp_name AS SELECT * FROM {contact}"); $res = db_query("SELECT * FROM $temp_name"); or $temp_name = rand(); db_query("CREATE TABLE {$temp_name} AS SELECT * FROM {contact}"); $res = db_query("SELECT * FROM {$temp_name}"); ------------------------------------------------------------------------ Thu, 08 Sep 2005 17:52:06 +0000 : Cvbge "cause i don't know the exact name of the constraint to drop e.g. the primary key " Yes, you know it, it's tablename_pkey. In this case it's contact_pkey. You can drop it by dropping index with it's name (you could drop constraint with it's name but it was not available in 7.2) Adding a column and adding unique attribute is also doable without temp table. You can look at (some) previous updates. But some of them are incorrect ;) By saying that I'll fix it later I meant that I want to introduce function to modify tables and don't want to make temporary fixes now. ------------------------------------------------------------------------ Thu, 08 Sep 2005 19:47:08 +0000 : Souvent22 Thanks. yes, you're right. didn't know if that was universal across the board though since that'st he default, but someone could give their primary key constraint a diff. name. But, with your comment, I'll wait and let you take care of fixing my pgsql portion of my patch. Thanks for your comments though, really helpful...and not confusing. :). ------------------------------------------------------------------------ Fri, 09 Sep 2005 19:16:19 +0000 : Souvent22 Attachment: http://drupal.org/files/issues/contact_full_1.patch (6.23 KB) Got the postgres correct this time. No temp table needed. ------------------------------------------------------------------------ Tue, 13 Sep 2005 14:23:09 +0000 : Souvent22 Attachment: http://drupal.org/files/issues/contact_full_2.patch (6.35 KB) patch re-roll due to head change. ------------------------------------------------------------------------ Tue, 13 Sep 2005 14:36:11 +0000 : chx Does this merit a DB change? what about passing md5(subject) instead? or is that too hackish? seems to be lot simpler to me... ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:15:16 +0000 : m3avrck chx, md5() would work but that does seem a bit hackish ;)

1 0

[drupal-devel] [task] Extend db_query()
by killes 13 Sep '05

13 Sep '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/db_query_2.patch (2.72 KB) Ok, I've included is_null() in the condition. 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. ------------------------------------------------------------------------ Tue, 30 Aug 2005 18:13:48 +0000 : killes(a)www.drop.org 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 ------------------------------------------------------------------------ Sun, 04 Sep 2005 18:13:36 +0000 : Cvbge A remark about NULL values: in this patch they are treated as integers and thus allways changed to '0'. In existing code the '' was INSERTed into text-like columns (%s) if didn't check if the variable was set (or did not care). If they now use %a and still do not check for NULLs the '0' will be inserted which migh or might not create problems (it won't matter for if()s, but maybe for other uses?) ------------------------------------------------------------------------ Sun, 04 Sep 2005 19:50:58 +0000 : killes(a)www.drop.org Cvbge: AFAIK we don't use NULL values inside the DB in Drupal core. ------------------------------------------------------------------------ Sun, 04 Sep 2005 20:56:41 +0000 : Cvbge You're probably right, then 1. not checking if a variable is set is a bug (and I've already filled a bug for one of such bugs, can't find it - sticky, moderated etc. fields were not set at all when not selected when submitting a post) 2. There's already a lot of DEFAULT NULL in the database schema... ------------------------------------------------------------------------ Fri, 09 Sep 2005 15:03:09 +0000 : Cvbge Hello, NULLs again. Previously I wrote that NULLs are treated as integers and passed with %d and converted to 0. This is of course not true [were I sick when writting it?]. They are treated as strings and converted to '' (empty string) This will create similar problems as FALSE. Maybe NULL should be treated as %d (and converted to 0) after all?

1 0

[drupal-devel] [feature] Enable customizing of "Title" field on node forms
by Thox 13 Sep '05

13 Sep '05

Issue status update for http://drupal.org/node/10056 Post a follow up: http://drupal.org/project/comments/add/10056 Project: Drupal Version: cvs Component: node system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: Anonymous Updated by: Thox Status: patch (ready to be committed) Perhaps node_validate_title() should be passed the name of the field? 'Title' by default. Thox Previous comments: ------------------------------------------------------------------------ Sat, 14 Aug 2004 18:17:11 +0000 : Anonymous Attachment: http://drupal.org/files/issues/node.set-title-field.patch (2.02 KB) The default "title" field in a node object is useful but not always appropriate. For some node types it would be preferable to rename the field for display to users (e.g., from "title" to "name" for a record of an organization or business), while in others the title might be best drawn from a combination of other fields (e.g., first_name and last_name fields, for a record of an individual). This patch enables two new alternatives for node titles: renamingThe title field can be renamed, e.g., from "title" to "name". substutingOther fields can be substituted for the title field. The functionality is implemented through a new proposed node hook, _title_field($op). There are two possible values for $op: "action" (either "rename" or "substitute") and "names" (a list of field names to be used in the title field). Sample uses: [?PHP function example_title_field($node, $op) { switch ($op) { case 'action': $return = 'rename'; break; case 'names': $return = array('Name'); } return $return; } ?] In this case, since the requested action is "rename", the title field would be labelled "Name" (instead of "Title") in node add and edit forms for nodes of type "example". [?PHP function example_title_field($node, $op) { switch ($op) { case 'action': $return = 'substitute'; break; case 'names': $return = array('first_name', 'last_name'); } return $return; } ?] In this case, no "Title" field would be displayed in node add or node edit forms of node type "example". Instead, the fields "first_name" and "last_name" would be concatenated to form a title. Implementing this patch would significantly increase the flexibility of the node system, making it feasible to store information of many types not currently supported because they don't have a "title" attribute. ------------------------------------------------------------------------ Sun, 15 Aug 2004 16:39:37 +0000 : Dries This is best accomplished through the existing nodeapi hook, using a separate module. ------------------------------------------------------------------------ Sun, 15 Aug 2004 23:02:13 +0000 : nedjo Thanks for the suggestion. I did look for a way to accomplish this through _nodeapi (and other existing hooks), but couldn't see how. The "title" field is hard-coded into node.module. The _nodeapi hook allows various types of manipulations, but not, so far as I could see, changes to elements that have been generated by, e.g., form_textfield() calls. Are there possibilities I'm missing? Other approaches? Any specific suggestions as to means we could use to change the hard-coded "title" field? ------------------------------------------------------------------------ Thu, 19 Aug 2004 18:27:15 +0000 : nedjo Attachment: http://drupal.org/files/issues/node.set-title-field_0.patch (741 bytes) Drawing on Dries's suggestion, I've substantially revised this patch to enable node title field customization with only three additional lines of code in node.module. In place of the existing line in function node_form() <?php $output .= form_textfield(t('Title'), 'title', $edit->title, 60, 128, NULL, NULL, TRUE); ?> the patch substitutes a the following: <?php $title = variable_get('node_titlefield_'. $edit->type, 'Title'); if(!($title == 'none')) { $output .= form_textfield(t($title), 'title', $edit->title, 60, 128, NULL, NULL, TRUE); } ?> With this change in place, node modules will be able to override the standard node title label by setting a variable, node_titlefield_nodetype, where nodetype is the type of node. This could be done, e.g., in a _settings hook, so that the first time settings were accessed the module would set the variable: <?php if (variable_get('node_titlefield_example', '') == '') { // since none is already set, set this variable variable_set('node_titlefield_example', 'Name'); drupal_set_message('Example module initialization completed.'); } ?> The special value 'none' would suppress the title field altogether in node forms, used as follows: <?php if (variable_get('node_titlefield_example', '') == '') { // since none is already set, set this variable variable_set('node_titlefield_example', 'none'); drupal_set_message('Example module initialization completed.'); } ?> In this case, a _validate hook would be used to set the title field value. Assuming a module where a first name-last name combination was to be used for a title: <?php function example_validate(&$node) { if(!(isset($node->title))) { $node->title = $node->first_name . ' ' . node->last_name; } } ?> In short, this three-line addition to the node.module would significantly increase the flexibility of the node system by removing the hard-coded 'Title' field in node add and edit forms and instead enabling custom labelling or concatenation of the field from other sources. ------------------------------------------------------------------------ Thu, 19 Aug 2004 18:50:42 +0000 : moshe weitzman I like this functionality. But the implementation still seems unclean. I would prefer to simply remove the 'title' form field from node_form() and require that modules present this field in whatever way they please. in some cases, they will use a hidden form field because no customer interaction is required. This causes a break with backward compatibility, but cleanliness is facored over compatibilityness (when you must choose). I'm moving this back to 'Active', since I don't think this patch is committable as is. ------------------------------------------------------------------------ Fri, 27 Aug 2004 23:46:45 +0000 : nedjo Attachment: http://drupal.org/files/issues/node.set-title-field_1.patch (830 bytes) Thanks for looking at this. The suggestion - dropping the title field altogether and leaving this for module authors - has the advantage of simplicity. However, this change would imply significant new work as it would would require all or nearly all existing modules - core and contributed - be modified. While I think enabling title field customization is important for Drupal as a whole, and while this is a change I need for two modules I'm working on, I'm not immediately convinced that the benefits of this change justify this cost. Also, in most cases, the present title field works fine, so keeping it as a default option would I feel be desirable. Hence the following patch. In place of my (poorly conceived) initial hook proposals, above, this patch introduces a single very straightforward new node hook, _title_field. By setting this hook to return FALSE, node modules authors will suppress the default title field, and be free therefore to substitute what they wish or need. Returning TRUE, or not using the hook, will result in the default title field being used. Sample usage: <?php function example_title_field() { return FALSE; } ?> This is, I feel, a much better solution than the previous two I suggested--proof, if it's true, that the Drupal issue system works! ------------------------------------------------------------------------ Mon, 06 Sep 2004 15:50:38 +0000 : nedjo Are there any objections to or issues raised by this small patch (introducing a new hook to make the title field optional in node forms)? If not, can it be applied? ------------------------------------------------------------------------ Tue, 07 Sep 2004 13:46:29 +0000 : Anonymous I think I agree with Moshe's opinion that this should be accomplished by having node modules display the title field. In any event, this is a new feature and shouldn't go in during the feature freeze for 4.5. ------------------------------------------------------------------------ Tue, 07 Sep 2004 13:58:34 +0000 : ccourtne +1 for just moving the title field to be a responsibility of the implementing module instead of node_form. There is already to some extent "hook overload", in addition there may be several modules which don't want to even display a title module. Why add that only fixes some of the problems just to have to remove it later when you make a fix that address the whole problem. ------------------------------------------------------------------------ Tue, 07 Sep 2004 21:09:53 +0000 : nedjo Attachment: http://drupal.org/files/issues/node.remove-title-field.patch (636 bytes) Thanks for comments. As per feedback, the attached patch removes the title field from node forms, leaving this instead to node modules. Understood that possible approval of this patch this will need to wait until code unfrozen. ------------------------------------------------------------------------ Sun, 24 Oct 2004 12:06:46 +0000 : moshe weitzman please patch all core node modules as well. ------------------------------------------------------------------------ Sun, 28 Nov 2004 01:00:31 +0000 : nedjo Attachment: http://drupal.org/files/issues/node-move-form-title-field.patch (4.29 KB) Good point Moshe. This patch of node.module plus all core node modules simply moves the title field out of node.module and into the node modules, enabling (if desired) customization of the title field by a module. Patches to contributed modules will in most cases be as simple as adding the line <?php $output = form_textfield(t('Subject'), 'title', $node->title, 60, 128, NULL, NULL, TRUE); ?> at the beginning of a typename_form() function (and, if necessary changing an existing $output = to $output .=). In the attached patch, I've customized the title field for only one of the node types, calling the title of a forum topic the "Subject". We could consider calling the title of a blog a "Subject" or "Topic" as well. I feel this is a good solution (thanks for the feedback and discussion) and, for minimal work (minor updates to contributed modules), will have the significant benefit of enabling flexibility in content types beyond those that have "title" attributes. ------------------------------------------------------------------------ Sun, 28 Nov 2004 17:45:35 +0000 : nedjo Ability to customize title as provided in this patch was requested here [1]: "When adding/editing, the 'title' field is manditory, which is fine. Except that its label is hardcoded to 'title'. A custom node modelling, say, a 'Company' would much rather this field be presented to the user as 'Company Name'. " [1] http://drupal.org/node/13596 ------------------------------------------------------------------------ Fri, 04 Mar 2005 03:44:55 +0000 : nedjo Here's another patch that's sat for months without being either applied or turned down, although there was expressed support and, after several iterations, it addressed all issues that had been raised. The point is to remove the hard-coded "Title" field - inapprioriate for many node types, e.g., companies - from node forms, instead allowing node modules to treat this as appropriate (e.g., a "Subject", "Name" or "Topic", or nothing at all). Are there problems with this approach that were not raised? Is there a reason this wasn't accepted? ------------------------------------------------------------------------ Fri, 04 Mar 2005 03:54:02 +0000 : Steven nedjo: if you read the backlog you can see that this feature was delayed for after the 4.6 release. "I've customized the title field for only one of the node types, calling the title of a forum topic the "Subject". " This is really inconsistent, as what you call the title of a blog, story or forum node is purely subjective. "Subject" "Topic" and "Title" are pretty much interchageable and any subtle differences will surely be eroded by translation. The only thing I can agree on so far is naming a polls' title "Question". ------------------------------------------------------------------------ Fri, 04 Mar 2005 05:15:03 +0000 : nedjo Thanks for the note. I'm not sure what you mean by the "backlog". Where would I find this? I agree that a "Question" field would make sense for polls. ------------------------------------------------------------------------ Fri, 04 Mar 2005 14:12:56 +0000 : JonBob Actually UnConeD, it was postponed until after the 4.5 release, not 4.6, and looks like everyone forgot about it when the release happened. And no, Nedjo, I don't take this as proof that Drupal needs more bureaucracy. :-) Marking this active again; I'm sure the patch will have to be updated once the code branches. We had rolled this function into the needs for CCK, but in our discussions split it off again. ------------------------------------------------------------------------ Sun, 03 Apr 2005 05:28:08 +0000 : chx Attachment: http://drupal.org/files/issues/node-move-form-title-field_0.patch (4.59 KB) The patch applies to ucrrent HEAD with some offset. I have rerolled it so it applies cleanly. ------------------------------------------------------------------------ Fri, 20 May 2005 10:09:50 +0000 : chx Patch still applies (yes, with some offset, but applies). ANd yes, we need this very badly. ------------------------------------------------------------------------ Fri, 20 May 2005 10:18:49 +0000 : Thox I'd benefit greatly from having a feature like this. The example already given is one of the cases I need it: I'd like the title field to be a concatenation of two fields: "First name" and "Last name". ------------------------------------------------------------------------ Tue, 24 May 2005 03:39:40 +0000 : gsperk I wonder if someone could help me with the following problem. After applying the patch I made the output for 'title' in my own_node.module this: $output .= form_hidden('title', $node->firstname . ' ' . $node->surname); When the form is first submitted I get the error 'You have to specify a title'. When I submit again the firstname/surname variables are obviously in $node, and the form submits with the desired title. So, I suppose my question is, how can get the values for firstname and surname into $node without submitting twice? (If it's not obvious I can confirm that I dont have much experience with this kind of stuff. Any suggestions would be a big help.) ------------------------------------------------------------------------ Wed, 25 May 2005 04:23:41 +0000 : Steven To make this patch work you also need to move the title validation out of node.module and into the node modules. I don't think this is that much of a problem, it is code that is not going to be touched much. Also, in light of the upcoming CCK it makes sense to do it like this. ------------------------------------------------------------------------ Sat, 02 Jul 2005 17:02:26 +0000 : hanoii Nice thread. I have come up with a slightly different approach I think that would need less patching/updating of other modules. Drupal 4.6.2 node.module:1275 <?php // Get the node-specific bits. // We can't use node_invoke() because $param must be passed by reference. $function = node_get_module_name($edit) .'_form'; $param = array(); if (function_exists($function)) { $form .= $function($edit, $param, $title); } ?> Notice the $title param on the hook_form invoke. This was what I added. As that hook is called like this an not with the node_invoke() I can define on my custom node module, or patch any module I need to modify the title description with the following hook: <?php function budget_form(&$node, &$params, &$title) { ?> and modify the $title argument there, as it is by reference, It will me modified on node.module. Then again on node.module:1319 I added/changed the following: <?php if ( ! $title ) { $title = t('Title') ; } $output .= form_textfield($title, 'title', $edit->title, 60, 128, NULL, NULL, TRUE); ?> So, if the module modify the title var, it uses that, if not, it uses the default one. Worked for me so far, nice to post on drupal. Greetings, a.= ------------------------------------------------------------------------ Sat, 02 Jul 2005 18:10:28 +0000 : hanoii I posted too soon the thing before... :) I did further patches because of the validation keep telling that I have tu put a "title". I was happy with the previous patch, not so much with this one, but I let you know what I did... on the node.module, node_validate() function I move the title validation block just after the <?php // Do node-type-specific validation checks. node_invoke($node, 'validate'); node_invoke_nodeapi($node, 'validate'); ?> and added another parameter to the form_set_error() <?php // Validate the title field. if (isset($node->title)) { if (trim($node->title) == '') { form_set_error('title', t('You have to specify a title.'), TRUE); } } ?> then, on the form_set_error function in common.inc I did the following: <?php /** * File an error against the form element with the specified name. */ function form_set_error($name, $message, $checkexistance = FALSE ) { if ( !$checkexistance || $checkexistance && !isset($GLOBALS['form'][$name]) ) { $GLOBALS['form'][$name] = $message; drupal_set_message($message, 'error'); } } ?> So now, if any previous title error was added, and the $checkexistance flag is TRUE, no error will be added to the message queue. The moving place of the validation block was to let the modules validate first than the node module. Errrr.. I don't like it, but again, it worked :) a.= ------------------------------------------------------------------------ Mon, 01 Aug 2005 00:54:22 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/title_0.patch (4.56 KB) I've updated the patch to apply to current cvs. As progress on the CCK has been rather slow, I'd appreciate if somebody could address Steven's concerns and the patch could be applied. ------------------------------------------------------------------------ Tue, 16 Aug 2005 19:19:23 +0000 : Thox Attached is a patch with an attempt to move the title validation code into the node modules (from node.module). ------------------------------------------------------------------------ Tue, 16 Aug 2005 19:20:21 +0000 : Thox Attachment: http://drupal.org/files/issues/title-validation.patch (6.22 KB) attached again (issue preview seems busted?) ------------------------------------------------------------------------ Wed, 24 Aug 2005 18:57:26 +0000 : nedjo Thanks chx, killes, and Thox for updates and fixes on the patch. I've applied and tested it and Thox's changes seem to address the title validation issue. It's a couple of releases later--it would be great to see this fix applied! ------------------------------------------------------------------------ Wed, 24 Aug 2005 23:17:25 +0000 : mathias Attachment: http://drupal.org/files/issues/title-validation_0.patch (6.04 KB) +1 I've needed to change the name of the title field or even altogether hide it for custom modules. Issue preview was working for me. We should document for module developers the implications title-less nodes (e.g., less informative watchdog entries, harder to edit nodes, node title lists become buggy, etc). I couldn't get the patch to apply cleanly to HEAD so here's a new one. ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:42:30 +0000 : Robrecht Jacques Attachment: http://drupal.org/files/issues/title-validation_1.patch (6.23 KB) +1 I need this too: I want to be able to set the title of a node automatically. Patch still applies with offset (rerolled patch attached). ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:59:39 +0000 : fago +1 for this functionality i've tested the patch and it looks nice, however node_validate_title() produces an validation error, where the field is called title, which isn't suitable if the modules changes the name, as the forum.module does.

1 0

[drupal-devel] [feature] Prevent accidentally navigating away from pages where content has changed
by m3avrck 13 Sep '05

13 Sep '05

Issue status update for http://drupal.org/node/30220 Post a follow up: http://drupal.org/project/comments/add/30220 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: m3avrck Updated by: m3avrck -Status: patch (code needs review) +Status: patch (ready to be committed) Well I'm gonna set this to commit then. Tested and working on IE, FF, Safari. Doesn't work or break Opera or Konqueror but not needed in these cases. Thox and Unconded have offered thoughts and I've modified code as needed. Doesn't seem like there is anything else left except a commit :) m3avrck Previous comments: ------------------------------------------------------------------------ Thu, 01 Sep 2005 22:47:42 +0000 : m3avrck How many times have you been posting a comment or working on a new post when you click on a link in another application and you navigate away from the content you are editing, losing all of your changes? Well I say we introduce a script, much similiar to that of Blogger, that uses the Javascript window.onbeforeunload handler to prevent this from happening. We can set this handler to call a function that compares the contents of all forms when the page loads and then quickly compares that to the current contents just before the user is about to leave the page. If they have changed, we should prompt the user so they don't lose their work. This would save many headaches and degrade nicely for users without Javascript. Additionally, this script should work with the newly introduced JS-based upload feature to prevent navigating away as a file is being uploaded as well. If I have sometime I'll start work on a patch tomorrow, just wanted to get the idea into the queue right now :) ------------------------------------------------------------------------ Sat, 10 Sep 2005 23:48:33 +0000 : StuartDH Sounds like a great feature to me. We regularly get authors and editors telling us that they've lost the last hour of work by accidentally navigating away, and I've even just had one of them email me about it a few minutes ago, so it'd be great if you could put something together for this. Cheers ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:33:58 +0000 : MichelleC Sounds great to me, too. I often have several tabs open doing stuff and it's easy to lose my place and leave a page with unfinished edits. Would this alert you when you're going to close an unsaved tab/window as well? Michelle ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:34:19 +0000 : MrMattles Great for us multitaskers that rush and click the wrong thing ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:36:52 +0000 : chx Very strong -1 as this is IE only. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:38:40 +0000 : praseodymium Konqueror already does this, +1 for the idea in general. ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:07:21 +0000 : webchick Yeah, I think chx is correc that onbeforeunload is an extension developed by MS [1] (it's not in the ECMAScript specification [2]. However, I found out that this is implemented in Mozilla since 1.7 [3] (which corresponds to Firefox 1.0, I think?) and I've tested Blogger.com's version of this functionality in: Firefox 1.0.6 (working) Firefox Deer Park Alpha 1 (working) Opera 8.0.2 (working) Safari 1.3 (does NOT work) Safari 2.0.1 (working) So it seems to be a 'de facto' standard if nothing else. [1] http://msdn.microsoft.com/workshop/author/dhtml/reference/events/onbeforeun… [2] http://www.ecma-international.org/publications/standards/Ecma-262.htm [3] http://www.mozilla.org/status/2004-03-01.html ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:09:15 +0000 : webchick Also, I should clarify... that "does NOT work" in Safari doesn't display any errors or anything, it just doesn't save the form results as expected (so pre-Tiger Safari users are going to be used to this). So I don't see any harm in including this functionality since it seems to degrade gracefully in non-supporting browsers. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:10:18 +0000 : m3avrck Attachment: http://drupal.org/files/issues/node.module_11.patch (951 bytes) And a patch is ready! Code was based on many example, including this [4] and this [5] along with adding my own thoughts and what not ;) [4] http://www.codestore.org/store.nsf/cmnts/451FA051398A9AF486256EE0000FB7D1?O… [5] http://www.blogger.com/app/scripts/formcheck.js ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:10:45 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck.js (1.54 KB) And here is the JS to throw in misc/. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:22:56 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_0.js (1.61 KB) Better JS file attached without tabs. Also, tested and working great in FF and IE6. Doesn't work in Opera 8, however no errors are present, I believe this is just because the event handler is not supported. If there is a work around for Opera, please let me know! ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:24:18 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_1.js (1.61 KB) One more try with those tabs. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:32:48 +0000 : moshe weitzman hmmm. i usually find these prompts annoying. any chance we can attach the behavior only to the node and comment forms? those are the "high risk" areas. what do others think? ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:42:39 +0000 : m3avrck Yes, the JS is only loaded on the node edit/create pages, won't affect any admin/login/etc form. Also, the prompt is unobstrusive and if you goto save a node you don't get a warning that the contents have changed because you are explicity saving the node (hence no reason!). ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:53:01 +0000 : Dries I want some JS-folks to review the code. ------------------------------------------------------------------------ Mon, 12 Sep 2005 22:09:10 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_15.patch (1.94 KB) After talking with Drumm on IRC, this patch makes the integration with Drupal much simpler and it's off by default. Only turned on for the node edit/change page right now but any module/form can easily add this by passing a 'TRUE' parameter. ------------------------------------------------------------------------ Mon, 12 Sep 2005 23:38:35 +0000 : nedjo +1 on idea, js looks generally good, a few suggestions: 1. Instead of writing onsubmit via PHP, use a js addLoadEvent call. 2. Shouldn't the return false in isElementChanged be at the end (outside the switch block)? 3. It's be nice to find a way to make messages like 'You have unsaved changes.' translatable. Pass in global js variables via a t('') call? 4. onbeforeunload event should probably be in a if isJsEnabled test, and should parallel drupal.js's event adding (see addLoadEvent). 5. Comments should be in standard format and in present tense, e.g., /** * Checks to see if a form has been changed after the page loads */ 6. e_ should be just e to match other js files, e.g., autocomplete. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:54:35 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_2.js (1.58 KB) Ok here is an updated JS file. A few notes, I couldn't get the addSubmitEvent() in drupa.js to reliably work so I had to set the isSubmit() true in the PHP creation of the form. If you look at Blogger.com, they do this as well... so either we both missed an obvious way to do this, or that is the most practical. Hopefully some wise JS gurus will chime in with an answer. Same goes for onbeforeunload event, which is completely different then addEvent defined in drupal.js. As for the t('') I agree this would be useful but I'm not sure of the best way to do this. One way would be to put in form() a t('') passed in, and then write this to a var in formcheck.php which returns a JS file type. Thoughts? ------------------------------------------------------------------------ Tue, 13 Sep 2005 14:06:44 +0000 : Junyor FWIW, Opera doesn't support the onbeforeunload event. However, this issue doesn't affect Opera anyway: form contents are retained in history as long as the page isn't closed. IOW, this feature doesn't work in Opera, but it isn't needed either. ------------------------------------------------------------------------ Tue, 13 Sep 2005 14:15:56 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_3.js (1.64 KB) Updated JS styling and JS-killswitch after talking with Unconed on IRC. ------------------------------------------------------------------------ Tue, 13 Sep 2005 14:39:15 +0000 : m3avrck Just to build on Junyor's comment [6] this functionality is built into Opera 8 and it *doesn't* produce any errors. [6] http://drupal.org/#comment-44066 ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:04:37 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_16.patch (2.1 KB) Updated patch to fix possible problem of overwriting onsubmit event handler thanks to Thox on IRC. ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:13:59 +0000 : webchick I tested this and can confirm that it works in *both* versions of Safari that I have access to: 2.0.1 (OS X Tiger) and 1.3 (OS X Panther). So looks like this code goes one better over the Blogger.com stuff, because their stuff doesn't workk in 1.3. Nice job! ;)

1 0

[drupal-devel] [bug] Can't delete subjects with '&' in title
by m3avrck 13 Sep '05

13 Sep '05

Issue status update for http://drupal.org/node/27140 Post a follow up: http://drupal.org/project/comments/add/27140 Project: Drupal Version: cvs Component: contact.module Category: bug reports Priority: normal Assigned to: Souvent22 Reported by: m3avrck Updated by: m3avrck Status: patch (ready to be committed) chx, md5() would work but that does seem a bit hackish ;) m3avrck Previous comments: ------------------------------------------------------------------------ Wed, 20 Jul 2005 14:57:03 +0000 : m3avrck Using the site-wide contact forms, any subjects with an '&' cannot be deleted. Clicking delete for these subjects brings up the expected action flow, however, confirming the delete does nothing. Subject still exists. ------------------------------------------------------------------------ Mon, 25 Jul 2005 13:45:38 +0000 : m3avrck It appears this issue might be linked with this one: http://drupal.org/node/23685 A problem with mod_rewrite and not the actual contact module. ------------------------------------------------------------------------ Wed, 31 Aug 2005 16:51:56 +0000 : Souvent22 Attachment: http://drupal.org/files/issues/contact.module_6.patch (4.7 KB) Redid how contacts work. It now references contacts by ID instead of their category as the unique identifier. It also allows for contacts to have weights, so that one may control where the contact will appear in the listing. ------------------------------------------------------------------------ Wed, 31 Aug 2005 16:53:01 +0000 : Souvent22 Attachment: http://drupal.org/files/issues/updates.inc (27.23 KB) This is the update.inc for the database cahnges that goes along with this patch. ------------------------------------------------------------------------ Wed, 31 Aug 2005 18:31:37 +0000 : Souvent22 Attachment: http://drupal.org/files/issues/contact.module_7.patch (4.5 KB) Updated patch. ------------------------------------------------------------------------ Wed, 31 Aug 2005 18:31:55 +0000 : Souvent22 Attachment: http://drupal.org/files/issues/updates.inc_4.patch (1.29 KB) Updated database patch. ------------------------------------------------------------------------ Sat, 03 Sep 2005 16:17:21 +0000 : Cvbge Hi, 0. You should keep all changes in one file, i.e. pake one patch, not patch for every file 1. You should include changes for database/database.{mysql,pgsql} 2. IIRC auto_increment was depreciated, you should use db_next_id() (but I see auto_increment in cvs...) 3. You use "WHERE cid = '%s'", but cid is integer so this should probably be %d (although this probably would not break anything..) ------------------------------------------------------------------------ Wed, 07 Sep 2005 17:31:21 +0000 : Souvent22 Attachment: http://drupal.org/files/issues/contact_full.patch (6.38 KB) Ok, I've taken your suggestions in, and i think I have it this time. Let me know if this is a correct format for a patch. This only fixes the bug of not being able to delete special characters. ------------------------------------------------------------------------ Wed, 07 Sep 2005 17:37:30 +0000 : m3avrck Check your database updates, the UNIQUE fields don't look right syntactically. ------------------------------------------------------------------------ Wed, 07 Sep 2005 18:23:07 +0000 : Souvent22 Attachment: http://drupal.org/files/issues/contact_full_0.patch (6.25 KB) Ok, i apologize for the last upload. Pleasle ignore. I send the wrong file. :-[. Anyway, here's a tested, tried, and correct patch. ------------------------------------------------------------------------ Wed, 07 Sep 2005 18:26:33 +0000 : m3avrck +1 patch works great! Latest HEAD version applied with update.php works great, modifies database correctly for MySQL (and PostgreSQL looks correct as well) and contacts can now be deleted if they have '&' in the subject. Also, dumped entire database and created from modified database.mysql and everything works great. I'd say this patch is ready to be committed! ------------------------------------------------------------------------ Wed, 07 Sep 2005 21:44:47 +0000 : Cvbge Hi, patch does not applies to currect cvs anymore. For the database.pgsql you need to remove (11) from int(11) and also first "category" from UNIQUE line, so it looks like this: CREATE TABLE contact ( cid int NOT NULL, category varchar(255) NOT NULL default '', recipients text NOT NULL default '', reply text NOT NULL default '', UNIQUE (category), PRIMARY KEY (cid) ); The updates are also not compatibile with postgres, but postgres upgrade path is completly broken in cvs and I'll fix it later, so do not pay attention to it. ------------------------------------------------------------------------ Thu, 08 Sep 2005 13:05:54 +0000 : Souvent22 Hello. Thanks for the feedback. I'm in the middle of fixing it, but i have a question. You'll have to excuse my ignorance with PostGres, I'm not as familiar with it as I am MySQL. With that said, I have a question..... I am trying to make my update where one does not lose data. However, in PostGres, I have to make a seperate "temp" table before hand and then copy the date over the the new table (cause i don't know the exact name of the constraint to drop e.g. the primary key). Basically I'm asking if there is a better suggestion in which I doint have to use a temp table, and i could just manipulate the original table? Thanks. ------------------------------------------------------------------------ Thu, 08 Sep 2005 13:12:35 +0000 : Souvent22 Also, I created a temp name for my temp table. However, for prefix purposes, should I enclose it? Example: $temp_name = rand(); $res = db_query("SELECT * FROM $temp_name"); or $temp_name = rand(); $res = db_query("SELECT * FROM {$temp_name}"); Thanks. ------------------------------------------------------------------------ Thu, 08 Sep 2005 13:55:40 +0000 : Souvent22 Clarification: $temp_name = rand(); db_query("CREATE TABLE $temp_name AS SELECT * FROM {contact}"); $res = db_query("SELECT * FROM $temp_name"); or $temp_name = rand(); db_query("CREATE TABLE {$temp_name} AS SELECT * FROM {contact}"); $res = db_query("SELECT * FROM {$temp_name}"); ------------------------------------------------------------------------ Thu, 08 Sep 2005 17:52:06 +0000 : Cvbge "cause i don't know the exact name of the constraint to drop e.g. the primary key " Yes, you know it, it's tablename_pkey. In this case it's contact_pkey. You can drop it by dropping index with it's name (you could drop constraint with it's name but it was not available in 7.2) Adding a column and adding unique attribute is also doable without temp table. You can look at (some) previous updates. But some of them are incorrect ;) By saying that I'll fix it later I meant that I want to introduce function to modify tables and don't want to make temporary fixes now. ------------------------------------------------------------------------ Thu, 08 Sep 2005 19:47:08 +0000 : Souvent22 Thanks. yes, you're right. didn't know if that was universal across the board though since that'st he default, but someone could give their primary key constraint a diff. name. But, with your comment, I'll wait and let you take care of fixing my pgsql portion of my patch. Thanks for your comments though, really helpful...and not confusing. :). ------------------------------------------------------------------------ Fri, 09 Sep 2005 19:16:19 +0000 : Souvent22 Attachment: http://drupal.org/files/issues/contact_full_1.patch (6.23 KB) Got the postgres correct this time. No temp table needed. ------------------------------------------------------------------------ Tue, 13 Sep 2005 14:23:09 +0000 : Souvent22 Attachment: http://drupal.org/files/issues/contact_full_2.patch (6.35 KB) patch re-roll due to head change. ------------------------------------------------------------------------ Tue, 13 Sep 2005 14:36:11 +0000 : chx Does this merit a DB change? what about passing md5(subject) instead? or is that too hackish? seems to be lot simpler to me...

1 0

[drupal-devel] [feature] Prevent accidentally navigating away from pages where content has changed
by webchick 13 Sep '05

13 Sep '05

Issue status update for http://drupal.org/node/30220 Post a follow up: http://drupal.org/project/comments/add/30220 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: Anonymous Reported by: m3avrck Updated by: webchick Status: patch (code needs review) I tested this and can confirm that it works in *both* versions of Safari that I have access to: 2.0.1 (OS X Tiger) and 1.3 (OS X Panther). So looks like this code goes one better over the Blogger.com stuff, because their stuff doesn't workk in 1.3. Nice job! ;) webchick Previous comments: ------------------------------------------------------------------------ Thu, 01 Sep 2005 22:47:42 +0000 : m3avrck How many times have you been posting a comment or working on a new post when you click on a link in another application and you navigate away from the content you are editing, losing all of your changes? Well I say we introduce a script, much similiar to that of Blogger, that uses the Javascript window.onbeforeunload handler to prevent this from happening. We can set this handler to call a function that compares the contents of all forms when the page loads and then quickly compares that to the current contents just before the user is about to leave the page. If they have changed, we should prompt the user so they don't lose their work. This would save many headaches and degrade nicely for users without Javascript. Additionally, this script should work with the newly introduced JS-based upload feature to prevent navigating away as a file is being uploaded as well. If I have sometime I'll start work on a patch tomorrow, just wanted to get the idea into the queue right now :) ------------------------------------------------------------------------ Sat, 10 Sep 2005 23:48:33 +0000 : StuartDH Sounds like a great feature to me. We regularly get authors and editors telling us that they've lost the last hour of work by accidentally navigating away, and I've even just had one of them email me about it a few minutes ago, so it'd be great if you could put something together for this. Cheers ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:33:58 +0000 : MichelleC Sounds great to me, too. I often have several tabs open doing stuff and it's easy to lose my place and leave a page with unfinished edits. Would this alert you when you're going to close an unsaved tab/window as well? Michelle ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:34:19 +0000 : MrMattles Great for us multitaskers that rush and click the wrong thing ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:36:52 +0000 : chx Very strong -1 as this is IE only. ------------------------------------------------------------------------ Mon, 12 Sep 2005 14:38:40 +0000 : praseodymium Konqueror already does this, +1 for the idea in general. ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:07:21 +0000 : webchick Yeah, I think chx is correc that onbeforeunload is an extension developed by MS [1] (it's not in the ECMAScript specification [2]. However, I found out that this is implemented in Mozilla since 1.7 [3] (which corresponds to Firefox 1.0, I think?) and I've tested Blogger.com's version of this functionality in: Firefox 1.0.6 (working) Firefox Deer Park Alpha 1 (working) Opera 8.0.2 (working) Safari 1.3 (does NOT work) Safari 2.0.1 (working) So it seems to be a 'de facto' standard if nothing else. [1] http://msdn.microsoft.com/workshop/author/dhtml/reference/events/onbeforeun… [2] http://www.ecma-international.org/publications/standards/Ecma-262.htm [3] http://www.mozilla.org/status/2004-03-01.html ------------------------------------------------------------------------ Mon, 12 Sep 2005 15:09:15 +0000 : webchick Also, I should clarify... that "does NOT work" in Safari doesn't display any errors or anything, it just doesn't save the form results as expected (so pre-Tiger Safari users are going to be used to this). So I don't see any harm in including this functionality since it seems to degrade gracefully in non-supporting browsers. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:10:18 +0000 : m3avrck Attachment: http://drupal.org/files/issues/node.module_11.patch (951 bytes) And a patch is ready! Code was based on many example, including this [4] and this [5] along with adding my own thoughts and what not ;) [4] http://www.codestore.org/store.nsf/cmnts/451FA051398A9AF486256EE0000FB7D1?O… [5] http://www.blogger.com/app/scripts/formcheck.js ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:10:45 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck.js (1.54 KB) And here is the JS to throw in misc/. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:22:56 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_0.js (1.61 KB) Better JS file attached without tabs. Also, tested and working great in FF and IE6. Doesn't work in Opera 8, however no errors are present, I believe this is just because the event handler is not supported. If there is a work around for Opera, please let me know! ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:24:18 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_1.js (1.61 KB) One more try with those tabs. ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:32:48 +0000 : moshe weitzman hmmm. i usually find these prompts annoying. any chance we can attach the behavior only to the node and comment forms? those are the "high risk" areas. what do others think? ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:42:39 +0000 : m3avrck Yes, the JS is only loaded on the node edit/create pages, won't affect any admin/login/etc form. Also, the prompt is unobstrusive and if you goto save a node you don't get a warning that the contents have changed because you are explicity saving the node (hence no reason!). ------------------------------------------------------------------------ Mon, 12 Sep 2005 21:53:01 +0000 : Dries I want some JS-folks to review the code. ------------------------------------------------------------------------ Mon, 12 Sep 2005 22:09:10 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_15.patch (1.94 KB) After talking with Drumm on IRC, this patch makes the integration with Drupal much simpler and it's off by default. Only turned on for the node edit/change page right now but any module/form can easily add this by passing a 'TRUE' parameter. ------------------------------------------------------------------------ Mon, 12 Sep 2005 23:38:35 +0000 : nedjo +1 on idea, js looks generally good, a few suggestions: 1. Instead of writing onsubmit via PHP, use a js addLoadEvent call. 2. Shouldn't the return false in isElementChanged be at the end (outside the switch block)? 3. It's be nice to find a way to make messages like 'You have unsaved changes.' translatable. Pass in global js variables via a t('') call? 4. onbeforeunload event should probably be in a if isJsEnabled test, and should parallel drupal.js's event adding (see addLoadEvent). 5. Comments should be in standard format and in present tense, e.g., /** * Checks to see if a form has been changed after the page loads */ 6. e_ should be just e to match other js files, e.g., autocomplete. ------------------------------------------------------------------------ Tue, 13 Sep 2005 13:54:35 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_2.js (1.58 KB) Ok here is an updated JS file. A few notes, I couldn't get the addSubmitEvent() in drupa.js to reliably work so I had to set the isSubmit() true in the PHP creation of the form. If you look at Blogger.com, they do this as well... so either we both missed an obvious way to do this, or that is the most practical. Hopefully some wise JS gurus will chime in with an answer. Same goes for onbeforeunload event, which is completely different then addEvent defined in drupal.js. As for the t('') I agree this would be useful but I'm not sure of the best way to do this. One way would be to put in form() a t('') passed in, and then write this to a var in formcheck.php which returns a JS file type. Thoughts? ------------------------------------------------------------------------ Tue, 13 Sep 2005 14:06:44 +0000 : Junyor FWIW, Opera doesn't support the onbeforeunload event. However, this issue doesn't affect Opera anyway: form contents are retained in history as long as the page isn't closed. IOW, this feature doesn't work in Opera, but it isn't needed either. ------------------------------------------------------------------------ Tue, 13 Sep 2005 14:15:56 +0000 : m3avrck Attachment: http://drupal.org/files/issues/formcheck_3.js (1.64 KB) Updated JS styling and JS-killswitch after talking with Unconed on IRC. ------------------------------------------------------------------------ Tue, 13 Sep 2005 14:39:15 +0000 : m3avrck Just to build on Junyor's comment [6] this functionality is built into Opera 8 and it *doesn't* produce any errors. [6] http://drupal.org/#comment-44066 ------------------------------------------------------------------------ Tue, 13 Sep 2005 15:04:37 +0000 : m3avrck Attachment: http://drupal.org/files/issues/drupal_16.patch (2.1 KB) Updated patch to fix possible problem of overwriting onsubmit event handler thanks to Thox on IRC.

1 0