- Development - lists.drupal.org

[drupal-devel] cvs tag -b DRUPAL-4-5 problem
by Axel 19 Feb '05

19 Feb '05

Hi. I upload module to contribs CVS and commit it. It's OK. But now I try to branch it for 4.5: $ cd modules/snippets $ cvs tag -b DRUPAL-4-5 cvs tag: warning: directory CVS specified in argument cvs tag: but CVS uses CVS for its own purposes; skipping CVS directory cvs tag: warning: taginfo line contains no format strings: "/cvs/drupal-contrib/CVSROOT/xcvs-taginfo.php /cvs/drupal-contrib/CVSROOT/xcvs-config.php axel" Filling in old defaults ('%t %o %p %{sv}'), but please be aware that this usage is deprecated. Invalid tag for /s/snippets. cvs tag: Pre-tag check failed cvs [tag aborted]: correct the above errors first! Oops :( It not my first upload to contrib and previous modules tagged successfuly, but now I don't tag it anymore. And I got similar problem with csvfilter - I can't add new file to 4.5 branch. Same error appeared. Where am I wrong? -- Alexander Grafov <axel(a)drupal.ru> Drupal Russia - www.drupal.ru

1 0

Re: [drupal-devel] Project management contribs: Bundles: grouped modules.
by Robert Douglass 19 Feb '05

19 Feb '05

I like the idea of having a vetting team which puts together a list of recommended modules. This should be nothing more than a list. If they are stored separately or not is unimportant, the important aspect being that someone who was given the job looked at the work and endorsed it. This is not to say that modules not on the "A" list are bad, but they might take more work to install, or are less stable/tested, or are not as actively supported and developed. Most of us on this list would still download them, install them and make our own decisions. Less capable users would rightfully avoid them. I like the idea of bundles too, but only as a way of suggesting to users what works best. This should in no way be an enforced grouping and should only be an alternative way to select modules. Example: image.module and img_assist.module (4.5 versions). Both would be listed in the "A" list, and in a separate list, a bundle called "image bundle" would offer both together, as well as special instructions for getting them both up and running. This approach has several advantages. First, it takes the best parts of everybody's ideas (so far). Second, it requires very few changes to our current system. Third, we have nothing to lose. We don't lose the good modules that everyone likes, and we don't lose the wild ones which can be pretty bad and useless, but like mentioned already, play an important role in the community. In the end, it is a presentation and packaging problem. The two things which we would change to achive this are: 1) set up a vetting committee and guidelines (not too restrictive - shouldn't become a huge political thing) 2) extend our build process to offer bundle tarballs.

2 1

[drupal-devel] Modules up for adoption
by Mark 19 Feb '05

19 Feb '05

Greetings drupal-devel, Sometime in the next month or 2, I'm going to be pulled into a non-Drupal project for a few months. As I won't have much time to spend on the modules that I've created thus far, I'm placing them up for adoption. Anyone interested in taking over the maintenance of any of these please let me know. I don't want to see them rot on the vine and would rather somebody potentially take them in a different direction then have them sit unused and neglected. attached_node <http://drupal.org/node/11988> Status: Working Needs: Future maintenance and occasional bug fixes (currently a few to look at) editasnew <http://drupal.org/node/16806> Status: Working Needs: Future maintenance and occasional bug fixes relativity <http://drupal.org/node/17004> Status: Functional but not quite 100%. Will be soon, though. Needs: TLC, proper documentation/help, rethink the UI a bit (simplify it) moblog <http://cvs.drupal.org/viewcvs/drupal/contributions/sandbox/javanaut/moblog/> Status: Functional but not very robust. Relies on mime_registry. Neglected for quite a while now. Needs: Current and future maintenance. Needs to be made stable enough to be part of a public project. This is not a simple project, so buyer beware. mime_registry <http://cvs.drupal.org/viewcvs/drupal/contributions/sandbox/javanaut/mime_re…> Status: Functional based on other drupal 4.5.1 modules (image,filestore2,etc). Neglected for quite a while now. Needs: An advocate to promote this to other module developers (so it's hooks are supported). Currently, there are dependencies on specific versions of other contrib modules. Also not a simple project. I'll still be around for another month or two, so anyone taking over a module won't be standing out in the cold wondering why the hell I wrote something the way I did (though I might not always have an answer :P ). This is from a forum post on drupal.org: http://drupal.org/node/17482 Thanks, Mark

4 3

[drupal-devel] Re: [contributions:axel] /modules/snippets snippets.module snippets.mysql
by Gerhard Killesreiter 18 Feb '05

18 Feb '05

On 18 Feb 2005 drupal-cvs(a)drupal.org wrote: > User: axel Branch: HEAD Date: Fri, 18 Feb 2005 00:21:26 +0000 > > Added files: > /modules/snippets snippets.module snippets.mysql > > Log message: > Alpha version (some features broken, some uncomplete) of the > Snippets. For WaterWiki. Axel, can you please include this module with waterwiki in a subdirectory if it is only usable with it? Cheers, Gerhard

2 1

[drupal-devel] [task] Pager Queries, Node Access and Performance
by mathias 18 Feb '05

18 Feb '05

