July 2005 - Development - lists.drupal.org

[drupal-devel] [task] Remove a testing line of code
by drumm 27 Jul '05

27 Jul '05

Issue status update for http://drupal.org/node/27614 Post a follow up: http://drupal.org/project/comments/add/27614 Project: Drupal Version: cvs Component: base system Category: tasks Priority: minor Assigned to: drumm Reported by: drumm Updated by: drumm Status: patch Attachment: http://drupal.org/files/issues/bootstrap.inc.diff (697 bytes) Don't need this line. drumm

1 0

[drupal-devel] [bug] Put revisions in their own table
by killes 27 Jul '05

27 Jul '05

Issue status update for http://drupal.org/node/7582 Post a follow up: http://drupal.org/project/comments/add/7582 Project: Drupal Version: cvs Component: node system Category: bug reports 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 Attachment: http://drupal.org/files/issues/revisions_31.patch (48.78 KB) @drumm: the timeout isn't usually the problem. Memory consumption is. I'll look into doing the updates in chunks of 1000 nodes or so. Clousseau found a bug in the patch, updated. killes(a)www.drop.org Previous comments: ------------------------------------------------------------------------ Wed, 05 May 2004 16:25:27 +0000 : killes(a)www.drop.org Currently all node revisions are stored in a serialized field in node.table and retrieved for _each_ page view although they are rarely needed. However, we have agreed that serializing data is bad and that we should try to keep the memory foot print pf Drupal small. Therefore I propose to create a separate revisions table which would be in principle identical to the node table, only that it could have several old copies of the same node. Extra data added by other modules could be added in a serialized field unless we find a better solution. ------------------------------------------------------------------------ Wed, 05 May 2004 17:06:35 +0000 : jhriggs I too think the serialized approach is less than desirable, but here's an alternative. This would likely take some considerable rework in core and contrib, but the following is how we handle similar types of situations in our databases at work. It is more elegant that a separate table, and avoids the (almost exact) duplication of a table. Instead of separate tables, keep all revisions of nodes in the node table as follows: * add field: active (0/1 or Y/N) * add field: revision * every revision of a node is stored in the node table; however, only one revision can be active at any given time * nid can no longer be unique -- primary/unique key becomes (nid, active) * any time a node is loaded, updated (without revision), etc., the active version is used. Thoughts? ------------------------------------------------------------------------ Wed, 05 May 2004 17:57:48 +0000 : killes(a)www.drop.org I am not opposed to your scheme, but I want to stress the following: * Duplicating a table's structure is not bad (IMHO) as long as the content is different. * having two tables will allow us to have a rather small node table. This is (maybe) a performance gain. ------------------------------------------------------------------------ Wed, 05 May 2004 18:37:29 +0000 : jhriggs I don't necessarily think that duplicating a table's structure is _bad_. It just seems to be wasteful and a pain to maintain. (Every change to the node table is made twice...easy to do, but also easy to miss perhaps.) As for performance, as long as nid and the active indicator are indexed, there shouldn't be any performance loss. Also, archiving an old version when making a new revision will be much simpler: just change the active indicator rather than copying an entire node to another table (and ensuring everything gets copied...again a potential maintainance issue). To be honest, I would just like to see the serialized data go away, regarless of what approach is taken. ------------------------------------------------------------------------ Fri, 30 Jul 2004 19:49:33 +0000 : Nick Nassar Attachment: http://drupal.org/files/issues/Drupal-Improved_Revision_Schema_07-30-2004.p… (10.47 KB) I'm interested in using Drupal for a large scale wiki-type project. In order to do this, I need revisions to be in their own table. Attached is a patch to do just that. Most of the changes are pretty self explanatory. Spreading out node data across two tables meant that I had to add database functions to do locking/transactions. Without this, race conditions in which the database becomes corrupted are possible. ------------------------------------------------------------------------ Fri, 30 Jul 2004 19:54:36 +0000 : Nick Nassar Oh yeah... The patch is a diff against Drupal CVS ------------------------------------------------------------------------ Sat, 31 Jul 2004 00:00:08 +0000 : Anonymous Gerhard speaking. Nick, thanks a lot for your nice patch! It saves me a great deal of labour. I looked through it and immediately liked it. You not only put the old revisions into a new table but also the current one. Do you have an estimate how much more expensive the additional join is? Besides a few minor coding style issues I found a major one: Just a few hours before you uploaded your patch JonBob's node access patch hit core. That means your patch won't apply anymore as all the queries you change have been changed. Can I bug you to update your patch? ------------------------------------------------------------------------ Sat, 31 Jul 2004 01:11:59 +0000 : Anonymous Also I think that your upgrade path loses existing revisions. ------------------------------------------------------------------------ Sat, 31 Jul 2004 02:39:12 +0000 : drumm I think this is the proper way to do things. No columns are duplicated, there is no serialized data, and only the fields that are logically revised are stored. Nothing jumped out at me as a way to have my node modules be able to keep a table of revisions of additional fields. I'm guessing this could be done within the confines of _insert and _update. Assuming the upgrade path works and modules can extend it I give it a +1. ------------------------------------------------------------------------ Sat, 31 Jul 2004 14:40:15 +0000 : Nick Nassar It figures that just as I finish a big patch, another patch comes along and breaks it. Oh well, it should be a pretty easy to fix. I'll work on it. Fixing the upgrade path to keep revisions should be fairly painless. I found another issue that needs to be fixed before this patch gets merged. There format of a node needs to be stored for each revision. Otherwise, for modules that store a format for the nodes, such as page and book, if you write one revision in PHP and the next in HTML, the PHP revision will be displayed as HTML. This is part of a larger issue of how node modules should store revisions of additional fields. I think each module that wants to do this should create another table with (nid, revid) as the primary key. Just as when they want to add fields to a node they create another table with nid as the primary key. As far as performance goes, for sites that make heavy use of revisions, an extra join on primary keys is going to be a lot faster than grabbing all of the revisions from that database everytime. We would need to run benchmarks to determine is the overall difference in speed is for an average site is a gain or a loss. I'm guessing it's very minor either way. ------------------------------------------------------------------------ Mon, 23 Aug 2004 13:55:49 +0000 : Nick Nassar Attachment: http://drupal.org/files/issues/Drupal-Improved_Revision_Schema_08-23-2004.p… (10.92 KB) Here's an updated patch against CVS that puts revisions in their own table, provides an upgrade path, and fixes the format related bugs in the last patch. Hopefully, this can make it into CVS as soon as the freeze is over. ------------------------------------------------------------------------ Mon, 23 Aug 2004 14:10:39 +0000 : moshe weitzman Interesting patch ... drumm's question is still outstanding. how do modules store revisions of their fields? Are they expected to manage this on their own? Thats not how it works today. As an aside, i am seeing profile_ fields in my node.revisions column. One could argue that those need not be saved. They pertain to the node author, not to the node itself. ------------------------------------------------------------------------ Mon, 23 Aug 2004 16:14:39 +0000 : Nick Nassar Having modules be responsible for storing revisions of their own fields is a side-effect of storing revision data in tables. There's really no way around it. However, revisions generally don't make sense for node types that don't have PHP/HTML content, such as polls. I think it's going to be a pretty rare scenario for a new node type to want another field to change per-revision, so it's a pretty good trade-off. Storing fields that shouldn't be part of revisions, such as the profile_ fields, is a side-effect of storing revisions as serialized objects. Applying this patch will free up that wasted space. :) ------------------------------------------------------------------------ Mon, 23 Aug 2004 17:20:57 +0000 : Anonymous There should be a hook that let's the module choose whether it supports history. This way a module author can prevent the user from doing something that may break his module or just cause undefined behavior. If the module doesn't support history then don't let the user/admin choose to add history to nodes of that type. Craig ------------------------------------------------------------------------ Mon, 23 Aug 2004 19:23:29 +0000 : Nick Nassar I agree, there should be an API change to make specifying support for revisions easier. In the interests of keeping patches small and keeping to one change per patch, I think the API change should be a separate issue. A sort of ad-hoc API to decide whether or not a module supports revisions by default already exists. Instead of having a hook, modules set the default value of the "Create new revision" field in the edit form. The admin can change this option in admin/node/configure/defaults. This patch doesn't change that. Revisions are broken for node types that have their own database structure, like polls, even when storing them as serialized objects. This patch doesn't change that, either. ------------------------------------------------------------------------ Tue, 26 Oct 2004 02:35:06 +0000 : moshe weitzman I'm guessing that someone is going to have to demonstrate that this patch performs as well as current drupal before it gets comitted. i think this patch is a few benchmarks from being comitted. ------------------------------------------------------------------------ Wed, 27 Oct 2004 01:04:09 +0000 : Nick Nassar Attachment: http://drupal.org/files/issues/Drupal-Improved_Revision_Schema_10-26-2004.p… (11 KB) I ran some really unscientific benchmarks, and it looks like this patch has a negligible affect on performance. I used apache bench and the database from theregular.org, which doesn't contain any revisions (worst case scenario for this patch) and contains several hundred nodes. Both the patched and unpatched versions hovered between 2.36 and 2.38 requests per second. The command I used was: ab -n50 -C 'PHPSESSID=b01a9f92880ef215b0ed6f1314a5eba2' http://192.168.0.100/ An updated patch that should apply to CVS is attached. ------------------------------------------------------------------------ Wed, 27 Oct 2004 01:05:16 +0000 : Nick Nassar I ran some really unscientific benchmarks, and it looks like this patch has a negligible affect on performance. I used apache bench and the database from theregular.org, which doesn't contain any revisions (worst case scenario for this patch) and contains several hundred nodes. Both the patched and unpatched versions hovered between 2.36 and 2.38 requests per second. The command I used was: ab -n50 -C 'PHPSESSID=b01a9f92880ef215b0ed6f1314a5eba2' http://192.168.0.100/ An updated patch that should apply to CVS is attached. ------------------------------------------------------------------------ Wed, 27 Oct 2004 01:05:26 +0000 : Nick Nassar Attachment: http://drupal.org/files/issues/Drupal-Improved_Revision_Schema_10-26-2004.p… (11 KB) I ran some really unscientific benchmarks, and it looks like this patch has a negligible affect on performance. I used apache bench and the database from theregular.org, which doesn't contain any revisions (worst case scenario for this patch) and contains several hundred nodes. Both the patched and unpatched versions hovered between 2.36 and 2.38 requests per second. The command I used was: ab -n50 -C 'PHPSESSID=b01a9f92880ef215b0ed6f1314a5eba2' http://192.168.0.100/ An updated patch that should apply to CVS is attached. ------------------------------------------------------------------------ Mon, 15 Nov 2004 05:05:30 +0000 : elias1884 please overthink the revision system default workflow as well. don't look at the revision system as an isolated system but as a part of the whole workflow system! if you combine revisions with the moderation queue the most logic default workflow would be like that: auth user creates node (revision #0) admin approves the node (status = 1, moderation = 0) => node publicly available auth user finds typo and changes node (revision #1, status = 0, moderation = 1) ------- what happens at that point at the moment is, that the node is not accessible anymore at all until the new revision is approved by admin. of course the new revision should not go online until reviewed and approved, this is absolutely correct, but there is no reason to not take the old revision offline, since it was already approved and should therefore be online until the new revision is approved. it is not practical if a node disappears only because the author corrected a typo. ------- admin approves the node (status = 1, moderation = 0) eventhough I first thought a plain boolean active field would not be capable of providing that functionality if finally came to the conclusion, that it can. The only thing to do is to not set that bit, when a new revision is created, but when it is approved (in case moderation is activated under default workflow). Every revision should have its own moderation, status and active field and on approval they are set like this (status=1, moderation=0, active=Y). When you wanna rollback to an old revision, you can chose between all revisions that already have the moderation bit set back to 0 again and the published to 1. There should be an extra permission for rollback! another concern that I have about the default workflow is, that users can't see the content, they have just created, when moderation is enabled. Eventhough, there is a big fat "submission accepted" presented after submissions, unexperienced users tend to question the information those stupid tincans give them, if they can't find their content afterwards. Many users are really lazy bastards and they don't even read the status messages. The best feedback about whether his story was submitted successfully or not of course is, if he can find the story somewhere on the site, maybe with a status message on top of it, mentioning, that the content is currently not publicly available since it has not been approved yet. there should be a my content section under my account, like somebody is trying to do with the workspace module I guess. so my suggestion is to make (status=0, moderation=1) still available for the creator under a my content section somewhere! ------------------------------------------------------------------------ Wed, 24 Nov 2004 04:21:18 +0000 : Nick Nassar I agree. The current workflow for moderation queues and revisions needs to change, but this patch isn't the place for it. The patch is already too big, and it only does the backend stuff. Instead of adding more to this patch and making it take even longer to get into core, would you mind creating a new issue for your UI suggestions, so the those changes can be added as a separate patch? Thanks, Nick ------------------------------------------------------------------------ Sat, 11 Dec 2004 12:26:02 +0000 : Dries This patch is _much_ needed so I'd love to see someone revive it. In order for this patch to be accepted, the following needs to be done: * Update this patch to CVS HEAD. * Rename revid to vid. * Rename node_rev to node_revisions. * Rename node_rev.changed to node_revisions.timestamp. * Rename $rnode to $revision. * Fix the coding style to match Drupal's: proper spacing, single quotes where possible, proper variable names. * Benchmark this patch with a large database with enough revisions. I'd be happy to benchmark this on my local copy of the drupal.org database. * The book.log field should probably move to the node_revisions table. This can be done in a separate patch. * Investigate whether transactions are well-supported. ------------------------------------------------------------------------ Mon, 13 Dec 2004 00:25:40 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/Drupal-Improved_Revision_Schema_10-26-2004-r… (11.02 KB) I've worked a bit on the patch (coding style issues as mentioned by Dries). One thing I noticed is that the patch uses REPLACE. IIRC this needs to be chagned to "UPDATE, if fail INSERT" for pgsql compatibility. Nick, are you still interested in working on that patch? I'd like to know how it works on your site and work on getting it into core. ------------------------------------------------------------------------ Mon, 13 Dec 2004 12:33:08 +0000 : Dries Gerhard: your patch does not apply. ------------------------------------------------------------------------ Mon, 13 Dec 2004 17:10:12 +0000 : killes(a)www.drop.org Yes, I know, that was the same version as I mailed to you earlier. ------------------------------------------------------------------------ Mon, 13 Dec 2004 21:02:06 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions.patch (52.96 KB) Ok, upüdated the patch to cvs. ------------------------------------------------------------------------ Tue, 14 Dec 2004 08:58:36 +0000 : Dries Some more comments: * db_begin_transaction() and db_end_transaction() do not belong in database.inc, but in database.mysql.inc and database.pgsql.inc respectively. * The node module calls node_revisionsision_list() which is not defined. (Fxed that on my local copy.) * Do db_begin_transaction() and db_end_transaction() deprecate Jeremy's table locking patch? * The upgrade path assigns the wrong user ID to each revision. * The upgrade path assigns the wrong date to each revision (that or a node's revision page shows the wrong usernames/dates). * The coding style needs a bit of work, but we can worry about that later. ------------------------------------------------------------------------ Tue, 14 Dec 2004 17:34:44 +0000 : Nick Nassar If you need any help getting those things fixed, just let me know. ------------------------------------------------------------------------ Tue, 14 Dec 2004 17:50:30 +0000 : Nick Nassar How this relates to Jeremy's node locking patch: There was lots of discussion, and node locking was decided against because from an end user point of view you never want a node to be locked. He's now advocating for a much simpler patch that warns users if their changes will overwrite someone elses. That patch still has a race condition, which might be fixed using db_begin_transaction(). http://drupal.org/node/6025 ------------------------------------------------------------------------ Tue, 14 Dec 2004 22:26:19 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_0.patch (55.96 KB) Here is an updated patch that tries to address Dries concerns. ------------------------------------------------------------------------ Wed, 15 Dec 2004 08:32:50 +0000 : Dries Attachment: http://drupal.org/files/issues/revisions-bug.png (76.06 KB) It didn't fix the aforementioned bugs. See attached screenshot. ------------------------------------------------------------------------ Thu, 06 Jan 2005 20:15:01 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_1.patch (51.77 KB) Ok, here is a new version. Dries and myself worked hart at it, so please have a look. what is still missing - database upgrades for the core modules with an own table - contrib modules need an upgrade too. - do we need nid and vid in both the node and the node_revisions table? - the amount of sql queries means a good stress testing for large databases. ------------------------------------------------------------------------ Wed, 19 Jan 2005 21:43:49 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_2.patch (49.49 KB) Here is an updated patch. We discussed to keep the current title in node module and also in the revisiosn table. This is content duplication but will save many joins as many queries only need the title of a node. Discussion is welcome. ------------------------------------------------------------------------ Wed, 19 Jan 2005 23:33:32 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_3.patch (29.93 KB) I've implemented the aforementioned solution. This makes the patch much smaller. The patch now also removes taxonomy_node_has_term() which wasn't used anywhere. I'd really apprciate if some people could test drive the patch. It will be another huge improvement for 4.6. ------------------------------------------------------------------------ Thu, 20 Jan 2005 00:05:54 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_4.patch (30 KB) Another revision. Steven didn't like my literal $node->vid in queries. ------------------------------------------------------------------------ Thu, 20 Jan 2005 01:10:50 +0000 : killes(a)www.drop.org - database upgrades for the core modules with an own table - contrib modules need an upgrade too. - do we need nid and vid in both the node and the node_revisions table? - the amount of sql queries means a good stress testing for large databases. These issues are still open, btw. Especially the first one needs to be tackled. ------------------------------------------------------------------------ Tue, 25 Jan 2005 20:11:59 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_5.patch (51.13 KB) Here is a patch that has the database tables updated for forum, book, and page module. ------------------------------------------------------------------------ Sat, 29 Jan 2005 22:55:59 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_6.patch (49.18 KB) Yet another update to keep it working with head. The patch now also removes the table definitons for the page table. ------------------------------------------------------------------------ Sat, 29 Jan 2005 22:57:40 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_7.patch (55.69 KB) Sorry, that was the old version, this is the right one. ------------------------------------------------------------------------ Mon, 31 Jan 2005 19:55:03 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_8.patch (55.71 KB) Updated once more. ------------------------------------------------------------------------ Mon, 31 Jan 2005 20:52:08 +0000 : Dries Anyone to help review/test this? ------------------------------------------------------------------------ Mon, 31 Jan 2005 21:22:36 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_9.patch (49.29 KB) Updated again, the update functions occurred twice. Thanks Bart. ------------------------------------------------------------------------ Wed, 02 Feb 2005 00:27:05 +0000 : killes(a)www.drop.org Don't know if the db I am using is corrupted or what. I still do have some didficulties. The latest patch is attached. ------------------------------------------------------------------------ Wed, 02 Feb 2005 00:27:49 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_10.patch (49.67 KB) I am probably slowly going mad ... ------------------------------------------------------------------------ Wed, 02 Feb 2005 01:54:58 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_11.patch (48.95 KB) The update issue still needs investigating. This patch is updated for cvs. ------------------------------------------------------------------------ Wed, 02 Feb 2005 20:20:51 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_12.patch (49.83 KB) Ok, here is a new version. I've solved my troubles with book.module. There are still some issues with forum module. Possibly due to inconsistent database. ------------------------------------------------------------------------ Wed, 02 Feb 2005 21:31:05 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_13.patch (49.83 KB) Turns out the drupal.org database had indeed some quirks. Please run this query in your oldest db and tell me the result: select nid,type from node where type like '%/%'; If you get a non-zero result we might need to add another security update. The patch could use still more testing, though. ------------------------------------------------------------------------ Thu, 03 Feb 2005 01:16:54 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_14.patch (49 KB) Ok, we are getting somewhere. At a first glance the update is working. There is a problem remaining: the revisions tab will be shown whether the node has revisions or not. Not sure we can/need to fix this. People with a drupal.org account can log in at http://killes.drupaldevs.org/revision/ and poke around. Your permissions will be the same as on drupal.org. Feel free to vreak everything but don't forget to file complaints here. (Note: this is only a pruned version of the drupal.org database with all project nodes and nodes with nids > 7000 dropped). ------------------------------------------------------------------------ Thu, 03 Feb 2005 04:19:14 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_15.patch (52.39 KB) There was some error in node_save and also the patches to the database.inc files got lost... ------------------------------------------------------------------------ Thu, 03 Feb 2005 07:07:27 +0000 : robertDouglass Submitting book pages doesn't work on your test site. It puts the entire content of the preview inside the body textarea. I wrote a sentence in the body and the log, and pressing preview put several lines of HTML containing both sentences in the body textarea on the preview page, plus the book page wouldn't submit. -R ------------------------------------------------------------------------ Thu, 03 Feb 2005 07:50:59 +0000 : Junyor 0 results here. I started using Drupal with version 4.4, though. ------------------------------------------------------------------------ Thu, 03 Feb 2005 23:56:18 +0000 : killes(a)www.drop.org @Junyor: Thanks, that's a good sign. Maybe somebody else has an older db to try. @robertDouglass: The first effect you describe is due to drupaldevs running on PHP 5. I am unsure why the second thing does not work. In node_save() the node object has a nid although there is none in the form. Very strange. I've enabled display of db queries on the testsite. ------------------------------------------------------------------------ Fri, 04 Feb 2005 19:17:55 +0000 : dmjossel No results here on the query: select nid,type from node where type like '%/%'; On a database that was put in place prior to Drupal 4 and is now running on 4.5.2. ------------------------------------------------------------------------ Fri, 04 Feb 2005 20:44:23 +0000 : killes(a)www.drop.org @dmjossel: thanks. @all. The strange problem I reported was apparently php 5 related. After applying Steven's php 5 patch it went away. One error is remaining: If I create a new forum topic it will be shown as part of the book on preview. Hmm, that was due to a db that got corrupted during testing so that is fixed too. Please poke around at the test site and look if you find more errors. ------------------------------------------------------------------------ Sat, 05 Feb 2005 07:16:22 +0000 : Steven By the way, I just remembered that Drupal.org has some blogs lingering on in the database even though blog.module is not enabled. Perhaps this is causing troubles? ------------------------------------------------------------------------ Sat, 05 Feb 2005 11:22:59 +0000 : Anonymous I can't see why it would. Drupal.org will need extra updates for images and project nodes because those have their own tables. GK. ------------------------------------------------------------------------ Sun, 06 Feb 2005 12:49:55 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_16.patch (52.49 KB) Updated to apply to cvs again. ------------------------------------------------------------------------ Tue, 22 Feb 2005 20:15:40 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_17.patch (49.64 KB) Updated again. All we need is a patch to upload module and an upgrade path for it. ------------------------------------------------------------------------ Fri, 04 Mar 2005 04:22:58 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_18.patch (52.31 KB) Updated once more. Moved log field from book to node_revisions table as discussed in Antwerp. upload module still missing. We need to decide under which circumstances the log field should be displayed. Should that be added to the workflow? Should it depend on the revisions setting? ------------------------------------------------------------------------ Sat, 05 Mar 2005 19:27:03 +0000 : Anonymous Attachment: http://drupal.org/files/issues/revisions_20.patch (75.52 KB) Ok, here it is: Yet another revision of this grrrrreat patch. Changes from previous versions: - supports versioning for uploaded files. A problem is that if you delete a file, it will be gone for all revisions. - the log field is now in the node_revsions table, but each module has to decide whether to show it or not. I've implemented it for the page and the book type odes. Also, the field can be edited when adding non-book nodes to the book. The log is displayed on the revisions page and if a node is moderated. - the revisions are moved to an old_revisions table to a) get the node table smaller and b) still leave the mavailable for contrib modules that want to retreive old version data. The patch has been applied to killes.drupaldevs.org/revision where it can be tested by anybody (especially people who have "site admin" rights on drupal.org) The database is from drupal.org and you should b able to log in with your pass or simply mail yourself a new one. Gerhard ------------------------------------------------------------------------ Sat, 05 Mar 2005 19:51:56 +0000 : Anonymous Attachment: http://drupal.org/files/issues/revisions_21.patch (59.42 KB) BTW, I marked this a bug because atm the revisions field can grow quite big. Neil has reported problems from some users who were not able to load some nodes due to to many large revisions. Also, som unrelated stuff crept into the patch. New version attached. ------------------------------------------------------------------------ Tue, 08 Mar 2005 05:56:01 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_22.patch (60.29 KB) Ok, I think I got it. Changes to last version: - uploads are no properly versioned. Missing are still pgsql checks and updates. ------------------------------------------------------------------------ Thu, 10 Mar 2005 16:58:41 +0000 : Anonymous Was able to get http://drupal.org/files/issues/revisions_21.patch to work with drupal-cvs.tar.gz (10 March 2005) by: - includes/database.mysql.inc: Commenting out duplicates for functions function db_begin_transaction and function db_commit_transaction - modules: node.module: Removing "'title' => $node->title," from $node_table_values variable declaration and removing "'title' => "'%s'"," from "$node_table_types" variable declaration. Happy to submit a patch if requested. I'll watch this thread. ------------------------------------------------------------------------ Fri, 11 Mar 2005 23:59:45 +0000 : killes(a)www.drop.org The duplicate function has been removed in rev 22 of this patch. Why do you think the changes in node_save are needed? Titles are saved in both tables for performance reasons. ------------------------------------------------------------------------ Sun, 13 Mar 2005 16:12:21 +0000 : jlerner Hi - I posted comment #62. The changes to node_save appear to be needed because recent patches (both 21 and 22) remove the field 'title' from table 'node'. So without the changes to node_save, node.module is broken and generates errors. Joshua ------------------------------------------------------------------------ Sun, 13 Mar 2005 16:29:42 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_23.patch (61.17 KB) Thanks, Joshua, for catching this. node:title is there to stay. ------------------------------------------------------------------------ Wed, 13 Apr 2005 16:29:23 +0000 : moshe weitzman since HEAD is open again, perhaps it is a good time to revisit this patch. once this is committed, lets address - http://drupal.org/node/11071 "node_validate does not respect group editing" ------------------------------------------------------------------------ Mon, 18 Apr 2005 15:43:42 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_24.patch (60.39 KB) Updated. ------------------------------------------------------------------------ Mon, 18 Apr 2005 16:16:32 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_24_0.patch (60.39 KB) Updated. ------------------------------------------------------------------------ Tue, 19 Apr 2005 05:19:42 +0000 : Dries I'll commit this patch later this week! If you haven't checked this patch already, I urge you to test/check it out because it will have significant impact on existing code and modules! ------------------------------------------------------------------------ Tue, 19 Apr 2005 05:21:33 +0000 : Dries Also, what do people think about the n.title being duplicated? ------------------------------------------------------------------------ Tue, 19 Apr 2005 05:26:58 +0000 : chx I won't lose any sleep because of duplicated titles... ------------------------------------------------------------------------ Tue, 19 Apr 2005 18:35:58 +0000 : killes(a)www.drop.org Let me explain why I have chosen to duplicate the title (and also the uid): If you look at all the queries in Drupal, you will find that most of them only need the title and th uid of a node. That is, by duplicating it, we save expensive joins on the node_revisions table. Due to this fact, this patch is actually a performance improvement. A note about updating contrib module: Strictly speaking they wouldn't need to be updated. They only need to if their authors decide that their info should be available for revisioning. The upgrade path for forum.module in my update.inc patch (+ the forum patch) should show you what needs to be done. I will write a note for the update page once the patch hits core. ------------------------------------------------------------------------ Sun, 24 Apr 2005 21:21:19 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_25.patch (60.38 KB) Updated to cvs. Dries: Based on some remarks in #drupal this is the last update I am going to do. Apply it or won't fix it. ------------------------------------------------------------------------ Sat, 30 Apr 2005 03:42:39 +0000 : Jeremy(a)kerneltrap.org Attachment: http://drupal.org/files/issues/revisions_25.patch.patch (528 bytes) That's a big patch. I've only started looking through it. I noticed one little typo, affecting updates. A patch to your last patch is attached. I'm running with the revision patch on my dev server now happily. I like the concept. What happens if you click 'stop' on your browser in the middle of a MySQL "transaction"? I assume that kills the connection to MySQL, and the lock is freed? But this then leaves changes only partially applied? What exactly does locking/unlocking the tables buy us in MySQL? I don't see anywhere that we detect if an apply fails part way through, and thus roll back...? What am I missing? ------------------------------------------------------------------------ Sat, 30 Apr 2005 07:11:28 +0000 : Dries Jeremy: many of us are worried about the performance ramifications this patch introduces. Early experiments showed a small performance improvement (while a performance regression might be expected). More performance reports from large sites like kerneltrap.org will certainly help this patch. Mind to do a quick performance comparision and to report back with some numbers? Thanks. ------------------------------------------------------------------------ Sat, 30 Apr 2005 12:38:02 +0000 : Jeremy(a)kerneltrap.org Dries: I'm not running HEAD on kerneltrap, so this really isn't a possibility. Furthermore, until I understand why we're locking tables, I don't like it. The idea of revisions in their own tables is great. The idea of locking tables to get (without any obvious benefit) there really worries me. ------------------------------------------------------------------------ Sat, 30 Apr 2005 14:16:01 +0000 : killes(a)www.drop.org @Jeremy: Thanks for looking at the patch! Also for catching the typo. :) Did you try to upgrade your database? If yes, how did it go? One of Dries' concerns is the complexity of the upgrade. How many nodes and revisions did the db have? About database locking: This part of the patch was created by Nick and I simply continued to use it. Maybe the code should rather be: if(db_begin_transaction(array('{node}', '{node_revisions}', '{watchdog}', '{sessions}', '{files}'))) { db_query($node_query, $node_table_values); db_query($revisions_query, $revisions_table_values); db_commit_transaction(); ... } The idea is probably to avoid two updates at the same time. I don't think the locking helps if you abort the script at an inconvenient time. Rollbacks aren't implemented in all mysql versions. We could omit the db locking if deemed inappropriate. Maybe Nick can explain his ideas behind this. @Dries: I wonder who the "many of us" are. They certainly haven't spoken to me. Moshe had some reservations about the upgrade path and project module, but the time that project module abused revisions to store issue updates was long ago and his reservations were resolved. Nobody else (besides you of course and now Jeremy) has voiced reservations in a way that was audible to me. If you grep through the patch you will notice that there are only four queries which have a join on the node_revisions table. Two of them are in node_load and in the other cases the join replaced a join on the node table. The two queries in node_load are the only ones that have both a join on the node and the revisions query. Thus, loading of individual nodes might become somewhat slower. All other queries will be faster since the node table is now much smaller. Also, node loading does not have to be slower, it depends on your node table. If you had a lot of revisions and thus a large table, then the new scheme will make your queries actually faster since we do not load the revisions on each and every node load anymore. If you didn't have many revisions your node_load migth be somewhat slower. WRT to the update script Karoly pointed out that we could use multiple insert queries instead one query per revision. This would probably make the update somewhat faster. I am willing to work on this iff you declare that you will commit the patch afterwards. I'd need to know if this will work on pgsql and on all supported mysql versions before I work on it. About locking: Database locking is dog slow, at least on mysql. I was using locks in an earlier version of the upgrade script but had to remove it for (serious!) performance reasons. ------------------------------------------------------------------------ Mon, 09 May 2005 15:07:34 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_26.patch (46.45 KB) Ok, another update, cause I need it myself. I've left out the transaction stuff for now. It is in principle unrelated to this patch and should be discussed elsewhere. This also makes the patch smaller and easier to review (hint, hint). ------------------------------------------------------------------------ Mon, 09 May 2005 20:32:09 +0000 : killes(a)www.drop.org The patch contained the update functions twice. ------------------------------------------------------------------------ Mon, 09 May 2005 20:32:26 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_27.patch (39.05 KB) The patch contained the update functions twice. ------------------------------------------------------------------------ Mon, 09 May 2005 21:23:06 +0000 : Dries Got one error during the upgrade path: ALTER TABLE {book} ADD PRIMARY KEY vid (vid) FAILED ------------------------------------------------------------------------ Mon, 09 May 2005 21:26:19 +0000 : killes(a)www.drop.org This had happend to me as well, when I tested this patch. The reason is that for some reason the vid is not unique. Most likely there are some entries with vid = 0 in there. Can you check which node types those have? it always was an error in the test database. See: http://drupal.org/node/7582#comment-20678 ------------------------------------------------------------------------ Mon, 09 May 2005 21:27:06 +0000 : Dries Actually, I got 2850 errors during the upgrade. Some of these: sprintf() [function.sprintf]: Too few arguments in drupal-cvs/includes/database.inc on line 154. Some of these: Query was empty query: in drupal-cvs/includes/database.mysql.inc on line 66. And this: Unknown table 'n' in field list query: SELECT n.nid, n.vid FROM node INNER JOIN files f ON n.nid = f.nid in drupal-cvs/includes/database.mysql.inc on line 66. :-) ------------------------------------------------------------------------ Mon, 09 May 2005 21:29:19 +0000 : Dries Or this: user error: Unknown column 'log' in 'field list' query: SELECT parent, weight, log FROM book WHERE nid = 1 in drupal-cvs/includes/database.mysql.inc on line 66 ------------------------------------------------------------------------ Mon, 09 May 2005 21:52:12 +0000 : Dries The time required to generate my main page went from 902 ms (before upgrade) to 2139 ms (after upgrade). The time required to generate a forum listing (?q=forum/x) went from 1872 ms (before upgrade) to 2874 ms (after upgrade). Maybe this is because my database is not consistent as the result of the upgrade errors (yet I don't see any errors on the pages I benchmarked). ------------------------------------------------------------------------ Tue, 10 May 2005 00:24:38 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_28.patch (53.47 KB) Ok, let me get to this from the bottom to the top: - my test runs indicated a different development wrt timing. If I had gotten your results, I had stopped working on this long ago. So your results are wrong for some reason. - user error: Unknown column 'log' in 'field list' Wasn't my day, the book patch got lost. It is contained now. First -R the old patch, then apply this one. - Unknown table 'n' in field list query: Walkah found this, but I forgot to fix it. Fixed now. - I've no idea where the other queries come from. I am hoping that either your test db is borken or they are follow ups from the other ones. If you let me have your test db, I'll try some debugging. Thanks for wasting your time, too. ------------------------------------------------------------------------ Tue, 10 May 2005 05:07:31 +0000 : Dries I double-checked and the numbers don't seem to lie. I'll test some more after work on another machine to make sure it is not platform-specific. ------------------------------------------------------------------------ Wed, 11 May 2005 03:32:47 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_29.patch (54.83 KB) Ok, here I am again. What I did: 1) Ask Dries to let me have drupal.org database 2) get 400MB of SQL inserts... 3) take 23 minutes to import said data 4) Remove all image and project nodes (don't want to install their modules), 11765 nodes left 5) back up data 6) take tests on non-cached /node page (as anonymous user). ab results: -c 1 -n 25: Requests per second: 1.29 [#/sec] (mean) Connection Times (ms) min mean[+/-sd] median max Total: 663 775 179.7 689 1264 7) Do the same for the tracker page: Requests per second: 0.83 [#/sec] (mean) Total: 1182 1199 7.4 1199 1217 8) Apply my patch (rev. 28). 9) run db update and hold breath 10) update times out... 11) play back backup from 5) 12) wait 13) getting annoyed and removing cache, watchdog, and accesslog before playing in backup. 14) wait again. Understand why Dries doesn't try this patch often. Maybe a smaller DB would do for testing? 15) wait more. get really annoyed. 16) Set time limit to 18000 in update.php 17) try again 18) fails again before the second update is completed. 19) curse. 20) delete search stuff from db. Ooops, sooo much smaller... 21) import again, below 2 minutes... 22) rewrite to use extended insert. Found a bug. 23) still does not complete. Mysql logging to the rescue! 24) tid = 0? Not good. 25) Well, the update works fine till node 10834. 5595 nodes done, 6136 to go. 26) Writing shell based update script. Discovery: 24MB aren't enough. Hopefully 64 are. Nope. extended inserts for revisions are apparently not the brightest idea: Huge memory consumption. Hmm, no, all updates got through. Selecting the revisions to put them into old_revisions table screwed it. Learned about CREATE TABLE old_revisions SELECT syntax. Yay! finally. 24 MB are just not enough the update.php script seems to still break. 27) Benchmarks! /node Requests per second: 1.54 [#/sec] (mean) Connection Times (ms) min mean[+/-sd] median max Total: 632 649 40.5 636 791 /tracker Requests per second: 0.86 [#/sec] (mean) Total: 1119 1165 65.8 1160 1461 Ok: So we get an improvement for many node_loads, but none for simple selects from node. More tests can be done. 28) roll new patch Ain't Drupal fun? ------------------------------------------------------------------------ Wed, 18 May 2005 13:38:05 +0000 : Dries I did another round of tests on _another_ machine and it is not looking good: Before upgrade After upgrade ?q= (main page) 218 ms/request 340 ms/request ?q=forum (forum overview) 754 ms/request 1520 ms/request ?q=about (book page) 375 ms/request 5400 ms/request The upgrade process itself gave me a number of 'query was empty' and 'sprintf(): too few arguments' reports. Everything seems to work fine though. Looking at the ?q=about page, I see that the following query is executed twice _and_ that each time, it take more than 2 seconds to complete: SELECT n.nid, n.title, b.parent, b.weight FROM node n INNER JOIN book b ON n.vid = b.vid WHERE n.status = 1 AND n.moderate = 0 ORDER BY b.weight, n.title; --8 SHOW INDEX FROM book; +-------+------------+-------------+--------------+-------------+-----------+-------------+----------+--------+------+------------+---------+ | Table | Non_unique | Key_name | Seq_in_index | Column_name | Collation | Cardinality | Sub_part | Packed | Null | Index_type | Comment | +-------+------------+-------------+--------------+-------------+-----------+-------------+----------+--------+------+------------+---------+ | book | 1 | book_parent | 1 | parent | A | 92 | NULL | NULL | | BTREE | | | book | 1 | nid | 1 | nid | A | 369 | NULL | NULL | | BTREE | | +-------+------------+-------------+--------------+-------------+-----------+-------------+----------+--------+------+------------+---------+ 2 rows in set (0.00 sec) The book module does not appear to have a primary key? Sounds like a bad idea so I added one: mysql> ALTER TABLE book ADD PRIMARY KEY nid (nid); Query OK, 369 rows affected (0.02 sec) Records: 369 Duplicates: 0 Warnings: 0 Next, I wanted to make the vid column a unique key in all node tables: mysql> ALTER TABLE node ADD UNIQUE vid (vid); Query OK, 20392 rows affected (0.81 sec) Records: 20392 Duplicates: 0 Warnings: 0 mysql> ALTER TABLE book ADD UNIQUE vid (vid); ERROR 1062: Duplicate entry '0' for key 2 mysql> ALTER TABLE forum ADD UNIQUE vid (vid); Query OK, 10806 rows affected (0.10 sec) Records: 10806 Duplicates: 0 Warnings: 0 As you can see, it fails for the book table which makes me believe there is some inconsistent data ... I set out to fix that: mysql> SELECT nid, COUNT(nid) AS vids FROM book GROUP BY vid HAVING vids > 1; +-----+------+ | nid | vids | +-----+------+ | 871 | 2 | +-----+------+ 1 row in set (0.00 sec) mysql> SELECT title FROM node WHERE nid = 871; Empty set (0.00 sec) mysql> DELETE FROM book WHERE nid = 871; Query OK, 1 row affected (0.00 sec) mysql> ALTER TABLE book ADD UNIQUE vid (vid); Query OK, 368 rows affected (0.01 sec) Records: 368 Duplicates: 0 Warnings: 0 Looks like everything is well now. Ran some new benchmarks: Before upgrade After upgrade With indices ?q= (main page) 218 ms/request 340 ms/request 336 ms/request ?q=forum (forum overview) 754 ms/request 1520 ms/request 1531 ms/request ?q=about (book page) 375 ms/request 5400 ms/request 475 ms/request Unfortunately, we're still slower than the original code. ------------------------------------------------------------------------ Wed, 18 May 2005 21:53:31 +0000 : killes(a)www.drop.org Dries, thanks for testing it again. I do think the broken queries you observer have something to do with the bad performance after the update. Please log the queries any I will have a look at them. I've never seen any such queries. My update script also tries to create the appropriate indices, but it will of course fail if the database contains cruft. the indices for the forum are probably missing, too. I am still convinced that the patch is core worthy. ------------------------------------------------------------------------ Thu, 19 May 2005 04:36:09 +0000 : Dries It wouldn't hurt if more people would benchmark this patch. The patch's current performance worries me. Did you check your watchdog messages after upgrading the drupal.org database? Depending on your settings, errors might only be shown in the watchdog. I'll look into the remaining glitches as time permits. Thanks for your persistence in keeping this patch up-to-date. :) ------------------------------------------------------------------------ Thu, 19 May 2005 11:59:22 +0000 : killes(a)www.drop.org Dries: Can you please let me have your updated database? I want to have a look at it and try my own benchmarks with it. And yes, if I did learn something on this project is how to be persistant. ;) ------------------------------------------------------------------------ Fri, 24 Jun 2005 16:25:34 +0000 : killes(a)www.drop.org Here is an idea that occurred to me: The problem with the upgrade process is that keeping the existing revisions requires a lot of work to do. This generates a huge amount of sql queries for a large database and also requires a huge amount of memory. My suggestions is to let update.php only handle the basic upgrade, ie without old revisions. An additional module could be created that would implement a cron based approach to upgrading old revisions one node at a time. it could expose a hook to let contrib modules do their own upgrades. Dries, what do you think? (I am writing "Dries" because he seems to be the only one who is interested in getting this into core...) ------------------------------------------------------------------------ Fri, 24 Jun 2005 22:25:11 +0000 : Junyor Killes: I'm also interested in seeing this hit core. What about adding something to legacy.module to do it? ------------------------------------------------------------------------ Sun, 26 Jun 2005 21:14:54 +0000 : chx This is a sensible approach. Maybe this is the _only_ sensible approach. I have a little problem though: while the conversion is running somehow both revision handlers should be available. ------------------------------------------------------------------------ Sun, 26 Jun 2005 22:16:21 +0000 : killes(a)www.drop.org hehe, one only has to whine and bug enough and one gets some feedback. ;) @junyor: legacy.module would be a good place. my current idea is to auto-enable it in update.php and then disable it again in legacy_cron after all nodes are updated.. ;) @chx: When somebody wants to look at revisions of a node that node could be auto-updated. The only problem are contrib modules: they's need to have some hook in order to update their own data. When somebody looks at the revisions of a node than cannot be updated because the contrib module in question has no such hook, we can optionally let the user discard old revisions I guess. Dries, what do you think? ------------------------------------------------------------------------ Mon, 25 Jul 2005 16:48:51 +0000 : killes(a)www.drop.org Attachment: http://drupal.org/files/issues/revisions_30.patch (53.13 KB) Sooooo; I've updated this patch once again. Dries didn't like my idea of legacy updates so we will have an option to discard old revisions in case the update should prove difficult. Always keep a backup. ------------------------------------------------------------------------ Mon, 25 Jul 2005 17:31:12 +0000 : Bèr Kessels can we please accept and commit this patch? We can iron out any issues later. This patch is just far too big and complex to be perfect-at-once. ------------------------------------------------------------------------ Mon, 25 Jul 2005 19:22:50 +0000 : Dries @Ber: no, we can't commit this patch blindly. Data loss is a much bigger problem than a syntax error or other code glitch. We can break Drupal but we can't break their data. I've spent quite a bit of time testing the previous version of this patch and noticed significant performance degradation. Tell me, Ber, why can't you test this patch first? ------------------------------------------------------------------------ Mon, 25 Jul 2005 19:40:37 +0000 : Bèr Kessels I was not referring to not testing it. If it is just the upgrade path that proves to be cumbersome, then why can we not fix it afterwards, I.e. when everyone has had a good chance to look at it. Testing such a huge patch requires a lot of work. something no-one just does in his spare hours. We had discussions before to deal in a slightly different way with big changes; to commit them quicker and to leave the ironing out of any left overs for the community. I hooked into that discussion here. For two reasons. One is that Gerhard has spent numerous hours on maintaining this patch. The other one is that the community can be of help with ironing out issues in such a large change, much better than that Gerhard can do on his own. And yes, dataloss is very bad, but no-one should loose data, when he/she followed the instructions (backing up)... ------------------------------------------------------------------------ Mon, 25 Jul 2005 20:00:38 +0000 : Dries Ber: applying this patch takes 15 seconds. Whether I apply this patch for you, or whether you apply it yourself, it will hardly reduce the 'testing cost'. The problem is that data loss can be subtle; it might go unnoticed for a couple days. Make no mistake, I'd like to see this patch committed ASAP, but it warrants some testing. Let's test/benchmark it and commit it. ------------------------------------------------------------------------ Mon, 25 Jul 2005 20:55:03 +0000 : drumm The upgrade script already takes a long time to execute and does not provide feedback to the user about how it is doing. I plan on making the upgrade script able to spread the updates across multiple page views and give user feedback showing that progress is being made. This will hopefully make the speed of this update a moot point. I am working on this for my dayjob, CivicSpace, so it should get done "real soon now", but it should be expected to take awhile (smallish number of weeks). Please make comprimises if necessary.

