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
- 9354 discussions
Project: Drupal
Version: cvs
Component: node system
Category: feature requests
Priority: normal
Assigned to: chx
Reported by: chx
Updated by: chx
Status: patch
Attachment: http://drupal.org/files/issues/node_rewrite_sql_9.patch (43.98 KB)
module_implementations had a minor, but performance eating bug, it's
caching was broken.
chx
Previous comments:
------------------------------------------------------------------------
December 23, 2004 - 22:25 : chx
Attachment: http://drupal.org/files/issues/node_builder.patch (5.56 KB)
Although I have posted this to the devel list, as Steve pointed out,
putting patches to sandbox is not the best way to do things. So I open
this thread. I have measured the time for build a node query in this is
about 0.22-0.24ms (on an Athlon 933 MHz machine).
------------------------------------------------------------------------
December 23, 2004 - 23:33 : Dries
I'm not likely to commit this. It's not conform with Drupal's coding
conventions, but more importantly, a node query builder doesn't solve
any real problems. It just adds a different way of doing things
without offering a significant advantage.
------------------------------------------------------------------------
December 23, 2004 - 23:51 : chx
Attachment: http://drupal.org/files/issues/node_builder_0.patch (5.67 KB)
At this moment there are direct queries into the node table everywhere.
After node_access_*_sql calls were invented, a lot of queries needed to
be changed, and I think we will find more to be inserted. If someone
does sg. else with the queries, permission, language etc. he needs to
patch 60+ quieries in core alone. This is not a good thing. That's why
I am proposing a central node query builder.
As for code standards, code-style.pl node.module returns 17 errors,
none of them comes from my patch. However, I should admit, there were a
few spaces missing from the database.inc patch. So I resubmit.
------------------------------------------------------------------------
December 23, 2004 - 23:56 : chx
Attachment: http://drupal.org/files/issues/node_builder_1.patch (5.67 KB)
OK, I was wrong, there is a space missing in one of the rewritten
queries. But I doubt this matters too much, as this is only a proposal
and I think there will be a lot of revisions before it gets to the
core.
------------------------------------------------------------------------
December 24, 2004 - 00:26 : drumm
The blank lines between the comments and function starts will confuse
the documentation parser on drupaldocs.org.
I'm not sure if I support this quite yet. Reducing the number of
queries and whatnot is great, but this patch doesn't do that quite yet.
I'd only want this if the new system is more readable and easy to write.
I'm not sure if this is true.
As for code-style.pl, I don't think this is actually used much and it
doesn't catch everything. Such as, most operators (=, =>, +, etc) like
a space on either side of themselves. I'd support removing
code-style.pl and throwing it in contributions somewhere. (If this
needs further discussion or action, please file another issue, lets
keep this focused on this patch).
I'm going to wait for a few more revisions of this before I weigh in a
+1 or -1.
------------------------------------------------------------------------
December 24, 2004 - 00:58 : Jose A Reyero
I can really think of a number of advantages of this, and though not
really for committing it yet, think is very good to see some discussion
on the issue and some concrete implementations.
When implementing i18n I run into this problem, of having to patch lots
of queries -actually too many- in about all the modules doing any kind
of node listing. This far, I'm stuck at this point. Then I realized it
was just the same for node permissions, and will be the same again for
whatever new functionality you want to add in the future which affects
node listings.
I think actually that direct access to 'node' table should be avoided
when possible out of the node.module. And the only way to achieve this
is to have some kind of query builder. And also think that chx's
approach is very good. it provides some upper layer at the 'node'
level, which can use some object level semantics while relying on a db
layer query builder, which could be used later for other objects
-users, taxonomy terms...-
Just think of all the work that this could have saved when implementing
permissions system, which I think is ugly and dirty anyway because of
all that node_join, node_where...
In the long run, adding such complex logic as sql code, patching and
re-patching queries is bad, very bad, making any new change one higher
level of complexity and driving us away from a OO model, which I think
we should tend to. It's not using PHP objects. It is thinking of nodes
as 'objects' and coding according to it.
Oh, yes, sorry, this is growing too long to write it here :-). But we
could talk also about db portability...
------------------------------------------------------------------------
December 24, 2004 - 01:14 : moshe weitzman
I should add that the Organic Groups module could benefit from this node
listing SQL builder. I have awkwardly worked around the problem for now
but I don't like it.
I don't know if this patch is good or not, but I do lean toward the
functionality that it offers. Remember that any developer is free not
to use the query builder and issue direct SQL as needed. If the query
builder is making life hard, simply don't use, just like today.
------------------------------------------------------------------------
December 24, 2004 - 10:35 : Bèr Kessels
I +1 for this functionality (i have no time to comment on coding style
etc).
It will not only improve maintainability, but will allow a far easier
implementation of the i18n.
i18n currently has a lot of patches, simpley to add logic to all those
node sql queries referred to.
------------------------------------------------------------------------
December 24, 2004 - 13:20 : chx
Attachment: http://drupal.org/files/issues/node_builder_2.patch (1.63 KB)
OK, maybe the former approach was too complex. How about this? This
requires a lot less change to the queries and no change to database.inc
and ten times faster.
As usual, a sample query is rewritten and an example of the proposed
hook is provided.
------------------------------------------------------------------------
December 25, 2004 - 00:09 : chx
Final thoughts for this day. If you like the last version, there is a
possibility to automate the whole thing by adding the following three
lines to the beginning of db_prefix_tables:
Of course, there is very small chance of a loop here, so this would
require more thought, but I think at the end this would be a good
thing.
------------------------------------------------------------------------
December 27, 2004 - 12:55 : Dries
I like the second approach better as it keep things readable and gives
me a bit more control over the order in which things are written.
Looking for more feedback from the others. Did you try updating more
queries to make sure it works as intended in all cases?
------------------------------------------------------------------------
December 27, 2004 - 19:48 : chx
Attachment: http://drupal.org/files/issues/node_builder_3.patch (1.23 KB)
Yes, I tried with many different queries. And I have found one bug --
forgot the underscore in the regexp following FORM so I have rewritten
the whole regexp. It is now a lot simpler :)
And node_query is now reentrant, so if a function in the process of
hook_sql calls node_query, it won't fall into an infinite loop. This
situation did not occur but it's better to forestall such things.
------------------------------------------------------------------------
December 28, 2004 - 05:48 : moshe weitzman
This patch is a step forward. I thought of another feature which becomes
easier with the proposed hook_sql(). think of filtering like
freshmeat.net. At freshmeat, you can say that you only want to see
projects where OS=Windows and License=GPL (for example). A Drupal
equivalent is 'only show me nodes related to Democrats and Poverty'.
This sort if global node filtering is very hard in Drupal today. With
this hook, it becomes easy.
------------------------------------------------------------------------
December 28, 2004 - 12:37 : Dries
It's starting to look good (and you're getting more support)!
Have you read the following threads:
http://lists.drupal.org/archives/drupal-devel/2004-11/msg00198.html and
http://lists.drupal.org/archives/drupal-devel/2004-11/msg00230.html?
It's an open issue related to your work. If you start joining tables,
you might have to add DISTINCT()s. When you are not joining tables, it
would be nice if we'd not pay the cost of a DISTINCT().
It would be good if you could update the node queries in core to take
advantage of it. It's not necessary while prototyping though.
It would be good if the il8n team could take a closer look at this
patch by trying to use it.
It would be good if you could add some PHPDoc.
------------------------------------------------------------------------
January 4, 2005 - 07:55 : chx
Here is a sum of changes in this new revision:
Following Dries' advice, I read those threads and now DISTINCT(nid) is
used only if there is something joined. Whether the access_grants
system is used or not, is determined at the module configuration
screen, so for efficiency reasons, I have patched system.module.
I the very first version of my patch (you know, with arrays) I passed
on the whole query so hook_sql implementations could easily determine
whether they are interested in this query or not. In a very cool
brainstorming with Jose, we found out, that passing the whole query is
not a good thing, hints are enough. So when I will patch the forum
queries, I will define a NODE_SQL_FORUM hint, and a filter which does
something cool with forums (I have heard something about private forums
not implemented -- I maybe wrong here) may do so.
We now have a separate helper function to call hook_sql which is cool,
'cos with it the last node_access_*_sql calls which were lurking in
node_search could be elinimated.
------------------------------------------------------------------------
January 4, 2005 - 07:56 : chx
Attachment: http://drupal.org/files/issues/central_node_query.patch (6.46 KB)
Funny thing. Forgot to attach the patch.
------------------------------------------------------------------------
January 6, 2005 - 00:46 : chx
Another day, another revision. As this patch now has nothing to do with
queries, but it is about rewriting node SQL statements, I have renamed
it node_rewrite_sql, the helper _node_rewrite_sql and there is
hook_node_rewrite_sql. hook_node_rewrite_sql expected return value has
changed: it can return an array('join'=> ..., 'where'=>...,
'distinct'=> ) so it may determine whether DISTINCT is needed or not.
Neither index is mandatory. That hook_node_rewrite_sql impementations
will decide whether they are asking for DISTINCT or not, is -- I think
-- a good opportunity to optimize for speed. For eg. I doubt every node
access mechanism requires DISTINCT.
One last note: hook return value can be also can be a string with a
WHERE statement for the sake of simplicity.
node_rewrite_sql now works with multiline queries.
------------------------------------------------------------------------
January 6, 2005 - 00:46 : chx
Attachment: http://drupal.org/files/issues/central_node_query_0.patch (7.43 KB)
I can't believe I once again did not attach the patch
------------------------------------------------------------------------
January 6, 2005 - 17:22 : chx
Attachment: http://drupal.org/files/issues/central_node_query_1.patch (6.65 KB)
Moshe, thanks for your patience and wisdom.
Extract is thrown out, list is used.
node_node_rewrite_sql looks better now.
access_grants_present is determined in run time, once per page.
------------------------------------------------------------------------
January 6, 2005 - 21:35 : Dries
The hint-stuff looks a bit hairy to me. It would be nice if this could
be avoided but I see what it is necessary.
The semaphore stuff is vague too. I don't see how you'd be able to get
into an endless loop.
It might actually be useful to turn node_access_grants_present() into a
more generic module.inc function. Maybe something along the lines of
module_implements($hook), which would return a list of modules
implementing the specified $hook.
I suggest you update more core modules. If you do, I'll measure the
performance gain against a copy of drupal.org's database.
------------------------------------------------------------------------
January 6, 2005 - 22:47 : chx
Attachment: http://drupal.org/files/issues/node_rewrite_sql.patch (15.72 KB)
The hint stuff is something is pretty useful: hook implementations can
fine tune themselves when to fire and when not. Good for features and
performance, too.
module_implements is done.
Semaphore -- we will have tons of node_rewrite_sql calls in the core.
Now someone writes a hook_node_rewrite_sql implementation which calls a
function which in turn calls another one which calls node_rewrite_sql .
I would rather not be the one who debugs such a scenario.
Thanks for the offer! forum.module is now patched. Others will follow
tomorrow.
------------------------------------------------------------------------
January 6, 2005 - 23:21 : Jose A Reyero
I think this looks good enough, so I'm starting to test and use this for
i18n. If this get into the node module, I don't think I will need any
node.module patch anymore...
A pair of performance hints:
- As this node_rewrite_sql is going to be called several times for each
page, we can cache the list of modules implementing the hook in a static
variable.
- We can even cache the whole result for each 'hint' value, so the
whole hook will only be called once or twice for each page.
Some suggested 'hint' values to be defined:
NODE_REWRITE_SQL_LIST -> This one for simple generic list of nodes for
the main page, could be default.
NODE_REWRITE_SQL_ADMIN_LIST -> List of nodes for administration pages
NODE_REWRITE_SQL_RELATED_LIST -> List of objects related to nodes, but
no nodes themselves -like
comments-, node table may have to be joined...
NODE_REWRITE_SQL_BLOCK_LIST -> Listing for a block
NODE_REWRITE_SQL_NO_LANGUAGE -> No need to add language conditions.
This could hapen when you need a listing for other languages, or mixed
languages, for translators or for administrators..
I would need something like this last one for i18n, just to have the
permissions working and some flexibility with language conditions.
And also the one you already have in place ...._SEARCH.
All this will allow for options like search, admin, list current/all
languages...
------------------------------------------------------------------------
January 7, 2005 - 17:30 : Jose A Reyero
Attachment: http://drupal.org/files/issues/central_node_query_node_module.patch (7.07 KB)
I found some problems when using the patch. The where conditions are not
merged well if there are more than once, and also conditions need some
parenthesis around.
So I replaced the $where and $join strings in _node_rewrite_sql by
arrays which are imploded at the end.
Also added some 'hint' definitions. Patches for i18n module are coming
next.
This works like charm for node listings, comment listings, searches,
etc...
Btw, I also think 'semaphores' are not really needed.
------------------------------------------------------------------------
January 7, 2005 - 19:04 : Dries
I don't see how the hint system can be portable: are contributed modules
supposed to define their own hits?
------------------------------------------------------------------------
January 7, 2005 - 22:54 : chx
The hints -- I was in a hurry yesterday evening, so I have forgot to
define a NODE_REWRITE_SQL_FORUM in the forum.module patch and also
forgot to use the hints. So I think core node types will have their
hint defined.
I have a vague idea about how contrib modules could define their own
hint type: we shall have a NODE_REWRITE_SQL_HINT_MAX, and every hint
define should like this -- beware this is not working! This is just an
idea.
Well, if defines does not work, maybe a global node_rewrite_sql_hints
array should be introduced...?
------------------------------------------------------------------------
January 8, 2005 - 09:54 : chx
Attachment: http://drupal.org/files/issues/node_rewrite_sql_0.patch (16.69 KB)
Here is a list of changes:
Complete rethought of hint system. Uses array of strings instead of
defines. There are a number of examples.
the new comment.module patch shows a good usage example for the third
parameter of node_rewrite_sql which is at the moment called node_alias.
It's really not a good name, it should be "the alias of the table which
holds the nid field for this query". BTW. there should be an index on
comment.nid.
Incorporated Jose's "replaced the $where and $join strings in
_node_rewrite_sql by arrays which are imploded at the end". I was a
fool to lose these, the first version (which used array) used a proper
implode for WHEREs.
------------------------------------------------------------------------
January 8, 2005 - 11:27 : Dries
I have two fundamental problems with it this patch:
The loop detection mechanism has to go.
The hints are difficult to understand and I'm not convinced they are
good/needed. Does the patch contain a good example of a
node_rewrite_sql function/hook that uses these hints? I can't find
one. I won't like these hints until I'm convinced they are needed and
that they represent the best possible solution. Convince me.
Could you extend the documentation about the hint-system? Currently it
reads "An array of hint strings about the query, passed on to hook_sql
handlers.". Clearly, that doesn't help me at all. The documentation
should answer the following questions: (1) what are the hints for, (2)
when should I use them (or not use them) and (3) what does hints look
like (what is their format/syntax)?
Note that I can't compare performance unless more queries have been
updated.
Thanks chx.
------------------------------------------------------------------------
January 8, 2005 - 11:30 : Dries
Why does the hints in the comment module include 'related_list'. This
is confusing.
------------------------------------------------------------------------
January 8, 2005 - 12:52 : Jose A Reyero
Well, it seems we have different views about the 'hints'.
At this point, the node_rewrite has two main uses: permissions and
language conditions (i18n). I don't know whether they could have any
use for permission system, but for i18n, some extra information about
the rewritten queries will be needed:
- Most of the queries will just select fields in the node table. Here,
only a where condition is needed, but language condition may be added
or not depending on the kind of query:
_LIST would be a list for the main page, so only in this case,
decissions can be made depending on the path. For some pages, you may
want a list of nodes for all languages and for some others, only
current language.
_ADMIN_LIST is a list of nodes for administration. An option will be
added in i18n for 'administering all languages together' or 'administer
each language separately', or even a language field could be added in
the search form. So language conditions will be merged diferently
_BLOCK_LIST is a list of nodes for blocks. I'm thinking here of a
global option like 'show mixed languages in blocks'. Anyway, this is a
list for a block and you cannot have any logic depending on path
here.
_SEARCH, would be only for searching, and in this case you may decide
to search 'all languages' or only the current one. This can be a global
option or some extra field in search form. When doing text searches,
searching all languages together will make a lot of sense, but also
restricting search by language.
About _RELATED_LIST, this is the case of comments, is a list of objects
which are not nodes themselves, but related objects in a different
table. Here, the node table itself may have to be joined as it is not
yet in the query.
When dealing with the translation system, the language conditions will
be added or not by the module requesting the query itself, so no
further language conditions will be needed. Queries will have to be run
anyway through 'node_rewrite_sql' to have permissions in place.
That's what the _NO_LANGUAGE hint is about.
So this is my proposed use for the 'hint' thing, though maybe the hints
themselves could be better defined or elaborated, but this is the main
idea. I mean no specific hints for modules but generic ones for 'kind
of listing' you are requesting.
Maybe for the permission system, it could be also some use, as you
could decide you want to show a list of objects to anyone, but deny
access to the objects themselves, something like 'subscribe or register
if you want to read the article'. So permissions here will apply
differently depending on what you are doing.
We can think also about more creative use of permissions like 'you can
read latest articles' but old ones will be available only for selected
people, or the opposite 'only old content available for everybody'. Or
even 'you can read articles but not comments'. This kind of permissions
would need also some 'hints'.
------------------------------------------------------------------------
January 8, 2005 - 13:17 : chx
Attachment: http://drupal.org/files/issues/node_rewrite_sql_1.patch (26.89 KB)
More queries rewritten. Loop detection gone. Will gather my thoughts on
hint system later, but I do hope this amount of queries will be enough
for a performance measurement. Now only blogapi, blog, book calls
node_access_*_sql, all others are done.
------------------------------------------------------------------------
January 8, 2005 - 13:20 : chx
Attachment: http://drupal.org/files/issues/node_rewrite_sql_2.patch (26.88 KB)
Forgot to save taxonomy.module before rolling the patch, so a DISTINCT
remained :(
------------------------------------------------------------------------
January 8, 2005 - 13:38 : chx
I see Jose has written a lot about hints, thanks! "Well, it seems we
have different views about the 'hints'." -- "we" probably means Dries
vs. "Jose and I" 'cos I agree with Jose, but I see some more uses,
namely what I have said before on making a filter on forums. If you
want to that, you need to know which queries are coming from the
forum.module. What shall we do? Pass on the whole query, and let the
filter proccess the query to decide whether now it is the forum block
it hunts for? I doubt. Let's give it a hint it's a forum and a block
query.
Some kind of extensionable hint mechanism is necessary, 'cos what about
a contrib module which filters another contrib node, say flexinode?
------------------------------------------------------------------------
January 9, 2005 - 15:05 : Dries
Could we drop the hint system if we'd choose not to support the il8n
module for now?
------------------------------------------------------------------------
January 9, 2005 - 16:54 : Jose A Reyero
:-(
Ok, let's try to think positively.
Could we at least pass the query along so i18n -or whatever module
tries to implement this hook- could support itself?
------------------------------------------------------------------------
January 9, 2005 - 18:32 : chx
Well, passing along the query would mean no problems, but I wonder
whether a better documented hint system would be better? I had no
problems with letting the loop detection system go it was just a safety
belt for some distant future possibility. But the hint system is -- in
my point -- is needed.
So, here are the valid hint strings:
node, forum, comment etc.name of the module from the query originates,
like forum, comment, flexinode etc.
genericthis would be be for generic listings, like node_default_page,
forum_get_forums etc.
relatednode table is not joined so if you want to use other fields from
node table than nid you shall join node table yourself. However, if you
have your table which is like JOIN {mytable} ON mt.nid=$nid_alias.nid
that's fine.
adminsomething meant only for administrators.
blocknodes in a block
searchsomething to do with a search
------------------------------------------------------------------------
January 9, 2005 - 18:36 : chx
Attachment: http://drupal.org/files/issues/node_rewrite_sql_3.patch (28.31 KB)
Killes said I did not had -F^f in the last version.
------------------------------------------------------------------------
January 9, 2005 - 22:37 : chx
Attachment: http://drupal.org/files/issues/node_rewrite_sql_4.patch (28.77 KB)
Automatic version as discussed and args for Killes.
------------------------------------------------------------------------
January 9, 2005 - 22:55 : chx
Attachment: http://drupal.org/files/issues/node_rewrite_sql_5.patch (28.31 KB)
Hm. It seems we have agreed on no automatization. However, arbitrary
arguments are kept for Killes.
------------------------------------------------------------------------
January 10, 2005 - 21:02 : Dries
The last version of this patch improved the performance of my test
site's main page by 257%. That is, my machine can serve 2.57 times as
much pages in the same time interval.
Out of curiousity, I investigated this some more and it turns out this
can be tracked down to the useless DISTINCT's which this patch
removes.
Unless someone objects to this patch or the approach taken, I'm going
to commit this shortly. Please review this patch carefully as it marks
an important change.
------------------------------------------------------------------------
January 10, 2005 - 22:39 : chx
Attachment: http://drupal.org/files/issues/node_rewrite_sql_6.patch (44.18 KB)
More DISTINCTs cleared, now there are no more node_access_*_sql calls in
the core modules. This may be a final version.
Or we can find a few more to rewrite. I've searched for {node} selects,
dropped those which had nid = %d as I thought those do not need rewrite
(I may be wrong) and dropped also those that are rewritten, dropped
those that are indexing or cron. Here are the remaining queries, which
may need rewriting:
------------------------------------------------------------------------
January 10, 2005 - 23:02 : Chris Johnson
This patch seems like both a Good Thing and a Bad Thing to me.
The idea of centralizing node queries is a good one, I think. It seems
to be the original motivation for this patch. It allows better SQL
optmization and hence performance. It puts all the code in one place
for better future maintenance and enhancement.
On the other hand, constructing complex SQL statements from
programmatic logic is full of peril. The task quickly becomes very
large and complex.
Manipulating the SQL language elements via code means it is harder to
see when one has made a syntactic mistake. Most such errors will be
fatal. As the SQL construction code becomes more complex with
development, the number of paths through it become huge and impossible
to exhaustively test. Some day, someone will pass parameters that
cause fatal SQL syntax to be generated in a "production" system.
It also obscures SQL syntax differences between supported databases.
It's more likely to introduce usage that works with MySQL but fails
with Postgres.
I've long been in favor of unifying and centralizing all of the
database access in Drupal, to the greatest extent which does not
adversely affect performance. So in this way, I think this patch is a
big step in that general direction.
But I've also been involved in maintaining a search engine which used
this technique to create the SQL statements on the fly. It became
extremely complex over time as one, then two, then three, etc. new
possibilities were added (e.g. new usage, new search criteria). The
more complex it became, the more regression errors we had and the
harder to debug it became. It got to the point where making one small
addition would result in 3 new bugs. It worked most of the time, and
when it worked, it was fast. But it failed fatally in a significant
number of ways.
I'm not sure whether to vote +3 or -4 on this one. It might be good;
it might be terrible.
------------------------------------------------------------------------
January 10, 2005 - 23:19 : chx
Attachment: http://drupal.org/files/issues/node_rewrite_sql_7.patch (44.18 KB)
Chris, I understand your concerns, but this method -- unlike the
original -- tries to keep things as simple as possible. You can only
JOIN tables and add WHERE conditions. We are not creating complex SQL
on the fly, I tried that originally, just read comment #1 for the warm
welcome by Dries: "I'm not likely to commit this". So do not worry,
while I may be a little hotheaded, Dries keeps order with an iron
hand.
And the actual rewrite mechanism is also very simple.
Ps. Two typos corrected in this version.
------------------------------------------------------------------------
January 13, 2005 - 23:20 : Dries
I'm going to test and benchmark the latest version of this patch over
the weekend. If all goes well, I'll commit this to HEAD as no concrete
alternatives have been proposed. (I can't think of a better approach
and believe that this patch does a good job balancing different
values/requirements.)
It would be much appreciated if those who maintain node-level
permission modules could take a look at this patch to evaluate its
usefulness/performance. If you maintain a module that queries the node
table you might want to look at this as well -- after all, you'll have
to update your module if this hits core.
(Note that the PHPdoc code in the patch is not up-to-date.)
------------------------------------------------------------------------
January 14, 2005 - 13:47 : chx
Attachment: http://drupal.org/files/issues/node_rewrite_sql_8.patch (43.97 KB)
PHPdoc is upgraded, I hope now it is up to date.
--
View: http://drupal.org/node/14731
Edit: http://drupal.org/project/comments/add/14731
1
0
Project: Drupal
Version: cvs
Component: node.module
Category: feature requests
Priority: normal
Assigned to: Steven
Reported by: Anonymous
Updated by: Steven
Status: patch
Attachment: http://drupal.org/files/issues/node admin.png (30.19 KB)
Steven
Previous comments:
------------------------------------------------------------------------
August 23, 2004 - 17:09 : Anonymous
Attachment: http://drupal.org/files/issues/node_admin_filter.patch (3.8 KB)
Here's a patch for the current CVS version which adds the option to
filter nodes by type on the node admin page.
Having built a few sites recently which make heavy use of flexinodes,
the ability to filter out particular node types would have been
extremely useful, so I hope this makes it to 4.5.0.
------------------------------------------------------------------------
August 23, 2004 - 21:06 : leafish_dylan
This would be a great feature.
------------------------------------------------------------------------
August 23, 2004 - 21:06 : leafish_dylan
This would be a great feature.
------------------------------------------------------------------------
September 4, 2004 - 00:38 : Bèr Kessels
I think we should really come up with a good solution for this screen.
In the past, I went looking for such a solution and found that the
various MP3 library tools might come to help.
They deal with large amounts of metadata, some of wch are important,
other are not.
I summarise here what the UI could be like:
** The menu-based (tree) approach: Most mp3 collections use this idea.
In a tree like structure on the side, you can make selections. The list
in the main area will follow this selection. (Itunes:
http://www.apple.com/itunes/playlists.html , Juk:
http://developer.kde.org/~wheeler/images/juk-2.0/juk-2.png,
finf/freeamp: http://www.zinf.org/images/zinf_mymusic_shot.png)
In drupal that would mean a menu-based filtering method. I once made a
patch for that, but that was rejected:
http://drupal.org/node/view/7766
The good thing here, is that it is virtually endless scalable. So in
theory, flexinode coud add menu items to show only nodes where foo is
bar.
|
- nodetype
| |- blog
| |= Flexinode
| | |=foo
| | | |=bar
| |- page
** The multiple selectlist UI: Another moethod is a extended method of
what we have now: select items from select boxes and press submit.
Above the main area one will find some selection UI where one can in-
and exlude sertain criteria. (rhythmbox:
http://www.rhythmbox.org/screenshots/Screenshot-Rhythmbox.png, and
winamp http://www.winamp.com/player/walkthrough.php) Note that both
still show some form of tree navigation.
In drupal that would mean multiple selects in three columns (rhytmbox
and winamp show only two artist and album) For drupal we need three of
thos blocks: one for node-types, one for node-settings (promoted, etc)
and one for taxonomy terms. The node settings should be logical too: If
a criteria is selected it it true, if not: false. That way we can also
get rod of the annoying logic: "view posts that are not promoted" and
the likes.
In both solutions the "perform batch action" needs to move somewhere
else. I would plead for moving it to the bottom of the list. A lot of
webmail cleints do it that way. The reason being simple: the steps you
take to perform the update are:
1) select criteria (find nodes)
2) checkmark the chages (we need columns of checkboxes)
3) perform the update.
Logically the way you go through the UI is left->right, top-> down (not
taking into consideration japanese/hebrew/arabs etc)
So all in all: i think indeed the UI needs serious work. I am willing
to take this one me, bu I do need some consensus first. I am not
willing to spend hours on a new UI (like the menu-based approach) to
find the patch being rejected.Therefore I like some discussion up fron,
rather than the " code first see later" approach.
Regards, Ber
------------------------------------------------------------------------
September 5, 2004 - 23:42 : Bèr Kessels
Attachment: http://drupal.org/files/issues/new_UI_proposal.png (175.31 KB)
Here is a gimped mockup from what option 2) could look like.
WE could add a fouth box with users too, but I am not sure how that
would go with permissions and the like. I need to do some thinking
about selecatibility by users first.
------------------------------------------------------------------------
September 6, 2004 - 00:01 : Bèr Kessels
Sorry for the spam: but I forgot to mention that there are a few
issues:
The "perform action dropdown" should list stuff as: promote, demote,
etc. But that list needs to be generated with the selection above in
mind. So if you choose " published" the action list should not have
"publish these nodes"
Also the action list could contain "recategorise" wich would need
another screen after the one in the "ugly screendump-gimp-mockup", to
be able to choose the categories in wich the selection should be
placed.
And of course the delete these items with a confimation dialog.
(http://drupal.org/node/view/7743#comment-11624)
------------------------------------------------------------------------
September 6, 2004 - 14:32 : Bèr Kessels
Attachment: http://drupal.org/files/issues/node_admin.html (29.95 KB)
After a long discussion with Steven And gerhard, I designed the page in
HTML, to let you all try it in some way.
Beware: the html is an ugly mock, It is meant as scetch, not as valid
html.
Some notes on the page:
"View all nodes that are:" contains three differnet possibilities for
searching. The first two (above the blank line) are there for
simplicity, but they might confuse users, because they need a lot of
text. The second idea uses a checkbox to enable the select next to it.
The third one uses some ajvascript to improve useablity.
We should of course choose one of these three methods.
"View all nodes that are classified in:"
There are quite some people that have questions with whether or not we
should include this selection aat all. Here is why I think it should
definately go Because most sites have only one purpose and thus a lot
of nodes of one type in one state. For example a personal blog site:
99% of the nodes are blogs. 99% are promoted. Only 1 or 2 are not
promoted, not published or of type page.
Or take a photo-album-site. All posts will be images, the main
difference is the classification (the galleries) I placed them in.
"any/all" There must be a way to choose whether the sected options
should be an AND or an OR. I tried three different methods, each block
has one method. We should choose one.
Please give me feedback, for I still have to decide whether or not to
make this a contributed module or start lobby-ing to get it in core.
------------------------------------------------------------------------
September 13, 2004 - 11:50 : moshe weitzman
looking good, and really useful. a few small comments:
- consider upping the number of nodes that appear on this page. it is
nearly pointless to use paging on this sort of UI
- i like the second idea of an 'enabled' checkbox, without the
javascript. i think you can use 2 checkboxes, where the second one is
called 'publish', 'promote', and 'has comments'
- I prefer the horizontal configuration of the 'Require: all any' radio
buttons
- don't repeat 'the selected posts' in the choices for the bottom
dropdown. perhaps use a title for that form_select() instead.
- the last choice in that dropdown is 'reclassify'. i don't see how
this screen could support that. instead, use my
taxonomy_multi_edit.module from contrib :)
i suggest making this a contrib module at first. it can be a tab off of
the current page. thats how taxonomy_multi_edit.module appears.
------------------------------------------------------------------------
September 13, 2004 - 13:03 : Bèr Kessels
About that reclassify:
That was exactly my intention: using the taxonomy_muliti_edit module
for that. All I wanted to do is pass a list of nids to your module and
open a screen using that module with the previously selected nodes!
About the other comments: Yes upping the number of nodes might be a
good idea. Anyone any idea what number would still be usefull. 10.000
nodes (on drupal.org) in one list is not a good idea, IMO, so some
paging is needed.
indeed: repeatinf words in dropdowns is considered "bad UI design"
thank you for pointing it out.
Regards, Ber
------------------------------------------------------------------------
September 28, 2004 - 13:37 : eafarris
Attachment: http://drupal.org/files/issues/admin_node_filter_by_type.patch (1.96 KB)
here is an updated version of this patch, syncing it with CVS HEAD.
Also, i did change the functionality somewhat: previous versions of the
patch asked the modules what node types were out there (via
hook_node_name()). This version asks the node table what types are in
there (via SELECT DISTINCT type FROM {node}). I find this more useful,
because it's entirely possible that, over time, modules will be removed
from the system, yet they still have rows in the node table. This method
gives a more accurate view of the types of nodes on the system.
The new UI looks excellent, but this patch does not address it.
------------------------------------------------------------------------
September 28, 2004 - 19:05 : drumm
-1
I applied and tested and it did not work at all for me. The type
options selection only contains 's' and 'W', selecting one does not
work. The list of types seems to come from the DB which is probably an
unnecessary query.
------------------------------------------------------------------------
September 29, 2004 - 12:51 : eafarris
Attachment: http://drupal.org/files/issues/admin_node_filter_by_type_1.patch (2.31 KB)
Here's a version of that patch that should work. Sorry.
I think that getting the types from the node table is necessary. The
list of installed nodes is not the authority on what types of nodes are
in the db; we should ask the db directly. It's a small query on one
admin page; the result is that the dropdown has in it the most accurate
information available. That's worth it, imo.
------------------------------------------------------------------------
September 29, 2004 - 12:52 : eafarris
s/list of installed nodes/list of installed modules/ . sheesh. teach me
to post before my tea.
------------------------------------------------------------------------
October 21, 2004 - 18:15 : Bèr Kessels
Attachment: http://drupal.org/files/issues/node.html (29.33 KB)
I do not know where it weant, but my proposal had a HTML mockup.
Attaching it here to see what i meant.
------------------------------------------------------------------------
October 23, 2004 - 13:14 : com2
Nice filter options, but what I really miss it the possibility to select
all checkboxes. Really boring job when you have to click the whole list
one by one.
------------------------------------------------------------------------
October 24, 2004 - 10:41 : Bèr Kessels
I am not a big fan of those options in forms.
They more often break things than that thy fix them. Rahter use a
bookmarklet r so for that:
http://www.squarefree.com/bookmarklets/forms.html
Ber
------------------------------------------------------------------------
October 25, 2004 - 01:03 : Steven
Attachment: http://drupal.org/files/issues/node_admin_ucd.html (29.83 KB)
Based on some more IRC discussion, I came up with this (attachment). It
is based on an earlier mockup.
It does use Javascript and doesn't degrade without script (yet), but I
think it's worth considering. It can be implemented quite cleanly. The
idea would be to have a filter like this initially, and after clicking
'Go', to show the current filter as well as the form again, allowing
you to refine your search.
I haven't tested browser compatibility, but it should work on most
browsers. The only ones I know for sure won't work are IE4 and NS4,
which are old enough to ignore IMO: after all, this is admin only, and
we can put some stricter browser requirements on that. Recent versions
of other browsers should work (it only does a simple visibility
toggle).
I like it because it's not an airplane cockpit, but still offers
complex filtering options through refinement. It is also quite similar
to the search/filtering UI in e.g. mail applications.
The question is of course, how to degrade it for people without
Javascript. My suggestion would be to simply show all filter options,
and put a radiobutton in front of each:
( ) Where type is [ blog | v ]
( ) Where published status is ( ) published ( ) unpublished
( ) Where category is [ term1 | v ]
...
It has more visual clutter, but the same principle of use.
------------------------------------------------------------------------
October 25, 2004 - 01:13 : Steven
In fact, I just realized that the radio-button version can be coded to
send exactly the same form data as the scripted version, which makes it
more interesting.
------------------------------------------------------------------------
October 29, 2004 - 00:52 : Steven
If you guys approve of my idea, I'll start coding. Assigning to myself
for now.
------------------------------------------------------------------------
November 1, 2004 - 17:35 : Bèr Kessels
I like this idea much more than my previous approaches. Gave it some
thought and came with the follwing behaviour:
* I think we need to discuss or at least design the tabular data too:
It makes 0 sense to show the "types" column, if you chose "type =
image"
* I think we need to introduce a hook to add info to that tabulat view.
Images can output thumbnails, for example
* In tabular view, we should make: title a textfield, type and author a
select and satus a select too.
* I think we should introduce another range of options: "where content
contains" [ ] and "where title contains" [ ]
* I would like to get some feedback on how many drupal-using people are
firmly against javascript, with a reason why. I bet that this is < 1% !
Bèr
------------------------------------------------------------------------
November 1, 2004 - 17:42 : Steven
We don't have an anti-JS policy in Drupal. It's just that until
recently, doing cross browser JS was a chore and making it accessible
and degradable would be a disaster.
This form can be implemented cleanly and will degrade without problems.
Anyone who opposes it because it is JS can bugger off ;).
------------------------------------------------------------------------
November 3, 2004 - 19:24 : com2
There is a more radical way around JS. You could forget the table idea
alltogether and present the nodes in Listbox. Then selecting becomes
much easier with Ctrl-Click and Shift-Click
------------------------------------------------------------------------
November 4, 2004 - 00:02 : Bèr Kessels
Did you have a look at the previous applied UI improvements? Did you
consider the pre's and cons of those earlier posts?
It seams you are repeating passed issues here, please reply with miore
detailed issues here,
Bèr
------------------------------------------------------------------------
December 3, 2004 - 10:49 : Steven
Attachment: http://drupal.org/files/issues/node-admin.patch (13.1 KB)
Here's a patch which implements the JS approach of refining the
selection.
------------------------------------------------------------------------
December 3, 2004 - 10:50 : Steven
Attachment: http://drupal.org/files/issues/node_admin.png (34.38 KB)
And here's a screenshot of how it looks.
------------------------------------------------------------------------
December 3, 2004 - 19:23 : FactoryJoe(a)civicspacelabs.org
This is looking better and better. I'm going to do a UI review of this a
post any additional thoughts or comments.
Chris
------------------------------------------------------------------------
December 3, 2004 - 21:50 : Anonymous
Ack, I tested this using the bluebeach theme, but there's a small CSS
bug with bluemarine, where the header for the operations form is stuck
to the right of the filter form. I'll make an updated patch later. In
the meantime, comments are much appreciated.
------------------------------------------------------------------------
December 3, 2004 - 23:15 : killes(a)www.drop.org
I actually prefer the non-JS menu over the JS menu. Not (only) because
of the JS but because I prefer radiobuttons over pulldown menus. The
argument that the non-JS approach uses more vertical space can be
mitigated by redirecting to a #results anchor.
------------------------------------------------------------------------
January 15, 2005 - 06:32 : Steven
Attachment: http://drupal.org/files/issues/node.admin.patch (14.41 KB)
Here's an updated patch... I have exams in a week, so I don't have time
to finish up the Javascript version, so I removed it (it can be added
later easily). However, the non-JS version is still very usable.
Changes since last time:
- Uses semantic XHTML (definition list, and yes, they are for more than
just definitions, even w3c agrees).
- Merges the various node flags (promoted, published, ...) into one
"status" dropdown. This keeps the whole selector shorter, and is better
UI-wise IMO.
- Adds drupal_gotos, they were missing.
- Updates node admin watchog messages to properly display the type. The
code used t($node->type) which is a big no-no. hook_node_name() is now
invoked instead.
- Removes some dead code.
- I rolled Ber's node mass deletion patch in as it affects the same
piece of code. Updating either patch for the other separately would be
a waste of time. I also added some more improvements to Bèr's code,
such as removing the incorrect help from the confirmation page,
cleaning up the code style a bit, not loading every node just to
display its title and displaying a single drupal_set_message() after
the deleting rather than one per node.
The old issue is here: http://drupal.org/node/7743
--
View: http://drupal.org/node/10296
Edit: http://drupal.org/project/comments/add/10296
1
0
Project: Drupal
Version: cvs
Component: node.module
Category: feature requests
Priority: normal
Assigned to: Steven
Reported by: Anonymous
Updated by: Steven
Status: patch
Attachment: http://drupal.org/files/issues/node.admin.patch (14.41 KB)
Here's an updated patch... I have exams in a week, so I don't have time
to finish up the Javascript version, so I removed it (it can be added
later easily). However, the non-JS version is still very usable.
Changes since last time:
- Uses semantic XHTML (definition list, and yes, they are for more than
just definitions, even w3c agrees).
- Merges the various node flags (promoted, published, ...) into one
"status" dropdown. This keeps the whole selector shorter, and is better
UI-wise IMO.
- Adds drupal_gotos, they were missing.
- Updates node admin watchog messages to properly display the type. The
code used t($node->type) which is a big no-no. hook_node_name() is now
invoked instead.
- Removes some dead code.
- I rolled Ber's node mass deletion patch in as it affects the same
piece of code. Updating either patch for the other separately would be
a waste of time. I also added some more improvements to Bèr's code,
such as removing the incorrect help from the confirmation page,
cleaning up the code style a bit, not loading every node just to
display its title and displaying a single drupal_set_message() after
the deleting rather than one per node.
The old issue is here: http://drupal.org/node/7743
Steven
Previous comments:
------------------------------------------------------------------------
August 23, 2004 - 17:09 : Anonymous
Attachment: http://drupal.org/files/issues/node_admin_filter.patch (3.8 KB)
Here's a patch for the current CVS version which adds the option to
filter nodes by type on the node admin page.
Having built a few sites recently which make heavy use of flexinodes,
the ability to filter out particular node types would have been
extremely useful, so I hope this makes it to 4.5.0.
------------------------------------------------------------------------
August 23, 2004 - 21:06 : leafish_dylan
This would be a great feature.
------------------------------------------------------------------------
August 23, 2004 - 21:06 : leafish_dylan
This would be a great feature.
------------------------------------------------------------------------
September 4, 2004 - 00:38 : Bèr Kessels
I think we should really come up with a good solution for this screen.
In the past, I went looking for such a solution and found that the
various MP3 library tools might come to help.
They deal with large amounts of metadata, some of wch are important,
other are not.
I summarise here what the UI could be like:
** The menu-based (tree) approach: Most mp3 collections use this idea.
In a tree like structure on the side, you can make selections. The list
in the main area will follow this selection. (Itunes:
http://www.apple.com/itunes/playlists.html , Juk:
http://developer.kde.org/~wheeler/images/juk-2.0/juk-2.png,
finf/freeamp: http://www.zinf.org/images/zinf_mymusic_shot.png)
In drupal that would mean a menu-based filtering method. I once made a
patch for that, but that was rejected:
http://drupal.org/node/view/7766
The good thing here, is that it is virtually endless scalable. So in
theory, flexinode coud add menu items to show only nodes where foo is
bar.
|
- nodetype
| |- blog
| |= Flexinode
| | |=foo
| | | |=bar
| |- page
** The multiple selectlist UI: Another moethod is a extended method of
what we have now: select items from select boxes and press submit.
Above the main area one will find some selection UI where one can in-
and exlude sertain criteria. (rhythmbox:
http://www.rhythmbox.org/screenshots/Screenshot-Rhythmbox.png, and
winamp http://www.winamp.com/player/walkthrough.php) Note that both
still show some form of tree navigation.
In drupal that would mean multiple selects in three columns (rhytmbox
and winamp show only two artist and album) For drupal we need three of
thos blocks: one for node-types, one for node-settings (promoted, etc)
and one for taxonomy terms. The node settings should be logical too: If
a criteria is selected it it true, if not: false. That way we can also
get rod of the annoying logic: "view posts that are not promoted" and
the likes.
In both solutions the "perform batch action" needs to move somewhere
else. I would plead for moving it to the bottom of the list. A lot of
webmail cleints do it that way. The reason being simple: the steps you
take to perform the update are:
1) select criteria (find nodes)
2) checkmark the chages (we need columns of checkboxes)
3) perform the update.
Logically the way you go through the UI is left->right, top-> down (not
taking into consideration japanese/hebrew/arabs etc)
So all in all: i think indeed the UI needs serious work. I am willing
to take this one me, bu I do need some consensus first. I am not
willing to spend hours on a new UI (like the menu-based approach) to
find the patch being rejected.Therefore I like some discussion up fron,
rather than the " code first see later" approach.
Regards, Ber
------------------------------------------------------------------------
September 5, 2004 - 23:42 : Bèr Kessels
Attachment: http://drupal.org/files/issues/new_UI_proposal.png (175.31 KB)
Here is a gimped mockup from what option 2) could look like.
WE could add a fouth box with users too, but I am not sure how that
would go with permissions and the like. I need to do some thinking
about selecatibility by users first.
------------------------------------------------------------------------
September 6, 2004 - 00:01 : Bèr Kessels
Sorry for the spam: but I forgot to mention that there are a few
issues:
The "perform action dropdown" should list stuff as: promote, demote,
etc. But that list needs to be generated with the selection above in
mind. So if you choose " published" the action list should not have
"publish these nodes"
Also the action list could contain "recategorise" wich would need
another screen after the one in the "ugly screendump-gimp-mockup", to
be able to choose the categories in wich the selection should be
placed.
And of course the delete these items with a confimation dialog.
(http://drupal.org/node/view/7743#comment-11624)
------------------------------------------------------------------------
September 6, 2004 - 14:32 : Bèr Kessels
Attachment: http://drupal.org/files/issues/node_admin.html (29.95 KB)
After a long discussion with Steven And gerhard, I designed the page in
HTML, to let you all try it in some way.
Beware: the html is an ugly mock, It is meant as scetch, not as valid
html.
Some notes on the page:
"View all nodes that are:" contains three differnet possibilities for
searching. The first two (above the blank line) are there for
simplicity, but they might confuse users, because they need a lot of
text. The second idea uses a checkbox to enable the select next to it.
The third one uses some ajvascript to improve useablity.
We should of course choose one of these three methods.
"View all nodes that are classified in:"
There are quite some people that have questions with whether or not we
should include this selection aat all. Here is why I think it should
definately go Because most sites have only one purpose and thus a lot
of nodes of one type in one state. For example a personal blog site:
99% of the nodes are blogs. 99% are promoted. Only 1 or 2 are not
promoted, not published or of type page.
Or take a photo-album-site. All posts will be images, the main
difference is the classification (the galleries) I placed them in.
"any/all" There must be a way to choose whether the sected options
should be an AND or an OR. I tried three different methods, each block
has one method. We should choose one.
Please give me feedback, for I still have to decide whether or not to
make this a contributed module or start lobby-ing to get it in core.
------------------------------------------------------------------------
September 13, 2004 - 11:50 : moshe weitzman
looking good, and really useful. a few small comments:
- consider upping the number of nodes that appear on this page. it is
nearly pointless to use paging on this sort of UI
- i like the second idea of an 'enabled' checkbox, without the
javascript. i think you can use 2 checkboxes, where the second one is
called 'publish', 'promote', and 'has comments'
- I prefer the horizontal configuration of the 'Require: all any' radio
buttons
- don't repeat 'the selected posts' in the choices for the bottom
dropdown. perhaps use a title for that form_select() instead.
- the last choice in that dropdown is 'reclassify'. i don't see how
this screen could support that. instead, use my
taxonomy_multi_edit.module from contrib :)
i suggest making this a contrib module at first. it can be a tab off of
the current page. thats how taxonomy_multi_edit.module appears.
------------------------------------------------------------------------
September 13, 2004 - 13:03 : Bèr Kessels
About that reclassify:
That was exactly my intention: using the taxonomy_muliti_edit module
for that. All I wanted to do is pass a list of nids to your module and
open a screen using that module with the previously selected nodes!
About the other comments: Yes upping the number of nodes might be a
good idea. Anyone any idea what number would still be usefull. 10.000
nodes (on drupal.org) in one list is not a good idea, IMO, so some
paging is needed.
indeed: repeatinf words in dropdowns is considered "bad UI design"
thank you for pointing it out.
Regards, Ber
------------------------------------------------------------------------
September 28, 2004 - 13:37 : eafarris
Attachment: http://drupal.org/files/issues/admin_node_filter_by_type.patch (1.96 KB)
here is an updated version of this patch, syncing it with CVS HEAD.
Also, i did change the functionality somewhat: previous versions of the
patch asked the modules what node types were out there (via
hook_node_name()). This version asks the node table what types are in
there (via SELECT DISTINCT type FROM {node}). I find this more useful,
because it's entirely possible that, over time, modules will be removed
from the system, yet they still have rows in the node table. This method
gives a more accurate view of the types of nodes on the system.
The new UI looks excellent, but this patch does not address it.
------------------------------------------------------------------------
September 28, 2004 - 19:05 : drumm
-1
I applied and tested and it did not work at all for me. The type
options selection only contains 's' and 'W', selecting one does not
work. The list of types seems to come from the DB which is probably an
unnecessary query.
------------------------------------------------------------------------
September 29, 2004 - 12:51 : eafarris
Attachment: http://drupal.org/files/issues/admin_node_filter_by_type_1.patch (2.31 KB)
Here's a version of that patch that should work. Sorry.
I think that getting the types from the node table is necessary. The
list of installed nodes is not the authority on what types of nodes are
in the db; we should ask the db directly. It's a small query on one
admin page; the result is that the dropdown has in it the most accurate
information available. That's worth it, imo.
------------------------------------------------------------------------
September 29, 2004 - 12:52 : eafarris
s/list of installed nodes/list of installed modules/ . sheesh. teach me
to post before my tea.
------------------------------------------------------------------------
October 21, 2004 - 18:15 : Bèr Kessels
Attachment: http://drupal.org/files/issues/node.html (29.33 KB)
I do not know where it weant, but my proposal had a HTML mockup.
Attaching it here to see what i meant.
------------------------------------------------------------------------
October 23, 2004 - 13:14 : com2
Nice filter options, but what I really miss it the possibility to select
all checkboxes. Really boring job when you have to click the whole list
one by one.
------------------------------------------------------------------------
October 24, 2004 - 10:41 : Bèr Kessels
I am not a big fan of those options in forms.
They more often break things than that thy fix them. Rahter use a
bookmarklet r so for that:
http://www.squarefree.com/bookmarklets/forms.html
Ber
------------------------------------------------------------------------
October 25, 2004 - 01:03 : Steven
Attachment: http://drupal.org/files/issues/node_admin_ucd.html (29.83 KB)
Based on some more IRC discussion, I came up with this (attachment). It
is based on an earlier mockup.
It does use Javascript and doesn't degrade without script (yet), but I
think it's worth considering. It can be implemented quite cleanly. The
idea would be to have a filter like this initially, and after clicking
'Go', to show the current filter as well as the form again, allowing
you to refine your search.
I haven't tested browser compatibility, but it should work on most
browsers. The only ones I know for sure won't work are IE4 and NS4,
which are old enough to ignore IMO: after all, this is admin only, and
we can put some stricter browser requirements on that. Recent versions
of other browsers should work (it only does a simple visibility
toggle).
I like it because it's not an airplane cockpit, but still offers
complex filtering options through refinement. It is also quite similar
to the search/filtering UI in e.g. mail applications.
The question is of course, how to degrade it for people without
Javascript. My suggestion would be to simply show all filter options,
and put a radiobutton in front of each:
( ) Where type is [ blog | v ]
( ) Where published status is ( ) published ( ) unpublished
( ) Where category is [ term1 | v ]
...
It has more visual clutter, but the same principle of use.
------------------------------------------------------------------------
October 25, 2004 - 01:13 : Steven
In fact, I just realized that the radio-button version can be coded to
send exactly the same form data as the scripted version, which makes it
more interesting.
------------------------------------------------------------------------
October 29, 2004 - 00:52 : Steven
If you guys approve of my idea, I'll start coding. Assigning to myself
for now.
------------------------------------------------------------------------
November 1, 2004 - 17:35 : Bèr Kessels
I like this idea much more than my previous approaches. Gave it some
thought and came with the follwing behaviour:
* I think we need to discuss or at least design the tabular data too:
It makes 0 sense to show the "types" column, if you chose "type =
image"
* I think we need to introduce a hook to add info to that tabulat view.
Images can output thumbnails, for example
* In tabular view, we should make: title a textfield, type and author a
select and satus a select too.
* I think we should introduce another range of options: "where content
contains" [ ] and "where title contains" [ ]
* I would like to get some feedback on how many drupal-using people are
firmly against javascript, with a reason why. I bet that this is < 1% !
Bèr
------------------------------------------------------------------------
November 1, 2004 - 17:42 : Steven
We don't have an anti-JS policy in Drupal. It's just that until
recently, doing cross browser JS was a chore and making it accessible
and degradable would be a disaster.
This form can be implemented cleanly and will degrade without problems.
Anyone who opposes it because it is JS can bugger off ;).
------------------------------------------------------------------------
November 3, 2004 - 19:24 : com2
There is a more radical way around JS. You could forget the table idea
alltogether and present the nodes in Listbox. Then selecting becomes
much easier with Ctrl-Click and Shift-Click
------------------------------------------------------------------------
November 4, 2004 - 00:02 : Bèr Kessels
Did you have a look at the previous applied UI improvements? Did you
consider the pre's and cons of those earlier posts?
It seams you are repeating passed issues here, please reply with miore
detailed issues here,
Bèr
------------------------------------------------------------------------
December 3, 2004 - 10:49 : Steven
Attachment: http://drupal.org/files/issues/node-admin.patch (13.1 KB)
Here's a patch which implements the JS approach of refining the
selection.
------------------------------------------------------------------------
December 3, 2004 - 10:50 : Steven
Attachment: http://drupal.org/files/issues/node_admin.png (34.38 KB)
And here's a screenshot of how it looks.
------------------------------------------------------------------------
December 3, 2004 - 19:23 : FactoryJoe(a)civicspacelabs.org
This is looking better and better. I'm going to do a UI review of this a
post any additional thoughts or comments.
Chris
------------------------------------------------------------------------
December 3, 2004 - 21:50 : Anonymous
Ack, I tested this using the bluebeach theme, but there's a small CSS
bug with bluemarine, where the header for the operations form is stuck
to the right of the filter form. I'll make an updated patch later. In
the meantime, comments are much appreciated.
------------------------------------------------------------------------
December 3, 2004 - 23:15 : killes(a)www.drop.org
I actually prefer the non-JS menu over the JS menu. Not (only) because
of the JS but because I prefer radiobuttons over pulldown menus. The
argument that the non-JS approach uses more vertical space can be
mitigated by redirecting to a #results anchor.
--
View: http://drupal.org/node/10296
Edit: http://drupal.org/project/comments/add/10296
1
0
Project: Drupal
Version: cvs
Component: node system
Category: feature requests
Priority: critical
Assigned to: Bèr Kessels
Reported by: killes(a)www.drop.org
Updated by: Bèr Kessels
Status: patch
Attachment: http://drupal.org/files/issues/batch_delete_4-6.patch (6.66 KB)
Updated for 4.6. (hint, hint)
Bèr Kessels
Previous comments:
------------------------------------------------------------------------
May 12, 2004 - 16:44 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/node_2.patch (1.41 KB)
It would be nice if admins could mass delete unpublished nodes. The
attached patch allows this. It also improved error handling.
------------------------------------------------------------------------
May 12, 2004 - 20:48 : Kjartan
Looks good to me. Since it only allows deletion of unpublished nodes
there probably isn't need for an approval page? Thats the only thing I
can think of as a critism. Everything else is reversable, but delete is
permanent (unless the site has some form of backup).
------------------------------------------------------------------------
May 12, 2004 - 23:43 : killes(a)www.drop.org
Well, I found I'd need this capability and therefore coded it. I don't
know how to fit a "are you sure" page in that filter mechanism.
------------------------------------------------------------------------
May 13, 2004 - 00:34 : killes(a)www.drop.org
Teh patch also does not call the neccessary delete hooks thu sleaving
data in additional tables like event or flexinode.
------------------------------------------------------------------------
May 13, 2004 - 20:08 : Dries
Setting the status to 'active' until it works like it should.
------------------------------------------------------------------------
June 29, 2004 - 22:01 : killes(a)www.drop.org
Ber is developing a better patch.
------------------------------------------------------------------------
June 29, 2004 - 22:33 : Bèr Kessels
Attachment: http://drupal.org/files/issues/batch_delete_easy_coding.patch (4.16 KB)
Extracting the bits and bytes from http://drupal.org/node/view/5162.
Hope it might help bring life into this feature.
Overall my comment is that that part of node.module is quite messy.
There's html hardcoded and the function handling this part is one big
bloat of code.
That makes it kinda hard to fix this issue in a more elegant way,
without cleaning up that mess before. I would like to use a nicer
method of confirmation once this part is cleaned. But as it currently
stands thats a quite hard task.
------------------------------------------------------------------------
June 29, 2004 - 22:46 : Bèr Kessels
Moshe had some better ideas. Those need a lot more coding however.
moshe_work>I think you can give a confrim screen just like we do for
nodes, comments, etc.
moshe_work>"You are about to delete ..." and then do a theme(item_list,
$items) showing all the titles
moshe_work>you would want to stick all the nids about to be deleted in
a hidden form field
the problem with this is, that somehow you need to bypass the update
and filter form generation. Since that whole admin part is gernerated
in one big (120 + lines.) function node_admin_nodes() this needs some
work too, if you wnat to apply moshe's idea.
For now i am satisfied with the above attached patch. And I would like
to hear feedback about the UI for confirmation.
Regards
------------------------------------------------------------------------
June 30, 2004 - 09:09 : stefan nagtegaal
Can you show us some screenshots Ber?
------------------------------------------------------------------------
June 30, 2004 - 10:20 : Bèr Kessels
Attachment: http://drupal.org/files/issues/batch_delete_step1.png (18.8 KB)
Screenshot: first you select the nodes you want to have deleted.
Than from the dropdown you select "delete selected nodes".
------------------------------------------------------------------------
June 30, 2004 - 10:22 : Bèr Kessels
Attachment: http://drupal.org/files/issues/batch_delete_step2.png (26.77 KB)
After pressing [update] you see the screen back, and in the "update
actions dropdown" you get a confirmation.
------------------------------------------------------------------------
June 30, 2004 - 10:22 : Bèr Kessels
Attachment: http://drupal.org/files/issues/batch_delete_step3.png (15.72 KB)
after pressing [update] again the nodes are actually deleted.
------------------------------------------------------------------------
June 30, 2004 - 14:59 : Dries
Wouldn't a dedicated confirmation page be a lot better usability-wise?
At least, that's what one would expect.
------------------------------------------------------------------------
June 30, 2004 - 15:52 : kika
Attachment: http://drupal.org/files/issues/typepad_multiitem_deletion.png (54.11 KB)
please, resign step 2 in you patch and build a proper confimation page.
Have look at the screenshot included, this is the Typepad
multi-item-deletion workflow (scroll down)
------------------------------------------------------------------------
June 30, 2004 - 15:57 : Bèr Kessels
True Dries, kika, thats why I pasted some comments in
http://drupal.org/node/view/7743#comment-9138.
I see only one big issue in the typepad approach. That will do for
deleting, but our range of updating- actions will not work there, or
you would need 5 columns of selection boxes. IMO thats a no-go. I linke
the current method. My gmx mail uses that, and Ive seen it a lot.
The dedicated screen for confirmation is a good idea, but kinda hard to
code, in the current state.
The big problem is that I tried that raod first (it seems much more
logical to go for that). But untill the node_admin_nodes() gets a big
cleanup, I think that adding a dedicated UI is a pain, and will make
the mess even worse. :(
So in fact the road to take here (IMO) is:
1) clean up the node.module admin area.
2) split all actions into separate functions (e.g.
node_admin_update_promote($nodelist) )
3) add a function forbatch deleting
4) maybe add some more actions (e.g batch re-taxonomising)
I tried to skip 1 and 2 and go straight for 3, but I ended up with
3-levels deep nested ifs nad foreaches and a 200+ lines big function
node_admin_nodes(). And still it had some bugs. I might have done it
worong there, but that made me decide to use this much simpler
approach.
------------------------------------------------------------------------
June 30, 2004 - 15:59 : kika
to clarify: I am not suggesting to use popup, just a standard
confimation page
------------------------------------------------------------------------
July 1, 2004 - 10:29 : Bèr Kessels
Allright,
All has been said, little done. I think we all agree on the fact that
there should be some confirmation screen.
But, as i pointed out, that will not be there for a while. Unless
someone else takes this task and does a total revamp of (the code of)
the node admin UI.
So, if we want some batch deletion, I would suggest to either start
coding and come up with a better UI, or use this solution.
Regards,
------------------------------------------------------------------------
July 1, 2004 - 11:58 : Dries
I don't intend to commit this patch as is. I'll see if I can make a
confirmration screen. Marking this active in the mean time.
------------------------------------------------------------------------
July 23, 2004 - 19:23 : laird
This patch is wonderful; I install it everywhere I use Drupal (several
sites now).
I'd like to make two feature requests:
1) It leaves behind records in the node-type specific tables. For
example, if you have node 1000 that's an image, this patch will delete
node 1000 from the node table, but won't delete node 1000's entry in
the image table. This isn't a huge problem, since it doesn't cause
anything to break (since they're not linked anywhere, so don't appear
in any selects joined against the node table). But it's a bit messy to
leave these disconnected records around in the database, so it should
be fixed on general principles.
2) If you are deleting a large number of nodes, you have to manually
click on each check box, then delete the batch. It'd be great to have a
'check all' button, to speed the process. It'd be even better to have a
means to 'delete all unpublished nodes' as a one-click way to clean up
the database. I have several automated feeds on one site, and it
accumulates tons of unused nodes that are quite tedious to delete
manually.
- LP
------------------------------------------------------------------------
August 15, 2004 - 21:59 : Anonymous
newbie who needs help
how do you incorporate the patch file into the node.module file?
does it have to be in a certain area or just at the top of the code?
------------------------------------------------------------------------
August 15, 2004 - 22:00 : Anonymous
newbie who needs help
how do you incorporate the patch file into the node.module file?
does it have to be in a certain area of the code?
------------------------------------------------------------------------
August 16, 2004 - 11:49 : Bèr Kessels
please refer to http://drupal.org/book/view/323 for info about patching.
And please be aware that you should be carefull with changing the
category of an issue.
------------------------------------------------------------------------
August 25, 2004 - 11:51 : Bèr Kessels
Attachment: http://drupal.org/files/issues/batch_delete_with_confirm page.patch (6.41 KB)
Final attempt. Now with a default confirmation screen. That confirmation
shows a list with titles of all nodes that are about to be deleted.
------------------------------------------------------------------------
October 29, 2004 - 19:46 : mpamphile
Is this patch 4.5 compliant ?
I would like to add it to a 4.5 site.
If anyone is intrested, I'm also modifying taxonomy.module so that
multiple terms can be added. If anyone is interested in this, let me
know... mpamphile hotmail . com.
------------------------------------------------------------------------
December 9, 2004 - 02:09 : michaelemeyers
can this be added to core? Very valuable/useful patch... thnx. _m
re - mpamphile on October 29, 2004: Is this patch 4.5 compliant ?
I just patched 4.5.1 and other than line numbers being slightly off,
the only difference was that in the Menu callback function node_admin()
I needed to make the following change:
case t('Delete'):
// $output = node_delete($edit); // remove this and add line
below
$output = node_admin_nodes($edit);
break;
--
View: http://drupal.org/node/7743
Edit: http://drupal.org/project/comments/add/7743
1
0
re: [drupal-devel] [feature] node_delete doesn't pass params to_deletecalls.
by Kent Livingston 15 Jan '05
by Kent Livingston 15 Jan '05
15 Jan '05
oops... Thought I was replying to the message below the one that I actually did :-)... It actually made sense in context to my friend who was saying she was looking for intelligent men....
"Kent Livingston" <klivingston(a)worldwebsystems.net> wrote:
__________
>
>At least you still, theoretically, believe there are intelligent men in the universe.
>
>Kent Livingston/World Web Systems
>Strategic Network, Web, and Email Systems - Sent via a wireless PDA
>
>darrian <drupal-devel(a)drupal.org> wrote:
>__________
1
0
re: [drupal-devel] [feature] node_delete doesn't pass params to _deletecalls.
by Kent Livingston 15 Jan '05
by Kent Livingston 15 Jan '05
15 Jan '05
At least you still, theoretically, believe there are intelligent men in the universe.
Kent Livingston/World Web Systems
Strategic Network, Web, and Email Systems - Sent via a wireless PDA
darrian <drupal-devel(a)drupal.org> wrote:
__________
1
0
15 Jan '05
Project: Drupal
Version: cvs
Component: node system
Category: feature requests
Priority: normal
Assigned to: Anonymous
Reported by: darrian
Updated by: darrian
Status: patch
A group is in fact multiple nodes. I did it this way for a few reasons,
one of them being other modules (such as remindme) work with it better
this way.
I am no longer working on development of this feature, so I am not
going to redesign the way its done. I only try and keep it updated so
other people that need this feature can use it, seeing as there is no
other alternative.
As for this particular patch to node.module I still think it is a bug
because variables available the first time any _delete function is
called dissappear after the confirm step. Those variables should be
available to module developers regardless of how they are used.
-Darrian
darrian
Previous comments:
------------------------------------------------------------------------
June 18, 2004 - 21:07 : darrian
Attachment: http://drupal.org/files/issues/node.module-cvs.patch (872 bytes)
When you delete a node, it asks for confirmation, after that step all
extra fields in $node get lost because $node is recreated with only 2
fields, nid and confirm. I created a small patch that adds extra
fields back into the $node variable so they get passed to subsequent
_delete calls.
------------------------------------------------------------------------
June 27, 2004 - 20:29 : Dries
AFAIK, deleting a node works fine. How can we reproduce your problem?
Not sure what you are trying to accomplish but this patch looks wrong.
------------------------------------------------------------------------
July 12, 2004 - 23:05 : darrian
Right, this patch is so the modification I made to event.module works.
I added functionality for repeating events to event.module, and in
order for the delete to work I had to make this small change outside of
event.module (believe me I really tried not to). What this patch does
is pass variables originally passed to the delete function to whats
called after the user clicks confirm.
In other words when a user clicks delete the first thing they get is a
prompt to confirm deletion, after the user clicks yes, the delete
function is called, but the only variables that are passed are nid and
confirmed, all other variables are lost, which is usually ok because no
other vars needed for deletion, however in my case I needed another
variable.
This patch has no effect on all current modules, the reason i made this
a bug report is because it seems to me like variables originally passed
to node_delete should still exist after the user clicks confirm, even
though current modules dont use other vars.
I made a reference to this bug from this bug:
http://drupal.org/node/view/4122
If the changes I made to event.module ever get put into the current cvs
version, then this patch also needs to get put in.
-Darrian
------------------------------------------------------------------------
July 13, 2004 - 07:28 : Dries
When an event (node) is deleted, the _delete hook is called and the
event (node) is passed as a parameter. That seems to work for all
other node modules/types so I don't understand why this shouldn't work
for the event module. What is so special about this particular field
or node type that this does not suffice?
------------------------------------------------------------------------
July 14, 2004 - 14:14 : darrian
$node is passed, but after the user clicks confirm, $node only has two
fields, nid and confirm. I need an extra field, which is whether I'm
supposed to delete only this event, or all events in the group. This
is for the reocurring events feature I added. My modification adds the
extra fields originally in $node back to $node after the user clicks
confirm.
-Darrian
------------------------------------------------------------------------
August 2, 2004 - 16:27 : darrian
In order for my repeating/reocurring event module patch to get submitted
to cvs, this very small patch needs to make it into core. It does not
effect the functionality of anything current.
Here's the details of what happens, >>> is what happens when my node
patch is there:
1. User is looking at the form to delete an existing event, there are a
bunch of fields like date/time title, etc. This holds true for any node,
but I am going to reference an event node.
2. User clicks delete to delete the node being looked at. (User checks
to delete all events in this group - from my event patch)
3. All the fields for that node are a form element at this point, some
hidden, like nid, and a variable I set telling the event_delete
function to delete all events in this group.
4. The code makes its way to the node_delete($edit) function. At this
point all the form elements from above are members of the $edit
variable, including my variable mentioned above.
5. The code checks to see if the user already clicked confirm, at this
point they haven't.
6. Now we're in the else branch that creates only two form elements,
the nid, and confirm. This has been ok in the past, because the only
thing that you needed to delete a node was its nid. But in my case I
want to delete multiple nodes, so this doesnt work.
>>> My node patch creates hidden form elements of all the previous
variables, not just nid and confirm.
7. The code makes it way back to node_delete, this time however there
are only two members of $edit, nid and confirm.
8. The code checks to see if the user has clicked confirm, and they
have.
9. Now we're in the true branch which deletes the core node components,
then calls the node specific delete function, in our case the
event_delete function, passing only the nid and confirm variables.
>>> My node patch makes it so all variables, including my varible to
check whether we're deleting all nodes or not, are passed back in.
10. Now we're in the event_delete function. The event delete function
checks for the variable to see if we're deleting all events in group,
it doesnt exist, so it acts as if the user didn't check to delete all
events, and deletes just this event.
>>> With my patch, all previous variables still exist, so event_delete
knows that it needs to delete multiple nodes.
Hopefully this patch gets into core before feature freeze for the sake
of the event module. If it doesnt, then my patch for
repeating/reocurring events also cannot make it into cvs.
Thanks, Darrian
------------------------------------------------------------------------
August 2, 2004 - 17:29 : moshe weitzman
the patch looks good to me. thanks for the clear description of the bug.
------------------------------------------------------------------------
August 13, 2004 - 08:03 : TDobes
+1, except the single comment prefixed with a # should be changed to //
------------------------------------------------------------------------
August 16, 2004 - 21:57 : darrian
Does the code freeze mean that this patch wont make it into 4.5?
-Darrian
------------------------------------------------------------------------
August 16, 2004 - 22:31 : Steven
This sounds like a bugfix or at least code fix, so it can still go in.
I'm wondering about your description though. It sounds that if I first
edit a couple of fields, then click delete, then my edited variables
are passed around rather than the original ones. Because I did not
click 'submit', this is not desirable.
It may be better to call node_load() before deleting the node, so the
correct, original data is passed to the nodeapi.
------------------------------------------------------------------------
August 17, 2004 - 01:09 : darrian
Remember, for node_delete, only nid and confirm are passed.
But I already do as you suggest, to be more specific:
node_load is called, those variables, along with extra fields that are
not loaded with node_load (such as my variable for deleting group
events), are passed to subsequent _delete calls.
Just to re-iterate, without this patch the ONLY variables that make it
to the final delete call are nid and confirm.
-Darrian
------------------------------------------------------------------------
August 17, 2004 - 01:55 : JonBob
The point is that (efficiency aside) if you have the node ID, you can
reconstruct the node with a simple node_load() call. If your delete
function is called with only $nid, you can use that to load the whole
node again if need be.
Now this may not be enough in your case, as you are presenting
additional interface to the user that adds fields to $edit that are not
part of the node, but all the parts *of the node* are certainly
available to you.
------------------------------------------------------------------------
September 5, 2004 - 17:47 : Steven
"The point is that (efficiency aside) if you have the node ID, you can
reconstruct the node with a simple node_load() call. If your delete
function is called with only $nid, you can use that to load the whole
node again if need be."
At the point where hook_delete gets called, the node itself has been
deleted already, so reconstruction is not an option. The point here
seems to be to pass variables from the editing form to the delete
operation. I'm not sure if this is a good idea.
If it's not about this, then maybe we should invoke deletes in the
opposite order: nodeapi, hook, node.module.
------------------------------------------------------------------------
September 9, 2004 - 23:05 : darrian
Right, the point is to get variables from the form passed to the delete
function, namely 1 variable, which tells me if I should delete 1 event,
or all grouped reoccuring events, that I need for the modification I
made to the event module.
The way I made the modification is very UN-obtrusive. No variables get
squashed, and everything current ignores the new variables.
Also, the variable that I want exists in the node_delete function the
first time around, its only after the user clicks confirm that all
variables except the nid are lost. So I could delete things with my
modified event module if i skip the confirm step, but I'm sure that
would bite someone down the road and I really dont want to do it that
way.
-Darrian
------------------------------------------------------------------------
October 4, 2004 - 21:06 : Steven
"Right, the point is to get variables from the form passed to the delete
function, namely 1 variable, which tells me if I should delete 1 event,
or all grouped reoccuring events, that I need for the modification I
made to the event module."
This sounds like really bad UI: the rest of the node form is about data
related to the node, which gets saved when you click "submit". Adding a
delete-related checkbox there sounds odd and confusing.
------------------------------------------------------------------------
October 8, 2004 - 21:58 : darrian
Attachment: http://drupal.org/files/issues/editevent.png (35.72 KB)
Here's the UI i'm referring to. This patch to node.module is extremely
small and unobtrusive... Why so much resistance to it?
------------------------------------------------------------------------
January 12, 2005 - 22:47 : darrian
Attachment: http://drupal.org/files/issues/node.module-4.5.1.patch (822 bytes)
Here's an updated patch for drupal 4.5.1
-Darrian
------------------------------------------------------------------------
January 13, 2005 - 00:28 : tangent
Is a recurring event a single node or a group of nodes? Without getting
into the appropriate implementation of that, your bug suggests that the
checkbox will allow multiple nodes to be deleted. This is why it has
been suggested that it is a confusing UI. It isn't necessarily clear
what the "group" of nodes is?
FWIW, if a recurring event is a single node (which is displayed in
multiple places on the calendar) then this would not be an issue.
--
View: http://drupal.org/node/8611
Edit: http://drupal.org/project/comments/add/8611
1
0
Project: Drupal
Version: cvs
Component: story.module
Category: tasks
Priority: normal
Assigned to: Bèr Kessels
Reported by: Bèr Kessels
Updated by: Bèr Kessels
Status: patch
Attachment: http://drupal.org/files/issues/story_improve_help.patch (3.19 KB)
**sigh**
Bèr Kessels
Previous comments:
------------------------------------------------------------------------
January 7, 2005 - 14:19 : Bèr Kessels
Attachment: http://drupal.org/files/issues/story_help.patch (3.06 KB)
Because of historical reasons the story module help contained all sortts
of odd texts, that are partly even untrue.
This patch changes the help so that it:
* stresses that stories are nodes in their simplest form
* removes all the bloat about workflow. workflow can be configured for
any node type, why should nstories mention it?
* removes parts of voting, queues, promotion, comments etc. Any node
type can have this.
* adds a note that you can use stories for personal blogging, and as
general pages, pages with no specific purpose (I consider, for example,
a forum node a node with a special purpose)
* stresses that you should use other modules (taxonnomy, menu) to give
the node a place on your site.
set to CVS but will apply to 4.5 too.....
------------------------------------------------------------------------
January 10, 2005 - 17:41 : Bèr Kessels
Attachment: http://drupal.org/files/issues/story_help_0.patch (3.06 KB)
previous patch was the one before spellchecking. This one is without the
two spelling errors.
------------------------------------------------------------------------
January 10, 2005 - 20:29 : Dries
Can someone review this please?
------------------------------------------------------------------------
January 12, 2005 - 14:53 : Bèr Kessels
Attachment: http://drupal.org/files/issues/story_help_1.patch (3.01 KB)
After some discussion on the drupal-docs ML, i came up with the
following help-texts:
Short:
Stories are articles in their simplest form: they have a title, a
teaser and a body, but can be extended by other modules. The teaser is
part of the body too. Stories may be used as a personal blog or for
news
articles.
Longer:
Stories are articles in their simplest form: they have a title, a
teaser and a body, but can be extended by other modules. The teaser is
part of the body too. Stories may be used as a personal blog or for
news
articles. By default, no menu item, or navigation element is
created for a story. An extra feature of a
story is, that an administrator can specify a submission guideline and
enforce a minimum word count for user submitted stories
------------------------------------------------------------------------
January 13, 2005 - 20:42 : Dries
This patch doesn't apply against HEAD.
patching file modules/story.module
Hunk #1 FAILED at 14.
Hunk #2 FAILED at 26.
2 out of 2 hunks FAILED -- saving rejects to file
modules/story.module.rej
------------------------------------------------------------------------
January 14, 2005 - 23:55 : Bèr Kessels
AARHGH, such a simple patch, so much hassle. My mistake, uplaoded the
wrong one again.
This one should be right.
--
View: http://drupal.org/node/15264
Edit: http://drupal.org/project/comments/add/15264
1
0
Project: Drupal
Version: cvs
Component: story.module
Category: tasks
Priority: normal
Assigned to: Bèr Kessels
Reported by: Bèr Kessels
Updated by: Bèr Kessels
Status: patch
AARHGH, such a simple patch, so much hassle. My mistake, uplaoded the
wrong one again.
This one should be right.
Bèr Kessels
Previous comments:
------------------------------------------------------------------------
January 7, 2005 - 14:19 : Bèr Kessels
Attachment: http://drupal.org/files/issues/story_help.patch (3.06 KB)
Because of historical reasons the story module help contained all sortts
of odd texts, that are partly even untrue.
This patch changes the help so that it:
* stresses that stories are nodes in their simplest form
* removes all the bloat about workflow. workflow can be configured for
any node type, why should nstories mention it?
* removes parts of voting, queues, promotion, comments etc. Any node
type can have this.
* adds a note that you can use stories for personal blogging, and as
general pages, pages with no specific purpose (I consider, for example,
a forum node a node with a special purpose)
* stresses that you should use other modules (taxonnomy, menu) to give
the node a place on your site.
set to CVS but will apply to 4.5 too.....
------------------------------------------------------------------------
January 10, 2005 - 17:41 : Bèr Kessels
Attachment: http://drupal.org/files/issues/story_help_0.patch (3.06 KB)
previous patch was the one before spellchecking. This one is without the
two spelling errors.
------------------------------------------------------------------------
January 10, 2005 - 20:29 : Dries
Can someone review this please?
------------------------------------------------------------------------
January 12, 2005 - 14:53 : Bèr Kessels
Attachment: http://drupal.org/files/issues/story_help_1.patch (3.01 KB)
After some discussion on the drupal-docs ML, i came up with the
following help-texts:
Short:
Stories are articles in their simplest form: they have a title, a
teaser and a body, but can be extended by other modules. The teaser is
part of the body too. Stories may be used as a personal blog or for
news
articles.
Longer:
Stories are articles in their simplest form: they have a title, a
teaser and a body, but can be extended by other modules. The teaser is
part of the body too. Stories may be used as a personal blog or for
news
articles. By default, no menu item, or navigation element is
created for a story. An extra feature of a
story is, that an administrator can specify a submission guideline and
enforce a minimum word count for user submitted stories
------------------------------------------------------------------------
January 13, 2005 - 20:42 : Dries
This patch doesn't apply against HEAD.
patching file modules/story.module
Hunk #1 FAILED at 14.
Hunk #2 FAILED at 26.
2 out of 2 hunks FAILED -- saving rejects to file
modules/story.module.rej
--
View: http://drupal.org/node/15264
Edit: http://drupal.org/project/comments/add/15264
1
0
[drupal-devel] [bug] new revisions not created if user doesn't have administer nodes permission
by robertDouglass 14 Jan '05
by robertDouglass 14 Jan '05
14 Jan '05
Project: Drupal
Version: 4.5.0
Component: node system
Category: bug reports
Priority: critical
Assigned to: wazdog
Reported by: wazdog
Updated by: robertDouglass
Status: patch
My opinions:
1. administer nodes is the wrong permission to determine revisions
behavior. At least one revision-specific permission is necessary,
perhaps two or more (view, create, delete, rollback revisions).
2. If the default workflow says revisions are made, this should
definitely make it so that normal users create revisions every time.
3. Users with admin rights (either a revisions specific permission or
administer nodes) can override the default. All others should not see
the checkbox.
4. Whether revisions are rolled back destructively or non-destructively
*could* be a setting, but if not, should be non-destructive by nature as
we already have a mechanism for deleting revisions.
-----
That much is the minimum I consider necessary to be a useable system. I
would additionally wish for the following:
5. Since I would like to have a site where normal users can view any of
the revisions, but not delete and not roll back, I would really like to
see the following permissions: read revision, rollback revision, delete
revision
6. The ability to compare two or more revisions [1]
7. The possibility for moderation of revisions ie. people with
administer nodes (or moderate revisions) look at a queue of revisions
and rollbacks and approve or disapprove them.
8. The possibility to vote on revisions. 7 could be an extension of 8.
If only administrators can vote on revisions, it would be the same as
moderating them.
I know the last bits sounds like a bunch of pipe dreams, and may be off
topic for this thread, but the system I imagine would be for a site
where the wording of entries must be considered very carefully by many
people and a concensus about the best version be found. The above
features would achieve this.
[1] http://drupal.org/node/15596
robertDouglass
Previous comments:
------------------------------------------------------------------------
November 1, 2004 - 10:33 : wazdog
If you enable revisions for a node type via content > configure >
default workflow, and the user who is editing a node of that type
doesn't have the administer nodes permission, then a new revision is
not created.
I see this a huge bug. I am trying to have all edits to a node type
saved as revisions (in this case a flexinode, but I also tested with
blog types). I want to recreate the book pages, but with more fields
(hence, flexinode).
But if one of my authenticated users creates a node, and then later
edits it, a revision is not made. However, if an admin or moderator
(who both have the administer nodes permission) edits the node, a new
revision is created. I don't want my regular users to have administer
nodes permission, that gives them too much power, but their revisions
need to be kept (just like I give them all publish permission via
default workflow).
If this is supposed to be how it works, what is the point of the
default workflow setting? (and why do the other default workflow
settings behave differently? i.e. they all apply no matter the users
permissions)
------------------------------------------------------------------------
November 3, 2004 - 02:48 : wazdog
Attachment: http://drupal.org/files/issues/node-rev.patch (721 bytes)
Okay, here's a short little patch. It seems like node_create_revision()
was being called before $node->revision was set. Since $node->revision
is what tells Drupal whether or not to make a new revision, this was
causing nodes to not keep revisions. I moved node_create_revision() to
after this check.
I'm not sure exactly why only regular users without 'administer nodes'
permission were affected, but that seems the case.
Now, if a particular node type has 'revision' checked in content >
default workflow, a new revision is always created, no matter who is
doing the editing...
I'm setting this to critical because I think the current behavior goes
against logic, and what admins may expect. And if they don't notice
it's not working like they expect (i.e. revisions aren't being kept
always), then content is lost...
The patch is against 4.5.0, but always applies to current cvs (with a
slight offset error).
------------------------------------------------------------------------
January 14, 2005 - 22:54 : mcd
I tried this patch against 4.5.1 and it worked, but it took me some time
to figure out that it was working because it's still not doing the
intuitive thing. The user still can't see the revisions tab unless
they have node administration privileges (which makes it unlike the
outline tab). They can't see the checkbox to decide whether their edit
is a new revision, so every edit they do becomes a revision from which
they derive no benefit.
I would let them see the revision checkbox and the revisions tab, and
also roll back (non-destructively as suggested in
http://drupal.org/node/12479)
I don't think there is an intuitive way to handle the five node
administration Options checkboxes as a group while appearing to enable
them separately in the default workflow. I've had similar issues with
the defaults, like a page I made sticky coming unstuck every time the
owner edited it (with the default non-sticky in the workflow). Now I
see why that was happening, but it's still highly counterintuitive.
------------------------------------------------------------------------
January 14, 2005 - 22:56 : mcd
Sorry, I didn't mean to change the main title.
------------------------------------------------------------------------
January 14, 2005 - 23:11 : chx
IMHO in some situations it is a good thing if a new revision is created
every time a user edits the node. It's not her privilege to roll back,
it's something a moderator should do. It'd be even better if the new
revision would not be automatically published only after approval by
the moderator.
------------------------------------------------------------------------
January 14, 2005 - 23:18 : mcd
Rolling back nondestructively is just a means of editing a page. Why
should a user with permission to edit be prevented from that edit?
They can certainly do more destructive things.
As for creating new revisions every time, it depends on usage patterns.
You may not want too many almost indistinguishable revisions collecting
in the revisions tab.
--
View: http://drupal.org/node/12401
Edit: http://drupal.org/project/comments/add/12401
1
0