Project: Drupal Version: cvs Component: node.module Category: tasks Priority: normal Assigned to: Anonymous Reported by: pyromanfo Updated by: mathias Status: patch +1 as well This patch would eliminate the performance penalty sites currently have that only use node-level permissions for group editing, and keep all content public. mathias Previous comments: ------------------------------------------------------------------------ February 1, 2005 - 09:54 : pyromanfo I've got a Drupal 4.5.2 site running with Taxonomy Access Control. When you go to view a taxonomy and it tries to do an access check, the SQL query takes literally minutes to complete. This is on a dual Xeon box, it's rather beefy and runs two other sites (vBulletin and a hacked up Postnuke site) just fine. The problem comes from the taxonomy_select_nodes function $sql_count = 'SELECT COUNT(DISTINCT(n.nid)) FROM {node} n '. node_access_join_sql() .' INNER JOIN {term_node} tn ON n.nid = tn.nid WHERE tn.tid IN ('. $str_tids .') AND n.status = 1 AND '. node_access_where_sql(); Which ends up looking like SELECT COUNT(DISTINCT(n.nid)) FROM node n INNER JOIN node_access na ON (na.nid = 0 OR na.nid = n.nid) INNER JOIN term_node tn0 ON n.nid = tn0.nid WHERE n.status = 1 AND na.grant_view = 1 AND CONCAT(na.realm, na.gid) IN ('all0','term_access1') AND tn0.tid IN (9) This will take several minutes to complete. The query next to it that selected the nodes themselves took 3 minutes to complete. Where is the problem exactly? I've seen elsewhere that this is a problem for node_privacy_byrole as well, which makes sense because really all the various access control modules are contributing here is an extra entry in the concat search. Let me know if you need any details of how the MySQL server is configured. Here's some data from my drupal database mysql> select count(nid) from node; +------------+ | count(nid) | +------------+ | 10376 | +------------+ 1 row in set (0.00 sec) mysql> select count(nid) from node_access; +------------+ | count(nid) | +------------+ | 41508 | +------------+ 1 row in set (0.00 sec) Drupal performs very snappily with the node_access stuff disabled, and otherwise it performs very well. A three minute increase in the execute time of an SQL query is just strange, there's something odd going on here I think. ------------------------------------------------------------------------ February 3, 2005 - 15:02 : pyromanfo Reducing the size of the 'realm' field of node_access to something manageable like 12 instead of the current 255 speeds up the query tremendously, still it's rather slow. I also added a search index on the grant_view permission, since the view permission is the only permission you'd want to check for more than one node usually. It did speed it up, however it still take a minute or so on my little test box. I'm going to tweak with it some more and see if I can get it performing better. ------------------------------------------------------------------------ February 3, 2005 - 17:12 : killes(a)www.drop.org Could you give us an estimate with numbers? Or even better, make available the database for testing? ------------------------------------------------------------------------ February 4, 2005 - 13:59 : pyromanfo I can't really make the database available without going through and setting all the text and titles to something random and the uid's to 2, I may try that. I can give you some more numbers tonight though when I start messing with it again. You want execution times and table sizes? Any other specific numbers? Should I just dump show status? ------------------------------------------------------------------------ February 4, 2005 - 14:21 : killes(a)www.drop.org Averages of execution times before and after would be fine, I think. The db would be great of course You could simple set all text to "foo" and email addresses to "root@localhost". ------------------------------------------------------------------------ February 5, 2005 - 11:31 : pyromanfo I'm running these tests on a similar database I haven't been developing on. Created from the same source data with the same script though, I just haven't been hacking it up and it runs on faster hardware. SELECT COUNT(DISTINCT(n.nid)) FROM node n INNER JOIN node_access na ON (na.nid = 0 OR na.nid = n.nid) INNER JOIN term_node tn0 ON n.nid = tn0.nid WHERE n.status = 1 AND na.grant_view = 1 AND CONCAT(na.realm, na.gid) IN ('all0','term_access1') AND tn0.tid IN (2); +------------------------+ | COUNT(DISTINCT(n.nid)) | +------------------------+ | 2993 | +------------------------+ 1 row in set (7 min 4.00 sec) Then I ran the following queries alter table node_access modify realm varchar(12) NOT NULL default ''; alter table node_access add index view_indx (nid,gid,realm,grant_view); Then I ran mysqlcheck --auto_repair -os Then the query again SELECT COUNT(DISTINCT(n.nid)) FROM node n INNER JOIN node_access na ON (na.nid = 0 OR na.nid = n.nid) INNER JOIN term_node tn0 ON n.nid = tn0.nid WHERE n.status = 1 AND na.grant_view = 1 AND CONCAT(na.realm, na.gid) IN ('all0','term_access1') AND tn0.tid IN (2); +------------------------+ | COUNT(DISTINCT(n.nid)) | +------------------------+ | 2993 | +------------------------+ 1 row in set (6 min 18.62 sec) Not as big a speed boost as I thought I was seeing unfortunately. Here's 'show variables' mysql> show variables; +---------------------------------+---------------------------------------------------+ | Variable_name | Value | +---------------------------------+---------------------------------------------------+ | back_log | 50 | | basedir | /usr/local/mysql/ | | binlog_cache_size | 32768 | | bulk_insert_buffer_size | 8388608 | | character_set_client | latin1 | | character_set_connection | latin1 | | character_set_database | latin1 | | character_set_results | latin1 | | character_set_server | latin1 | | character_set_system | utf8 | | character_sets_dir | /usr/local/mysql/share/mysql/charsets/ | | collation_connection | latin1_swedish_ci | | collation_database | latin1_swedish_ci | | collation_server | latin1_swedish_ci | | concurrent_insert | ON | | connect_timeout | 5 | | datadir | /usr/local/mysql/var/ | | date_format | %Y-%m-%d | | datetime_format | %Y-%m-%d %H:%i:%s | | default_week_format | 0 | | delay_key_write | ON | | delayed_insert_limit | 100 | | delayed_insert_timeout | 300 | | delayed_queue_size | 1000 | | expire_logs_days | 0 | | flush | OFF | | flush_time | 0 | | ft_boolean_syntax | + -><()~*:""&| | | ft_max_word_len | 84 | | ft_min_word_len | 4 | | ft_query_expansion_limit | 20 | | ft_stopword_file | (built-in) | | group_concat_max_len | 1024 | | have_archive | NO | | have_bdb | NO | | have_compress | YES | | have_crypt | YES | | have_csv | NO | | have_example_engine | NO | | have_geometry | YES | | have_innodb | YES | | have_isam | NO | | have_ndbcluster | NO | | have_openssl | NO | | have_query_cache | YES | | have_raid | NO | | have_rtree_keys | YES | | have_symlink | YES | | init_connect | | | init_file | | | init_slave | | | innodb_additional_mem_pool_size | 1048576 | | innodb_autoextend_increment | 8 | | innodb_buffer_pool_awe_mem_mb | 0 | | innodb_buffer_pool_size | 8388608 | | innodb_data_file_path | ibdata1:10M:autoextend | | innodb_data_home_dir | | | innodb_fast_shutdown | ON | | innodb_file_io_threads | 4 | | innodb_file_per_table | OFF | | innodb_locks_unsafe_for_binlog | OFF | | innodb_flush_log_at_trx_commit | 1 | | innodb_flush_method | | | innodb_force_recovery | 0 | | innodb_lock_wait_timeout | 50 | | innodb_log_arch_dir | | | innodb_log_archive | OFF | | innodb_log_buffer_size | 1048576 | | innodb_log_file_size | 5242880 | | innodb_log_files_in_group | 2 | | innodb_log_group_home_dir | ./ | | innodb_max_dirty_pages_pct | 90 | | innodb_table_locks | ON | | innodb_max_purge_lag | 0 | | innodb_mirrored_log_groups | 1 | | innodb_open_files | 300 | | innodb_thread_concurrency | 8 | | interactive_timeout | 28800 | | join_buffer_size | 131072 | | key_buffer_size | 8388600 | | key_cache_age_threshold | 300 | | key_cache_block_size | 1024 | | key_cache_division_limit | 100 | | language | /usr/local/mysql/share/mysql/english/ | | large_files_support | ON | | license | GPL | | local_infile | ON | | locked_in_memory | OFF | | log | OFF | | log_bin | OFF | | log_error | | | log_slave_updates | OFF | | log_slow_queries | OFF | | log_update | OFF | | log_warnings | 1 | | long_query_time | 10 | | low_priority_updates | OFF | | lower_case_file_system | OFF | | lower_case_table_names | 0 | | max_allowed_packet | 1048576 | | max_binlog_cache_size | 4294967295 | | max_binlog_size | 1073741824 | | max_connect_errors | 10 | | max_connections | 100 | | max_delayed_threads | 20 | | max_error_count | 64 | | max_heap_table_size | 16777216 | | max_insert_delayed_threads | 20 | | max_join_size | 4294967295 | | max_length_for_sort_data | 1024 | | max_relay_log_size | 0 | | max_seeks_for_key | 4294967295 | | max_sort_length | 1024 | | max_tmp_tables | 32 | | max_user_connections | 0 | | max_write_lock_count | 4294967295 | | myisam_data_pointer_size | 4 | | myisam_max_extra_sort_file_size | 2147483648 | | myisam_max_sort_file_size | 2147483647 | | myisam_recover_options | OFF | | myisam_repair_threads | 1 | | myisam_sort_buffer_size | 8388608 | | net_buffer_length | 16384 | | net_read_timeout | 30 | | net_retry_count | 10 | | net_write_timeout | 60 | | new | OFF | | old_passwords | ON | | open_files_limit | 1024 | | pid_file | ********** | | port | 3306 | | preload_buffer_size | 32768 | | protocol_version | 10 | | query_alloc_block_size | 8192 | | query_cache_limit | 1048576 | | query_cache_min_res_unit | 4096 | | query_cache_size | 0 | | query_cache_type | ON | | query_cache_wlock_invalidate | OFF | | query_prealloc_size | 8192 | | range_alloc_block_size | 2048 | | read_buffer_size | 131072 | | read_only | OFF | | read_rnd_buffer_size | 262144 | | relay_log_purge | ON | | rpl_recovery_rank | 0 | | secure_auth | OFF | | server_id | 0 | | skip_external_locking | ON | | skip_networking | OFF | | skip_show_database | OFF | | slave_net_timeout | 3600 | | slow_launch_time | 2 | | socket | /tmp/mysql.sock | | sort_buffer_size | 2097144 | | sql_mode | | | storage_engine | MyISAM | | sync_binlog | 0 | | sync_frm | ON | | system_time_zone | EST | | table_cache | 64 | | table_type | MyISAM | | thread_cache_size | 0 | | thread_stack | 196608 | | time_format | %H:%i:%s | | time_zone | SYSTEM | | tmp_table_size | 33554432 | | tmpdir | | | transaction_alloc_block_size | 8192 | | transaction_prealloc_size | 4096 | | tx_isolation | REPEATABLE-READ | | version | 4.1.7 | | version_comment | Source distribution | | version_compile_machine | i686 | | version_compile_os | pc-linux | | wait_timeout | 28800 | +---------------------------------+---------------------------------------------------+ 176 rows in set (0.03 sec) I'm attaching my test database which takes several minutes to execute a similar query If you want me to run the tests on the test database I can do that, it's just on a much skimpier machine and it takes quite a bit longer (Athlon 600, 256MB Ram vs Dual Xeon). ------------------------------------------------------------------------ February 5, 2005 - 11:33 : pyromanfo Oops, preview removed the test db, here it is. ------------------------------------------------------------------------ February 5, 2005 - 11:38 : pyromanfo It's too big, it's 4.47 MB, I'm gonna put it up on GWJ and link to it. Get it here [1] [1] http://www.gamerswithjobs.com/testdb/drupal_test_db.tar.bz2 ------------------------------------------------------------------------ February 5, 2005 - 11:46 : pyromanfo Attachment: http://drupal.org/files/issues/drupal_test_db.tar.bz2 (734.78 KB) Nevermind, I left in some sensitive info and forgot to wipe the search index, now it's much smaller. ------------------------------------------------------------------------ February 7, 2005 - 16:58 : killes(a)www.drop.org Thanks, but I think you stripped the DB a bit too much. If I execute the query it is very fast because it does not return anything... ------------------------------------------------------------------------ February 7, 2005 - 17:12 : killes(a)www.drop.org When I use the query from your original post, I get some result. I guess I will write a script and run some tests over night... ------------------------------------------------------------------------ February 7, 2005 - 17:55 : killes(a)www.drop.org Ok, here are some preliminary results: - The unchanged query from the original post runs nearly 14 minutes on the hardware redLED lent to us. - If I shorten the varchar field to 18 from 255, it gets about halfed. - neither adding an index for the realm nor the grant_view field does make a change - Omitting the "na.nid = 0 OR" part makes the query very fast (quarter of a second) - SELECT COUNT(DISTINCT(n.nid)) FROM node n INNER JOIN node_access na ON ( na.nid = n.nid) left jo in node_access nb ON nb.nid = 0 INNER JOIN term_node tn0 ON n.nid = tn0.nid WHERE n.status = 1 AND ((n a.grant_view = 1 AND CONCAT(na.realm, na.gid) IN ('all0','term_access1')) OR (nb.grant_view = 1 AND CON CAT(nb.realm, nb.gid) IN ('all0','term_access1'))) AND tn0.tid IN (9); This query is still very much faster (about half a second) than the original query and gives the same result for your database. I don't know if it is fully equivalent, though. Note that it is a left join. ------------------------------------------------------------------------ February 8, 2005 - 07:06 : pyromanfo The na.nid = 0 stuff is basically a catchall, that's sort of a default permission that overrides anything else. This is good when you're not using node_access, however when you are the na.nid=0 stuff probably isn't there, I know Node Privacy By Role and Taxonomy Access Control both delete the entries for nid=0. Perhaps it's a good idea to control this another way, by setting a variable and using that to determine whether or not some sort of node_access is installed, as it seems using nid = 0 as a default simply kills performance. ------------------------------------------------------------------------ February 8, 2005 - 07:08 : pyromanfo Oh, and thanks for taking a good look at it killes, I should've explained better. The DB I provided, the test DB, has a large number of nodes in tid=9, the other DB I ran the tests on has alot in tid=2. ------------------------------------------------------------------------ February 8, 2005 - 10:17 : Anonymous The more I think about replacing nid=0 with a configuration setting the more it makes sense. The two major node access modules right now have to have a seperate "enable/disable" setting to deal with nid=0, since it overrides everything. In fact, it seems like it's only purpose it to override all other node access settings with a universal default. Why not instead have a global configuration setting under permissions that enables or disable node level permissions, then the various node access modules wouldn't need a seperate disable/enable setting. Furthermore, this performance problem would disappear, because either the node_access stuff would be there and you aren't looking for nid=0, or it isn't and you don't care what's in node_access. It seems it'd save queries to node access when you're not using node level permissions, and make it cleaner to implement a node access modifying module. ------------------------------------------------------------------------ February 8, 2005 - 10:17 : pyromanfo I hate the site randomly logging you out, that was me above. ------------------------------------------------------------------------ February 8, 2005 - 13:23 : JonBob "The more I think about replacing nid=0 with a configuration setting the more it makes sense. The two major node access modules right now have to have a seperate "enable/disable" setting to deal with nid=0, since it overrides everything. In fact, it seems like it's only purpose it to override all other node access settings with a universal default." This is not the case. I think you're confusing the case of nid=0 with the case of realm=all. As an example of the utility of nid=0, consider the scenario given in the example access module [2]. This module implements a simple system whereby anyone can view "public" nodes, and only users with a specific permission can view "private" nodes. This can be rephrased as follows: anonymous users can only view "public" nodes, while privileged users can view all nodes. A grant with nid=0 takes care of the second case, so that the module only needs to deal with granting permission to view the public nodes as necessary. Gerhard's query works in the case where every node has at least one node_access entry with its nid, but I'm afraid it fails in the above example. A node that is private would not have any corresponding entry, and so the inner join would fail for it, returning no record. The left join doesn't even have a chance to be matched. It would be worth benchmarking the scenario in which the nid=0 possibility is removed from the query, but the node_access table has an entry added for each node instead of a single nid=0 entry. If this is faster than the OR in the join, the extra DB storage is probably worth the change. [2] http://drupaldocs.org/api/head/file/contributions/docs/developer/examples/n… ------------------------------------------------------------------------ February 8, 2005 - 19:08 : pyromanfo "As an example of the utility of nid=0, consider the scenario given in the example access module. This module implements a simple system whereby anyone can view "public" nodes, and only users with a specific permission can view "private" nodes. This can be rephrased as follows: anonymous users can only view "public" nodes, while privileged users can view all nodes. A grant with nid=0 takes care of the second case, so that the module only needs to deal with granting permission to view the public nodes as necessary. " I think there's a bug in the module, in the docs in the top shouldn't the initial SQL statement be adding that permission to gid=1? Though you're right, I'd forgotten because I hadn't seen any of the current modules use it. As for that performance comparison mysql> SELECT COUNT(DISTINCT(n.nid)) FROM node n INNER JOIN term_node tn0 ON n.nid = tn0.nid INNER JOIN node_access na ON (na.nid = n.nid) WHERE n.status = 1 AND na.grant_view = 1 AND CONCAT(na.realm, na.gid) IN ('all0','term_access1') AND tn0.tid IN (2); +------------------------+ | COUNT(DISTINCT(n.nid)) | +------------------------+ | 2993 | +------------------------+ 1 row in set (0.23 sec) mysql> update node_access set realm='blam' where realm='term_access'; Query OK, 41508 rows affected (5.42 sec) Rows matched: 41508 Changed: 41508 Warnings: 0 mysql> insert into node_access (nid,gid,realm,grant_view) values (0,1,'term_access',1); Query OK, 1 row affected (0.00 sec) mysql> SELECT COUNT(DISTINCT(n.nid)) FROM node n INNER JOIN term_node tn0 ON n.nid = tn0.nid INNER JOIN node_access na ON (na.nid = 0 OR na.nid=n.nid) WHERE n.status = 1 AND na.grant_view = 1 AND CONCAT(na.realm, na.gid) IN ('all0','term_access1') AND tn0.tid IN (2); +------------------------+ | COUNT(DISTINCT(n.nid)) | +------------------------+ | 2993 | +------------------------+ 1 row in set (0.16 sec) So I don't think it improves performance for the nid=0 case, though it is suprisingly slow. ------------------------------------------------------------------------ February 8, 2005 - 19:57 : moshe weitzman perhaps we should split the query into 2 queries in order to avoid the 'na.nid = 0' part of the JOIN. Alternatively, perhaps we could inject that clause only when needed using the db_rewrite_sql hook. not sure. ------------------------------------------------------------------------ February 9, 2005 - 22:36 : pyromanfo Splitting it into two queries would make sense I think. Basically you could have it entirely in node_access_join_sql and node_access_where_sql. There we can check for the nid=0 case for any gids and realms. Then if it returns true, we know we don't have to check anything else, so we return null for the SQL to enter. We could even store this in a static variable to avoid calling this twice for join_sql and where_sql So in the nid=0 case, this should be similarly fast as we're removing a table from the JOIN in exchange for checking node_access for one nid. mysql> select count(nid) from node_access where nid=0 and gid=1 and realm in ('all','term_access','blah'); +------------+ | count(nid) | +------------+ | 0 | +------------+ 1 row in set (0.37 sec) SELECT COUNT(DISTINCT(n.nid)) FROM node n INNER JOIN term_node tn0 ON n.nid = tn0.nid WHERE n.status = 1 AND tn0.tid IN (9); +------------------------+ | COUNT(DISTINCT(n.nid)) | +------------------------+ | 4202 | +------------------------+ 1 row in set (0.15 sec) SELECT COUNT(DISTINCT(n.nid)) FROM node n INNER JOIN node_access na ON (na.nid = 0 OR na.nid = n.nid) INNER JOIN term_node tn0 ON n.nid = tn0.nid WHERE n.status = 1 AND na.grant_view = 1 AND CONCAT(na.realm, na.gid) IN ('all0','blah1') AND tn0.tid IN (9); +------------------------+ | COUNT(DISTINCT(n.nid)) | +------------------------+ | 4202 | +------------------------+ 1 row in set (1.97 sec) It looks to be faster, actually. Then, when we don't get a result, we check node access with the 'na.nid=0 OR' removed. This is certain to be faster as the test have shown, taking the query down from 7 minutes to a matter of seconds. In either case, this method would be faster. Plus, it wouldn't require any changes to other modules, simply change the node_access_where_sql and node_access_join_sql functions. JonBob, would you be okay with that? I can code it if you think you'll accept the patch. ------------------------------------------------------------------------ February 10, 2005 - 07:38 : Anonymous Splitting the queries into two is probably the only way to go. However, I'd like to avoid calling the nid = 0 query if possible. My Drupal installations that use node access do not use nid = 0 at all. I think we could store the available (nid = 0) => realm combinations somewhere and only execute the query if a matching condition is found. Gerhard ------------------------------------------------------------------------ February 10, 2005 - 09:14 : moshe weitzman that nid=0 is going to be very fast when done all by itself. no need to introduce any other mechanism IMO. testing will tell us for sure though ------------------------------------------------------------------------ February 10, 2005 - 12:31 : killes(a)www.drop.org The nid = 0 query would be executed for every node query on a page call even if your access module doesn't use nid = 0 at all. Most implementations do not use nid = 0. If we can avoid a query we should do so. ------------------------------------------------------------------------ February 11, 2005 - 14:00 : JonBob Attachment: http://drupal.org/files/issues/node_access.patch (2.91 KB) Here's a patch that splits the nid=0 check off into a separate query. If the user has such a grant, things should be really fast since we don't need any join or even DISTINCT() check. If not, the join is done as before, but without the "OR nid=0" piece. This ideally needs performance checking on systems using node access with nid=0 rows, using it without them, and not using it at all. ------------------------------------------------------------------------ February 14, 2005 - 06:33 : killes(a)www.drop.org Ok, here is a first test result: After installing the node_privacy_by_role module for the drupal.org database, the Drupal install became unusable for all but the No. 1 user and the load went high due to many hanging mysql processes. After applying the patch the site does work again. The module seems to generate one entry per node and role in the node_access table. I did not make any benchmarks, the result is so obvious. ------------------------------------------------------------------------ February 14, 2005 - 08:10 : killes(a)www.drop.org I've used a script provided by JonBob to create entries in the node_access table and installed the node_access_example module. The script generated about 500 node_access entries. I then benchmarked the tracker page. Results: 11.78 +- 0.48 s with patch 17.04 +- 0.70 s without patch The patched version needs only 70% of the time the non patched version needs. ------------------------------------------------------------------------ February 14, 2005 - 08:13 : Anonymous I haven't run numbers, but subjectively my test site seems much faster with the patch in the case where there is a nid=0 row, a bit faster where there is no such row (but not as obvious), and about the same with node access turned off. ------------------------------------------------------------------------ February 18, 2005 - 12:11 : JonBob I've reviewed the changes once again, and I'd like to see this committed. Seems a very necessary performance improvement, and more and more sites are using node-level access rights and would benefit from it. -- View: http://drupal.org/node/16558 Edit: http://drupal.org/project/comments/add/16558