1 0

[drupal-devel] [feature] add inline functionaliy to uploads
by Bèr Kessels 27 Jul '05

27 Jul '05

Issue status update for http://drupal.org/node/26288 Post a follow up: http://drupal.org/project/comments/add/26288 Project: Drupal Version: cvs Component: upload.module Category: feature requests Priority: normal Assigned to: Anonymous Reported by: Bèr Kessels Updated by: Bèr Kessels Status: patch inline module will continue working. but in order for it to use this inline variable (and become even better) it needs to be patched. Bèr Kessels Previous comments: ------------------------------------------------------------------------ Sun, 03 Jul 2005 18:03:35 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/upload_inline.patch (19.05 KB) One of the most often asked features is proper inlnie handling of files. Look at the amount of solutions, the popularity of image_assist, and the amount of peolpe dowloading image.module! That alone should be enough proof that Drupal lacks proper inline image support. This patch adds that to core. In fact, it does little more then appending a link of img tag to the body or the teaser. Off course that is configurable per file. Next to the [] list checkbox, this patch adds an [] inline checkbox. Simplicity is the foundation of this patch. I want no stles for inline editing, no fancy html wrappers, no tokens, just $node->body or teaser appended with a small html string. Another small themable funtion is introduced, (hey, you cannot expect me to develop something without adding more power for themers, now, can you? ;) ), that allows people to theme the string that is appended to the body or the teaser. Oh, and also note hat the biggest part of this patch is some cleaning I had to do in order to be able to develop properly. I dont like Ifs inside cases in foreaches inside swiches. in other words: nodeapi now calls functions instead of executing code directly. ------------------------------------------------------------------------ Sun, 03 Jul 2005 18:19:25 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/inline.patch.screenshot.png (26.68 KB) here is how the form now looks ------------------------------------------------------------------------ Sun, 03 Jul 2005 18:19:58 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/inline.patch.screenshot3.png (30.53 KB) and this is an example of inlined images and a .doc file. ------------------------------------------------------------------------ Sun, 03 Jul 2005 20:44:00 +0000 : sepeck changing to patch per request from berkes ------------------------------------------------------------------------ Tue, 05 Jul 2005 07:46:13 +0000 : Kobus This gets a +1 from me in principle, however, the [inline:xx] tag with inline.module gives you greater freedom as to where the inline image must be displayed. If you can add this functionality (I haven't checked the code, I don't know if it is in there) it would be a great addition for Core. This same strategy can be used for inline blocks, I am sure. Regards, Kobus ------------------------------------------------------------------------ Tue, 05 Jul 2005 08:57:50 +0000 : Bèr Kessels there are a couple of reasons why i did not include the [inline] tags. * I aimed for extreme simplicity: a checkbox shows an image inline: its up to the theme where it appears (if one does not like it before/above the body and teaser.). Simplicity was the main goal. * We don't have any tokens in core. And we should not have them. * Tokens are a very bad substitute for a good interface. They give less power then plain HTLM. Are much worst documented then HTML, but in the mean time, they are still as hard to learn as HTML. (Yes, I know people _think_ they are easier, but there _is_ not difference between [ ] and , only that its a different ascii char. So, no. I don't allow any placement of the image. I leave that that for dedicated modules, or the themer to decide. ------------------------------------------------------------------------ Tue, 05 Jul 2005 09:34:06 +0000 : Kobus So you will provide an API to do this? For example, with perhaps minor modifications, the inline.module will be able to display these files? In that case I am happy. If not, then I can't give my support to this patch (as if that matters...). A themer can't do this task as inline images (and blocks for that matter) is too dynamic for theming; it can be placed anywhere in a node where the user pleases. This means for me that there should be a module for this, such as inline module that allows you to define [inline:xx] tags. If your module emits an array of uploaded files (such as upload.module), inline.module can be, with minor changes, adapted to show these files inline. To show you what I mean with the content is too dynamic for a themer to perform this task, look at this screenshot related to inline blocks (inline images can follow the same pattern): http://drupal.org/files/issues/regions---possibility-3.png ------------------------------------------------------------------------ Tue, 05 Jul 2005 11:26:15 +0000 : Bèr Kessels Kobus, We are dealing with different issues here: You want a method to place images, files or so anywhere in the post. That is fine, but certainly NOT addressed in the patch! I made patch that *only* adds marked files to the body. its really nothing more then <?php $node->body = $filestring . $node->body ?> No APIS, no, dynamic tokens, no filters, nothing. However, what I meant with themers, is that there now is a theme_upload_inline available, so you can theme the abovementioned $filestring. On top of that $node->files[FILEID]->inline is TRUE if a file is flagged for inline. So in node.tpl.php, or wherever you want to theme a node, you can print nice images inline, when that flag is set. And about the comment that a themer cant do this: Simply not true. On most sites images are always placed in the same places. REally, even the sites see which use img_assist or inline, use them to place the images on the exact same places in every node. People /think/ they want the power to place images anywhere, but they hardly ever use that power. Just look at all the big news/publishing sites out there (BBC, CNN, BoigBoing, r even freshmeat) images are all placed acc. to the theme. They are not placed in random places by authors. So if you are one of the few that still want that power, there aer mots of power tools like inline module or img_assist. We should offer a good default, one that is simple. ------------------------------------------------------------------------ Tue, 05 Jul 2005 11:41:34 +0000 : Gerhard Killesreiter I fully agree that tags are evil. Kobus: You can always look for tags in $node->body in your theme's node function. ------------------------------------------------------------------------ Tue, 05 Jul 2005 11:50:52 +0000 : stefan nagtegaal Ber, the theme-function in your code looks like: <?php +/** + * Theme function for rendering of inline images + * @param $file a file object. + * @param $image a Boolean, indicating whether an img tag (TRUE) or an anchor tag (FALSE) should be used. + * @ingroup themable + */ +function theme_upload_inline($file, $image) { + if ($image) { + return '<img src="'. check_url(($file->fid ? file_create_url($file->filepath) : url(file_create_filename($file->filename, file_create_path())))) .'" alt="'. check_plain($file->filename) .'" class="upload inline">'; + } + else { + return ''. check_plain($file->filename) .' [1]'; + } + +} ?> After looking at your code it's not clear to me when $image is TRUE or FALSE, can you elaborate on me please? [1] http://drupal.org/'. check_url(($file->fid ? file_create_url($file->filepath) : url(file_create_filename($file->filename, file_create_path())))) .'\" class=\"upload inline ------------------------------------------------------------------------ Tue, 05 Jul 2005 12:06:39 +0000 : Bèr Kessels The function calling the theme function should decide whether its an image. IMO that is far too hardcore code for a themer ;) <?php $image = ereg('^(image/)', $file->filemime); ?> inside _upload_inline() does the trick. I did find one issue, though, with svgs, which are image/svg so maybe we should limit this to really only inline jpg, png, and gif, by extension? But I am no fan of determining files by extension, and IMO getimgsize is too heavy; ------------------------------------------------------------------------ Tue, 05 Jul 2005 12:13:52 +0000 : stefan nagtegaal Can't you make use of PHP's <?php image_type_to_mime_type(); ?> or <?php image_type_to_extension(); ?> ------------------------------------------------------------------------ Tue, 05 Jul 2005 12:45:43 +0000 : Kobus Well, if not in the patch, I need inline functionality one way or another. inline.module works perfectly with the current upload.module to fulfill my needs, and if there is a way where someone can extend your proposed upload.module, that would get my +1, otherwise I will just remain neutral about this issue. inline.module makes use of $node->files and displays that image. If your patch still uses $node->files, then this is a non-issue. In other words, if I can, with inline.module, or with minor modifications to inline.module still show files inline, I am +1 for this. As for the themability question... I still disagree. You say "Simply not true. On most sites images are always placed in the same places." That is ONLY true because they have NO alternatives. Means the content is in the same places, because they have no other places to put them. I have made tens of static sites where the content does not resemble a fixed pattern or structure. For me, free-flow layout is my expression of my creativity. This is why I still even BOTHER with static sites; when Drupal don't do what I want it to do. If Drupal could do inline display of content properly, I'd never even build another static site. It is a great mistake people make (including myself), and that is they design the site before ANY content is available, and this happens a lot in static sites. But in dynamic sites, you have far less control over this, unless you have a VERY clean structure with limited information that can be added, and only a few people posting content. If you can have your content before you start the design, you will have a better fit. I can't see how you can anticipate every possible posting posted on your site if you have a diversity of users. Especially not if you're using a site such as an designer's showcase site where your creativity is shown by your web sites. Possible, but not easy. If I claim that I have "unique, fresh designs" I can certainly NOT give them a "box-like" site with "left and right side bars only". That's why I need inline content, this includes images AND boxes. Gerhard: Your concern about tags is answered as well above, if I am not mistaken. The patch need not provide the tags, but should provide for someone to develop a module that provide the filters or tags. To summarize: I don't need your patch to do the inline images. I just need your patch to be able to allow someone else to develop a module that can display inline files. Kobus ------------------------------------------------------------------------ Mon, 18 Jul 2005 09:56:38 +0000 : Bèr Kessels okay, so for all clarity, let me sumarise: * this patch introduces a chackbox "inline". If checked, the file will show up inline. IT will be appended to the teaser and the body. * this patch does *not* allow one to place images of files in a userdefined place in the body. * this patch does *not* do any resizing nor any thumbnailing. * This patch is meant to be simple, clear and transparant. No tokens, no javascript, no nothing. ------------------------------------------------------------------------ Mon, 18 Jul 2005 11:21:54 +0000 : Kobus Then I can't see how this patch could replace the existing upload.module, which allows you to use inline.module to place images inline. I therefore withdraw my +1 and go -1 on this (not that it matters, I believe). I think you didn't properly read my previous message. I said I don't need your patch to do all this, but I can't live without the functionality currently provided by the upload.module and inline.module combination. I agree that your patch don't need to have that functionality, but if your patch takes away this functionality, in other words, I can't (by hook or by crook) manage what I need to do, my vote is -1. Regards, Kobus ------------------------------------------------------------------------ Mon, 18 Jul 2005 19:58:34 +0000 : Bèr Kessels Kobus, And all the others looking fofr advanced inline systems: please do not -1 this. It will hep inline module a lot too. allthough not yet in this patch, but can you imagine a perission system for inline module, that comes for free? Or a core system, that makes inline module ten times smaller? it is this path! so, please, if it is not /exactly/ what you are looking for, at least look at the advantages for Joe Average, and even for the possibilties for your favorite inline-module. even imag_assist will gain an anomous usability impcact when usiong this module, for it can then finally use the upload module in full scale. Ber ------------------------------------------------------------------------ Sat, 23 Jul 2005 18:59:10 +0000 : Bèr Kessels Sohodjo jim: " I hope it matters. A big -1 on enforcing over-simplification at the expense of important and existing functionality. If automatic, uncontrollable placement of images becomes the standard, it will mean fewer not more image-rich Drupal sites as site owners/developers get complaints about "Why can't I control my images!" I _strongly_ encourage Ber to provide a "have it both ways" solution. Why not have an admin configuration setting for "allow inline tags" with default off. Change it to on and you get an additional upload checkbox for 'inline at tag' to accommodate the current functionality of the inline module while simultaneously allowing for default placement. " NOOO. people; please; look at the PATCH. Again: and hopefully last time: Simplicity. This patch does NOT replace ANYTHING inline module or img_assist wants to do. This patch hands better data to such modules. It hands a variable over to these modules, $node->file[FID]->inline, which tells these sorts of modules if people wnat that file to appear inline. AND it CAN (by default will) render a file in the most simple way inline. So really, if you want to -1 this patch, fine. But do not -1 it, because it does too little IYO. It still allows MUCH more than what you can do no, without that patch! And any solution, for core, that allows advanced handling of inline images will either require an enourmous amount of work, or it will simply no get in. So, unless you come up with a good core-worthy patch, this is still a big leap forward from what we have now. ------------------------------------------------------------------------ Sat, 23 Jul 2005 19:01:51 +0000 : Bèr Kessels :$ this is an attempt to fix the borken HML in the previous follow up. ------------------------------------------------------------------------ Sun, 24 Jul 2005 04:07:34 +0000 : TDobes Please don't combine unrelated changes together in the same patch. Moving stuff in/out of the _nodeapi hook has absolutely nothing to do with the patch and makes it much harder to read through the patch and see what it's actually doing. I'd appreciate it if you could separate the changes made here which are really relevant and move the other changes into another issue. Also: You change form_group_collapsible(t('File attachments') to form_group_collapsible(t('File Attachments'). A quick glance at some other core code seems to indicate that we've standardized on capitalizing only the first word in multi-word group titles. As for the functionality itself, I don't think this belongs in upload.module. It would be useful to have the functionality in core, but as a separate module which can be switched on and off. (upload_inline.module or something like that) If a site were set up using an inlining system from contrib and the functionality you're proposing could not be switched off, it would be extremely confusing to users that there are two different ways of inlining images. A minor comment: I'm not so sure whether the results of this patch, when inlining non-image attachments, will be the one users expect. A non-web-savvy person may expect his/her Word document (for instance) to appear inline with the node when they click "inline" -- just like their images do. I'd suggest that we limit inlining to images only. I'm not as adamant about this as the other issues, so I could be convinced otherwise if others disagree. ------------------------------------------------------------------------ Sun, 24 Jul 2005 07:58:19 +0000 : Bèr Kessels Thank you for your thoughts Tdobbes. As for the "moving things in and out of nodeapi" This was necessary, because otherwise the patch would have made the code even more unreadable. I agree with you on this point, but I simply had to clean it up a little, in order to be able to work in it. As it stands now, it is simply not possible to make this a separate module. We cannot hook into the files table. In a next stage, I might work out some hook for that table. But first things first. Can Dries or Steven please comment on his. I want to know If i should bother spending this enormous amount of time on commenting and debating this issue. If the chances are small this is accepted, Ill just drop it This ridiculous long thread reminds me again why I try to avoid making patches for core. ------------------------------------------------------------------------ Mon, 25 Jul 2005 05:49:37 +0000 : Steven Overall I like the concept, but I think it at least needs some default styling to make the image appear okay, like floating it. Right now it appears inline between whatever was there... very ugly. Sure you can say "leave it up to the themer" but if the goal is to provide a simple default, we can't expect people to dive into theming if they are going to use this feature instead of img_assist or whatever. Also, I wonder about the classes "upload" and "image". Is there a reason to use two classes? Isn't "image" a bit too generic? Oh and there is a missing XHTML / for the img tag. ------------------------------------------------------------------------ Mon, 25 Jul 2005 12:42:26 +0000 : Bèr Kessels I will fix the broken Xhttml. I chose "image" because it is exactly the same as the .image class used by image.module. But I will see to it, to add more functionality. I agree that it is not up to the themer for some good defaults, thus I will add a line of CSS to drupal.css (*shiver* ;) ) upload, IMO does not communicate well enough what the Image is doing. Not sure yet how I ill fix this, but I will see to it to come up with a better CSS/theme solution. Thanks! ------------------------------------------------------------------------ Tue, 26 Jul 2005 14:18:23 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/upload_inline_0.patch (12.24 KB) Suggestions by Steven are now included. I very simple float is added to the inline images in drupal.css. XHTML fixed. And I cleaned up some old crufty comments, as suggested by Dries. ------------------------------------------------------------------------ Tue, 26 Jul 2005 17:05:45 +0000 : fax8 What is proposed here is a good idea. But I think that this is done in not good way. I think that upload.module should be only a layer between file system and modules. So modify upload module to add a specific feature like your inline for me is not good. but I like the idea to add checkboxes for enable interaction modules between modules and upload.module. Maybe we could design an of api wich let modules tell upload.module that they can interact with it. I make an example: I'm a developer of video.module. Now to add a video file to video.module one need to put the file into drupal folders using ftp and then insert the path to the file during node creation. One of the features our users are asking a lot is the possibility to directly upload a video file to dupal. So I should write a lot of code, settings, security buggy code.. that for me isn't useful. What about if only calling an upload.module api I should enable a new checkbox on upload form called "use as video file" which let me use the file as a video file on my module??? What do you think about it???? ------------------------------------------------------------------------ Tue, 26 Jul 2005 20:00:03 +0000 : Bèr Kessels fax: upload /is/ such a layer. Just look at how image module does it. You need not write any additional 'buggy' code, you can use all teh upload functionality. The upload interface (the table and list), however, is tightly integrated, so that drupal offers an out-of-the-box file system. Nox, my patch simply adds an "out of the box" inline functionality. Small and simple. For any more advanced file features you should use or write a module that, in turn, uses the uplaod.module. ------------------------------------------------------------------------ Wed, 27 Jul 2005 11:22:40 +0000 : stefan nagtegaal After discussing with Ber, I think this patch has some great potential though the simplicity of it.. For the people who don't completely agree what this patch does: - It adds an extra checkbox for each attachment. If the 'Inline' checkbox is checked, images are displayed inline in your post and other attachments (like .pdf, .ppt, or whatever) were transformed into links. The potential of this patch is in the fact that this makes it much, much easier to have another contrib module which makes it possible to let you choose at the node submission how you would like your node to be displayed, incl. the selected inline attachments to be displayed (using _nodeapi()). So, a big +1 from me on this.. ------------------------------------------------------------------------ Wed, 27 Jul 2005 11:27:52 +0000 : Kobus > - It adds an extra checkbox for each attachment. If the 'Inline' > checkbox is checked, images are displayed inline in your post and other > attachments (like .pdf, .ppt, or whatever) were transformed into links. Does this mean it is done automatically or do I have control over this? I don't want the images to be inline if I can't control where I put it. Let me just clarify my position on this short-and-sweet-like: If it breaks the existing functionality that upload.module + inline.module provides without opening the door for a replacement, then I am -1, otherwise +1. :-) Kobus ------------------------------------------------------------------------ Wed, 27 Jul 2005 11:51:50 +0000 : Bèr Kessels Kobus: Again: it does not break anything! It opens up enormous potential for modules such as inline/module. Because inline module now has access to a variable $node->files[fid]->inline. in other words: modules such as inline.module can now find out what images a user wants to see inline! ------------------------------------------------------------------------ Wed, 27 Jul 2005 11:59:53 +0000 : Kobus Ber: Great, then I +1 it. From our previous talks about this, it seemed as if this functionality was being taken away. ------------------------------------------------------------------------ Wed, 27 Jul 2005 16:18:21 +0000 : Sohodojo Jim Agreed, +1. The most on-going and especially most recent discussion has clarified that we can have our simple cake and contrib mod cake as well. Does this mean that inline module will work as is, or will a post-patch update be required? --Sohodojo Jim--

