Development
Threads by month
- ----- 2026 -----
- June
- May
- April
- March
- February
- January
- ----- 2025 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2024 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2023 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2022 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2021 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2020 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2019 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2018 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2017 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2016 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2015 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2014 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2013 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2012 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2011 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2010 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2009 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2008 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2007 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2006 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
- ----- 2005 -----
- December
- November
- October
- September
- August
- July
- June
- May
- April
- March
- February
- January
February 2005
- 66 participants
- 347 discussions
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
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
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
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
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
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
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
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
[drupal-devel] [feature] Option to disable printer friendly version for book module
by Anonymous 18 Feb '05
by Anonymous 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: Anonymous
Status: patch
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
Anonymous
Previous comments:
------------------------------------------------------------------------
November 20, 2004 - 06: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 - 17:01 : kbahey
Setting status to patch, so that it is discussed on the mailing list.
------------------------------------------------------------------------
November 25, 2004 - 23: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 - 00: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 - 21: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 - 14: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 - 15: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 - 20: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 - 20: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 - 02: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 - 05: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?
--
View: http://drupal.org/node/13211
Edit: http://drupal.org/project/comments/add/13211
1
0
David Lee ( Advent ) wrote:
> Kindly include our infos in the Drupal Services section:
>
> Advent Consulting
> info(a)adventconsultants.com
> Cambridge, MA USA
>
> Advent Consulting, based in Cambridge, MA - is an independent, open-source
> strategy consulting firm provides best-of-breed, totally integrated open
> source solutions for delivering cost savings, stability and performance
> required for companies embracing open source technology.
>
> Services: We provide complete Drupal implementation and deployment needs.
> Consulting, setup, training, custom extensions to existing and development
> of new modules that best match for client's business requirements.
>
> Qualifications: Our successful projects with clients from non-profit to
> small & mid-size companies. To learn more about our services and solutions,
> simply visit our website at: http://www.adventconsultants.com
We only list people/companies who have made contributions to Drupal.
Please provide a list of the contributions you've made and we'll
consider adding you. Thanks.
--
Dries Buytaert :: http://www.buytaert.net/
1
0