1 0

[drupal-devel] [task] Pager Queries, Node Access and Performance
by JonBob 18 Feb '05

18 Feb '05

Project: Drupal Version: cvs Component: node.module Category: tasks Priority: normal Assigned to: Anonymous Reported by: pyromanfo Updated by: JonBob Status: patch I've reviewed the changes once again, and I'd like to see this committed. Seems a very necessary performance improvement, and more and more sites are using node-level access rights and would benefit from it. JonBob Previous comments: ------------------------------------------------------------------------ February 1, 2005 - 10:54 : pyromanfo I've got a Drupal 4.5.2 site running with Taxonomy Access Control. When you go to view a taxonomy and it tries to do an access check, the SQL query takes literally minutes to complete. This is on a dual Xeon box, it's rather beefy and runs two other sites (vBulletin and a hacked up Postnuke site) just fine. The problem comes from the taxonomy_select_nodes function $sql_count = 'SELECT COUNT(DISTINCT(n.nid)) FROM {node} n '. node_access_join_sql() .' INNER JOIN {term_node} tn ON n.nid = tn.nid WHERE tn.tid IN ('. $str_tids .') AND n.status = 1 AND '. node_access_where_sql(); Which ends up looking like SELECT COUNT(DISTINCT(n.nid)) FROM node n INNER JOIN node_access na ON (na.nid = 0 OR na.nid = n.nid) INNER JOIN term_node tn0 ON n.nid = tn0.nid WHERE n.status = 1 AND na.grant_view = 1 AND CONCAT(na.realm, na.gid) IN ('all0','term_access1') AND tn0.tid IN (9) This will take several minutes to complete. The query next to it that selected the nodes themselves took 3 minutes to complete. Where is the problem exactly? I've seen elsewhere that this is a problem for node_privacy_byrole as well, which makes sense because really all the various access control modules are contributing here is an extra entry in the concat search. Let me know if you need any details of how the MySQL server is configured. Here's some data from my drupal database mysql> select count(nid) from node; +------------+ | count(nid) | +------------+ | 10376 | +------------+ 1 row in set (0.00 sec) mysql> select count(nid) from node_access; +------------+ | count(nid) | +------------+ | 41508 | +------------+ 1 row in set (0.00 sec) Drupal performs very snappily with the node_access stuff disabled, and otherwise it performs very well. A three minute increase in the execute time of an SQL query is just strange, there's something odd going on here I think. ------------------------------------------------------------------------ February 3, 2005 - 16:02 : pyromanfo Reducing the size of the 'realm' field of node_access to something manageable like 12 instead of the current 255 speeds up the query tremendously, still it's rather slow. I also added a search index on the grant_view permission, since the view permission is the only permission you'd want to check for more than one node usually. It did speed it up, however it still take a minute or so on my little test box. I'm going to tweak with it some more and see if I can get it performing better. ------------------------------------------------------------------------ February 3, 2005 - 18:12 : killes(a)www.drop.org Could you give us an estimate with numbers? Or even better, make available the database for testing? ------------------------------------------------------------------------ February 4, 2005 - 14:59 : pyromanfo I can't really make the database available without going through and setting all the text and titles to something random and the uid's to 2, I may try that. I can give you some more numbers tonight though when I start messing with it again. You want execution times and table sizes? Any other specific numbers? Should I just dump show status? ------------------------------------------------------------------------ February 4, 2005 - 15:21 : killes(a)www.drop.org Averages of execution times before and after would be fine, I think. The db would be great of course You could simple set all text to "foo" and email addresses to "root@localhost". ------------------------------------------------------------------------ February 5, 2005 - 12:31 : pyromanfo I'm running these tests on a similar database I haven't been developing on. Created from the same source data with the same script though, I just haven't been hacking it up and it runs on faster hardware. SELECT COUNT(DISTINCT(n.nid)) FROM node n INNER JOIN node_access na ON (na.nid = 0 OR na.nid = n.nid) INNER JOIN term_node tn0 ON n.nid = tn0.nid WHERE n.status = 1 AND na.grant_view = 1 AND CONCAT(na.realm, na.gid) IN ('all0','term_access1') AND tn0.tid IN (2); +------------------------+ | COUNT(DISTINCT(n.nid)) | +------------------------+ | 2993 | +------------------------+ 1 row in set (7 min 4.00 sec) Then I ran the following queries alter table node_access modify realm varchar(12) NOT NULL default ''; alter table node_access add index view_indx (nid,gid,realm,grant_view); Then I ran mysqlcheck --auto_repair -os Then the query again SELECT COUNT(DISTINCT(n.nid)) FROM node n INNER JOIN node_access na ON (na.nid = 0 OR na.nid = n.nid) INNER JOIN term_node tn0 ON n.nid = tn0.nid WHERE n.status = 1 AND na.grant_view = 1 AND CONCAT(na.realm, na.gid) IN ('all0','term_access1') AND tn0.tid IN (2); +------------------------+ | COUNT(DISTINCT(n.nid)) | +------------------------+ | 2993 | +------------------------+ 1 row in set (6 min 18.62 sec) Not as big a speed boost as I thought I was seeing unfortunately. Here's 'show variables' mysql> show variables; +---------------------------------+---------------------------------------------------+ | Variable_name | Value | +---------------------------------+---------------------------------------------------+ | back_log | 50 | | basedir | /usr/local/mysql/ | | binlog_cache_size | 32768 | | bulk_insert_buffer_size | 8388608 | | character_set_client | latin1 | | character_set_connection | latin1 | | character_set_database | latin1 | | character_set_results | latin1 | | character_set_server | latin1 | | character_set_system | utf8 | | character_sets_dir | /usr/local/mysql/share/mysql/charsets/ | | collation_connection | latin1_swedish_ci | | collation_database | latin1_swedish_ci | | collation_server | latin1_swedish_ci | | concurrent_insert | ON | | connect_timeout | 5 | | datadir | /usr/local/mysql/var/ | | date_format | %Y-%m-%d | | datetime_format | %Y-%m-%d %H:%i:%s | | default_week_format | 0 | | delay_key_write | ON | | delayed_insert_limit | 100 | | delayed_insert_timeout | 300 | | delayed_queue_size | 1000 | | expire_logs_days | 0 | | flush | OFF | | flush_time | 0 | | ft_boolean_syntax | + -><()~*:""&| | | ft_max_word_len | 84 | | ft_min_word_len | 4 | | ft_query_expansion_limit | 20 | | ft_stopword_file | (built-in) | | group_concat_max_len | 1024 | | have_archive | NO | | have_bdb | NO | | have_compress | YES | | have_crypt | YES | | have_csv | NO | | have_example_engine | NO | | have_geometry | YES | | have_innodb | YES | | have_isam | NO | | have_ndbcluster | NO | | have_openssl | NO | | have_query_cache | YES | | have_raid | NO | | have_rtree_keys | YES | | have_symlink | YES | | init_connect | | | init_file | | | init_slave | | | innodb_additional_mem_pool_size | 1048576 | | innodb_autoextend_increment | 8 | | innodb_buffer_pool_awe_mem_mb | 0 | | innodb_buffer_pool_size | 8388608 | | innodb_data_file_path | ibdata1:10M:autoextend | | innodb_data_home_dir | | | innodb_fast_shutdown | ON | | innodb_file_io_threads | 4 | | innodb_file_per_table | OFF | | innodb_locks_unsafe_for_binlog | OFF | | innodb_flush_log_at_trx_commit | 1 | | innodb_flush_method | | | innodb_force_recovery | 0 | | innodb_lock_wait_timeout | 50 | | innodb_log_arch_dir | | | innodb_log_archive | OFF | | innodb_log_buffer_size | 1048576 | | innodb_log_file_size | 5242880 | | innodb_log_files_in_group | 2 | | innodb_log_group_home_dir | ./ | | innodb_max_dirty_pages_pct | 90 | | innodb_table_locks | ON | | innodb_max_purge_lag | 0 | | innodb_mirrored_log_groups | 1 | | innodb_open_files | 300 | | innodb_thread_concurrency | 8 | | interactive_timeout | 28800 | | join_buffer_size | 131072 | | key_buffer_size | 8388600 | | key_cache_age_threshold | 300 | | key_cache_block_size | 1024 | | key_cache_division_limit | 100 | | language | /usr/local/mysql/share/mysql/english/ | | large_files_support | ON | | license | GPL | | local_infile | ON | | locked_in_memory | OFF | | log | OFF | | log_bin | OFF | | log_error | | | log_slave_updates | OFF | | log_slow_queries | OFF | | log_update | OFF | | log_warnings | 1 | | long_query_time | 10 | | low_priority_updates | OFF | | lower_case_file_system | OFF | | lower_case_table_names | 0 | | max_allowed_packet | 1048576 | | max_binlog_cache_size | 4294967295 | | max_binlog_size | 1073741824 | | max_connect_errors | 10 | | max_connections | 100 | | max_delayed_threads | 20 | | max_error_count | 64 | | max_heap_table_size | 16777216 | | max_insert_delayed_threads | 20 | | max_join_size | 4294967295 | | max_length_for_sort_data | 1024 | | max_relay_log_size | 0 | | max_seeks_for_key | 4294967295 | | max_sort_length | 1024 | | max_tmp_tables | 32 | | max_user_connections | 0 | | max_write_lock_count | 4294967295 | | myisam_data_pointer_size | 4 | | myisam_max_extra_sort_file_size | 2147483648 | | myisam_max_sort_file_size | 2147483647 | | myisam_recover_options | OFF | | myisam_repair_threads | 1 | | myisam_sort_buffer_size | 8388608 | | net_buffer_length | 16384 | | net_read_timeout | 30 | | net_retry_count | 10 | | net_write_timeout | 60 | | new | OFF | | old_passwords | ON | | open_files_limit | 1024 | | pid_file | ********** | | port | 3306 | | preload_buffer_size | 32768 | | protocol_version | 10 | | query_alloc_block_size | 8192 | | query_cache_limit | 1048576 | | query_cache_min_res_unit | 4096 | | query_cache_size | 0 | | query_cache_type | ON | | query_cache_wlock_invalidate | OFF | | query_prealloc_size | 8192 | | range_alloc_block_size | 2048 | | read_buffer_size | 131072 | | read_only | OFF | | read_rnd_buffer_size | 262144 | | relay_log_purge | ON | | rpl_recovery_rank | 0 | | secure_auth | OFF | | server_id | 0 | | skip_external_locking | ON | | skip_networking | OFF | | skip_show_database | OFF | | slave_net_timeout | 3600 | | slow_launch_time | 2 | | socket | /tmp/mysql.sock | | sort_buffer_size | 2097144 | | sql_mode | | | storage_engine | MyISAM | | sync_binlog | 0 | | sync_frm | ON | | system_time_zone | EST | | table_cache | 64 | | table_type | MyISAM | | thread_cache_size | 0 | | thread_stack | 196608 | | time_format | %H:%i:%s | | time_zone | SYSTEM | | tmp_table_size | 33554432 | | tmpdir | | | transaction_alloc_block_size | 8192 | | transaction_prealloc_size | 4096 | | tx_isolation | REPEATABLE-READ | | version | 4.1.7 | | version_comment | Source distribution | | version_compile_machine | i686 | | version_compile_os | pc-linux | | wait_timeout | 28800 | +---------------------------------+---------------------------------------------------+ 176 rows in set (0.03 sec) I'm attaching my test database which takes several minutes to execute a similar query If you want me to run the tests on the test database I can do that, it's just on a much skimpier machine and it takes quite a bit longer (Athlon 600, 256MB Ram vs Dual Xeon). ------------------------------------------------------------------------ February 5, 2005 - 12:33 : pyromanfo Oops, preview removed the test db, here it is. ------------------------------------------------------------------------ February 5, 2005 - 12:38 : pyromanfo It's too big, it's 4.47 MB, I'm gonna put it up on GWJ and link to it. Get it here [1] [1] http://www.gamerswithjobs.com/testdb/drupal_test_db.tar.bz2 ------------------------------------------------------------------------ February 5, 2005 - 12:46 : pyromanfo Attachment: http://drupal.org/files/issues/drupal_test_db.tar.bz2 (734.78 KB) Nevermind, I left in some sensitive info and forgot to wipe the search index, now it's much smaller. ------------------------------------------------------------------------ February 7, 2005 - 17:58 : killes(a)www.drop.org Thanks, but I think you stripped the DB a bit too much. If I execute the query it is very fast because it does not return anything... ------------------------------------------------------------------------ February 7, 2005 - 18:12 : killes(a)www.drop.org When I use the query from your original post, I get some result. I guess I will write a script and run some tests over night... ------------------------------------------------------------------------ February 7, 2005 - 18:55 : killes(a)www.drop.org Ok, here are some preliminary results: - The unchanged query from the original post runs nearly 14 minutes on the hardware redLED lent to us. - If I shorten the varchar field to 18 from 255, it gets about halfed. - neither adding an index for the realm nor the grant_view field does make a change - Omitting the "na.nid = 0 OR" part makes the query very fast (quarter of a second) - SELECT COUNT(DISTINCT(n.nid)) FROM node n INNER JOIN node_access na ON ( na.nid = n.nid) left jo in node_access nb ON nb.nid = 0 INNER JOIN term_node tn0 ON n.nid = tn0.nid WHERE n.status = 1 AND ((n a.grant_view = 1 AND CONCAT(na.realm, na.gid) IN ('all0','term_access1')) OR (nb.grant_view = 1 AND CON CAT(nb.realm, nb.gid) IN ('all0','term_access1'))) AND tn0.tid IN (9); This query is still very much faster (about half a second) than the original query and gives the same result for your database. I don't know if it is fully equivalent, though. Note that it is a left join. ------------------------------------------------------------------------ February 8, 2005 - 08:06 : pyromanfo The na.nid = 0 stuff is basically a catchall, that's sort of a default permission that overrides anything else. This is good when you're not using node_access, however when you are the na.nid=0 stuff probably isn't there, I know Node Privacy By Role and Taxonomy Access Control both delete the entries for nid=0. Perhaps it's a good idea to control this another way, by setting a variable and using that to determine whether or not some sort of node_access is installed, as it seems using nid = 0 as a default simply kills performance. ------------------------------------------------------------------------ February 8, 2005 - 08:08 : pyromanfo Oh, and thanks for taking a good look at it killes, I should've explained better. The DB I provided, the test DB, has a large number of nodes in tid=9, the other DB I ran the tests on has alot in tid=2. ------------------------------------------------------------------------ February 8, 2005 - 11:17 : Anonymous The more I think about replacing nid=0 with a configuration setting the more it makes sense. The two major node access modules right now have to have a seperate "enable/disable" setting to deal with nid=0, since it overrides everything. In fact, it seems like it's only purpose it to override all other node access settings with a universal default. Why not instead have a global configuration setting under permissions that enables or disable node level permissions, then the various node access modules wouldn't need a seperate disable/enable setting. Furthermore, this performance problem would disappear, because either the node_access stuff would be there and you aren't looking for nid=0, or it isn't and you don't care what's in node_access. It seems it'd save queries to node access when you're not using node level permissions, and make it cleaner to implement a node access modifying module. ------------------------------------------------------------------------ February 8, 2005 - 11:17 : pyromanfo I hate the site randomly logging you out, that was me above. ------------------------------------------------------------------------ February 8, 2005 - 14:23 : JonBob "The more I think about replacing nid=0 with a configuration setting the more it makes sense. The two major node access modules right now have to have a seperate "enable/disable" setting to deal with nid=0, since it overrides everything. In fact, it seems like it's only purpose it to override all other node access settings with a universal default." This is not the case. I think you're confusing the case of nid=0 with the case of realm=all. As an example of the utility of nid=0, consider the scenario given in the example access module [2]. This module implements a simple system whereby anyone can view "public" nodes, and only users with a specific permission can view "private" nodes. This can be rephrased as follows: anonymous users can only view "public" nodes, while privileged users can view all nodes. A grant with nid=0 takes care of the second case, so that the module only needs to deal with granting permission to view the public nodes as necessary. Gerhard's query works in the case where every node has at least one node_access entry with its nid, but I'm afraid it fails in the above example. A node that is private would not have any corresponding entry, and so the inner join would fail for it, returning no record. The left join doesn't even have a chance to be matched. It would be worth benchmarking the scenario in which the nid=0 possibility is removed from the query, but the node_access table has an entry added for each node instead of a single nid=0 entry. If this is faster than the OR in the join, the extra DB storage is probably worth the change. [2] http://drupaldocs.org/api/head/file/contributions/docs/developer/examples/n… ------------------------------------------------------------------------ February 8, 2005 - 20:08 : pyromanfo "As an example of the utility of nid=0, consider the scenario given in the example access module. This module implements a simple system whereby anyone can view "public" nodes, and only users with a specific permission can view "private" nodes. This can be rephrased as follows: anonymous users can only view "public" nodes, while privileged users can view all nodes. A grant with nid=0 takes care of the second case, so that the module only needs to deal with granting permission to view the public nodes as necessary. " I think there's a bug in the module, in the docs in the top shouldn't the initial SQL statement be adding that permission to gid=1? Though you're right, I'd forgotten because I hadn't seen any of the current modules use it. As for that performance comparison mysql> SELECT COUNT(DISTINCT(n.nid)) FROM node n INNER JOIN term_node tn0 ON n.nid = tn0.nid INNER JOIN node_access na ON (na.nid = n.nid) WHERE n.status = 1 AND na.grant_view = 1 AND CONCAT(na.realm, na.gid) IN ('all0','term_access1') AND tn0.tid IN (2); +------------------------+ | COUNT(DISTINCT(n.nid)) | +------------------------+ | 2993 | +------------------------+ 1 row in set (0.23 sec) mysql> update node_access set realm='blam' where realm='term_access'; Query OK, 41508 rows affected (5.42 sec) Rows matched: 41508 Changed: 41508 Warnings: 0 mysql> insert into node_access (nid,gid,realm,grant_view) values (0,1,'term_access',1); Query OK, 1 row affected (0.00 sec) mysql> SELECT COUNT(DISTINCT(n.nid)) FROM node n INNER JOIN term_node tn0 ON n.nid = tn0.nid INNER JOIN node_access na ON (na.nid = 0 OR na.nid=n.nid) WHERE n.status = 1 AND na.grant_view = 1 AND CONCAT(na.realm, na.gid) IN ('all0','term_access1') AND tn0.tid IN (2); +------------------------+ | COUNT(DISTINCT(n.nid)) | +------------------------+ | 2993 | +------------------------+ 1 row in set (0.16 sec) So I don't think it improves performance for the nid=0 case, though it is suprisingly slow. ------------------------------------------------------------------------ February 8, 2005 - 20:57 : moshe weitzman perhaps we should split the query into 2 queries in order to avoid the 'na.nid = 0' part of the JOIN. Alternatively, perhaps we could inject that clause only when needed using the db_rewrite_sql hook. not sure. ------------------------------------------------------------------------ February 9, 2005 - 23:36 : pyromanfo Splitting it into two queries would make sense I think. Basically you could have it entirely in node_access_join_sql and node_access_where_sql. There we can check for the nid=0 case for any gids and realms. Then if it returns true, we know we don't have to check anything else, so we return null for the SQL to enter. We could even store this in a static variable to avoid calling this twice for join_sql and where_sql So in the nid=0 case, this should be similarly fast as we're removing a table from the JOIN in exchange for checking node_access for one nid. mysql> select count(nid) from node_access where nid=0 and gid=1 and realm in ('all','term_access','blah'); +------------+ | count(nid) | +------------+ | 0 | +------------+ 1 row in set (0.37 sec) SELECT COUNT(DISTINCT(n.nid)) FROM node n INNER JOIN term_node tn0 ON n.nid = tn0.nid WHERE n.status = 1 AND tn0.tid IN (9); +------------------------+ | COUNT(DISTINCT(n.nid)) | +------------------------+ | 4202 | +------------------------+ 1 row in set (0.15 sec) SELECT COUNT(DISTINCT(n.nid)) FROM node n INNER JOIN node_access na ON (na.nid = 0 OR na.nid = n.nid) INNER JOIN term_node tn0 ON n.nid = tn0.nid WHERE n.status = 1 AND na.grant_view = 1 AND CONCAT(na.realm, na.gid) IN ('all0','blah1') AND tn0.tid IN (9); +------------------------+ | COUNT(DISTINCT(n.nid)) | +------------------------+ | 4202 | +------------------------+ 1 row in set (1.97 sec) It looks to be faster, actually. Then, when we don't get a result, we check node access with the 'na.nid=0 OR' removed. This is certain to be faster as the test have shown, taking the query down from 7 minutes to a matter of seconds. In either case, this method would be faster. Plus, it wouldn't require any changes to other modules, simply change the node_access_where_sql and node_access_join_sql functions. JonBob, would you be okay with that? I can code it if you think you'll accept the patch. ------------------------------------------------------------------------ February 10, 2005 - 08:38 : Anonymous Splitting the queries into two is probably the only way to go. However, I'd like to avoid calling the nid = 0 query if possible. My Drupal installations that use node access do not use nid = 0 at all. I think we could store the available (nid = 0) => realm combinations somewhere and only execute the query if a matching condition is found. Gerhard ------------------------------------------------------------------------ February 10, 2005 - 10:14 : moshe weitzman that nid=0 is going to be very fast when done all by itself. no need to introduce any other mechanism IMO. testing will tell us for sure though ------------------------------------------------------------------------ February 10, 2005 - 13:31 : killes(a)www.drop.org The nid = 0 query would be executed for every node query on a page call even if your access module doesn't use nid = 0 at all. Most implementations do not use nid = 0. If we can avoid a query we should do so. ------------------------------------------------------------------------ February 11, 2005 - 15:00 : JonBob Attachment: http://drupal.org/files/issues/node_access.patch (2.91 KB) Here's a patch that splits the nid=0 check off into a separate query. If the user has such a grant, things should be really fast since we don't need any join or even DISTINCT() check. If not, the join is done as before, but without the "OR nid=0" piece. This ideally needs performance checking on systems using node access with nid=0 rows, using it without them, and not using it at all. ------------------------------------------------------------------------ February 14, 2005 - 07:33 : killes(a)www.drop.org Ok, here is a first test result: After installing the node_privacy_by_role module for the drupal.org database, the Drupal install became unusable for all but the No. 1 user and the load went high due to many hanging mysql processes. After applying the patch the site does work again. The module seems to generate one entry per node and role in the node_access table. I did not make any benchmarks, the result is so obvious. ------------------------------------------------------------------------ February 14, 2005 - 09:10 : killes(a)www.drop.org I've used a script provided by JonBob to create entries in the node_access table and installed the node_access_example module. The script generated about 500 node_access entries. I then benchmarked the tracker page. Results: 11.78 +- 0.48 s with patch 17.04 +- 0.70 s without patch The patched version needs only 70% of the time the non patched version needs. ------------------------------------------------------------------------ February 14, 2005 - 09:13 : Anonymous I haven't run numbers, but subjectively my test site seems much faster with the patch in the case where there is a nid=0 row, a bit faster where there is no such row (but not as obvious), and about the same with node access turned off. -- View: http://drupal.org/node/16558 Edit: http://drupal.org/project/comments/add/16558