1 0

[drupal-devel] [feature] Roles should not be separated by a BR.
by Morbus Iff 27 Jul '05

27 Jul '05

Issue status update for http://drupal.org/node/20457 Project: Drupal Version: cvs Component: user.module Category: feature requests Priority: normal Assigned to: Morbus Iff Reported by: Morbus Iff Updated by: Morbus Iff Status: patch Attachment: http://drupal.org/files/issues/_patch_usermodulebr.patch (848 bytes) On admin/user, the user's roles are separated both by a comma and a [br /]. The comma is quite enough - the [br /] merely makes the page look retarded ("fat" rows) on screens that have plenty of width to accomodate the entire table without the BR. On smaller width'd browsers, the "fat" rows will return, but naturally wrapped, not forced with inline HTML. Likewise, [BR] and the comma are both delimiters: only one is needed (if we use [BR], the newline becomes the visual delimiter; if we use comma, then we don't need the newlines). The attached patch removes the [BR]. Morbus Iff

5 6

[drupal-devel] [feature] Localize date formats
by Bèr Kessels 27 Jul '05

27 Jul '05

Issue status update for http://drupal.org/node/11623 Post a follow up: http://drupal.org/project/comments/add/11623 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: pancho Reported by: Goba Updated by: Bèr Kessels Status: patch +1 for the idea. +1 for the implementation. The idea is simple: just pull the format string trough the locales. No new interfaces (+1), no new configuration options (+1) and no additional cruft. po files will then take care of the real format. However, I see one issue: we should remove the dopdowns in /admin/settings. The patch still applies. Bèr Kessels Previous comments: ------------------------------------------------------------------------ Fri, 15 Oct 2004 18:52:24 +0000 : Goba Attachment: http://drupal.org/files/issues/Drupal-translate-date-formats.patch (1.99 KB) Currently if I set a date format for the site, then it will be used with all interface translations. Different languages however have different needs when presenting dates. The Hungarian date format does not allow the day name to be the first and it calls for a dot between the hour and minute (yes, a dot not a colon). This means that Drupal ships with no suitable date format for Hungarians. Also none of the Hungarian date formats will be suitable on an English interface, while none of the English date formats are acceptable on a Hungarian interface on a bilingual site. The only option is to make date formats translateable. Since these are dynamic t() calls, extractor.php needs to be extended to include the date formats shipped with drupal in the pot output, so translators will get them (much like we do for some numbers and day names). I will take care of extending extractor.php, once this patch gets applied. ------------------------------------------------------------------------ Fri, 15 Oct 2004 19:00:27 +0000 : Goba Attachment: http://drupal.org/files/issues/Drupal-translate-date-formats_0.patch (1.99 KB) Well, the t() needs to be called on the date format set by the admin, so I needed to fix three t() calls. Updated patch attached. ------------------------------------------------------------------------ Fri, 15 Oct 2004 19:01:56 +0000 : killes(a)www.drop.org *g* I recall that there was a time when you didn't like this approach. Anyway, I fully endorse it and would like to see it in 4.5. There is also no suitable data format for German in core. And yes, that's a bug not a feature request. ------------------------------------------------------------------------ Fri, 15 Oct 2004 19:05:33 +0000 : Goba Yes, I didn't like it, because these are dynamic t()s. But since the selected format always needs to be presented in the actual language, it needs to be dynamic by nature. We have some of these dynamic t()s already, handled specially in extractor.php. ------------------------------------------------------------------------ Fri, 15 Oct 2004 23:07:29 +0000 : Steven I like this idea a lot, but I don't like the simplistic "wrap it all in t" feature. There are instances where format_date output is intended for script output. Changing those date formats would mean a bug in the code or UI (for example the way a node date is specified and parsed). Only those strings that are intended for user consumption should be translated. That's why I think we need a more specialized date handling component. It would deal with stuff like variations ('show hour/hide hour'), long/short variations (we need to review the usage of small/medium/long), as well as localizing them. We would need to revise the formats shipped with Drupal, but this gets tricky. For example would we only ship US-style AM/PM date formats with core? Should we only have M/D/Y ordering, or also D/M/Y ordering? If we move the time format settings to locale (where they do below, in principle) we might get a drop in usability: moving from a simple selection box, to requiring the whole localisation framework 'just to switch from 12 hours to 24 hours'. In some parts of the world, there is only one format for dates (e.g. chinese/japanese 2004年10月16日 1時00分). For them, the only variations are long vs short (whether to show the year, the time, etc), in other parts of the world all bets are off and people use what they like. This would mean that if we provide a default set of formats, and let them be translated through PO files, some languages might require more items than provided and some languages would have to use duplicates to fill all available options. On top of that, there is no guarantee that format #3 in French matches format #3 in German, so you'd need to be able to specify this per language. Ideally, we would let a language define any number of date formats, and let the user choose one of those per language. We could also provide a custom option where you enter your own date format, for the realy picky ones. The question remains: what do we do for sites without locale enabled? We can't go to requiring locale just to switch the date on a non-localized site. In that case, perhaps we could do this: - Have locale.module deal with date formats when it is enabled. Have a per-language format setting, selected from a language-specific, po-defined date format list. - Have system.module implement a 'localization' admin menu item if locale.module is /not/ enabled. This page carries the old selector, which acts like a simplified version of locale's format selector, with the list we current have in Drupal (i.e. variations of 24/12hours, d/m/y, y/m/d, m/d/y, ...). To prevent confusion, we add a link to system.module's version of the localization page, which instructs users to enable locale.module for full localization abilities). I know it sounds a bit weird (we certainly don't do this anywhere else), but it would be consistent in terms of UI (the date format is "localization" no matter how you look at it) and keep both non-localized site admins and localized site admins happy, without sacrificing too much ease of set up. ------------------------------------------------------------------------ Sat, 16 Oct 2004 09:34:09 +0000 : Goba Steven, you say: "/There are instances where format_date output is intended for script output. Changing those date formats would mean a bug in the code or UI/ " But the current approach is to translate the month and day names in every case, so format_date **already** assumes that the date it outputs is for human consumption and not for machines. So changing the order of the fields in user date formats should not hurt. Sure it might not be a useability dream, but it does not hurt. By the way, adjusting date formats based on user language is only a small piece of localisation not covered by Drupal core yet. I need to use one mission statement and site name in all languages, and even my primary and secondary links need to have the same text in all site languages, since they are not translateable. If we are about to find a generic solution for locale covered settings, we need to take these into account too. ------------------------------------------------------------------------ Fri, 29 Oct 2004 00:10:44 +0000 : Steven ""so format_date *already* assumes that the date it outputs is for human consumption and not for machines." " What I mean is changing the placement of the date components (day, month, year). For numerical dates which are parsed by a program, this will introduce bugs. I'd much rather have a cleaner date format selection method integrated with locale. ------------------------------------------------------------------------ Fri, 29 Oct 2004 13:19:01 +0000 : Goba Steven, the custom date formats are not translated, only those selected by the admin. And the admin can select date types with spelled out month/day names at any level, so they are not machine parsable anyway in the current Drupal version. **This patch does not introduce bugs.** I admit it does not look good, but there is no system for locale dependant admin settings yet. ------------------------------------------------------------------------ Fri, 29 Oct 2004 15:05:28 +0000 : Steven I'm saying we do need such a system. With your patch, the format that the admin selects in language A will not necessarily match the one in language B. We should go for locale-defined formats with a locale-neutral list to fallback on. ------------------------------------------------------------------------ Sun, 13 Mar 2005 14:55:22 +0000 : pancho Attachment: http://drupal.org/files/issues/dateformat.patch.zip (14.91 KB) ************************ DATEFORMAT-PATCH PACKAGE ************************ by pancho(a)suenderhauf.net 2005-03-13 03:07 This 'DateFormat' patch package only works with the latest CVS version of Drupal 4.60 RC. This 'DateFormat' patch package applies the following changes: 1. It introduces a more flexible system of standard date and time formats which helps both users and developers while staying backwards-compatible. We often see special date formats be used in modules, which is only second best, as these can neither be adapted by the user nor localized. More flexibility allows the developers to use the built-in format functions wherever possible. Changes in detail: - The standard date formats used to be combined date&time-formats. This is now split into separate date formats and time formats, to allow dates and times to be rendered in a more flexible way. - Still it's backward compatible: the combined formats - while deprecated - still exist. - A "Tiny" format is added to the "Small", "Medium" and "Large" formats. 2. It enables Drupal to show e.g. German and Hungarian date formats (with a colon as separator), which was _not_ possible before. 3. It prepares both the Drupal system and module developers for the upcoming locale_dateformat module. This module will fully localize date and time formats according to the localization of the current user. The locale_dateformat module will be available within the next weeks. I'm always happy about suggestions and criticism, if constructive. You can either write a comment on www.cvs.net or an email to pancho(a)suenderhauf.net. Bye, Pancho ------------- INSTALLATION: ------------- The package contains two patches for the latest CVS versions of Drupal 4.60 RC, both of which should be applied: 1. common.inc (patch for v.1.429) 2. system.module (patch for v.1.197) There are no changes in database structure. ------------------------------------------------------------------------ Sun, 13 Mar 2005 14:59:33 +0000 : pancho P.S. What the hell did I write about www.cvs.net? No, what I meant was of course www.drupal.org (here), where you can write a comment! ------------------------------------------------------------------------ Sun, 13 Mar 2005 15:08:35 +0000 : pancho I feel better if I refile the thread as a feature request. The only thing you could describe as a bug was that German & Hungarian date formats were not supported. The rest are definitely feature improvements. ------------------------------------------------------------------------ Sun, 13 Mar 2005 16:38:02 +0000 : Steven - Localizing date formats seems like something that should be solved properly in core once and tied into locale.module. We need a solution which does not pose problems with mixing languages, and which does not require us to keep adding more and more date formats to the list (e.g. what about asian date formats? They don't have spaces.) Your "if (!module_exist('locale_dateformat'))" solution is unacceptable for core. - What's the point of the 'tiny' date format if it's not used anywhere? Where would it be used? - Please don't post patches as zips, this is annoying. Just make one global patch file instead of separate ones. Multiple files can be posted with multiple follow ups. - You need to see how this affects the date usage around Drupal, e.g. the profile date field formatting, which depends on the date format being in a specific format. - Code style: check spaces around quotes. Also, avoid trailing spaces on lines, this is untidy. - This issue is not "critical" IMO. It is a new feature request, and as such should probably wait until after 4.6, as 4.6 is in feature freeze. - The apostrophes around the separator item seem a bit unnecessary. ------------------------------------------------------------------------ Mon, 14 Mar 2005 17:08:34 +0000 : pancho */First of all: This patch works! Even it may be not yet ready to be adopted in the core, it is good for individual use./* To Steven: Okay, Steven, I was asking for criticism... :) With some of the points you make I can agree: - This feature should be completely and properly rewritten and not just fixed to have a good solution for a longer period and not only for now. - The "if (!module_exist('locale_dateformat'))" solution is not good code style at least for core, as well as the spaces. I didn't take enough care about that, as I wanted to present a solution now, before the release of 4.6. - Sorry about the zipped patches. I didn't expect it to bother anyone. But on the other hand: - It's not about adding more and more date formats. You will agree that the German web is a big enough market which should not be left aside anymore. Therefore the feature request /is critical/ from my perspective at least. And the solution I presented doesn't blow up the date format lists, in the contrary it makes them easier to use. - How can you complain about the tiny version not to be used. If it doesn't exist it /can't be used/. - The apostrophees do make sense, because the separator can also be a single space. Nobody would identify that in the menu without apostrophees. So you were critizising some points which I will fix in a new version of the patch as soon as possible. But you didn't say anything about the split of date and time formats. To me it makes sense, as there have already been more people asking for "just date" or "just time". While I already started the development of a "locale_dateformat.module", I will put this on hold, until we figured out a good solution altogether. Anyway, I'm ready to help developing this feature within "locale.module" if I'm being asked. Otherwise developing a separate module would at least offer the so needed functionality /within this century/ ;-) Pancho ------------------------------------------------------------------------ Fri, 18 Mar 2005 07:50:14 +0000 : Steven Firstly: "Even it may be not yet ready to be adopted in the core, it is good for individual use. " The patch queue is really part of the Drupal development process. It does not help us much to work on patches that are not intended for core. "It's not about adding more and more date formats. You will agree that the German web is a big enough market which should not be left aside anymore. Therefore the feature request is critical from my perspective at least. And the solution I presented doesn't blow up the date format lists, in the contrary it makes them easier to use. " I'm just saying, that solutions like the German date format (which has a special notation for the day numbers) or Asian formats (which have no spaces and reversed order) should really be handled through locale IMO, where date formats are picked per language, and can be added manually or through .po files. "How can you complain about the tiny version not to be used. If it doesn't exist it can't be used. " I imagine you introduced this format for a reason, no? Adding new features without knowing exactly where they should be used is silly. If you add a new feature in a patch, you should put in the work to see that this feature is actually used. I didn't really talk about splitting date/time formats for now, as it is mostly a usability thing. I'd much rather wait for a solution integrated with locale before I comment on it.