1 0

[drupal-devel] [feature] Option to disable printer friendly version for book module
by moshe weitzman 18 Feb '05

18 Feb '05

Project: Drupal Version: cvs Component: book.module Category: feature requests Priority: normal Assigned to: Anonymous Reported by: kbahey Updated by: moshe weitzman Status: patch Personally, I like the 'Advanced' tab. There should be a permission which is universal across all modules which says 'access advanced configuration options'. If user has that permission, he can see these tabs. -moshe moshe weitzman Previous comments: ------------------------------------------------------------------------ November 20, 2004 - 01:30 : kbahey Attachment: http://drupal.org/files/issues/book_17.patch (2.18 KB) I wanted an option for the book module to NOT display a 'printer-friendly version' link for every node. So, I implemented an option for the book module to allow this to be turned off. Using this option: - Go to the /admin/node/book - Uncheck the box that say 'show printer friendly links for books' if you want - Click 'Save settings' There will be no printer friendly links after you do this. Please consider applying this patch to CVS. ------------------------------------------------------------------------ November 21, 2004 - 12:01 : kbahey Setting status to patch, so that it is discussed on the mailing list. ------------------------------------------------------------------------ November 25, 2004 - 18:36 : kbahey Attachment: http://drupal.org/files/issues/book_18.patch (2.3 KB) I have remade this patch to match what is in the current CVS version. The previous patch had conflicts with the latest CVS version. Will this be included in the base any time soon? ------------------------------------------------------------------------ November 25, 2004 - 19:07 : Bèr Kessels options, options, options. Alltough an option is an easy method to add fetaures, while not breaking backwards compatibility, it is generally very bad for useability. I am therefore a -1 for this patch. kbahey, Eventhough the functionality is very nice, I would rather see you either make thisa single site wide setting (in books settings). Or -even better IMO- make the link dis(appear) through a theme function. Or -and thats the best option IMO- help the folks who are working on a better $links (the links under each node) and introduce a general API and theme system to show, hide and markup those links on nodes. ------------------------------------------------------------------------ November 26, 2004 - 16:55 : Anonymous "I would rather see you either make thisa single site wide setting (in books settings). " That is what it already does. The setting is global in books settings, and not in every individual book. It just so happens, for a reason unknown to me, that book's setting page is under administer -> content -> books, and not under administer -> settings -> book like other modules. "Or -even better IMO- make the link dis(appear) through a theme function. Or -and thats the best option IMO- help the folks who are working on a better $links (the links under each node) and introduce a general API and theme system to show, hide and markup those links on nodes. " Those sound like a better option for sure. If the option is in admin -> themes -> configure, under the "Toggle display" part. I am not familiar with that code at this point. Will do some digging to see what can be done. ------------------------------------------------------------------------ February 17, 2005 - 09:58 : kbahey One of the objections to this patch is that it introduces one more option. If I resubmit this patch without an option, does it have a change to be accepted? I will rely on the new $conf variable being able to override variable_get(), so there is no option screen needed. How about that? ------------------------------------------------------------------------ February 17, 2005 - 10:23 : stefan nagtegaal Imo this is a theme feature, so it should be handled in there.. A link management system should be the best to handle such things, but is - unfortunatly - quite hard to inplement... ------------------------------------------------------------------------ February 17, 2005 - 15:41 : kbahey Ideally it would be a theme function. However, most themes, specially the ones that ship with the standard Drupal distribution, do not provide a way to turn that off at will. Most ideal solution is the link management interface you mention, where site admins can turn on off any link they like for any module. But that does not exist so far, so until either one of the above solutions exist, I will be submitting a patch that allows turning off the printer friendly link. ------------------------------------------------------------------------ February 17, 2005 - 15:52 : Bèr Kessels With a theme function, is not meant a function that will make an option on your screen. "Ideally it would be a theme function. However, most themes, specially the ones that ship with the standard Drupal distribution, do not provide a way to turn that off at will." With a theme function, stefan most probablt meant a function theme_printer_friendly(). That function would return the link. If you want to turn it off, you woudl have to make a function yourtheme_printer_friendly() that does /not/ return that link. Problemd solved, without adding more clutter to the UI. ------------------------------------------------------------------------ February 17, 2005 - 21:48 : kbahey Attachment: http://drupal.org/files/issues/book-2.patch (1017 bytes) There are no UI changes in this patch. If someone wants to turn off the printer friendly link, then all they do is : - Go to settings.php - Uncomment the lines that have the $conf variable and the closing bracket - Add a line like this: 'book_hide_printer_friendly' => 1, That is it. No menus, no options, nothing ... Preferrably, we should document these 'hidden' options in the future, under an 'Advanced Configuration' section. ------------------------------------------------------------------------ February 18, 2005 - 00:42 : nedjo kbahey's point reflects a general problem in Drupal--much of the content returned is hard-coded, generated in modules, and therefore without any way of being overridden. As a site administrator, my feeling is that I should have full control over what I present to my users and therefore /nothing/ should be forced on me. My observation is that, many times when suggestions are made to try to free Drupal's core from hard-coded content, there is resistance on the basis that this would require more configuration. Fair enough--but sticking us with content we don't want isn't much of an option either. The three options suggested in this issue - theming, ui configuration options, and settings.php configuration options - all have some merit, and are all currently in use; but it looks like we need a larger discussion, and perhaps some fresh approaches. I'm thinking the proposed solution of passing the content filtering to theming is tempting, but ultimately problematic. Basically, the theme approach would say: generate content pass to theme function optionally (and manually), insert theme override function that doesn't display the content that's been generated In a practical sense, this approach is cumbersome because it requires, for every optional content element, (a) edits to the core module to pass content through a theme_ function, and (b) manual theme editing to write an override function. (This second piece, of course, necessitates extra work every time a theme is updated, even if we use otherwise unmodified themes). The workflow of first generating content and then overriding it is more than inefficient--it suggests a confusion in the separation of content and presentation. Ideally, I still believe, a theme should answers the question /how/ do you want to present content--not /what/ do you want to present. Of course, in Drupal, we break this ideal routinely. But to push even more decision-making logic into the themes feels like a step in the wrong direction. Ultimately, if we don't want content displayed, probably we shouldn't generate it in the first place. My own preference is configuration options over hard-coded content (thought the lastest suggestion of hiding this option in settings.php seems a bit confusing--most admins will never know about this option). But maybe we need a completely different approach. It strikes me that the menu system provides a possible model. It offers a way to assign generated content to various categories, e.g., MENU_SUGGESTED_ITEM. Could we extend this approach to other types of content--allowing administrative override, as we do with the menu module? ------------------------------------------------------------------------ February 18, 2005 - 06:28 : Anonymous I plan to address this issue shortly on the Interface and useability meetings on dropalCOM. My vision on this is: If a setting requires frequent change, or if a setting is that of an oblect, such as a taxonomy term, or if a setting is user, role or permission sepcific, or if we cannot find a "good default" because an option differs per case [1] ** we will allow UI elelements ** we should not have UI config options, but settings in conf.php or htacess If a setting is technical, server aimed site specific and a good default can be found (e.g. settings for time to remain logged in) ** we should not have UI config options, but theme functions, for setting that are: clearly layout settings or, provide output (e.g should horizontal tabs be UL or not) or, very site specific [1] and on all outputted HTML,( off course.) [1] this is a debatable part, and thus needs some clarification. With cases i mean a targetted group. An example of a case is "community site" or "personal blog". Specific suites, will fit within these cases, but is more specific. This print_link thing is obviously very specific. There is not one case that would not want a print link. on contrary: the user-details with each post are only interesting for cases of community sites. "corporate sites" are not interested. So that post-info should be ui configurable, since 50% of the sites wants it, and 50% does not. A print link will suit 98% of the people, while hiding is only interesting for 2%. Thus it it specific. Bèr ------------------------------------------------------------------------ February 18, 2005 - 06:29 : Bèr Kessels ^-- That was me. logged out for some reason ------------------------------------------------------------------------ February 18, 2005 - 10:57 : kbahey Nedjo, You feel my pain. Here is another example of the need for options to turn on/off certain links. This site uses image gallery to display products. http://originaltilly.com/node/view/144&n=all As you notice, there are no links at the bottom for resolution, gallery, ...etc. When I asked the author (asitis.org) how he did it, he said he edited out the links he does not need! On other sites, he overrode it in the theme. Doing this stuff in the theme is really ugly, because a theme should not have a lot of logic in it, and I agree with nedjo it should be about presentation of content not fiddling with content. The other problem that I pointed out is that no 'standard' theme does that today. My resorting to $conf was basically to get over the "no new options" objection. The option would be there and are hidden from view. We can have a note in a ADVANCED.txt file describing what options can be set there are how to set them, then we have satisfied both camps, the average user will not get confused, the advanced user can do what they want. Another approach is to have those infrequent options grouped under an advanced tab or something, where theywould not confuse the average user. Guys: Please! Let us be more pragmatic about this options thing. We may be introducing options that have the potential for confusion, but the price we pay is more than that: - Creating fragmented and ugly hacks in some themes - Forcing the introduction of unneeded complexity in some themes - Not having a general solution out of the box - Depriving functionality for others who cannot modify themes. -- View: http://drupal.org/node/13211 Edit: http://drupal.org/project/comments/add/13211

1 0

[drupal-devel] [feature] Option to disable printer friendly version for book module
by kbahey 18 Feb '05

18 Feb '05

Project: Drupal Version: cvs Component: book.module Category: feature requests Priority: normal Assigned to: Anonymous Reported by: kbahey Updated by: kbahey Status: patch Nedjo, You feel my pain. Here is another example of the need for options to turn on/off certain links. This site uses image gallery to display products. http://originaltilly.com/node/view/144&n=all As you notice, there are no links at the bottom for resolution, gallery, ...etc. When I asked the author (asitis.org) how he did it, he said he edited out the links he does not need! On other sites, he overrode it in the theme. Doing this stuff in the theme is really ugly, because a theme should not have a lot of logic in it, and I agree with nedjo it should be about presentation of content not fiddling with content. The other problem that I pointed out is that no 'standard' theme does that today. My resorting to $conf was basically to get over the "no new options" objection. The option would be there and are hidden from view. We can have a note in a ADVANCED.txt file describing what options can be set there are how to set them, then we have satisfied both camps, the average user will not get confused, the advanced user can do what they want. Another approach is to have those infrequent options grouped under an advanced tab or something, where theywould not confuse the average user. Guys: Please! Let us be more pragmatic about this options thing. We may be introducing options that have the potential for confusion, but the price we pay is more than that: - Creating fragmented and ugly hacks in some themes - Forcing the introduction of unneeded complexity in some themes - Not having a general solution out of the box - Depriving functionality for others who cannot modify themes. kbahey Previous comments: ------------------------------------------------------------------------ November 20, 2004 - 01:30 : kbahey Attachment: http://drupal.org/files/issues/book_17.patch (2.18 KB) I wanted an option for the book module to NOT display a 'printer-friendly version' link for every node. So, I implemented an option for the book module to allow this to be turned off. Using this option: - Go to the /admin/node/book - Uncheck the box that say 'show printer friendly links for books' if you want - Click 'Save settings' There will be no printer friendly links after you do this. Please consider applying this patch to CVS. ------------------------------------------------------------------------ November 21, 2004 - 12:01 : kbahey Setting status to patch, so that it is discussed on the mailing list. ------------------------------------------------------------------------ November 25, 2004 - 18:36 : kbahey Attachment: http://drupal.org/files/issues/book_18.patch (2.3 KB) I have remade this patch to match what is in the current CVS version. The previous patch had conflicts with the latest CVS version. Will this be included in the base any time soon? ------------------------------------------------------------------------ November 25, 2004 - 19:07 : Bèr Kessels options, options, options. Alltough an option is an easy method to add fetaures, while not breaking backwards compatibility, it is generally very bad for useability. I am therefore a -1 for this patch. kbahey, Eventhough the functionality is very nice, I would rather see you either make thisa single site wide setting (in books settings). Or -even better IMO- make the link dis(appear) through a theme function. Or -and thats the best option IMO- help the folks who are working on a better $links (the links under each node) and introduce a general API and theme system to show, hide and markup those links on nodes. ------------------------------------------------------------------------ November 26, 2004 - 16:55 : Anonymous "I would rather see you either make thisa single site wide setting (in books settings). " That is what it already does. The setting is global in books settings, and not in every individual book. It just so happens, for a reason unknown to me, that book's setting page is under administer -> content -> books, and not under administer -> settings -> book like other modules. "Or -even better IMO- make the link dis(appear) through a theme function. Or -and thats the best option IMO- help the folks who are working on a better $links (the links under each node) and introduce a general API and theme system to show, hide and markup those links on nodes. " Those sound like a better option for sure. If the option is in admin -> themes -> configure, under the "Toggle display" part. I am not familiar with that code at this point. Will do some digging to see what can be done. ------------------------------------------------------------------------ February 17, 2005 - 09:58 : kbahey One of the objections to this patch is that it introduces one more option. If I resubmit this patch without an option, does it have a change to be accepted? I will rely on the new $conf variable being able to override variable_get(), so there is no option screen needed. How about that? ------------------------------------------------------------------------ February 17, 2005 - 10:23 : stefan nagtegaal Imo this is a theme feature, so it should be handled in there.. A link management system should be the best to handle such things, but is - unfortunatly - quite hard to inplement... ------------------------------------------------------------------------ February 17, 2005 - 15:41 : kbahey Ideally it would be a theme function. However, most themes, specially the ones that ship with the standard Drupal distribution, do not provide a way to turn that off at will. Most ideal solution is the link management interface you mention, where site admins can turn on off any link they like for any module. But that does not exist so far, so until either one of the above solutions exist, I will be submitting a patch that allows turning off the printer friendly link. ------------------------------------------------------------------------ February 17, 2005 - 15:52 : Bèr Kessels With a theme function, is not meant a function that will make an option on your screen. "Ideally it would be a theme function. However, most themes, specially the ones that ship with the standard Drupal distribution, do not provide a way to turn that off at will." With a theme function, stefan most probablt meant a function theme_printer_friendly(). That function would return the link. If you want to turn it off, you woudl have to make a function yourtheme_printer_friendly() that does /not/ return that link. Problemd solved, without adding more clutter to the UI. ------------------------------------------------------------------------ February 17, 2005 - 21:48 : kbahey Attachment: http://drupal.org/files/issues/book-2.patch (1017 bytes) There are no UI changes in this patch. If someone wants to turn off the printer friendly link, then all they do is : - Go to settings.php - Uncomment the lines that have the $conf variable and the closing bracket - Add a line like this: 'book_hide_printer_friendly' => 1, That is it. No menus, no options, nothing ... Preferrably, we should document these 'hidden' options in the future, under an 'Advanced Configuration' section. ------------------------------------------------------------------------ February 18, 2005 - 00:42 : nedjo kbahey's point reflects a general problem in Drupal--much of the content returned is hard-coded, generated in modules, and therefore without any way of being overridden. As a site administrator, my feeling is that I should have full control over what I present to my users and therefore /nothing/ should be forced on me. My observation is that, many times when suggestions are made to try to free Drupal's core from hard-coded content, there is resistance on the basis that this would require more configuration. Fair enough--but sticking us with content we don't want isn't much of an option either. The three options suggested in this issue - theming, ui configuration options, and settings.php configuration options - all have some merit, and are all currently in use; but it looks like we need a larger discussion, and perhaps some fresh approaches. I'm thinking the proposed solution of passing the content filtering to theming is tempting, but ultimately problematic. Basically, the theme approach would say: generate content pass to theme function optionally (and manually), insert theme override function that doesn't display the content that's been generated In a practical sense, this approach is cumbersome because it requires, for every optional content element, (a) edits to the core module to pass content through a theme_ function, and (b) manual theme editing to write an override function. (This second piece, of course, necessitates extra work every time a theme is updated, even if we use otherwise unmodified themes). The workflow of first generating content and then overriding it is more than inefficient--it suggests a confusion in the separation of content and presentation. Ideally, I still believe, a theme should answers the question /how/ do you want to present content--not /what/ do you want to present. Of course, in Drupal, we break this ideal routinely. But to push even more decision-making logic into the themes feels like a step in the wrong direction. Ultimately, if we don't want content displayed, probably we shouldn't generate it in the first place. My own preference is configuration options over hard-coded content (thought the lastest suggestion of hiding this option in settings.php seems a bit confusing--most admins will never know about this option). But maybe we need a completely different approach. It strikes me that the menu system provides a possible model. It offers a way to assign generated content to various categories, e.g., MENU_SUGGESTED_ITEM. Could we extend this approach to other types of content--allowing administrative override, as we do with the menu module? ------------------------------------------------------------------------ February 18, 2005 - 06:28 : Anonymous I plan to address this issue shortly on the Interface and useability meetings on dropalCOM. My vision on this is: If a setting requires frequent change, or if a setting is that of an oblect, such as a taxonomy term, or if a setting is user, role or permission sepcific, or if we cannot find a "good default" because an option differs per case [1] ** we will allow UI elelements ** we should not have UI config options, but settings in conf.php or htacess If a setting is technical, server aimed site specific and a good default can be found (e.g. settings for time to remain logged in) ** we should not have UI config options, but theme functions, for setting that are: clearly layout settings or, provide output (e.g should horizontal tabs be UL or not) or, very site specific [1] and on all outputted HTML,( off course.) [1] this is a debatable part, and thus needs some clarification. With cases i mean a targetted group. An example of a case is "community site" or "personal blog". Specific suites, will fit within these cases, but is more specific. This print_link thing is obviously very specific. There is not one case that would not want a print link. on contrary: the user-details with each post are only interesting for cases of community sites. "corporate sites" are not interested. So that post-info should be ui configurable, since 50% of the sites wants it, and 50% does not. A print link will suit 98% of the people, while hiding is only interesting for 2%. Thus it it specific. Bèr ------------------------------------------------------------------------ February 18, 2005 - 06:29 : Bèr Kessels ^-- That was me. logged out for some reason -- View: http://drupal.org/node/13211 Edit: http://drupal.org/project/comments/add/13211