1 0

[drupal-devel] [feature] Submission queue menu item should have count
by hba 27 Jul '05

27 Jul '05

Issue status update for http://drupal.org/node/19370 Project: Drupal Version: cvs Component: queue.module Category: feature requests Priority: normal Assigned to: Anonymous Reported by: hba Updated by: hba Status: active Attachment: http://drupal.org/files/issues/queue.module_0.patch (512 bytes) The 'submission queue' item in the menu block should have a count, so that users easily can see if there are new articles to vote for. Working patch attached. hba

2 2

[drupal-devel] [feature] Pass include and exclude parameters to cron.php for fine grained cron timing
by Bèr Kessels 27 Jul '05

27 Jul '05

Issue status update for http://drupal.org/node/19173 Post a follow up: http://drupal.org/project/comments/add/19173 Project: Drupal Version: cvs Component: base system Category: feature requests Priority: normal Assigned to: robertDouglass Reported by: robertDouglass Updated by: Bèr Kessels Status: patch -1 on this one. I think the hook_cron should carry the logic whether or not it wants to be ran. Maybe aintroduce an API that allows theis checkto be done easier? drupal_elapsed_run($name, interval) that will check when $name was last updated, if it was longer then $interval ago, return TRUE, and update the last run of $name. Bèr Kessels Previous comments: ------------------------------------------------------------------------ Sun, 20 Mar 2005 10:08:59 +0000 : robertDouglass Attachment: http://drupal.org/files/issues/cronpatch.txt (1.99 KB) Some sites need fine grained timing for cron runs. For example, a site that relies heavily on moblogging, mailhandler or any other email based function would want to connect with the email server pretty often, say ever 2-5 minutes. On another site I'm working on we generate flat files for all of a certain node type on a regular basis, though not every 2-5 minutes. Thus the need for cron tasks on different schedules. This patch allows you to make a list of modules to either include or exclude when running cron, so that several cron tasks for one site can be defined. The default behavior in the absence of both parameters is to run all, so the patch preserves backwards compatibility. * Examples: * * runs all hooks * http://mysite.com/cron.php * * runs all hooks except the search and aggregator modules * http://mysite.com/cron.php?exclude=search,aggregator * * runs only search, archive and aggregator modules * http://mysite.com/cron.php?includ=search,archive,aggregator ------------------------------------------------------------------------ Wed, 23 Mar 2005 21:23:59 +0000 : Dries Isn't it possible to run all cron functions every 2-5 minutes? The impact of that should be minimal. ------------------------------------------------------------------------ Wed, 23 Mar 2005 21:33:06 +0000 : moshe weitzman an alternative approach is to make a quick PHP page which simply calls mailhandler_cron and any others that need to be called more frequently than normal. then call that php page from cron every couple minutes ... not so sure this complexity is worth having. ------------------------------------------------------------------------ Thu, 24 Mar 2005 07:32:32 +0000 : robertDouglass Whether or not this "complexity" is needed depends a lot on what you expect to do with cron. As soon as one starts aggregating not tens but several hundred RSS feeds or more, it is impossible to run cron every 2 minutes. Yet if the site depends on mailhandler keeping user moblog submissions current, 2 minutes is already about the maximum latency time acceptable. Thus the need for crons on different schedules. In terms of usage, writing ?include=mailhandler,search,event isn't so complex. On the code level, splitting that list on comma and looping over the resultant array also isn't really that complex. The patch defaults to normal behavior for people who don't need the feature, and I've seen more than one site that had to solve this problem. ------------------------------------------------------------------------ Thu, 24 Mar 2005 10:48:09 +0000 : Robert Castelo Could different cron runs be set to run different tasks based on a cron ID number.... For instance, set up 3 separate cron to run every hour, each one calls the cron.php page including a number variable which runs a certain set of tasks. An Admin control panel would list all cron tasks and allow administrators to split them into separate cron runs based on the number variable passed to the script when calling cron.php. ------------------------------------------------------------------------ Thu, 24 Mar 2005 11:24:53 +0000 : robertDouglass Robert, the id parameter is already a practical necessity for this patch if cron_busy is to be effective. I'm open to suggestions as to how we would track various cron tasks and whether they are busy. First things first, though: does anyone except me need this? I'm not going to program an admin-configurable cron mechanism unless there is demand for it. Furthermore, such a system would really be incomplete unless there were some actual mechanism for setting these cron runs up from the admin interface as well. This wouldn't be such a bad idea, I know a lot of people who still type /cron.php into their browser every day or so to updated thier sites (yes I've mentioned poormanscron). Anyway, first reactions to this patch have been cold, so maybe nobody wants it. ------------------------------------------------------------------------ Thu, 24 Mar 2005 11:39:13 +0000 : Robert Castelo +1 from me. I'll be needing something like this once my email newsletter is released and starts hogging cron runs. ------------------------------------------------------------------------ Thu, 24 Mar 2005 16:06:31 +0000 : Chris Johnson It does seem like a good idea to allow running different cron tasks at different intervals, so as to be efficient and reduce web server loads. There is little point in running tasks that do not need to be run. Many cron tasks only need to be run once to a few times per day. Note also that cron job scheduling on most Unix-variants is only within a +/- 1 minute range, so trying to run a cron program every 2 minutes is approaching the crumbling edge of being "on time." ------------------------------------------------------------------------ Thu, 24 Mar 2005 16:37:39 +0000 : robertDouglass I think I am leaning in favor of extending cron.php in the following way: 1) Add admin screen which scans all modules for existence of cron hook 2) set a cron_interval variable for each one 3) allow admin to set an interval 4) check whether the current cron run falls outside of each module's interval before invoking its cron hook 5) before a module's cron hook gets called a {module_name}_cron_busy variable gets set. This would function like the current cron_busy now, just at a modular level. The admin would then set one cron tab to run at the smallest needed interval and could then decide on a module-by-module basis how often each is to run. How does this sound to people?

1 0

[drupal-devel] [feature] add inline functionaliy to uploads
by Sohodojo Jim 27 Jul '05

27 Jul '05

Issue status update for http://drupal.org/node/26288 Post a follow up: http://drupal.org/project/comments/add/26288 Project: Drupal Version: cvs Component: upload.module Category: feature requests Priority: normal Assigned to: Anonymous Reported by: Bèr Kessels Updated by: Sohodojo Jim Status: patch Agreed, +1. The most on-going and especially most recent discussion has clarified that we can have our simple cake and contrib mod cake as well. Does this mean that inline module will work as is, or will a post-patch update be required? --Sohodojo Jim-- Sohodojo Jim Previous comments: ------------------------------------------------------------------------ Sun, 03 Jul 2005 18:03:35 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/upload_inline.patch (19.05 KB) One of the most often asked features is proper inlnie handling of files. Look at the amount of solutions, the popularity of image_assist, and the amount of peolpe dowloading image.module! That alone should be enough proof that Drupal lacks proper inline image support. This patch adds that to core. In fact, it does little more then appending a link of img tag to the body or the teaser. Off course that is configurable per file. Next to the [] list checkbox, this patch adds an [] inline checkbox. Simplicity is the foundation of this patch. I want no stles for inline editing, no fancy html wrappers, no tokens, just $node->body or teaser appended with a small html string. Another small themable funtion is introduced, (hey, you cannot expect me to develop something without adding more power for themers, now, can you? ;) ), that allows people to theme the string that is appended to the body or the teaser. Oh, and also note hat the biggest part of this patch is some cleaning I had to do in order to be able to develop properly. I dont like Ifs inside cases in foreaches inside swiches. in other words: nodeapi now calls functions instead of executing code directly. ------------------------------------------------------------------------ Sun, 03 Jul 2005 18:19:25 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/inline.patch.screenshot.png (26.68 KB) here is how the form now looks ------------------------------------------------------------------------ Sun, 03 Jul 2005 18:19:58 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/inline.patch.screenshot3.png (30.53 KB) and this is an example of inlined images and a .doc file. ------------------------------------------------------------------------ Sun, 03 Jul 2005 20:44:00 +0000 : sepeck changing to patch per request from berkes ------------------------------------------------------------------------ Tue, 05 Jul 2005 07:46:13 +0000 : Kobus This gets a +1 from me in principle, however, the [inline:xx] tag with inline.module gives you greater freedom as to where the inline image must be displayed. If you can add this functionality (I haven't checked the code, I don't know if it is in there) it would be a great addition for Core. This same strategy can be used for inline blocks, I am sure. Regards, Kobus ------------------------------------------------------------------------ Tue, 05 Jul 2005 08:57:50 +0000 : Bèr Kessels there are a couple of reasons why i did not include the [inline] tags. * I aimed for extreme simplicity: a checkbox shows an image inline: its up to the theme where it appears (if one does not like it before/above the body and teaser.). Simplicity was the main goal. * We don't have any tokens in core. And we should not have them. * Tokens are a very bad substitute for a good interface. They give less power then plain HTLM. Are much worst documented then HTML, but in the mean time, they are still as hard to learn as HTML. (Yes, I know people _think_ they are easier, but there _is_ not difference between [ ] and , only that its a different ascii char. So, no. I don't allow any placement of the image. I leave that that for dedicated modules, or the themer to decide. ------------------------------------------------------------------------ Tue, 05 Jul 2005 09:34:06 +0000 : Kobus So you will provide an API to do this? For example, with perhaps minor modifications, the inline.module will be able to display these files? In that case I am happy. If not, then I can't give my support to this patch (as if that matters...). A themer can't do this task as inline images (and blocks for that matter) is too dynamic for theming; it can be placed anywhere in a node where the user pleases. This means for me that there should be a module for this, such as inline module that allows you to define [inline:xx] tags. If your module emits an array of uploaded files (such as upload.module), inline.module can be, with minor changes, adapted to show these files inline. To show you what I mean with the content is too dynamic for a themer to perform this task, look at this screenshot related to inline blocks (inline images can follow the same pattern): http://drupal.org/files/issues/regions---possibility-3.png ------------------------------------------------------------------------ Tue, 05 Jul 2005 11:26:15 +0000 : Bèr Kessels Kobus, We are dealing with different issues here: You want a method to place images, files or so anywhere in the post. That is fine, but certainly NOT addressed in the patch! I made patch that *only* adds marked files to the body. its really nothing more then <?php $node->body = $filestring . $node->body ?> No APIS, no, dynamic tokens, no filters, nothing. However, what I meant with themers, is that there now is a theme_upload_inline available, so you can theme the abovementioned $filestring. On top of that $node->files[FILEID]->inline is TRUE if a file is flagged for inline. So in node.tpl.php, or wherever you want to theme a node, you can print nice images inline, when that flag is set. And about the comment that a themer cant do this: Simply not true. On most sites images are always placed in the same places. REally, even the sites see which use img_assist or inline, use them to place the images on the exact same places in every node. People /think/ they want the power to place images anywhere, but they hardly ever use that power. Just look at all the big news/publishing sites out there (BBC, CNN, BoigBoing, r even freshmeat) images are all placed acc. to the theme. They are not placed in random places by authors. So if you are one of the few that still want that power, there aer mots of power tools like inline module or img_assist. We should offer a good default, one that is simple. ------------------------------------------------------------------------ Tue, 05 Jul 2005 11:41:34 +0000 : Gerhard Killesreiter I fully agree that tags are evil. Kobus: You can always look for tags in $node->body in your theme's node function. ------------------------------------------------------------------------ Tue, 05 Jul 2005 11:50:52 +0000 : stefan nagtegaal Ber, the theme-function in your code looks like: <?php +/** + * Theme function for rendering of inline images + * @param $file a file object. + * @param $image a Boolean, indicating whether an img tag (TRUE) or an anchor tag (FALSE) should be used. + * @ingroup themable + */ +function theme_upload_inline($file, $image) { + if ($image) { + return '<img src="'. check_url(($file->fid ? file_create_url($file->filepath) : url(file_create_filename($file->filename, file_create_path())))) .'" alt="'. check_plain($file->filename) .'" class="upload inline">'; + } + else { + return ''. check_plain($file->filename) .' [1]'; + } + +} ?> After looking at your code it's not clear to me when $image is TRUE or FALSE, can you elaborate on me please? [1] http://drupal.org/'. check_url(($file->fid ? file_create_url($file->filepath) : url(file_create_filename($file->filename, file_create_path())))) .'\" class=\"upload inline ------------------------------------------------------------------------ Tue, 05 Jul 2005 12:06:39 +0000 : Bèr Kessels The function calling the theme function should decide whether its an image. IMO that is far too hardcore code for a themer ;) <?php $image = ereg('^(image/)', $file->filemime); ?> inside _upload_inline() does the trick. I did find one issue, though, with svgs, which are image/svg so maybe we should limit this to really only inline jpg, png, and gif, by extension? But I am no fan of determining files by extension, and IMO getimgsize is too heavy; ------------------------------------------------------------------------ Tue, 05 Jul 2005 12:13:52 +0000 : stefan nagtegaal Can't you make use of PHP's <?php image_type_to_mime_type(); ?> or <?php image_type_to_extension(); ?> ------------------------------------------------------------------------ Tue, 05 Jul 2005 12:45:43 +0000 : Kobus Well, if not in the patch, I need inline functionality one way or another. inline.module works perfectly with the current upload.module to fulfill my needs, and if there is a way where someone can extend your proposed upload.module, that would get my +1, otherwise I will just remain neutral about this issue. inline.module makes use of $node->files and displays that image. If your patch still uses $node->files, then this is a non-issue. In other words, if I can, with inline.module, or with minor modifications to inline.module still show files inline, I am +1 for this. As for the themability question... I still disagree. You say "Simply not true. On most sites images are always placed in the same places." That is ONLY true because they have NO alternatives. Means the content is in the same places, because they have no other places to put them. I have made tens of static sites where the content does not resemble a fixed pattern or structure. For me, free-flow layout is my expression of my creativity. This is why I still even BOTHER with static sites; when Drupal don't do what I want it to do. If Drupal could do inline display of content properly, I'd never even build another static site. It is a great mistake people make (including myself), and that is they design the site before ANY content is available, and this happens a lot in static sites. But in dynamic sites, you have far less control over this, unless you have a VERY clean structure with limited information that can be added, and only a few people posting content. If you can have your content before you start the design, you will have a better fit. I can't see how you can anticipate every possible posting posted on your site if you have a diversity of users. Especially not if you're using a site such as an designer's showcase site where your creativity is shown by your web sites. Possible, but not easy. If I claim that I have "unique, fresh designs" I can certainly NOT give them a "box-like" site with "left and right side bars only". That's why I need inline content, this includes images AND boxes. Gerhard: Your concern about tags is answered as well above, if I am not mistaken. The patch need not provide the tags, but should provide for someone to develop a module that provide the filters or tags. To summarize: I don't need your patch to do the inline images. I just need your patch to be able to allow someone else to develop a module that can display inline files. Kobus ------------------------------------------------------------------------ Mon, 18 Jul 2005 09:56:38 +0000 : Bèr Kessels okay, so for all clarity, let me sumarise: * this patch introduces a chackbox "inline". If checked, the file will show up inline. IT will be appended to the teaser and the body. * this patch does *not* allow one to place images of files in a userdefined place in the body. * this patch does *not* do any resizing nor any thumbnailing. * This patch is meant to be simple, clear and transparant. No tokens, no javascript, no nothing. ------------------------------------------------------------------------ Mon, 18 Jul 2005 11:21:54 +0000 : Kobus Then I can't see how this patch could replace the existing upload.module, which allows you to use inline.module to place images inline. I therefore withdraw my +1 and go -1 on this (not that it matters, I believe). I think you didn't properly read my previous message. I said I don't need your patch to do all this, but I can't live without the functionality currently provided by the upload.module and inline.module combination. I agree that your patch don't need to have that functionality, but if your patch takes away this functionality, in other words, I can't (by hook or by crook) manage what I need to do, my vote is -1. Regards, Kobus ------------------------------------------------------------------------ Mon, 18 Jul 2005 19:58:34 +0000 : Bèr Kessels Kobus, And all the others looking fofr advanced inline systems: please do not -1 this. It will hep inline module a lot too. allthough not yet in this patch, but can you imagine a perission system for inline module, that comes for free? Or a core system, that makes inline module ten times smaller? it is this path! so, please, if it is not /exactly/ what you are looking for, at least look at the advantages for Joe Average, and even for the possibilties for your favorite inline-module. even imag_assist will gain an anomous usability impcact when usiong this module, for it can then finally use the upload module in full scale. Ber ------------------------------------------------------------------------ Sat, 23 Jul 2005 18:59:10 +0000 : Bèr Kessels Sohodjo jim: " I hope it matters. A big -1 on enforcing over-simplification at the expense of important and existing functionality. If automatic, uncontrollable placement of images becomes the standard, it will mean fewer not more image-rich Drupal sites as site owners/developers get complaints about "Why can't I control my images!" I _strongly_ encourage Ber to provide a "have it both ways" solution. Why not have an admin configuration setting for "allow inline tags" with default off. Change it to on and you get an additional upload checkbox for 'inline at tag' to accommodate the current functionality of the inline module while simultaneously allowing for default placement. " NOOO. people; please; look at the PATCH. Again: and hopefully last time: Simplicity. This patch does NOT replace ANYTHING inline module or img_assist wants to do. This patch hands better data to such modules. It hands a variable over to these modules, $node->file[FID]->inline, which tells these sorts of modules if people wnat that file to appear inline. AND it CAN (by default will) render a file in the most simple way inline. So really, if you want to -1 this patch, fine. But do not -1 it, because it does too little IYO. It still allows MUCH more than what you can do no, without that patch! And any solution, for core, that allows advanced handling of inline images will either require an enourmous amount of work, or it will simply no get in. So, unless you come up with a good core-worthy patch, this is still a big leap forward from what we have now. ------------------------------------------------------------------------ Sat, 23 Jul 2005 19:01:51 +0000 : Bèr Kessels :$ this is an attempt to fix the borken HML in the previous follow up. ------------------------------------------------------------------------ Sun, 24 Jul 2005 04:07:34 +0000 : TDobes Please don't combine unrelated changes together in the same patch. Moving stuff in/out of the _nodeapi hook has absolutely nothing to do with the patch and makes it much harder to read through the patch and see what it's actually doing. I'd appreciate it if you could separate the changes made here which are really relevant and move the other changes into another issue. Also: You change form_group_collapsible(t('File attachments') to form_group_collapsible(t('File Attachments'). A quick glance at some other core code seems to indicate that we've standardized on capitalizing only the first word in multi-word group titles. As for the functionality itself, I don't think this belongs in upload.module. It would be useful to have the functionality in core, but as a separate module which can be switched on and off. (upload_inline.module or something like that) If a site were set up using an inlining system from contrib and the functionality you're proposing could not be switched off, it would be extremely confusing to users that there are two different ways of inlining images. A minor comment: I'm not so sure whether the results of this patch, when inlining non-image attachments, will be the one users expect. A non-web-savvy person may expect his/her Word document (for instance) to appear inline with the node when they click "inline" -- just like their images do. I'd suggest that we limit inlining to images only. I'm not as adamant about this as the other issues, so I could be convinced otherwise if others disagree. ------------------------------------------------------------------------ Sun, 24 Jul 2005 07:58:19 +0000 : Bèr Kessels Thank you for your thoughts Tdobbes. As for the "moving things in and out of nodeapi" This was necessary, because otherwise the patch would have made the code even more unreadable. I agree with you on this point, but I simply had to clean it up a little, in order to be able to work in it. As it stands now, it is simply not possible to make this a separate module. We cannot hook into the files table. In a next stage, I might work out some hook for that table. But first things first. Can Dries or Steven please comment on his. I want to know If i should bother spending this enormous amount of time on commenting and debating this issue. If the chances are small this is accepted, Ill just drop it This ridiculous long thread reminds me again why I try to avoid making patches for core. ------------------------------------------------------------------------ Mon, 25 Jul 2005 05:49:37 +0000 : Steven Overall I like the concept, but I think it at least needs some default styling to make the image appear okay, like floating it. Right now it appears inline between whatever was there... very ugly. Sure you can say "leave it up to the themer" but if the goal is to provide a simple default, we can't expect people to dive into theming if they are going to use this feature instead of img_assist or whatever. Also, I wonder about the classes "upload" and "image". Is there a reason to use two classes? Isn't "image" a bit too generic? Oh and there is a missing XHTML / for the img tag. ------------------------------------------------------------------------ Mon, 25 Jul 2005 12:42:26 +0000 : Bèr Kessels I will fix the broken Xhttml. I chose "image" because it is exactly the same as the .image class used by image.module. But I will see to it, to add more functionality. I agree that it is not up to the themer for some good defaults, thus I will add a line of CSS to drupal.css (*shiver* ;) ) upload, IMO does not communicate well enough what the Image is doing. Not sure yet how I ill fix this, but I will see to it to come up with a better CSS/theme solution. Thanks! ------------------------------------------------------------------------ Tue, 26 Jul 2005 14:18:23 +0000 : Bèr Kessels Attachment: http://drupal.org/files/issues/upload_inline_0.patch (12.24 KB) Suggestions by Steven are now included. I very simple float is added to the inline images in drupal.css. XHTML fixed. And I cleaned up some old crufty comments, as suggested by Dries. ------------------------------------------------------------------------ Tue, 26 Jul 2005 17:05:45 +0000 : fax8 What is proposed here is a good idea. But I think that this is done in not good way. I think that upload.module should be only a layer between file system and modules. So modify upload module to add a specific feature like your inline for me is not good. but I like the idea to add checkboxes for enable interaction modules between modules and upload.module. Maybe we could design an of api wich let modules tell upload.module that they can interact with it. I make an example: I'm a developer of video.module. Now to add a video file to video.module one need to put the file into drupal folders using ftp and then insert the path to the file during node creation. One of the features our users are asking a lot is the possibility to directly upload a video file to dupal. So I should write a lot of code, settings, security buggy code.. that for me isn't useful. What about if only calling an upload.module api I should enable a new checkbox on upload form called "use as video file" which let me use the file as a video file on my module??? What do you think about it???? ------------------------------------------------------------------------ Tue, 26 Jul 2005 20:00:03 +0000 : Bèr Kessels fax: upload /is/ such a layer. Just look at how image module does it. You need not write any additional 'buggy' code, you can use all teh upload functionality. The upload interface (the table and list), however, is tightly integrated, so that drupal offers an out-of-the-box file system. Nox, my patch simply adds an "out of the box" inline functionality. Small and simple. For any more advanced file features you should use or write a module that, in turn, uses the uplaod.module. ------------------------------------------------------------------------ Wed, 27 Jul 2005 11:22:40 +0000 : stefan nagtegaal After discussing with Ber, I think this patch has some great potential though the simplicity of it.. For the people who don't completely agree what this patch does: - It adds an extra checkbox for each attachment. If the 'Inline' checkbox is checked, images are displayed inline in your post and other attachments (like .pdf, .ppt, or whatever) were transformed into links. The potential of this patch is in the fact that this makes it much, much easier to have another contrib module which makes it possible to let you choose at the node submission how you would like your node to be displayed, incl. the selected inline attachments to be displayed (using _nodeapi()). So, a big +1 from me on this.. ------------------------------------------------------------------------ Wed, 27 Jul 2005 11:27:52 +0000 : Kobus > - It adds an extra checkbox for each attachment. If the 'Inline' > checkbox is checked, images are displayed inline in your post and other > attachments (like .pdf, .ppt, or whatever) were transformed into links. Does this mean it is done automatically or do I have control over this? I don't want the images to be inline if I can't control where I put it. Let me just clarify my position on this short-and-sweet-like: If it breaks the existing functionality that upload.module + inline.module provides without opening the door for a replacement, then I am -1, otherwise +1. :-) Kobus ------------------------------------------------------------------------ Wed, 27 Jul 2005 11:51:50 +0000 : Bèr Kessels Kobus: Again: it does not break anything! It opens up enormous potential for modules such as inline/module. Because inline module now has access to a variable $node->files[fid]->inline. in other words: modules such as inline.module can now find out what images a user wants to see inline! ------------------------------------------------------------------------ Wed, 27 Jul 2005 11:59:53 +0000 : Kobus Ber: Great, then I +1 it. From our previous talks about this, it seemed as if this functionality was being taken away.