1 0

[drupal-devel] contribs -- another idea
by Negyesi Karoly 18 Feb '05

18 Feb '05

Hi! I propose that contribs be broken into two repository. One of them is sandbox, no restrictions neither projects. Everyone may get access. However, those who want to contrib modules, their work shall be reviewed by a few people -- much fewer things shall be in contribs. A board shall review the current modules directory, contact authors of overlapping ones and long-not-refreshed and act accordingly. When we talked about this on IRC, walkah asked whether my hand is up. I answered yes, and this remains so. I will be in Antwerp next Friday and I will be happy to discuss this further. Or in Brussels during the 'Con. Hope to see you all! Károly Négyesi

3 4

[drupal-devel] [feature] Option to disable printer friendly version for book module
by Bèr Kessels 18 Feb '05

18 Feb '05

Project: Drupal Version: cvs Component: book.module Category: feature requests Priority: normal Assigned to: Anonymous Reported by: kbahey Updated by: Bèr Kessels Status: patch ^-- That was me. logged out for some reason Bèr Kessels Previous comments: ------------------------------------------------------------------------ November 20, 2004 - 07:30 : kbahey Attachment: http://drupal.org/files/issues/book_17.patch (2.18 KB) I wanted an option for the book module to NOT display a 'printer-friendly version' link for every node. So, I implemented an option for the book module to allow this to be turned off. Using this option: - Go to the /admin/node/book - Uncheck the box that say 'show printer friendly links for books' if you want - Click 'Save settings' There will be no printer friendly links after you do this. Please consider applying this patch to CVS. ------------------------------------------------------------------------ November 21, 2004 - 18:01 : kbahey Setting status to patch, so that it is discussed on the mailing list. ------------------------------------------------------------------------ November 26, 2004 - 00:36 : kbahey Attachment: http://drupal.org/files/issues/book_18.patch (2.3 KB) I have remade this patch to match what is in the current CVS version. The previous patch had conflicts with the latest CVS version. Will this be included in the base any time soon? ------------------------------------------------------------------------ November 26, 2004 - 01:07 : Bèr Kessels options, options, options. Alltough an option is an easy method to add fetaures, while not breaking backwards compatibility, it is generally very bad for useability. I am therefore a -1 for this patch. kbahey, Eventhough the functionality is very nice, I would rather see you either make thisa single site wide setting (in books settings). Or -even better IMO- make the link dis(appear) through a theme function. Or -and thats the best option IMO- help the folks who are working on a better $links (the links under each node) and introduce a general API and theme system to show, hide and markup those links on nodes. ------------------------------------------------------------------------ November 26, 2004 - 22:55 : Anonymous "I would rather see you either make thisa single site wide setting (in books settings). " That is what it already does. The setting is global in books settings, and not in every individual book. It just so happens, for a reason unknown to me, that book's setting page is under administer -> content -> books, and not under administer -> settings -> book like other modules. "Or -even better IMO- make the link dis(appear) through a theme function. Or -and thats the best option IMO- help the folks who are working on a better $links (the links under each node) and introduce a general API and theme system to show, hide and markup those links on nodes. " Those sound like a better option for sure. If the option is in admin -> themes -> configure, under the "Toggle display" part. I am not familiar with that code at this point. Will do some digging to see what can be done. ------------------------------------------------------------------------ February 17, 2005 - 15:58 : kbahey One of the objections to this patch is that it introduces one more option. If I resubmit this patch without an option, does it have a change to be accepted? I will rely on the new $conf variable being able to override variable_get(), so there is no option screen needed. How about that? ------------------------------------------------------------------------ February 17, 2005 - 16:23 : stefan nagtegaal Imo this is a theme feature, so it should be handled in there.. A link management system should be the best to handle such things, but is - unfortunatly - quite hard to inplement... ------------------------------------------------------------------------ February 17, 2005 - 21:41 : kbahey Ideally it would be a theme function. However, most themes, specially the ones that ship with the standard Drupal distribution, do not provide a way to turn that off at will. Most ideal solution is the link management interface you mention, where site admins can turn on off any link they like for any module. But that does not exist so far, so until either one of the above solutions exist, I will be submitting a patch that allows turning off the printer friendly link. ------------------------------------------------------------------------ February 17, 2005 - 21:52 : Bèr Kessels With a theme function, is not meant a function that will make an option on your screen. "Ideally it would be a theme function. However, most themes, specially the ones that ship with the standard Drupal distribution, do not provide a way to turn that off at will." With a theme function, stefan most probablt meant a function theme_printer_friendly(). That function would return the link. If you want to turn it off, you woudl have to make a function yourtheme_printer_friendly() that does /not/ return that link. Problemd solved, without adding more clutter to the UI. ------------------------------------------------------------------------ February 18, 2005 - 03:48 : kbahey Attachment: http://drupal.org/files/issues/book-2.patch (1017 bytes) There are no UI changes in this patch. If someone wants to turn off the printer friendly link, then all they do is : - Go to settings.php - Uncomment the lines that have the $conf variable and the closing bracket - Add a line like this: 'book_hide_printer_friendly' => 1, That is it. No menus, no options, nothing ... Preferrably, we should document these 'hidden' options in the future, under an 'Advanced Configuration' section. ------------------------------------------------------------------------ February 18, 2005 - 06:42 : nedjo kbahey's point reflects a general problem in Drupal--much of the content returned is hard-coded, generated in modules, and therefore without any way of being overridden. As a site administrator, my feeling is that I should have full control over what I present to my users and therefore /nothing/ should be forced on me. My observation is that, many times when suggestions are made to try to free Drupal's core from hard-coded content, there is resistance on the basis that this would require more configuration. Fair enough--but sticking us with content we don't want isn't much of an option either. The three options suggested in this issue - theming, ui configuration options, and settings.php configuration options - all have some merit, and are all currently in use; but it looks like we need a larger discussion, and perhaps some fresh approaches. I'm thinking the proposed solution of passing the content filtering to theming is tempting, but ultimately problematic. Basically, the theme approach would say: generate content pass to theme function optionally (and manually), insert theme override function that doesn't display the content that's been generated In a practical sense, this approach is cumbersome because it requires, for every optional content element, (a) edits to the core module to pass content through a theme_ function, and (b) manual theme editing to write an override function. (This second piece, of course, necessitates extra work every time a theme is updated, even if we use otherwise unmodified themes). The workflow of first generating content and then overriding it is more than inefficient--it suggests a confusion in the separation of content and presentation. Ideally, I still believe, a theme should answers the question /how/ do you want to present content--not /what/ do you want to present. Of course, in Drupal, we break this ideal routinely. But to push even more decision-making logic into the themes feels like a step in the wrong direction. Ultimately, if we don't want content displayed, probably we shouldn't generate it in the first place. My own preference is configuration options over hard-coded content (thought the lastest suggestion of hiding this option in settings.php seems a bit confusing--most admins will never know about this option). But maybe we need a completely different approach. It strikes me that the menu system provides a possible model. It offers a way to assign generated content to various categories, e.g., MENU_SUGGESTED_ITEM. Could we extend this approach to other types of content--allowing administrative override, as we do with the menu module? ------------------------------------------------------------------------ February 18, 2005 - 12:28 : Anonymous I plan to address this issue shortly on the Interface and useability meetings on dropalCOM. My vision on this is: If a setting requires frequent change, or if a setting is that of an oblect, such as a taxonomy term, or if a setting is user, role or permission sepcific, or if we cannot find a "good default" because an option differs per case [1] ** we will allow UI elelements ** we should not have UI config options, but settings in conf.php or htacess If a setting is technical, server aimed site specific and a good default can be found (e.g. settings for time to remain logged in) ** we should not have UI config options, but theme functions, for setting that are: clearly layout settings or, provide output (e.g should horizontal tabs be UL or not) or, very site specific [1] and on all outputted HTML,( off course.) [1] this is a debatable part, and thus needs some clarification. With cases i mean a targetted group. An example of a case is "community site" or "personal blog". Specific suites, will fit within these cases, but is more specific. This print_link thing is obviously very specific. There is not one case that would not want a print link. on contrary: the user-details with each post are only interesting for cases of community sites. "corporate sites" are not interested. So that post-info should be ui configurable, since 50% of the sites wants it, and 50% does not. A print link will suit 98% of the people, while hiding is only interesting for 2%. Thus it it specific. Bèr -- View: http://drupal.org/node/13211 Edit: http://drupal.org/project/comments/add/13211

1 0