1 0

[drupal-devel] [task] Support for Taxonomy Images in theme settings.
by Bèr Kessels 27 Jul '05

27 Jul '05

Issue status update for http://drupal.org/node/14027 Post a follow up: http://drupal.org/project/comments/add/14027 Project: Drupal Version: cvs Component: system.module Category: tasks Priority: normal Assigned to: Anonymous Reported by: Anonymous Updated by: Bèr Kessels Status: patch -1 I dislike any if(module_exist) for contrib modules in core. Core should not go calling /any/ contributions Bèr Kessels Previous comments: ------------------------------------------------------------------------ Tue, 07 Dec 2004 15:11:57 +0000 : Anonymous Attachment: http://drupal.org/files/issues/system_taxonomy_images.patch (727 bytes) This patch adds a checkbox on admin/themes/settings to toggle display of taxonomy images, only if this module is installed and activated. I've also sent a patch to phptemplate theme engine that it uses this checkbox to fill a new variable usable for templates. ------------------------------------------------------------------------ Tue, 07 Dec 2004 15:19:13 +0000 : Anonymous PHPtemplate patch that uses this new checkbox: http://drupal.org/node/14028 ------------------------------------------------------------------------ Tue, 07 Dec 2004 15:23:03 +0000 : Anonymous Patch for box_grey theme: http://drupal.org/node/14029 ------------------------------------------------------------------------ Wed, 15 Dec 2004 01:45:09 +0000 : jxs2151 I am unable to get this working after applying all three patches. When patching the phptemplate engine I get this error: patching file phptemplate.engine Hunk #1 succeeded at 73 (offset 1 line). Hunk #2 FAILED at 239. Hunk #3 succeeded at 237 (offset -23 lines). 1 out of 3 hunks FAILED -- saving rejects to file phptemplate.engine.rej The checkbox for allowing Taxonomy images in posts appears, I created the category, uploaded the image. I can view the image by direct reference in a url, it just does not show up on the page. ------------------------------------------------------------------------ Thu, 16 Dec 2004 23:50:38 +0000 : carlosparamio Attachment: http://drupal.org/files/issues/phptemplate_taxonomy_images_0.patch (1.31 KB) It seems that phptemplate.engine file has changed a little. Here is a new patch. When this patch was applied, then you can use the array $taxonomy_images[] in your node.tpl.php theme template to access to the image files for correspondant terms. For example, $taxonomy_images[0] to access to the image file for the first term. This is what box_grey patch does. ------------------------------------------------------------------------ Thu, 16 Dec 2004 23:52:48 +0000 : carlosparamio Attachment: http://drupal.org/files/issues/phptemplate_taxonomy_images_4.5.0.patch (1.13 KB) And here is a patch for Drupal 4.5.0 release also. ------------------------------------------------------------------------ Mon, 21 Feb 2005 17:56:13 +0000 : grohk Attachment: http://drupal.org/files/issues/system_module_taxonomy_images.patch (507 bytes) Trying to keep this idea alive by resectioning to the correct place (I think) and freshening the patch against the latest CVS system.module. Please forgive me it this is inappropriate, but I think this patch is important for many users.

1 0

[drupal-devel] Open Season
by Steven Wittens 27 Jul '05

27 Jul '05

You probably noticed, but the patch queue has been growing steadily. We're at 9 pages now... and as we all know, a veritable apocalypse will fall on our heads if the page count reaches 10... or maybe not. Still, if you have some time to spare, please hop on over to http://drupal.org/patches and do some patch reviewing. 'Thumb up' stuff that is good to go, but be critical of everything: pick patches apart, challenge design decisions and look for crufty code. You should follow up on your own pet-patches of course, but it never hurts to get your hands dirty and work on other people's patches too. It's open season for bug hunting. The dogs are riled up, the game is plentyful and I'm wearing my lucky hat. Steven

1 0