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
May 2005
- 81 participants
- 503 discussions
[drupal-devel] [bug] taxonomy.module should run check_plain on rss output
by crunchywelch 02 May '05
by crunchywelch 02 May '05
02 May '05
Issue status update for http://drupal.org/node/21751
Project: Drupal
Version: cvs
Component: taxonomy.module
Category: bug reports
Priority: normal
Assigned to: Anonymous
Reported by: crunchywelch
Updated by: crunchywelch
Status: patch
Attachment: http://drupal.org/files/issues/taxonomy_8.patch (694 bytes)
In the case of a category name with an amphersand or other reserved xml
character the rss output of taxonomy module can break the feed. This
one line patch runs check_plain on the category value in the
taxonomy_rss_item() function which is the way that node_feed works and
solves the problem. This patch is against cvs, can provide one for 4.6
if needed.
Thanks!
crunchywelch
2
1
A lot of unfortunate and unplanned events held me from finally writing up the
notes made at the usability meetings. The first two, most important two, are
up.
http://drupal.org/node/21753
http://drupal.org/node/21754
I tried to summarise as much as possible. TRied to remove all the cruft and
defined tasks. Please let me know if you have any comments (other then that
the images are big and the space above the tables is funny).
If you like a task, please feel free to take it on you. Just add a tas in the
issue tracker and link (or send methe link) to it in these book pages.
Regards
Bèr
--
[ Bèr Kessels | Drupal services www.webschuur.com ]
1
0
Issue status update for http://drupal.org/node/13148
Project: Drupal
Version: cvs
Component: base system
Category: bug reports
Priority: normal
Assigned to: Anonymous
Reported by: kbahey
Updated by: clydefrog
Status: patch
That should be "encourage other more marginal crawlers to observe the
standards."
clydefrog
Previous comments:
------------------------------------------------------------------------
November 18, 2004 - 20:29 : kbahey
Looking at my site's logs, there seem to be several problems that are
caused by Drupal's use of relative path names.
If Drupal causes all the site's urls to be absolute, then none of this
would be an issue.
A. Search Engine Crawlers
Getting lots of 404s on things like: linux/index.html/robots.txt
Where 'linux' is an alias to a taxonomy, and 'index.html' is an alias
to a node within that taxonomy.
Another example, is recursing unnecessarily. I see 404s on things like:
/linux/index.html/linux/index.html
Where 'linux' is a path alias for a taxonomy term, and 'index.html' is
an alias to the main node within it.
This does not seem to happen when Google crawls my sites, but Yahoo's
Slurp suffers from this problem, and keeps recursing. MSNBot also
suffers from this.
Another crawler/harvester called Blinkx/DFS-Fetch keeps adding the .css
file to the relative path, getting a 404 on things like:
/linux/themes/xtemplate/pushbutton/logo.gif
And Fast Search Engine also attempts to access:
/linux/contact/tracker/tracker/user/password
The same goes for grub.org, another crawler.
B. Google Cache / Archive Way Back Machine
Pages in Google cache and archive.org Way Back Machine suffer form a
similar problem: the .css files cannot be found, and hence rendering of
the pages is not correct.
Examples:
Compare this: http://www.drupal.org/node/4647
To this:
http://www.google.ca/search?q=cache:www.drupal.org/node/view/4647
Notice the following:
How there is no formatting at all, because of the lack of a .css file
The httpd log on Drupal will show errors for:
linux/themes/pushbutton/style.css and linux/misc/drupal.css
Also see:
http://web.archive.org/web/20031016184902/http://www.drupal.org/
C. Proxy Caches:
When someone is browsing my site from behind a proxy cache, the web
site is hit with a rapid succession of requests, and many of it is just
for bogus pages.
Examples:
2004/11/17 - 17:47 404 error: linux/user/1 not found.
2004/11/17 - 17:47 404 error: linux/feedback not found.
2004/11/17 - 17:47 404 error: linux/tracker not found.
2004/11/17 - 17:47 404 error: linux/sitemap not found.
2004/11/17 - 17:47 404 error: linux/search not found.
2004/11/17 - 17:47 404 error: linux/misc not found.
2004/11/17 - 17:47 404 error: linux/programming not found.
2004/11/17 - 17:47 404 error: linux/programming not found.
2004/11/17 - 17:47 404 error: linux/linux not found.
2004/11/17 - 17:47 404 error: linux/technology not found.
2004/11/17 - 17:47 404 error: linux/writings not found.
2004/11/17 - 17:47 404 error: linux/family not found.
And also:
2004/11/17 - 07:23 404 error: history/user/1 not found.
2004/11/17 - 07:23 404 error: history/tracker not found.
2004/11/17 - 07:23 404 error: history/feedback not found.
2004/11/17 - 07:23 404 error: history/sitemap not found.
2004/11/17 - 07:23 404 error: history/search not found.
2004/11/17 - 07:23 404 error: history/misc not found.
2004/11/17 - 07:23 404 error: history/technology not found.
2004/11/17 - 07:23 404 error: history/science not found.
2004/11/17 - 07:22 404 error: history/history not found.
2004/11/17 - 07:22 404 error: history/writings not found.
2004/11/17 - 07:22 404 error: history/family not found.
As you can tell, history and linux are aliases to taxonomy terms, and
so is misc, technology, writings, family, ...etc. The user agent is
appending the taxonomy term alias to the url and forming a new URL.
D. Regular Browsing:
There is even at least one extreme case where the following URL was
accessed (the result was 404 of course)
/book/view/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/logo.gif
It seems it was a normal user, because the user agent is: "Mozilla/4.0
(compatible; MSIE 5.5; Windows 98; Win 9x 4.90)"
Proposed Solution:
As a proposed solution, all URLs in Drupal can be made into absolute
path names. This can be done by the following:
The variable $base_url in the conf.php file is broken down into two
components:
$base_host (the 'http://whatever-host.example.com' part WITHOUT the
trailing slash)
$base_path (the '/path-to-drupal' part, WITH the leading slash. If this
is the DocumentRoot, then it is just a '/' character)
$base_url is now $base_host concatenated with $base_path
A simple filter can be written to preceed every href="path" with the
$base_path variable, so it becomes "/path"
This option can be turned on and off for a site. The default is to have
it off so current behavior is maintained.
A similar scheme applies for style sheets as well.
So, did I miss something obvious? Am I seriously off the mark?
Your thoughts!
------------------------------------------------------------------------
November 19, 2004 - 21:10 : chrisada
I am getting similar 404 errors, mainly from rss feed link that looks
like /blog/blog/feed and many manual links that are relative to drupal
root.
It was not a problem before Drupal 4.5, so I think there might not be a
need to change all URIs to absolute. I can't see where the problem is
coming from though.
------------------------------------------------------------------------
November 19, 2004 - 21:35 : kbahey
I am pretty sure that these problems were happening for at least the
past 10 months (ever since I moved to Drupal in January 2004).
The main issue here is that crawlers and other user agents get confused
by the relative path names.
Using absolute paths will definitely solve this. However, is this the
only solution?
I am looking for a discussion of this.
------------------------------------------------------------------------
November 20, 2004 - 02:55 : Goba
No absolute paths please. Having the path start with '/' solves all the
mentioned problems, and is not absolute, it is relative to the domain.
Sadly some crawlers and even the Google Cache does not obey to the base
href. I have reported this cache problem in April to Google, and they
promised they will keep it in mind... Hehe...
What we need is to have the printed relative path values relative to
the domain name, and not relative to the Drupal installation path.
Note that this issue will appear on the drupal devel mailing list if
someone finally provides a patch we can talk about :)
------------------------------------------------------------------------
November 20, 2004 - 03:19 : Dries
Goba is right. We need paths relative to the domain name to fix this
'problem'.
------------------------------------------------------------------------
November 20, 2004 - 13:19 : kbahey
Sorry for not making my self clear.
When I said absolute, I meant that they start with just a /. I did NOT
mean that they start with http://host.example.com. That would be a very
bad idea.
In any case, what do people think about the proposed solution (breaking
down $base_url into two parts?)
Also, does this address the style sheets as well, or more is needed?
------------------------------------------------------------------------
November 21, 2004 - 09:24 : chx
Attachment: http://drupal.org/files/issues/common_inc_patch.txt (471 bytes)
I have implemented what Goba suggested.
------------------------------------------------------------------------
November 21, 2004 - 09:43 : chx
Attachment: http://drupal.org/files/issues/common_inc_patch_0.txt (825 bytes)
Maybe this one is faster?
------------------------------------------------------------------------
November 21, 2004 - 10:34 : kbahey
Man! You are fast!
I tried the second version. It works fine for things that are not
inside the node body, I mean they have a / in front of them, as we
want it to be.
Two comments/issues:
- If there is a URL that is already "/" representing the home page, it
gets set to "//". Perhaps it should check for that case?
- URLs in nodes that do not start with / do not get changed to have a /
prepended to them. Do we need a filter for this?
- Do we need to do something for the style sheets in the page header? I
mean the "misc/drupal.css" and "themes/themename/style.css"?
Thanks
------------------------------------------------------------------------
November 21, 2004 - 17:28 : kbahey
Hi chx
Here is a fix for the case where you have a url that is just "/".
In your patch, instead of:
<?php
$base = $parts['path'] . '/' ;
?>
Replace that by:
<?php
$base = ( $path == '/' ? $base : $parts['path'] . '/' );
?>
------------------------------------------------------------------------
November 27, 2004 - 21:08 : kbahey
Did this patch make it into CVS yet?
If there are any objections to it, can someone please explain what they
are?
Thanks
------------------------------------------------------------------------
November 28, 2004 - 02:33 : Dries
Shouldn't your changes be included in the patch?
Also, it's better to cache $base rather than $parts.
Lastly, it this patch makes it to HEAD, we should probably remove some
'base url' cruft from the themes.
------------------------------------------------------------------------
November 28, 2004 - 11:54 : kbahey
Attachment: http://drupal.org/files/issues/x.diff (1 KB)
Here is the patch including my fix.
I am asking chx to comment on caching $base instead of $parts.
Will this make it faster?
------------------------------------------------------------------------
November 28, 2004 - 12:26 : chx
Hm. $base = ( $path == '/' ? $base : $parts['path'] . '/' ); this
depends on path which is a parameter. Thus I fail to see how could we
cache $base. I'd correct this code however $base = ( $path == '/' ? ''
: $parts['path'] . '/' ); 'cos I think $base is not defined before, but
this is not a problem, PHP will be happy to replace NULL with NULL...
Maybe instead of all parts, only $parts['path'] is enough to be cached,
yes, but the performance and memory usage difference -- I guess -- would
not be noticable...
------------------------------------------------------------------------
November 28, 2004 - 16:06 : kbahey
Attachment: http://drupal.org/files/issues/common-inc-patch.txt (1 KB)
OK.
I put in chx suggested change.
This patch can go in CVS then, to rid us of the problems with paths not
beginning with slash.
This is not an ultimate solution still. We need to address the problem
with .css files. Although the header contains a:
<base href="http://example.com" />
it does not seem that major search engines and archiving sites obey it
anyway.
------------------------------------------------------------------------
December 2, 2004 - 13:31 : Dries
Your coding style needs work. Also, I'm not going to commit this unless
the themes get fixed up: we'd end up with invalid URLs all over the
place. Lastly, I wonder how portable the themes will be when Drupal is
run from within a subdirectory.
------------------------------------------------------------------------
December 2, 2004 - 14:15 : chx
Attachment: http://drupal.org/files/issues/common_inc_patch_1.txt (849 bytes)
Well, my patch worked from a subdirectory very well, as fact, I have not
tested it from the root dir. And I think that it adheres to coding
standards. So I resubmit it with the root path fix. However, my Drupal
work is focused on i18n these days, and I was never into themeing so it
won't be me who fixes those.
------------------------------------------------------------------------
December 2, 2004 - 15:07 : kbahey
I have tested the previous patch (including my fix) with drupal
installed in the DocumentRoot of the server.
So, in effect, it is tested with both Drupal in / and Drupal in a
subdirectory.
This change fixes the problem for the crawlers and other browsers from
getting confused.
While it is true that there is no fix for the .css files in the HTML
head section yet, this fix deals with a major part of the problem, and
rids us of a major pain. Check your web server's logs some time to see
what I mean.
Someone who is familiar with the themes can contribute a patch later.
This patch and the future fix for themes are not mutually exclusive, so
let it go in CVS.
------------------------------------------------------------------------
December 9, 2004 - 08:31 : Goba
Please commit this into Drupal core, this fix is badly needed.
------------------------------------------------------------------------
January 17, 2005 - 06:00 : chx
Attachment: http://drupal.org/files/issues/base_url_kill.patch (4.34 KB)
Well as noone have stepped in to fix this problem, I have tried to fix
the themes also. themes.inc , xtemplate.engine and the bluemarine
template is patched besides common.inc.
Of course, more templates could follow, but first I'd like to see your
opinions.
------------------------------------------------------------------------
January 17, 2005 - 06:09 : Goba
I don't think that removing <base> from the themes is a good idea, using
$parts['path'] should be encouraged though before the files, which would
fix the google cache problem, and would still keep the HTML size low. It
would also help those, who save the file to find the originating site
easier, since clicking on a non-pagelocal link would lead to the online
version.
------------------------------------------------------------------------
January 17, 2005 - 09:03 : Steven
Definitely -1 on removing the <base> tag or using absolute or
root-relative URLs. This tag has been around for ages, and it is the
only way to make clean URLs work without bloating in the code. FYI,
"base" is (first?) mentioned in Berners-Lee's HTML 1.0 draft [1].
That's June 1993.
As the amount of clean URL-using sites grows, the crawlers will have to
be updated. Perhaps we could prevent crawlers from going too insane by
404ing for URLs with more than say 10 components? That would prevent
the really crappy ones from hammering your site.
I'm all for making the <base> tag easier to handle for the user (say,
by including a filter to allow simple anchor links to work as most
people expect them to), but we should keep Drupal-generated URLs clean
and completely relative.
[1] http://www.w3.org/MarkUp/draft-ietf-iiir-html-01.txt
------------------------------------------------------------------------
January 17, 2005 - 09:57 : kbahey
The problem with css is this: The @import argument does not start with a
/.
This is simple to fix.
We keep the "base" as it is today, but add the new variable: $base
before it.
So for a site where Drupal is installed in the DocumentRoot, all that
will change is that /misc/drupal.css and /themes/themename/style.css
will be preceded by a slash. For sites that use another path, that path
will be prepended to the css file name.
How about that?
------------------------------------------------------------------------
January 17, 2005 - 10:38 : Steven
What exactly is the problem with the @import? As far as I know:
- url() in stylesheets is interpreted relative to the base of the
stylesheet, not the source document.
- However, if the styles are inside an HTML document, through a style
tag or style attribute, then the stylesheet's location is the same as
the HTML document.
- Thus, the stylesheet's base is the same as the base of the HTML
document (which can be altered through the <base> tag).
I just don't see why it is necessary. As far as I know, the only
browser that has had problems resolving CSS urls properly was Netscape
4, which does not support @import at all, and which Drupal does not
support either, because of its CSS usage.
------------------------------------------------------------------------
January 17, 2005 - 11:35 : kbahey
The problem for stylesheets is as follows. I think it mainly affect
crawlers and Google's cache.
Say you have an installtion of Drupal in DocumentRoot. You then use url
aliases, and put slashes in them.
For example, you use news/general/2004-12-15.html for a node.
That node still has misc/drupal.css and themes/pushbutton/style.css in
the head section if the document. Crawlers get fooled by that and try
to look for /news/general/misc/drupal.css and
/news/general/themes/pushbutton/style.css, which don't exist.
So, just prepending the new $base variable (in chx's patch) before the
stylesheet @import argument would fix this issue. Assuming you are in
DocumentRoot, then /misc and /themes would be used instead of just misc
and themes.
It would still be compliant with standards, be relative to the web
site, and no ambiguous to anyone, be they crawler or browser.
I hope it is clearer now.
I think chx can change the patch to use the $base instead of $base_url
everywhere, so as to avoid the host/domain name in the urls.
------------------------------------------------------------------------
January 17, 2005 - 14:36 : Steven
But typical crawlers don't even pay attention to stylesheets, hence it
wouldn't have much use for them. I just don't see why we should adjust
to rare cases of buggy software. Reading out a base URL from an HTML
document is dead easy, and on top of that it doesn't add more
complexity as without the base tag, the document's URL is already an
implicit base which has to be parsed anyway.
I did not like it when we altered the <link> tag to accomodate buggy
RSS readers and I certainly don't like it now, as this is even rarer.
In both cases, it is not Drupal which is at fault.
------------------------------------------------------------------------
January 17, 2005 - 14:48 : kbahey
Steven
While I agree with most of what you said, the 404s show up in the logs
enough to be a bother.
Perhaps the original design of Drupal did not forsee that people will
use url aliases to mimic directory/file hierarchies. Whether this was
intended or not, it is the way many use Drupal today.
It does not matter where the bug is (Drupal or the external world), as
long as we can stop it ourselves, by adjusting our end of it.
The fix is simple enough and does not break standards (if implemented
as described with a leading / before the css).
------------------------------------------------------------------------
January 17, 2005 - 14:54 : Steven
It does not break standards, but it does bloat the code in an ugly way.
Why not send an e-mail to the owners of the crawlers and tell them to
implement a standard that is nearly 10 years old [2] (RFC 1808)?
Note that Google Cache now seems to correctly interpret base URLs [3]
and even adds a <base> tag of its own.
By the way, this problem has nothing to do with people using URL
aliases or not, as for a browser the regular nested paths that Drupal
uses (e.g. "node/1" is no different from aliases mimicking files
"foo/bar.html").
[2] http://www.faqs.org/rfcs/rfc1808.html
[3]
http://www.google.be/search?q=cache%3Awww.drupal.org&sourceid=mozilla-searc…
------------------------------------------------------------------------
January 17, 2005 - 15:34 : Goba
Steven, part of the problem is that Google cache does add a base href
even if there is a base href in the document. Eg adds a <BASE
HREF="http://drupal.org/node/13733"> on the plone comparision page
cached. Now that since HTML does not allow more than one base tag [4]
to be present, it is up to the browsers, to use the first or the last
base value, or any of the base values on the page for that matter as
the used base. So even pages displayed from the google cache will be
buggy if a full relative path to the domain root is not specified, due
to this problem.
[4]
http://www.w3.org/TR/1999/REC-html401-19991224/sgml/dtd.html#head.content
------------------------------------------------------------------------
January 18, 2005 - 00:14 : chx
Attachment: http://drupal.org/files/issues/base_url_kill_0.patch (4.75 KB)
This one does not use the whole base_url only the path part of it. HTML
bloat is kept at minimal.
------------------------------------------------------------------------
February 1, 2005 - 16:33 : clairem
Please please can this be done?
It's a good idea in itself, but if using fully-qualified paths means we
can get rid of the BASE HREF, then page anchors will work without having
the overhead of a filter. That's be a huge bonus for those creating
larger nodes, or who just want to be able to put a "skip navigation"
link in their theme without having to abandon Xtemplate or PHPtemplate
------------------------------------------------------------------------
February 1, 2005 - 16:37 : Goba
Well, speaking of skip navigation links, phptemplate and xtemplate
should expose the REQUEST_URI to the templates, so when a link to an
anchor on the same page is needed, the link can be formatted with the
complete request URI in mind.
------------------------------------------------------------------------
February 2, 2005 - 16:40 : clairem
"hptemplate and xtemplate should expose the REQUEST_URI to the templates
"
Should, but don't :(
If BASE HREF isn't removed, surely it wouldn't be a big job to
implement this tweak?
------------------------------------------------------------------------
February 17, 2005 - 07:50 : kbahey
This patch is badly needed. The lack of a leading / in many paths is
causing lots of problems.
------------------------------------------------------------------------
March 12, 2005 - 13:32 : kbahey
Can this patch be applied for 4.6? it is really badly needed.
------------------------------------------------------------------------
March 22, 2005 - 13:50 : Dries
I don't see why this is badly needed. We generate perfectly valid URLs
which are supposed to be short and crispy. This patch has some
advantages though, yet it is unclear which patch to go with.
------------------------------------------------------------------------
March 22, 2005 - 14:00 : chx
The second patch is better.
------------------------------------------------------------------------
March 22, 2005 - 14:46 : grohk
Forgive me for saying so, but since the way Drupal is generating
hyperlinks is completely valid, why are you suggesting Drupal should
move away from an accepted standard when the problem lies with the
search engines?
At the very least, this needs to be optional -- which it appears to be
-- I hate the 404s too, but I hate to hear that a change in Drupal is
needed to fix a Google problem.
------------------------------------------------------------------------
March 22, 2005 - 15:03 : jhriggs
I have to agree with the last comment from grohk.
------------------------------------------------------------------------
March 22, 2005 - 15:39 : mathias
I also agree with the two previous Drupaleers, but I wouldn't mind
enabling a 'quirks mode' via my conf file to stop the flood of 404
messages.
------------------------------------------------------------------------
March 22, 2005 - 17:37 : kbahey
I really can't fathom why some of us cannot deal with with the realities
out there in the world.
These problems are not because Drupal is broken. It is because crawlers
are. We cannot just bury our collective heads in the sand and say that
we are standards compliant and forget about what is out there.
As an analogy, people who design themes or write CSS have to deal with
the ugliness of Microsoft Internet Explorer and its intentional going
against standards. You cannot tell a client or your boss that you are
not modifying a theme that works perfectly on Konqueror and Firefox
because MS IE is broken.
Similarly, we cannot ignore that crawlers from major search engine
companies are broken or confused, and keep recursing through site using
Drupal causing countless errors in the logs. We cannot tell our users to
ask Google and Yahoo et al to fix their software.
Remember that we are not breaking any standards by implementing this
patch. All we are doing is putting the entire path out (from the first
/ down) and thus eliminating ambiguity for everyone.
Sorry if I am a bit blunt in this post, but I am tired of what may be
seen as isolationist thinking.
I do not mind if this is implemented in an advanced mode or via a
settings.php thing. All I care about is getting it fixed somehow.
------------------------------------------------------------------------
April 29, 2005 - 16:29 : kbahey
Circular log errors reported here too http://drupal.org/node/9499
------------------------------------------------------------------------
April 30, 2005 - 12:35 : shane
I agree with KBAHEY. Burrying our head in the sand and saying "it ain't
our problem" is not going to fix the issue. I despise companies that
break standards - and I applaud Drupal for working hard to keep within
those confines. But the reality is money grubbing, lazy ass
programmers exist the world over, and the consequence is things like
MSIE breaking everything wantonly and intentionally, Google, Yahoo, et
al implementing poor bot code, etc...
I believe this desperately needs to get fixed. Ever since I started
hand writing HTML code in 1992 I have always insured that my URL paths
are absolute to the base html document root (eg, preceeded with "/" and
the full path). It avoids confusion, problems, or issues. It seems odd
that the debate over this would rage as it has in this thread.
...and I don't get the "bloat" discussion. How is this bloating
things? Are we talking a few dozen extra characters? I hope I'm
missing something more obvious and insidious than that!?
I've been a loyal Drupaler for ages now, and I love it. But this new
problem is causing me a lot of grief, I see frequent munging of the
URLs, and it worries me; particularly when I see that there are end
users getting 404s. They don't give a rats ba-tu-tey that Drupal is
"standards compliant" ... they just know they got an error when they
supposedely did exactly what they should have, click on a URL. That
reflects poorly on the site owner and ultimately on the software
itself.
Please reconsider this issue, and let a patch go into core to fix it.
It doesn't make sense to let it rage on as an issue that is causing
lots of people obvious grief. I'm betting it's a bigger issue than
most admins think - most don't spend the anal-retentitive time that I
and others do grubbing through our logs, trying to insure a "perfect"
surfing experience for our end-users...
------------------------------------------------------------------------
May 1, 2005 - 15:09 : clydefrog
This is truly not a problem with Drupal, but it may be reasonable to
change Drupal's behavior anyway.
The base href tag has been in the W3C standards since /1997/ [5].
Failing to observe this tag isn't about being slow on the uptake (as
with MSIE and CSS2). It's about deliberately breaking existing
compatibility.
Has anyone contacted Yahoo, MSN, etc. and told them of this problem? If
and when they fix their crawlers, we need to be able to turn off this
kludge to discourage other more marginal crawlers to observe the
standards.
[5] http://www.w3.org/TR/REC-html32#base
1
0
Issue status update for http://drupal.org/node/13148
Project: Drupal
Version: cvs
Component: base system
Category: bug reports
Priority: normal
Assigned to: Anonymous
Reported by: kbahey
Updated by: clydefrog
Status: patch
This is truly not a problem with Drupal, but it may be reasonable to
change Drupal's behavior anyway.
The base href tag has been in the W3C standards since /1997/ [1].
Failing to observe this tag isn't about being slow on the uptake (as
with MSIE and CSS2). It's about deliberately breaking existing
compatibility.
Has anyone contacted Yahoo, MSN, etc. and told them of this problem? If
and when they fix their crawlers, we need to be able to turn off this
kludge to discourage other more marginal crawlers to observe the
standards.
[1] http://www.w3.org/TR/REC-html32#base
clydefrog
Previous comments:
------------------------------------------------------------------------
November 18, 2004 - 20:29 : kbahey
Looking at my site's logs, there seem to be several problems that are
caused by Drupal's use of relative path names.
If Drupal causes all the site's urls to be absolute, then none of this
would be an issue.
A. Search Engine Crawlers
Getting lots of 404s on things like: linux/index.html/robots.txt
Where 'linux' is an alias to a taxonomy, and 'index.html' is an alias
to a node within that taxonomy.
Another example, is recursing unnecessarily. I see 404s on things like:
/linux/index.html/linux/index.html
Where 'linux' is a path alias for a taxonomy term, and 'index.html' is
an alias to the main node within it.
This does not seem to happen when Google crawls my sites, but Yahoo's
Slurp suffers from this problem, and keeps recursing. MSNBot also
suffers from this.
Another crawler/harvester called Blinkx/DFS-Fetch keeps adding the .css
file to the relative path, getting a 404 on things like:
/linux/themes/xtemplate/pushbutton/logo.gif
And Fast Search Engine also attempts to access:
/linux/contact/tracker/tracker/user/password
The same goes for grub.org, another crawler.
B. Google Cache / Archive Way Back Machine
Pages in Google cache and archive.org Way Back Machine suffer form a
similar problem: the .css files cannot be found, and hence rendering of
the pages is not correct.
Examples:
Compare this: http://www.drupal.org/node/4647
To this:
http://www.google.ca/search?q=cache:www.drupal.org/node/view/4647
Notice the following:
How there is no formatting at all, because of the lack of a .css file
The httpd log on Drupal will show errors for:
linux/themes/pushbutton/style.css and linux/misc/drupal.css
Also see:
http://web.archive.org/web/20031016184902/http://www.drupal.org/
C. Proxy Caches:
When someone is browsing my site from behind a proxy cache, the web
site is hit with a rapid succession of requests, and many of it is just
for bogus pages.
Examples:
2004/11/17 - 17:47 404 error: linux/user/1 not found.
2004/11/17 - 17:47 404 error: linux/feedback not found.
2004/11/17 - 17:47 404 error: linux/tracker not found.
2004/11/17 - 17:47 404 error: linux/sitemap not found.
2004/11/17 - 17:47 404 error: linux/search not found.
2004/11/17 - 17:47 404 error: linux/misc not found.
2004/11/17 - 17:47 404 error: linux/programming not found.
2004/11/17 - 17:47 404 error: linux/programming not found.
2004/11/17 - 17:47 404 error: linux/linux not found.
2004/11/17 - 17:47 404 error: linux/technology not found.
2004/11/17 - 17:47 404 error: linux/writings not found.
2004/11/17 - 17:47 404 error: linux/family not found.
And also:
2004/11/17 - 07:23 404 error: history/user/1 not found.
2004/11/17 - 07:23 404 error: history/tracker not found.
2004/11/17 - 07:23 404 error: history/feedback not found.
2004/11/17 - 07:23 404 error: history/sitemap not found.
2004/11/17 - 07:23 404 error: history/search not found.
2004/11/17 - 07:23 404 error: history/misc not found.
2004/11/17 - 07:23 404 error: history/technology not found.
2004/11/17 - 07:23 404 error: history/science not found.
2004/11/17 - 07:22 404 error: history/history not found.
2004/11/17 - 07:22 404 error: history/writings not found.
2004/11/17 - 07:22 404 error: history/family not found.
As you can tell, history and linux are aliases to taxonomy terms, and
so is misc, technology, writings, family, ...etc. The user agent is
appending the taxonomy term alias to the url and forming a new URL.
D. Regular Browsing:
There is even at least one extreme case where the following URL was
accessed (the result was 404 of course)
/book/view/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/themes/xtemplate/pushbutton/logo.gif
It seems it was a normal user, because the user agent is: "Mozilla/4.0
(compatible; MSIE 5.5; Windows 98; Win 9x 4.90)"
Proposed Solution:
As a proposed solution, all URLs in Drupal can be made into absolute
path names. This can be done by the following:
The variable $base_url in the conf.php file is broken down into two
components:
$base_host (the 'http://whatever-host.example.com' part WITHOUT the
trailing slash)
$base_path (the '/path-to-drupal' part, WITH the leading slash. If this
is the DocumentRoot, then it is just a '/' character)
$base_url is now $base_host concatenated with $base_path
A simple filter can be written to preceed every href="path" with the
$base_path variable, so it becomes "/path"
This option can be turned on and off for a site. The default is to have
it off so current behavior is maintained.
A similar scheme applies for style sheets as well.
So, did I miss something obvious? Am I seriously off the mark?
Your thoughts!
------------------------------------------------------------------------
November 19, 2004 - 21:10 : chrisada
I am getting similar 404 errors, mainly from rss feed link that looks
like /blog/blog/feed and many manual links that are relative to drupal
root.
It was not a problem before Drupal 4.5, so I think there might not be a
need to change all URIs to absolute. I can't see where the problem is
coming from though.
------------------------------------------------------------------------
November 19, 2004 - 21:35 : kbahey
I am pretty sure that these problems were happening for at least the
past 10 months (ever since I moved to Drupal in January 2004).
The main issue here is that crawlers and other user agents get confused
by the relative path names.
Using absolute paths will definitely solve this. However, is this the
only solution?
I am looking for a discussion of this.
------------------------------------------------------------------------
November 20, 2004 - 02:55 : Goba
No absolute paths please. Having the path start with '/' solves all the
mentioned problems, and is not absolute, it is relative to the domain.
Sadly some crawlers and even the Google Cache does not obey to the base
href. I have reported this cache problem in April to Google, and they
promised they will keep it in mind... Hehe...
What we need is to have the printed relative path values relative to
the domain name, and not relative to the Drupal installation path.
Note that this issue will appear on the drupal devel mailing list if
someone finally provides a patch we can talk about :)
------------------------------------------------------------------------
November 20, 2004 - 03:19 : Dries
Goba is right. We need paths relative to the domain name to fix this
'problem'.
------------------------------------------------------------------------
November 20, 2004 - 13:19 : kbahey
Sorry for not making my self clear.
When I said absolute, I meant that they start with just a /. I did NOT
mean that they start with http://host.example.com. That would be a very
bad idea.
In any case, what do people think about the proposed solution (breaking
down $base_url into two parts?)
Also, does this address the style sheets as well, or more is needed?
------------------------------------------------------------------------
November 21, 2004 - 09:24 : chx
Attachment: http://drupal.org/files/issues/common_inc_patch.txt (471 bytes)
I have implemented what Goba suggested.
------------------------------------------------------------------------
November 21, 2004 - 09:43 : chx
Attachment: http://drupal.org/files/issues/common_inc_patch_0.txt (825 bytes)
Maybe this one is faster?
------------------------------------------------------------------------
November 21, 2004 - 10:34 : kbahey
Man! You are fast!
I tried the second version. It works fine for things that are not
inside the node body, I mean they have a / in front of them, as we
want it to be.
Two comments/issues:
- If there is a URL that is already "/" representing the home page, it
gets set to "//". Perhaps it should check for that case?
- URLs in nodes that do not start with / do not get changed to have a /
prepended to them. Do we need a filter for this?
- Do we need to do something for the style sheets in the page header? I
mean the "misc/drupal.css" and "themes/themename/style.css"?
Thanks
------------------------------------------------------------------------
November 21, 2004 - 17:28 : kbahey
Hi chx
Here is a fix for the case where you have a url that is just "/".
In your patch, instead of:
<?php
$base = $parts['path'] . '/' ;
?>
Replace that by:
<?php
$base = ( $path == '/' ? $base : $parts['path'] . '/' );
?>
------------------------------------------------------------------------
November 27, 2004 - 21:08 : kbahey
Did this patch make it into CVS yet?
If there are any objections to it, can someone please explain what they
are?
Thanks
------------------------------------------------------------------------
November 28, 2004 - 02:33 : Dries
Shouldn't your changes be included in the patch?
Also, it's better to cache $base rather than $parts.
Lastly, it this patch makes it to HEAD, we should probably remove some
'base url' cruft from the themes.
------------------------------------------------------------------------
November 28, 2004 - 11:54 : kbahey
Attachment: http://drupal.org/files/issues/x.diff (1 KB)
Here is the patch including my fix.
I am asking chx to comment on caching $base instead of $parts.
Will this make it faster?
------------------------------------------------------------------------
November 28, 2004 - 12:26 : chx
Hm. $base = ( $path == '/' ? $base : $parts['path'] . '/' ); this
depends on path which is a parameter. Thus I fail to see how could we
cache $base. I'd correct this code however $base = ( $path == '/' ? ''
: $parts['path'] . '/' ); 'cos I think $base is not defined before, but
this is not a problem, PHP will be happy to replace NULL with NULL...
Maybe instead of all parts, only $parts['path'] is enough to be cached,
yes, but the performance and memory usage difference -- I guess -- would
not be noticable...
------------------------------------------------------------------------
November 28, 2004 - 16:06 : kbahey
Attachment: http://drupal.org/files/issues/common-inc-patch.txt (1 KB)
OK.
I put in chx suggested change.
This patch can go in CVS then, to rid us of the problems with paths not
beginning with slash.
This is not an ultimate solution still. We need to address the problem
with .css files. Although the header contains a:
<base href="http://example.com" />
it does not seem that major search engines and archiving sites obey it
anyway.
------------------------------------------------------------------------
December 2, 2004 - 13:31 : Dries
Your coding style needs work. Also, I'm not going to commit this unless
the themes get fixed up: we'd end up with invalid URLs all over the
place. Lastly, I wonder how portable the themes will be when Drupal is
run from within a subdirectory.
------------------------------------------------------------------------
December 2, 2004 - 14:15 : chx
Attachment: http://drupal.org/files/issues/common_inc_patch_1.txt (849 bytes)
Well, my patch worked from a subdirectory very well, as fact, I have not
tested it from the root dir. And I think that it adheres to coding
standards. So I resubmit it with the root path fix. However, my Drupal
work is focused on i18n these days, and I was never into themeing so it
won't be me who fixes those.
------------------------------------------------------------------------
December 2, 2004 - 15:07 : kbahey
I have tested the previous patch (including my fix) with drupal
installed in the DocumentRoot of the server.
So, in effect, it is tested with both Drupal in / and Drupal in a
subdirectory.
This change fixes the problem for the crawlers and other browsers from
getting confused.
While it is true that there is no fix for the .css files in the HTML
head section yet, this fix deals with a major part of the problem, and
rids us of a major pain. Check your web server's logs some time to see
what I mean.
Someone who is familiar with the themes can contribute a patch later.
This patch and the future fix for themes are not mutually exclusive, so
let it go in CVS.
------------------------------------------------------------------------
December 9, 2004 - 08:31 : Goba
Please commit this into Drupal core, this fix is badly needed.
------------------------------------------------------------------------
January 17, 2005 - 06:00 : chx
Attachment: http://drupal.org/files/issues/base_url_kill.patch (4.34 KB)
Well as noone have stepped in to fix this problem, I have tried to fix
the themes also. themes.inc , xtemplate.engine and the bluemarine
template is patched besides common.inc.
Of course, more templates could follow, but first I'd like to see your
opinions.
------------------------------------------------------------------------
January 17, 2005 - 06:09 : Goba
I don't think that removing <base> from the themes is a good idea, using
$parts['path'] should be encouraged though before the files, which would
fix the google cache problem, and would still keep the HTML size low. It
would also help those, who save the file to find the originating site
easier, since clicking on a non-pagelocal link would lead to the online
version.
------------------------------------------------------------------------
January 17, 2005 - 09:03 : Steven
Definitely -1 on removing the <base> tag or using absolute or
root-relative URLs. This tag has been around for ages, and it is the
only way to make clean URLs work without bloating in the code. FYI,
"base" is (first?) mentioned in Berners-Lee's HTML 1.0 draft [2].
That's June 1993.
As the amount of clean URL-using sites grows, the crawlers will have to
be updated. Perhaps we could prevent crawlers from going too insane by
404ing for URLs with more than say 10 components? That would prevent
the really crappy ones from hammering your site.
I'm all for making the <base> tag easier to handle for the user (say,
by including a filter to allow simple anchor links to work as most
people expect them to), but we should keep Drupal-generated URLs clean
and completely relative.
[2] http://www.w3.org/MarkUp/draft-ietf-iiir-html-01.txt
------------------------------------------------------------------------
January 17, 2005 - 09:57 : kbahey
The problem with css is this: The @import argument does not start with a
/.
This is simple to fix.
We keep the "base" as it is today, but add the new variable: $base
before it.
So for a site where Drupal is installed in the DocumentRoot, all that
will change is that /misc/drupal.css and /themes/themename/style.css
will be preceded by a slash. For sites that use another path, that path
will be prepended to the css file name.
How about that?
------------------------------------------------------------------------
January 17, 2005 - 10:38 : Steven
What exactly is the problem with the @import? As far as I know:
- url() in stylesheets is interpreted relative to the base of the
stylesheet, not the source document.
- However, if the styles are inside an HTML document, through a style
tag or style attribute, then the stylesheet's location is the same as
the HTML document.
- Thus, the stylesheet's base is the same as the base of the HTML
document (which can be altered through the <base> tag).
I just don't see why it is necessary. As far as I know, the only
browser that has had problems resolving CSS urls properly was Netscape
4, which does not support @import at all, and which Drupal does not
support either, because of its CSS usage.
------------------------------------------------------------------------
January 17, 2005 - 11:35 : kbahey
The problem for stylesheets is as follows. I think it mainly affect
crawlers and Google's cache.
Say you have an installtion of Drupal in DocumentRoot. You then use url
aliases, and put slashes in them.
For example, you use news/general/2004-12-15.html for a node.
That node still has misc/drupal.css and themes/pushbutton/style.css in
the head section if the document. Crawlers get fooled by that and try
to look for /news/general/misc/drupal.css and
/news/general/themes/pushbutton/style.css, which don't exist.
So, just prepending the new $base variable (in chx's patch) before the
stylesheet @import argument would fix this issue. Assuming you are in
DocumentRoot, then /misc and /themes would be used instead of just misc
and themes.
It would still be compliant with standards, be relative to the web
site, and no ambiguous to anyone, be they crawler or browser.
I hope it is clearer now.
I think chx can change the patch to use the $base instead of $base_url
everywhere, so as to avoid the host/domain name in the urls.
------------------------------------------------------------------------
January 17, 2005 - 14:36 : Steven
But typical crawlers don't even pay attention to stylesheets, hence it
wouldn't have much use for them. I just don't see why we should adjust
to rare cases of buggy software. Reading out a base URL from an HTML
document is dead easy, and on top of that it doesn't add more
complexity as without the base tag, the document's URL is already an
implicit base which has to be parsed anyway.
I did not like it when we altered the <link> tag to accomodate buggy
RSS readers and I certainly don't like it now, as this is even rarer.
In both cases, it is not Drupal which is at fault.
------------------------------------------------------------------------
January 17, 2005 - 14:48 : kbahey
Steven
While I agree with most of what you said, the 404s show up in the logs
enough to be a bother.
Perhaps the original design of Drupal did not forsee that people will
use url aliases to mimic directory/file hierarchies. Whether this was
intended or not, it is the way many use Drupal today.
It does not matter where the bug is (Drupal or the external world), as
long as we can stop it ourselves, by adjusting our end of it.
The fix is simple enough and does not break standards (if implemented
as described with a leading / before the css).
------------------------------------------------------------------------
January 17, 2005 - 14:54 : Steven
It does not break standards, but it does bloat the code in an ugly way.
Why not send an e-mail to the owners of the crawlers and tell them to
implement a standard that is nearly 10 years old [3] (RFC 1808)?
Note that Google Cache now seems to correctly interpret base URLs [4]
and even adds a <base> tag of its own.
By the way, this problem has nothing to do with people using URL
aliases or not, as for a browser the regular nested paths that Drupal
uses (e.g. "node/1" is no different from aliases mimicking files
"foo/bar.html").
[3] http://www.faqs.org/rfcs/rfc1808.html
[4]
http://www.google.be/search?q=cache%3Awww.drupal.org&sourceid=mozilla-searc…
------------------------------------------------------------------------
January 17, 2005 - 15:34 : Goba
Steven, part of the problem is that Google cache does add a base href
even if there is a base href in the document. Eg adds a <BASE
HREF="http://drupal.org/node/13733"> on the plone comparision page
cached. Now that since HTML does not allow more than one base tag [5]
to be present, it is up to the browsers, to use the first or the last
base value, or any of the base values on the page for that matter as
the used base. So even pages displayed from the google cache will be
buggy if a full relative path to the domain root is not specified, due
to this problem.
[5]
http://www.w3.org/TR/1999/REC-html401-19991224/sgml/dtd.html#head.content
------------------------------------------------------------------------
January 18, 2005 - 00:14 : chx
Attachment: http://drupal.org/files/issues/base_url_kill_0.patch (4.75 KB)
This one does not use the whole base_url only the path part of it. HTML
bloat is kept at minimal.
------------------------------------------------------------------------
February 1, 2005 - 16:33 : clairem
Please please can this be done?
It's a good idea in itself, but if using fully-qualified paths means we
can get rid of the BASE HREF, then page anchors will work without having
the overhead of a filter. That's be a huge bonus for those creating
larger nodes, or who just want to be able to put a "skip navigation"
link in their theme without having to abandon Xtemplate or PHPtemplate
------------------------------------------------------------------------
February 1, 2005 - 16:37 : Goba
Well, speaking of skip navigation links, phptemplate and xtemplate
should expose the REQUEST_URI to the templates, so when a link to an
anchor on the same page is needed, the link can be formatted with the
complete request URI in mind.
------------------------------------------------------------------------
February 2, 2005 - 16:40 : clairem
"hptemplate and xtemplate should expose the REQUEST_URI to the templates
"
Should, but don't :(
If BASE HREF isn't removed, surely it wouldn't be a big job to
implement this tweak?
------------------------------------------------------------------------
February 17, 2005 - 07:50 : kbahey
This patch is badly needed. The lack of a leading / in many paths is
causing lots of problems.
------------------------------------------------------------------------
March 12, 2005 - 13:32 : kbahey
Can this patch be applied for 4.6? it is really badly needed.
------------------------------------------------------------------------
March 22, 2005 - 13:50 : Dries
I don't see why this is badly needed. We generate perfectly valid URLs
which are supposed to be short and crispy. This patch has some
advantages though, yet it is unclear which patch to go with.
------------------------------------------------------------------------
March 22, 2005 - 14:00 : chx
The second patch is better.
------------------------------------------------------------------------
March 22, 2005 - 14:46 : grohk
Forgive me for saying so, but since the way Drupal is generating
hyperlinks is completely valid, why are you suggesting Drupal should
move away from an accepted standard when the problem lies with the
search engines?
At the very least, this needs to be optional -- which it appears to be
-- I hate the 404s too, but I hate to hear that a change in Drupal is
needed to fix a Google problem.
------------------------------------------------------------------------
March 22, 2005 - 15:03 : jhriggs
I have to agree with the last comment from grohk.
------------------------------------------------------------------------
March 22, 2005 - 15:39 : mathias
I also agree with the two previous Drupaleers, but I wouldn't mind
enabling a 'quirks mode' via my conf file to stop the flood of 404
messages.
------------------------------------------------------------------------
March 22, 2005 - 17:37 : kbahey
I really can't fathom why some of us cannot deal with with the realities
out there in the world.
These problems are not because Drupal is broken. It is because crawlers
are. We cannot just bury our collective heads in the sand and say that
we are standards compliant and forget about what is out there.
As an analogy, people who design themes or write CSS have to deal with
the ugliness of Microsoft Internet Explorer and its intentional going
against standards. You cannot tell a client or your boss that you are
not modifying a theme that works perfectly on Konqueror and Firefox
because MS IE is broken.
Similarly, we cannot ignore that crawlers from major search engine
companies are broken or confused, and keep recursing through site using
Drupal causing countless errors in the logs. We cannot tell our users to
ask Google and Yahoo et al to fix their software.
Remember that we are not breaking any standards by implementing this
patch. All we are doing is putting the entire path out (from the first
/ down) and thus eliminating ambiguity for everyone.
Sorry if I am a bit blunt in this post, but I am tired of what may be
seen as isolationist thinking.
I do not mind if this is implemented in an advanced mode or via a
settings.php thing. All I care about is getting it fixed somehow.
------------------------------------------------------------------------
April 29, 2005 - 16:29 : kbahey
Circular log errors reported here too http://drupal.org/node/9499
------------------------------------------------------------------------
April 30, 2005 - 12:35 : shane
I agree with KBAHEY. Burrying our head in the sand and saying "it ain't
our problem" is not going to fix the issue. I despise companies that
break standards - and I applaud Drupal for working hard to keep within
those confines. But the reality is money grubbing, lazy ass
programmers exist the world over, and the consequence is things like
MSIE breaking everything wantonly and intentionally, Google, Yahoo, et
al implementing poor bot code, etc...
I believe this desperately needs to get fixed. Ever since I started
hand writing HTML code in 1992 I have always insured that my URL paths
are absolute to the base html document root (eg, preceeded with "/" and
the full path). It avoids confusion, problems, or issues. It seems odd
that the debate over this would rage as it has in this thread.
...and I don't get the "bloat" discussion. How is this bloating
things? Are we talking a few dozen extra characters? I hope I'm
missing something more obvious and insidious than that!?
I've been a loyal Drupaler for ages now, and I love it. But this new
problem is causing me a lot of grief, I see frequent munging of the
URLs, and it worries me; particularly when I see that there are end
users getting 404s. They don't give a rats ba-tu-tey that Drupal is
"standards compliant" ... they just know they got an error when they
supposedely did exactly what they should have, click on a URL. That
reflects poorly on the site owner and ultimately on the software
itself.
Please reconsider this issue, and let a patch go into core to fix it.
It doesn't make sense to let it rage on as an issue that is causing
lots of people obvious grief. I'm betting it's a bigger issue than
most admins think - most don't spend the anal-retentitive time that I
and others do grubbing through our logs, trying to insure a "perfect"
surfing experience for our end-users...
1
0
Hi all. Hopefully I won't be bothering anyone with this post (I figure you
might want this in forum, but I figure the feedback wouldn't be
overwhelming).
I'm almost finished creating an interest match module (current source
available for limited time [1]) and wonder if or how it could be useful, and
hope to be able to contribute it. It currently works something like this:
The administrator create and manage subject fields which the users may rate
their interest in. The users may after they have rated their interest in the
available subjects find users with similar interests. The search is
currently done through a single query looking up all subject fields the
current user has rated his/her interest in, finds all other ratings of those
subjects, groups the difference by SUM(ABS(rating - currentuser_rating)) as
matchvalue by user and orders by matchvalue.
Example______
A: interest in movies (5), books (0), computers (3)
B: interest in movies (4), books (2), computers (3)
C: interest in movies (0), books (5), computers (0)
A search for matches should show B is best match, then C.
B: total(movies (4-5), books (2-0), computers (3-3)) = 3
C: total(movies (0-5), books (5-0), computers (5-3)) = 12
It is thus possible for instance to have a block showing best match of users
online etc.
Could this fit into Drupal somewhere? I figure a good use would be for a
dating site, but I suppose other community sites also could use this
ability. I am aware of the profile module and it's use for instance on
drupal.org <http://drupal.org>, however, this provides a "total" matchvalue
of users in a snap, and because of it's nature should be treated seperately
from the profile module. The administration is much simpler. Furthermore,
the profile module lacks the ability to find matching users in this modules
manner.
First, I would appreciate suggestions on good ways to integrate, second I
would like to be able to contribute the module when ready. As you may have
guessed, I am in the process of setting up a dating service. For this site I
will probably create a relationship module where the admin would be able to
define relationshiptypes users may put other users (with additional
features), as well as use the profile module btw.
Thanks for reading.
Adagio [2]
[1] http://www.mypastebin.com/?code=1880021952
[2] http://drupal.org/user/20636
1
0
01 May '05
Issue status update for http://drupal.org/node/21515
Project: Drupal
Version: 4.6.0
Component: user.module
Category: bug reports
Priority: minor
Assigned to: Anonymous
Reported by: fago
Updated by: fago
-Status: active
+Status: patch
Attachment: http://drupal.org/files/issues/saveform.patch (4.38 KB)
i've written a patch, so that $edit is checked before.
unfortunately i couldn't see a better method than introducing a new
hook type for hook_user 'saveform' for which each module has to return
an array of values, which it wants to be saved. as an affect some
modules have to be changed to work correctly after applying this patch
:(
my patch (for the 4.6 branch) includes the changes for profile.module
and contact.module and for the user.module of course.
what do you think about this?
i think something like this is necessary.
consider a module, which introduces profile fields, which can only be
edited by administrative users.
and yes of course, i don't like it, if users are able to fill up there
user object with additional variables ;)
fago
Previous comments:
------------------------------------------------------------------------
April 28, 2005 - 17:07 : fago
as i already mentioned in this post [1] some time ago,
arbitrary data can be stored in the serialized data field of the user
table.
example:
go to my account, save the page local, edit it to send the "post" data
to the website and edit an form name to edit[somethingnew] und press
save
-> $somethingnew will be saved serialized in the data field
i think the fault is in user_edit (on line 1145) where user_save() is
called without checking $edit completely.
however, i 've felt like that's known and tolerated, so i did, the a
little bit missunderstood, post above first.
but i think, it's not acceptable to save just everything, what is
coming in with the form data... what's if a bad guy saves 1GB of
nonesense in my database? furthermore it will be loaded into the memory
everytime user_save is called - seems to me to be a nice possibilty for
a DOS.
[1] http://drupal.org/node/17744
------------------------------------------------------------------------
April 28, 2005 - 19:13 : moshe weitzman
i think apache/php have limits on how much can be posted. if you are
worried, perhaps use those settings.
------------------------------------------------------------------------
April 28, 2005 - 20:53 : fago
yes, but then it's still possible to inject data in little pieces by
using a new name every time.
further you can't limit the size too much, because file uploading
should also work.
1
0
[drupal-devel] [feature] Improve functionality of block generation for book module
by andremolnar 01 May '05
by andremolnar 01 May '05
01 May '05
Issue status update for http://drupal.org/node/14120
Project: Drupal
Version: cvs
Component: book.module
Category: feature requests
Priority: normal
Assigned to: andremolnar
Reported by: nysus
Updated by: andremolnar
Status: patch
The normal behaviour of these blocks is to show up everywhere by
default. So we are on the right track.
I am going to have to bake a new patch if there are changes to the
block module (particularly new settings). More to come.
andre
andremolnar
Previous comments:
------------------------------------------------------------------------
December 9, 2004 - 06:12 : nysus
Attachment: http://drupal.org/files/issues/book_19.patch (4.85 KB)
Attached is a patch for the book module that does the following:
1) Allows book blocks to appear on any page at any time, not just when
a node from the book is being viewed.
2) Allows multiple book blocks to appear on the same page.
This functionality is achieved by the automatic creation of individual
blocks for each book when the book is created. Simply enable the
book's block to enjoy the benefits of 1 & 2 above. If the blocks are
not enabled, the blocks will appear only when a node from that block is
being viewed (the same way it works now).
------------------------------------------------------------------------
December 9, 2004 - 12:35 : andremolnar
Attachment: http://drupal.org/files/issues/book_19_1.patch (4.84 KB)
+1 This is great. A good many people have asked for something like
this, and I think its a nice solution. But in the end this isn't up to
me.
One minor error in the patch...
+ $result = db_query('SELECT n.title, b.block, b.nid FROM {book} b
INNER JOIN {node} n on n.nid = b.nid WHERE b.parent = 0');
b.block is not a valid field in this query. this updated patch removes
reference to it.
andre
------------------------------------------------------------------------
December 9, 2004 - 13:54 : nysus
Glad you like it and thanks for fixing that up. It was left over from
an older version of my patch.
------------------------------------------------------------------------
December 9, 2004 - 13:54 : andremolnar
Steve,
If I apply this patch, and I attempt to configure one of the newly
created blocks. I noticed that for some reason the block.module is
returning "true" at line 249 (of the block module)- and is creating a
form for module-specific configuration. But all that shows up on the
screen is the word "array".
Can you trace this back to see why - and maybe update the patch.
I can continue to test your changes - anything to help this patch make
it into core.
andre
------------------------------------------------------------------------
December 9, 2004 - 15:29 : nysus
Hmmm...probably because I tested my patch on my version of Drupal,
version 4.5.1. I'm having no problems. Are you using a cvs version of
Drupal to test. If so, I'll set up cvs on my site and track this down.
------------------------------------------------------------------------
December 9, 2004 - 18:53 : drumm
See http://drupal.org/node/12347 for information on how the block system
has been updated. When I saw that the elseif ($op == 'view') was taken
out I knew immedaitely that there was something weird about this patch.
------------------------------------------------------------------------
December 9, 2004 - 18:58 : nysus
OK, thanks for the tip. Sorry for the confusion. Still kind of new to
making open source contributions and it's easy for me to overlook some
obvious stuff like this. I'll fix this up when I get a chance.
------------------------------------------------------------------------
December 9, 2004 - 19:06 : Dries
Please do, because this being a new feature, it has no chance getting
committed to the DRUPAL-4-5 branch. The DRUPAL-4-5 branch is in
bugfix-mode. New features go into CVS HEAD.
------------------------------------------------------------------------
December 9, 2004 - 22:32 : Dries
There is quite a bit of duplicated code in the patch. Maybe it can be
simplified (using a function)? Either way, it is a little weird. I
haven't tried the path, and I'm not sure I understood the description.
It's somewhat vague. Is the book module exporting multiple blocks that
are nearly identical, yet have different display behavior? If so, why
not add a simple block configuration setting to the original book
block?
------------------------------------------------------------------------
December 10, 2004 - 02:51 : nysus
Dries,
Yes, there is some code that can be factored out and some general
cleaning up that can be done. It was a little tricky to write so I
left it kind of ragged around the edges until I'm sure there are no
bugs. It has worked very well on 4.5.1 but I obviously need to update
it to work with cvs. I'll be on that soon.
As far as what it does:
1) For every new book that a user creates, a new block is associated
with it. So if you create "Book A", "Book B", "Book C", you will get
three new blocks visible on the block administration page called "Book
A", "Book B", "Book C". The original "Book Navigation" block is still
there, too. The functionality of the "Book Navigation" block is not
affected at all by this patch.
2) If Book A's block is enabled, the block containing its menu will
appear not only when a node within Book A is viewed, but at all times
(unless the user suppresses it on certain pages with the "path"
feature). When any node is that is NOT in Book A is viewed, Book A's
menu still appears but it is fully collapsed. When a node that DOES
belong to Book A is viewed, Book A's Book menu expands accordingly.
3) The user can also enable Book B & C's block, and have their menus
appear in a block at all times as well.
4) If none of the book's blocks are enabled by the user, the module
will behave just as it did without the patch. That is, when the "Book
Navigation" block is enabled, the only time any book menu will appear
is when a book node is being viewed.
5) It's important to note (and this was the tricky part to write) that
if both the "Book Navigation" block is enabled and "Book A" is enabled,
they will play nice with each other and not do nasty things like create
the same book menu twice.
------------------------------------------------------------------------
December 10, 2004 - 05:51 : nysus
Attachment: http://drupal.org/files/issues/book_20.patch (5.37 KB)
Andre,
Attached is a new patch that will resolve the problem of the
block-specific stuff showing on the block configuration form.
Let me know if you spot any other bugs. If it looks good to you, I'll
go to work making the code look leaner and prettier.
------------------------------------------------------------------------
December 10, 2004 - 07:26 : nysus
Attachment: http://drupal.org/files/issues/book_21.patch (4 KB)
Dries, Andre:
Here is a new and improved streamlined version of the patch. Have a
look if you get a chance.
------------------------------------------------------------------------
December 10, 2004 - 08:11 : nysus
Attachment: http://drupal.org/files/issues/book_22.patch (4.15 KB)
Found a bug in the last version that would cause the block to jump to a
different location. I think this should do it. Everything appear to
work well (famous last words).
------------------------------------------------------------------------
December 10, 2004 - 14:49 : andremolnar
Steve: bugs appear to be gone, and I didn't run across any other
errors. This is functioning exactly as described.
everyone: I personally would encourage support for this functionality.
Book is a powerful navigation building tool in a site, not only with
its ability to move next and back through a hierarchy of pages - but
also its ability to build the appropriate navigation block without
further user intervention (unlike the admin features in the menu module
or taxonomy).
The most frequently cited complaint about the book module is its
inflexibility when it comes to when and where the block shows up. I
also frequently hear requests for the ability to show multiple book
blocks at the same time. Up until now the best alternatives suggested
required users to do a hack (e.g. build a custom block that calls such
and such a hook). Most abandon their request at that point because its
over their heads.
With this patch all those requests are covered and more. Now all books
can automatically have their own block and admins can easily decide when
and where each of those individual blocks show up (left right, up down)
and coupled with the new configuration features of the block module -
its very easy for admins to decide on which individual pages a block
will show up.
I would be interested what other have to say about this feature.
My only reservation (which is minor compared to the benefits of the
functionality offered) is that there is no way to turn this
functionality off. i.e. The default behaviour is to build individual
blocks for each book. If there could be a way to toggle this feature
on and off somewhere - it would be perfect. Still, AS IS - this is a
major improvement and offers great flexibility to admins and site
creators.
andre
------------------------------------------------------------------------
December 10, 2004 - 15:04 : Anonymous
Andre,
Thanks for the feedback on the usefulness of this module. Glad I could
pitch in and help.
I agree about the inability to turn the feature on/off and I was
thinking about that myself. I think it could easily be accomplished
by creating a checkbox in the "book navigation" block individual
configuration's settings. Call it "Enable individual book blocks."
When enabled, the individual book blocks will appear on the block
administration menu.
One question: Where would the state of this checkbox get saved? Has
Drupal moved away from serializing data in the data base?
------------------------------------------------------------------------
December 10, 2004 - 18:36 : Dries
I'm afraid that 'Enable individual book blocks.' is not
descriptive/clear at all. Are you suggesting a setting to toggle
between 'show block on all pages' and 'show block on book pages only'?
------------------------------------------------------------------------
December 10, 2004 - 19:45 : nysus
Dries,
No. Andre and I suggest a setting within the "Book Navigation"
cofiguration page, that would toggle whether or not individual books
appear on the list of all blocks on the block administration page.
Hence the name 'Enable individual book navigation blocks.' The help
text for this checkbox might read something like: "By default, a book's
navigation block is visible only when a page from that book is being
viewed. Check this box if you want more control over where and when an
book's navigation block is visible. You will then be able to control
the book's navigation block location and visibility settings on the
"admin/block" page."
Hope this makes it pretty clear.
------------------------------------------------------------------------
December 10, 2004 - 20:08 : Dries
I understand what you are trying to do, but not how you are trying to do
it, or how the setting is supposed to work. I guess I'll have to try it
when a new patch lands.
------------------------------------------------------------------------
December 11, 2004 - 09:52 : nysus
Attachment: http://drupal.org/files/issues/book_23.patch (5.02 KB)
Alright, fellas, I'm proud to unveil my crowning achievement in the open
source development world (no big deal to most of you guys but pretty
good for a hack like me).
Thanks for all the input so far. It's been helpful. I've streamlined
the heck out of it per Dries suggestion and I've created an option to
turn this functionality on an off per Andre's suggestion. Does this
look good to you guys? Anything else I have to fix or improve?
Thanks.
------------------------------------------------------------------------
December 11, 2004 - 13:02 : Dries
I tried the patch.
It works great but to me, the 'Give books their own block' settings
seems to be redundant. Why not export the current book navigation
block, along with an additional block for each book? Looks a lot
simpler to me.
I think I spotted a bug: orphaned book pages (or possibly book pages
that are unpublished) appear to be getting book navigation blocks.
------------------------------------------------------------------------
December 11, 2004 - 17:34 : nysus
I'll see if I can fix the bug. Might be tricky.
But I don't understand your recommendation to "export the current book
navigation block, along with an additional block for each book". Can
you expand on this thought?
------------------------------------------------------------------------
December 11, 2004 - 18:06 : nysus
Dries,
I am unable to duplicate the bug. I have three orphaned pages. I also
tried unpublishing some pages. But as far as I can tell, the patch
works as expected.
------------------------------------------------------------------------
December 11, 2004 - 19:00 : Dries
If you can't reproduce the problem, chances are my node/book table is
somewhat fubar.
As for the configuration option. I suggest removing it and to always
make these new blocks available on the /admin/block configuration
screen.
------------------------------------------------------------------------
December 11, 2004 - 19:52 : moshe weitzman
I am hoping that we maintain the option to keep the behavior where the
appropriate book block only shows up when its book page viewed. this is
a nice, tidy arrangement.
------------------------------------------------------------------------
December 11, 2004 - 20:03 : nysus
Yes, if you don't enable any of the individual book blocks, the a book's
block (i.e. navigation menu) will only appear when a page in a book is
viewed. In other words, you have the option to have the book block act
like this patch isn't even installed.
------------------------------------------------------------------------
December 11, 2004 - 20:54 : Dries
I guess I'll have to try the patch again, because I don't understand why
it works like this -- or at least, why it can't be made simpler.
------------------------------------------------------------------------
December 11, 2004 - 21:13 : nysus
Can you be more specific? Why it works like what?
------------------------------------------------------------------------
December 11, 2004 - 21:24 : nysus
Attachment: http://drupal.org/files/issues/book_24.patch (4.1 KB)
This patch reverses a change made in the last patch which required a
user to enable individual book block before they could enable any
individual book blocks.
------------------------------------------------------------------------
December 11, 2004 - 21:41 : andremolnar
As I mentioned earlier I am *fine* with either version of this patch as
long as it makes it to core.
But, as I said earlier, I clearly think the preference for admins would
be to have the option to enable or disable this functionality.
BTW - if this does make it in, I would be happy to create a Handbook
page that describes the new features - something like "how to build
robust site navigation using the book module". (on or after December
17th).
andre
------------------------------------------------------------------------
December 12, 2004 - 12:11 : Anonymous
I am not at all happy with these features. They are incosistant,
confusing and should use exising methods and UIs.
May main concern is the incosistancy: it is confusing, will require
extra attention with each core code change, adds extra logic to the
core, and is not re-useable.
so here are my questions:
1) why do we not use the menu system and apis to build and adminster
the trees? Saves code, does not add extra UIs, and gives users more
power.
2) why do we need that extra showing logic? a book block should not get
exeptional if clauses, it should use the existing path setting methods
on block admin. extra logic is confusing for administrators (hey, i set
the path so that the book-block should show up here, but it does not,
why?) we should really not provide extra logic in the block hook, but
should rather use default settings in block admin (the book could fill
the bookblock sql cells with custom paths, for example)
3) we should avoid extra UIs. We already have far too much, and far too
much different ones. Please rather improve the block admin, than add new
separate interfaces.
4) why do book blocks need al these expeptions in the first place? If
they are so exeptional, we could consider not using blocks, but
something else, like in-line book layout (pages with the index etc)
5) why did you not choose for a general, standard, block gerneation
API? that way modules, such as taxonomy, image gallery, weblinks,
article, etc can reuse it and introduce block gernation.
All that sayd, i like the idea of this functionality, but i fear for a
great useability downfall if we start introducing all sorts of
exeptions for all sorts of modules. Because now chances are very big
that taxonomy, image gallery etc will need to introduce other UIs,
other code, new methods and new documentation, if they too want some
sort of better block handling.
so a -1 from me.
------------------------------------------------------------------------
December 12, 2004 - 12:12 : Bèr Kessels
^^--- That was me (bèr kessels) forgot to log in.....
------------------------------------------------------------------------
December 12, 2004 - 12:34 : nysus
I'm not going to pretend I can argue if my patch does or does not fit
well into Drupal's larger architecture. My motivation for writing it
is that I had an immediate need to create an easy way to make it easy
for users to create menus.
I'll let others decide whether or not the patch has merit from the big
picture perspective. But if it doesn't, why not just use it for its
immediate benefits and then throw it away when something better comes
along?
------------------------------------------------------------------------
December 12, 2004 - 13:01 : Dries
My summary is this: +1 on the functionality, -1 on the implementation.
The code itself is good, but the usability/integration is not.
------------------------------------------------------------------------
December 12, 2004 - 13:13 : nysus
When you say "usability" is that from the user's perspective or the code
maintainers? I'm guessing it's the latter but I'm unsure.
What about the idea of using the patch until a more permanent solution
comes along? Yes, it's much better to live in a home with indoor
plumbing but why not use the outhouse while you wait for a toilet to
get installed? Or are there other considerations I'm overlooking that
would make this a bad idea?
------------------------------------------------------------------------
December 12, 2004 - 13:20 : Goba
nysus it is just generally against the Drupal philosophy to add
improperly implemented functionality until something better comes
along. There even used to be ocassions in Drupal releases, when some
functionality was removed (not fixed) for a release, because its
implementation was not adequate.
------------------------------------------------------------------------
December 12, 2004 - 13:55 : Dries
Usability for the user. The extra setting on the block configuration
page is both confusing and awkward. I don't understand why things must
be configured/enabled that way (see my and Ber's previous comments on
this issue).
------------------------------------------------------------------------
December 12, 2004 - 14:03 : nysus
Well, just for the record, I reversed that functionality per your
suggestion and uploaded the patch. The indiviudal book blocks now
appear automatically.
------------------------------------------------------------------------
December 13, 2004 - 13:56 : Bèr Kessels
Hi,
I am sure you can make not only simpler, but better usefull for admins
and users.
All you need to do is use the menus for this. i.e. make a menu entry
for each book page.
For each book make a menu on level 0, without a parent. that way they
become a seoprate menu, each with an own block.
it saves code, makes things more consistent, and most important, uses
drupal functionality where it should.
------------------------------------------------------------------------
December 13, 2004 - 14:50 : nysus
Ber,
Are you suggesting that for every single book page that a menu item be
created? That's really not practical. That was my main motivation
for writing this patch: to make it easy to put links, not necessarily
related to one another, into a block. Any more pages than 10 and the
sheer tedium of the job would prevent anyone from ever doing that. The
menu.module is great, but adding new menu items is far from quick and
painless. I just added about 10 to my menu for different taxonomies on
my site and it wasn't fun.
Plus, if you do as you suggest, there is also the problem of the book
showing up twice. It will be generated by the menu and then it will be
generated again by the book module which is programmed to design a
block. You'd have to put some logic in the book.module _block hook to
try to anticipate if a user has enabled a book in the menu. That
wouldn't be pretty code.
I'm all for putting automatic generation of book navigation blocks as
part of the menu module. It does make more sense to have it there.
But it forces me to ask the question: "Then why do we currently have
code in the book module that generates a menu? Shouldn't that belong
in the menu.module, too?"
------------------------------------------------------------------------
December 13, 2004 - 22:31 : killes(a)www.drop.org
I think what Ber is trying to say is that you can write a contrib module
that monitors the changes to the book table and creates menu items
automatically. nodeapi is your friend. I would also prefer this
solution.
------------------------------------------------------------------------
December 15, 2004 - 16:39 : Anonymous
""Then why do we currently have code in the book module that generates a
menu? Shouldn't that belong in the menu.module, too?"
"
Because it's old code. It would be nice if book.module generated this
block using its _menu hook, so that the admin would have a few options
in terms of configuration.
"Plus, if you do as you suggest, there is also the problem of the book
showing up twice. It will be generated by the menu and then it will be
generated again by the book module which is programmed to design a
block.
"
No. The old code which manually builds a block in book.module would be
removed. Book blocks would only be generated by the menu.
This would also have the added benefit of allow the administrator to
easily place a book in an appropriate spot in the menu tree, while
still allowing the possibility of displaying it in a separate block.
Because of menu caching, I don't expect a large performance hit for
creating the menu items.
"That was my main motivation for writing this patch: to make it easy to
put links, not necessarily related to one another, into a block.
"
It sounds like instead of (mis)using book.module, your time would be
better spent in a usability improvement to menu.module so it's easier
to do this.
------------------------------------------------------------------------
December 16, 2004 - 06:55 : nysus
Thanks for the feedback and input. I appreciate it. However, I would
also appreciate if you took more care to avoid the condescending tone
in your post:
"It sounds like instead of (mis)using book.module, your time would be
better spent in a usability improvement to menu.module so it's easier
to do this.
"
It's really quite unnecessary and off-putting. Though it won't stop me
from contributing to Drupal in the future, I'm sure others would be
really turned off by such a patronizing comment and it could dissuade
them. I'd like the Drupal community to be a welcoming and friendly
place that will inspire people to contribute, not discourage them.
Thanks.
------------------------------------------------------------------------
December 16, 2004 - 10:07 : Bèr Kessels
nyesus,
Please do not start /that/ discussion here. :) Drupal community is
known fo being direct, maybe because of the big number of
western-Europeans attending, maybe because of other reasons.
No-one commented that you are wasting your time. But the commentor was
telling you somthing likethis:
"If you would follow the previous sugeestions, your added feature would
be much better appreciated, and will probably work much better for you
too".
He/she was by no means telling you to stop your silly coding, or
anything in that line. He/She only wanted to show you the obvious and
better direction.
We often deal with issues that add some feature, and a complete new UI,
because the author does not like, or cannot use the existing UIs and
features. This is nogt good, because if that same author would have
spend his/her time on improving that existing functionality or UI
(improving is not neccesarily the same as extending!!) that code and
time would benefit all much better.
Thats what the commentor tried to say. And so is it here: If you
dislike the way the books handle the blocks, and if you do not want to
use the menu, because you do not like its UI, then do not add another
UI and more features, but rather merge these, and improve the parts (in
the menu) you dislike.
------------------------------------------------------------------------
December 16, 2004 - 10:35 : Dries
We can worry about the menu integration later. Let's focus on the new
option's usability/interaction design first.
------------------------------------------------------------------------
December 16, 2004 - 14:51 : nysus
I understand what the commenter was saying and like I said, I appreciate
and understand it. I'm not upset and I'm not looking for an argument, I
was just being direct as well. :) As part of the Drupal community
(albeit a minor player), all I'm saying is that I would like to see
folks not have a tin ear to the humanity in all of us. It will help
make Drupal an even stronger community and attract even more talented
developers.
Human interaction is part of the development process. Whether we like
it or not, we must cope and deal with it.
------------------------------------------------------------------------
December 18, 2004 - 09:54 : Dries
I thought some more about this and am starting to believe that
integration with the menu system should take priority. Here are common
cool scenario's:
I want to create a separate navigation block for the 'Drupal handbook'.
I want to add a menu item called 'handbook' to the user block. That
is, I want the book navigation to be part of the existing user
navigation block.
I want to add a menu item called 'handbook' to the top-level navigation
menu.
How would that work from a user's point of view? What do I have to
click and where to configure this?
------------------------------------------------------------------------
December 21, 2004 - 18:47 : andremolnar
I was actually thinking about the same thing last night (must have been
something in the arctic air).
/
2. I want to add a menu item called 'handbook' to the user block. That
is, I want the book navigation to be part of the existing user
navigation block.
3. I want to add a menu item called 'handbook' to the top-level
navigation menu.
/
This is already possible (to a certain extent) with the current
Book.module and Menu.module - A 'Books' menu item is created in the
user navigation by having the Book.module enabled. Menu.module allows
you to enable/disable this menu item. Menu.module also allows you to
re-name this menu item. But, this only helps if you only intend to
have 1 book (or else the name 'Handbook' is misleading if the user
finds more than 1 book listed). - so this isn't good enough (or is it).
/1. I want to create a separate navigation block for the 'Drupal
handbook'./
This is what I was trying to figure out. Not just this, but a
different way to do what Nysus was attempting. i.e. create a menu
block for each and every book that is created. There is a way to write
code that would (re)build a 'custom menu' on every add/edit/update to a
book page - or book outline update. Custom menu's automatically have a
block created for them. But, this I think would be a misleading use of
the 'custom' menu - as the menu would not be custom if they are a part
of core.
So, I would think that two new constants could be added to the menu.inc
file - MENU_BOOK_MENU - and MENU_BOOK_ITEM - each would behave as custom
menu's, but would be reserved for books. These menu types should NOT
show up in the Menu.module administration - because the administration
of the book_menu items would be done by administering the book itself
(adding an item, removing an item, moving an item up/down in the
hierarchy, assigning parents etc.).
However, the blocks that the book_menus would create would show up in
the block administration (so users could enable/disable each block -
and decide where on the site they show up). The existing book block
logic would be removed.
So the logic would be:
If a creates a book in the book administration page - the Book.module
automatically creates a new MENU_BOOK_MENU
Any time a user adds to or updates or delets a book page - the entire
book menu is deleted and recreated based on the hierarchy defined by
the book itself.
The blocks for each book would show up in block administration.
Any thoughts - I know that this doesn't exactly address points 1 and 2
- but it could act as a first step.
Is this approach a bad idea? It would add special conditions for the
proposed book menu's - but books would be a special case.
Even if people don't like this solution, maybe it will give someone a
better idea. I'd love to hear them.
andre
------------------------------------------------------------------------
December 21, 2004 - 22:55 : Boris Mann
+1 to this andre
I had promised to put down my thoughts on this matter, as it relates to
1) primary and secondary links and how they are managed and 2)
auto-generation of primary/secondary navigation based on book outlines
So, for 1), we currently have functional-but-not-very-usable plain
textfields to manage primary and secondary links. I would like to see
menu.module used to control all navigation links, whether it is the
navigation block or primary/secondary links. What is needed:
a) default system menus labelled "primary" and "secondary" which would
store; this is where modules could insert navigation
b) support for full URLs (e.g. http://myexternaldomain.com) instead of
just path
2) if we got 1 working, and andre does his book menus, this could get
taken care of automatically. Basically, for brochureware/business/etc.
sites that have static content, you could have a root book as one of
the primary navigation links, and then the secondary links are
generated automatically.
------------------------------------------------------------------------
December 22, 2004 - 21:02 : Dries
I agree with Andre that the book module's integration with the menu
system needs to be worked on. I support any effort that makes it
easier to structure pages and that makes it easy to link pages/nodes
from within a menu.
However, I'm opposed to putting book module specific names in the menu
system (eg. MENU_BOOK_MENU and MENU_BOOK_ITEM). I can imagine a
handful of modules that want to maintain a menu tree (or part thereof)
so I'm in favor of generic names.
I'd have to read up on the menu system code, but last time I checked
there was a MENU_MODIFIABLE_BY_ADMIN flag. You could choose not to set
this flag for the menu items generated by the book module. Maybe it's
already possible to implement to implement Andre's suggestion without
having to modify/extend the menu system.
Are you exploring this path?
------------------------------------------------------------------------
December 24, 2004 - 10:01 : andremolnar
I've had a few spare hours to work on this.
I've started to cobble together a solution - but in doing so I
discovered a bug in the menu module (for which I will submit a seperate
issue - if one doesn't already exist).
The principal I suggested works. I added some code to the book module
that creates custom menu's (MENU_CUSTOM_MENU with MENU_CUSTOM_ITEMs)
for each Book that exists in a site. This is just as a proof of
concept - I chose this menu type to start with because they
automatically have a block created for them in admin/block (which is
ultimatly the functionality we want).
I ran a test and the menus are created as expected - the blocks are
also created. But when I tested the menu blocks by enabling them I ran
into a problem.
It seems as though the menu system does not expand/explode sub menu
items if the node type is book. I'm not sure why this is, and I
haven't traced the source code yet - I thought I would ask if anyone
has intimate knowledge of the menu system if they can point me in the
right direction.
No patch attached because until that problem is fixed this proposed
change won't fly :(
andre
------------------------------------------------------------------------
December 24, 2004 - 11:11 : Dries
Just a wild guess: maybe it doesn't work because the book module's URI
scheme is not hierarchical?
------------------------------------------------------------------------
January 7, 2005 - 10:45 : andremolnar
Attachment: http://drupal.org/files/issues/book_module_1.patch (5.1 KB)
I finally took some time out to do some work on this. As mentioned in a
post to dev the problem I was having with menus not
expanding/contracting properly was some crud in my database. Once that
was cleared up my changes worked fine.
I am attaching a patch for comments and for the brave willing to give
it a test drive.
History: This thread and http://drupal.org/node/15198 and
http://drupal.org/node/15153
node/15198 has a patch that is required for this patch to work.
What this does:
Pretty straight forward.
Any time a user adds/updates/deletes book or book pages (including via
outline) - a function is called to create a new menu for each book.
Any existing book menus are wiped out and then the new book menus are
inserted - and the system menu is rebuilt to reflect the changes.
The menus created consist of type MENU_MODULE_MENU and
MENU_MODULE_ITEM. These menus show up in the menu/admin page so that
admins can be aware of them, but the menu types are not editable via
menu/admin (all changes are handled by the book module).
Since the menus are in the menu table, the menu_block() hook handles
the creation of the blocks for each of them. The blocks can then be
administered in the usual ways via the block/admin interface.
KNOWN ISSUE: (suggested solutions welcome)
Since the menu table is updated on every change to books - the blocks
associated with the menus are also recreated with default settings
(i.e. disabled, and with no path or throttle settings) requiring a user
to take an extra step and re-configure the book blocks for viewing on
their site. I think this is unacceptable. For the casual user of the
book module that only has a single book that doesn't change often, this
would not be a big deal. But, if anyone wanted to make use of book
module to handle dynamic site navigation this would create more work
than it saves.
Looking forward:
If the block generation issue can be solved in a tidy way, this patch
could allow users to use book to handle all their site navigation
generation needs.
Also, this patch could allow for the removal of a large chunk of code
in the book module dedicated to building its own block via the block
system. I left it there for now because I suppose there may be those
out there that want to have book.module work the way book.module always
worked (only show the book block when viewing a book page). Even so,
since each book would have its own block, an admin could specify when
and where the block shows via admin/block.
I would appreciate feedback. If nothing else I hope this gives someone
some new ideas.
I'll continue to work on the block regeneration issue.
andre
------------------------------------------------------------------------
January 7, 2005 - 10:47 : andremolnar
Attachment: http://drupal.org/files/issues/book_module_2.patch (4.98 KB)
sorry - here's a patch with prefered line breaks.
andre
------------------------------------------------------------------------
January 7, 2005 - 16:47 : nysus
Andre,
I've been meaning to give this a throrough look when I get a chance.
Hopefully this weekend.
---Steve
------------------------------------------------------------------------
January 20, 2005 - 02:02 : andremolnar
Attachment: http://drupal.org/files/issues/book_module_3.patch (6.66 KB)
Here is an updated patch.
Same comments as followup 51 - except that the known issue has been
resolved.
This changes book module so that any action taken on a book, including
adding new books or book pages will create a menu in the menu system
for that book - and thus create blocks for those menus that can be
administered in the usual ways.
This is my first attempt at a major patch to core - comments are
welcome
I will be happy to update documentation once revisions to the code have
been taken care of.
andre
------------------------------------------------------------------------
January 20, 2005 - 02:53 : andremolnar
Attachment: http://drupal.org/files/issues/book_module_4.patch (6.17 KB)
Sorry - would be nice if I removed some debugging code - ;-)
also previous patch also would have introduced a need for a change to
the menu table that isn't required yet.
This patch is a correct version
andre
------------------------------------------------------------------------
January 20, 2005 - 19:16 : Dries
The menu is recreated every time a book page is updated. I believe this
is unwanted behavior because it requires the book block to be
reconfigured upon every update.
------------------------------------------------------------------------
January 20, 2005 - 19:32 : andremolnar
The menu is indeed re-created with every book page edit - because if the
book page title changes the menu needs to reflect this change.
The book block re-configuration is not required by the user. The code
stores this information and re-sets the block settings.
I will see if I can test for 'title change' before forcing the re-build
of the menu - It would save a bit of processing power.
andre
------------------------------------------------------------------------
January 20, 2005 - 20:21 : andremolnar
Took another at this - and it turns out that I already have a check to
see if title or weight change (rather - they already existed in the
module and I used them).
andre
------------------------------------------------------------------------
January 24, 2005 - 02:22 : andremolnar
Attachment: http://drupal.org/files/issues/book_module_6.patch (7.42 KB)
Feedback received indicated that the original block generation code in
this module should be removed - since this patch hands block generation
off to the menu and block system.
This patch removes that code. BUT - It should be noted, that in order
to have book blocks only show up on pages of node type book - an
additional patch found at http://drupal.org/node/16074 is required.
andre
------------------------------------------------------------------------
January 24, 2005 - 18:36 : andremolnar
Attachment: http://drupal.org/files/issues/book_module_7.patch (7.43 KB)
Yet another patch:
Brings this patch in line with - http://drupal.org/node/16074 (i.e. the
column name change in {block} table)
andre
------------------------------------------------------------------------
January 25, 2005 - 18:56 : andremolnar
Attachment: http://drupal.org/files/issues/book_module_8.patch (7.53 KB)
And another minor patch to cover coding style and variable names.
Just to bring everyone up to speed:
1) Each book and its children pages have a menu created for them (on
any add, edit, delete to a book or book page).
2) Those book menus automatically have a block created for them via
menu_block hook.
3) Block settings are preserved any time there is a change to a book
that requires the menu and its associated block to be re-built.
4) book_block hook is removed because it is redundant.
5) With the addition of the configuration option in blocks to only show
blocks on certain node types (see: http://drupal.org/node/16074)
administrators have full control over when and where a book block shows
up (including maintaining the status quo of only having book blocks show
up on book pages).
6) This patch does require http://drupal.org/node/15198
andre
------------------------------------------------------------------------
January 25, 2005 - 21:49 : Boris Mann
+1!!
Book-based automatic navigation for corporate/brochureware sites!
Fantastic! Chris Messina, get in here and add a +1 to this.
------------------------------------------------------------------------
January 25, 2005 - 22:26 : Dries
Boris, have you tested this patch extensively, or are you just giving
this +1 based on what you've read? If you want to see this committed,
take the time to review/test it.
------------------------------------------------------------------------
January 26, 2005 - 05:45 : andremolnar
Yes - please - testers are welcome.
Nysus (aka Steve) has already agreed to give this a test drive when he
had a spare moment. I hope for some feedback in the near future. But
if you have a moment Borris I would appreciate it.
OR - If someone would just like to demo this:
http://s92258948.onlinehome.us/greenbeach/
UN: drupaldev PW: drupaldev
Don't mind the mess - it is my test environment and there is a lot of
junk data and settings floating around - but this patch is running
there along with the additional block configuration settings.
andre
------------------------------------------------------------------------
January 31, 2005 - 19:12 : Boris Mann
You got me, Dries...
I have since spent time testing the module on andre's site, and it does
everything I can foresee needing at this point, with no functional
errors.
This functionality alone will make creating standalone sites that are
composed of mainly static pages very, very simple.
------------------------------------------------------------------------
February 3, 2005 - 08:58 : Dries
The demo-site appears to be down. Bummer.
------------------------------------------------------------------------
February 3, 2005 - 22:59 : andremolnar
Attachment: http://drupal.org/files/issues/book_module_9.patch (7.58 KB)
Sorry - demo site was down while database was being cleaned and latest
CVS modules were being installed (general site maintenence for
testing).
The site is up now with a fresh database and install of Drupal CVS.
UN drupaldev PW drupaldev (has access to manage blocks, books, and
menus to see this patch in action)
Attached is an updated patch to make use of db_rewrite_sql (instead of
node_rewrite_sql). Also fixes a minor bug that appeared if you were
creating the first book in the site.
andre
------------------------------------------------------------------------
February 3, 2005 - 23:13 : Anonymous
This patch seems to modify the blocks and menu tables directly from
within book.module. (in the new book_build_menu_module_menu function)
This sets a terrible precedent for other modules. IMO, these tables
should be modified by functions defined within the block module or menu
module (or menu.inc). Modifying the tables randomly throughout many
other points in the code makes changes to these tables difficult and
makes troubleshooting problems much trickier. Let's not sacrafice code
organization for the sake of making the patch as small as possible.
------------------------------------------------------------------------
February 4, 2005 - 06:25 : andremolnar
With regard to the blocks - the block module is doing all the block
building (with the call to _block_rehash) - the only thing that this
code does is preserve block settings (the select sql) before the menu
is updated - and then restores those block settings (with the update
sql).
"Modifying the tables randomly throughout...
"
While I appreciate the point you are making, I would hesitate to call
this random. After all the menu items being added to the menu table
'belong' to book module.
There is a little bit of a discussion about the best way to go about
doing this sort of thing (i.e. allowing modules to build their own
menus) over at http://drupal.org/node/15198 - which includes the patch
required to make this patch work. Right now there is no existing hook
or method of doing this. hook_menu does not have anything in place as
of yet.
I have suggested the introduction of a new hook (name yet to be
determined) that would be in charge of these menu_module_menu type
inserts. But, I haven't heard any feedback on whether that is a good
idea - or if anyone has a better idea.
Anyone else have thoughts on the approach taken here or suggestions for
a different/better approach? I'm willing to write the code, I would
just like a little bit of input.
andre
------------------------------------------------------------------------
February 4, 2005 - 06:46 : Dries
I added a test page and all book blocks dissapeared? I also got an SQL
error (check your watchdog messages) upon pressing the 'Submit' button.
------------------------------------------------------------------------
February 5, 2005 - 07:31 : Steven
Doesn't this patch still suffer from the inability to show the menu
block only on the relevant book(s)? I know we can now restrict blocks
per node type, but that still doesn't help with multiple books.
By the way, big code style violation:
$menu_block_settings[$menu_block->path][weight]
"weight" should be surrounded by quotes. Right now it is being treated
as an undefined constant. This is not a good idea. Other than that
there's a bunch of missing spaces... if the code looks cramped, it
usually violates the coding style.
+ $menu_block_settings[$result->path] =
array('status'=>$result->status, 'weight'=>$result->weight,
'region'=>$result->region, 'throttle'=>$result->throttle,
'visibility'=>$result->visibility, 'pages'=>$result->pages,
'types'=>$result->types);
+ db_query('DELETE FROM {menu} WHERE type=%d OR type=%d',
MENU_MODULE_MENU | MENU_MODIFIED_BY_MODULE, MENU_MODULE_ITEM |
MENU_MODIFIED_BY_MODULE);
//restore menu_module_menu block settings
There are several more examples. Please pay attention to this. It's
incredibly annoying to have to fix it later, and it makes the code
harder to read as you cannot make certain assumptions about how it is
layed out.
------------------------------------------------------------------------
February 5, 2005 - 22:56 : andremolnar
I am fixing up the code as we speak.
I actually made a bigger faux pas by not escaping my INSERT sql string
properly - (There is a quick fix, but I understand 'addslashes' is not
good enough for drupal ;-). (Thank you to Dries for catching this right
away on his test of the demo site).
As for multiple books - you actually have more flexibility to enable as
many book menus as you like - not only based on type but also by path.
e.g. Could allow for:
drupal.org/handbook (with some general book menu blocks enabled)
drupal.org/handbook/developer (with developer book menu blocks enabled)
drupal.org/handbook/developer/theme (with theme developer book menu
blocks enabled)
etc. (the sky is the limit).
I will resubmit a patch with all of the coding style fixes requested -
and I will be more careful with the coding style in the future.
andre
------------------------------------------------------------------------
February 6, 2005 - 00:05 : andremolnar
Attachment: http://drupal.org/files/issues/book_module_10.patch (7.7 KB)
Updated Patch,
Implemented code style improvements as per Steven.
Fixed unescaped sql INSERT string (turns out that addslashes is more
than enough in this situation, because the 'title' string has already
been valided and properly inserted into the database before any of this
code ever runs - i.e. this isn't coming directly from user input, but
rather from the drupal database).
Demo site is once again live and welcomes all testers (Thank you again
to those that have already tested this out).
andre
------------------------------------------------------------------------
March 13, 2005 - 22:11 : killes(a)www.drop.org
Still applies.
Andre: Please create patches from the Drupal root directory.
------------------------------------------------------------------------
March 15, 2005 - 10:42 : Paul Iliano
This is exactly the functionality I have been waiting for. As I can see
many uses for showing a book menu on all pages, I never really
understood why book menus could only be shown on book pages.
I'm still at DP 4.5, but as soon as 4.6 is out I will install and try
it out. I'd love to see this kind of functionality
in core!
------------------------------------------------------------------------
March 18, 2005 - 07:43 : Steven
Your visibility 'fix' requires that the entire handbook has clean,
hierarchical URL aliasing. This is often not the case.
I understand that for sites where the book is used to organise the main
structure, you might want the block visible everywhere. But it is not
good for sites where this isn't the case, like Drupal.org itself.
------------------------------------------------------------------------
April 3, 2005 - 17:12 : Paul Iliano
As Steven says, for some type of sites (like perhaps drupal.org) it
could not be desirable to show book menus on all pages. But for other
sites it could be very much wanted.
It seems to me a good solution would be to let administrators simply
choose which node types they want book menus to appear on, like we
currently can opt to switch off author/date info per node type.
------------------------------------------------------------------------
April 3, 2005 - 22:49 : factoryjoe
In general, +1 on the auto-updating aspect of this. It does seem to be a
welcome improvement to the book system, though I question Boris’
assertion that this is really a good solution for “brochure-ware”
sites.
For one thing, when I design a site, no matter who I am, I highly doubt
that I would think “ah ha, make a book to contain the pages of my
site... and the navigation will be generated from it!” While the
functionality is pretty much spot-on, the name is not. So what we end
up with is an overloaded book module or worse, a misnamed module.
I also wonder about moving the book-generated menus around. For
example, the way I would like this patch to work is for there to be
top-level “chapters” (like sections of a website) and then book
pages within each section, representing the individual pages. I would
like to be able to put the “chapters” horizontally in the header of
my site (like on OurMedia.org [1]) and then have the pages show up in
the sidebar when you navigate to each section. I guess I just wonder
about the flexibility of this approach and whether or not it will serve
the functional needs that Boris thinks it might. What if I want the page
titles to be different from the menu link titles? It doesn't seem that
this is something I can control with the current partial menu
integration.
Anyway, with a specific eyes towards improving book navigation, I do
agree that it functionally looks pretty good.
I did find one serious bug and one major usability issue. It has to do
with integration with the menu system. Andre will hate me for this, but
I clicked through the “reset menus” command in the menu system.
Doing so wiped out all the book navigation menus. If these menu items
were really “locked”, then why were the book menu items wiped out?
How do I restore them?
As for the usability issue, it is unclear to me where I would go
reorganize my book-generated navigation menus. I looked in menus, where
it should logically exist, but everything was locked. Does this mean I
have to go to each individual node to sort my content?
[1] http://www.ourmedia.org/
------------------------------------------------------------------------
April 5, 2005 - 23:24 : menesis
>From what i see on demo site, there is no way to configure the site to
act as it does now in case of multiple books - show book navigation
block only for current book. I could only configure a block to show up
on any book page, but it is shown while viewing other books, too.
IMHO the preferred way to modify a module is to introduce new
functionality while keeping it's behavior without any extra
configuration. In this case, when creating a new book, a menu would be
created, the block enabled, and configured to show up only on pages of
that book.
Visibility by path will not work, because all book pages have url like
node/1653 by default, need a block setting for that (true="Show on any
book's pages", false="Show only on current book's pages", false by
default). The tricky part is that menu module, which provides the block
and it's configuration, does not know which book is being displayed.
------------------------------------------------------------------------
April 17, 2005 - 21:38 : bobo
For those of us without linux, can someone please add the latest patch
to the Drupal 4.5 book.module. I would like to use it asap for a
website I'm creating for an animal welfare group. This is really cool,
I hope you guys find a way to get it put into the latest version of
Drupal. It should be a core peice of code. It seems strange to me that
an option to click wether or not you want a book to display on the main
page doesn't exist...
Bobo
------------------------------------------------------------------------
April 18, 2005 - 17:23 : bomarmonk
This functionality seems crucial to completing the organizational
website that I have developed with Drupal. Yet, I've applied the
book.module and menu.inc patches to Drupal 4.6 and can't seem to get by
book blocks to display on the front page. Why not? Is it because I'm
using the front_page.module? I'm still using a themed drupal page for
the front page. I definitely need help with this.
------------------------------------------------------------------------
April 20, 2005 - 17:45 : andremolnar
It occurs to me, as I finally dive back into this, that the main issue
is that people want BOTH the new way of handling blocks (because of
greater flexibility of putting a book block anywhere they like) AND the
old way (because people have sites where they only want to book block to
show up in the current book's nodes - at the exclusion of all other
books).
At some point in the patch history, both options were available by
simply leaving book.module's own block generation code intact AND
implementing the new code that inserted items into the menu system and
let menu.module handle block creation.
This was deemed to be bad. (The argument being that there shouldn't be
two sections of code generating blocks for one module. Essentially
duplicating each other's efforts). The good news is that in an attempt
to address the issue I added a welcome configuration option to the
block.module - but it wasn't enough to REALLY solve the problem.
As menesis pointed out, the problem is that when the block generation
is handed off to the menu module, the menu module doesn't have any way
of knowing that the blocks it is generating based on the menu structure
are 'books' - and hence there is no way to know that a particular book's
navigation block should only show up in a particular book - it simply
becomes a block like any other block and must follow the same rules.
Quite frankly I think people should just suck up the fact that in order
to move forward and offer improved functionality some other
functionality is deprecated. But, I suspect that that argument won't
fly in this case - its just too big a paradigm shift *wink*.
All kidding aside, I am going to try and find a solution. In the mean
time a REAL issue was raised by factoryjoe when he noticed the blocks
could be wiped out simply by 'resetting' menu's in the menu admin.
This is NOT desired and should be reasonably easy to fix by telling
menu module not to dump menu's generated by modules during a 'reset'.
{And no joe I didn't mind you messing up the blocks on the demo site -
it raised a very valid problem}.
If anyone is interested I am going to be placing pre-patched files in
my sandbox - so if people like the new functionality despite losing
traditional book block functionality they can just download the
pre-patched files. And if people want to monkey with the code and
contribute solutions there is a place to do it.
http://cvs.drupal.org/viewcvs/drupal/contributions/sandbox/andremolnar/book…
More to come.
andre
------------------------------------------------------------------------
April 28, 2005 - 15:43 : bomarmonk
Andre,
I really appreciate the work you and others have put into making the
book module and menus/blocks better. I am fairly new to PHP, mySQL,
Drupal etc., but I would like to offer some suggestions anyway (even if
they are a bit naive). I have tested the patched versions of menu.inc
and book module and would like to offer my support for the following
added functionality:
1) Have support to allow or disallow the root page of the book in the
navigation block (this would definitely be desireable for many using
book blocks for hierarchal menus).
a) However-- another desired (more useful?) behavior might be to
not display the currently viewed
book page whether it is the root page, second page, third
page, etc. Food for thought.
2) Allow individual book blocks to behave in the traditional manner, or
to enable certain book blocks to show up on every page (as site
navigation).
3) This feature would also be complemented by the ability to show
navigation blocks for sections or chapters within a larger book. This
way, you could create a "master" book with a larger table of contents
(providing an index of book titles) and still allow concise, focused
navigation within the chapter nav blocks.
Again, I am sorry If I've repeated anything I've already said in
another post (a little double posting), but I felt this was the best
place to sum up some of the functionality I feel is currently missing
from the book module. Since I am a novice, feel free to correct me or
give me verbal lashings. I can take it.
------------------------------------------------------------------------
April 29, 2005 - 15:29 : mcduarte2000
I also have one desire...
I wanted an option to expand completly the menu block to the 2nd or 3rd
level. Not only the section the user is currently in, but all the
sections.
This would be great to be able to use it as well as site navigation.
And I believe is not difficult to implement.
Regards,
Miguel
------------------------------------------------------------------------
May 1, 2005 - 15:57 : Dries
I committed a change to the block module that lets people use PHP to
specify when a particular block should show up. So if you make it so
that the book blocks show up on _every_ page (like all other blocks),
administrators can still use PHP to limit their visibility. Showing
the book block on every page (unless instructed otherwise) makes for
improved consistency and is more likely to match the administrator's
expectations. Mind to take a look at this to see how this would affect
the proposed patch? Let us focus on getting the right blocks in core,
and worry about their visibility later.
1
0
01 May '05
Issue status update for http://drupal.org/node/20831
Project: Drupal
Version: 4.6.0
Component: book.module
Category: feature requests
Priority: normal
Assigned to: Anonymous
Reported by: clydefrog
Updated by: clydefrog
Status: patch
This is a patch to include the top-level book page in the book
navigation block. I think this makes the navigation block a lot more
useful. Before this patch, all pages in the book were included in the
block except for the root level page, so it was difficult to get back
to the root level.
I don't know if this is the best way to do it, but it works and affects
only book_tree() and book_block().
clydefrog
5
13
Issue status update for http://drupal.org/node/21441
Project: Drupal
Version: cvs
Component: book.module
Category: bug reports
Priority: normal
Assigned to: Anonymous
Reported by: moshe weitzman
Updated by: Dries
Status: patch
Note that you have to wrap strings in t().
How about removing the books from the navigation menu, and returning an
itemized list of books?
Dries
Previous comments:
------------------------------------------------------------------------
April 27, 2005 - 21:20 : moshe weitzman
probably a result of the recent patch where we return content instead of
print.
------------------------------------------------------------------------
April 29, 2005 - 02:41 : puregin
Attachment: http://drupal.org/files/issues/book.module_blank_admin_page.patch (488 bytes)
The function book_admin_view() does not return a non-empty string if
$nid is zero, which is the default value if /admin/node/book is
visited.
The attached patch returns 'No book selected.'. There's probably a
better way to do this.
1
0
[drupal-devel] [feature] Improve functionality of block generation for book module
by Dries 01 May '05
by Dries 01 May '05
01 May '05
Issue status update for http://drupal.org/node/14120
Project: Drupal
Version: cvs
Component: book.module
Category: feature requests
Priority: normal
Assigned to: andremolnar
Reported by: nysus
Updated by: Dries
Status: patch
I committed a change to the block module that lets people use PHP to
specify when a particular block should show up. So if you make it so
that the book blocks show up on _every_ page (like all other blocks),
administrators can still use PHP to limit their visibility. Showing
the book block on every page (unless instructed otherwise) makes for
improved consistency and is more likely to match the administrator's
expectations. Mind to take a look at this to see how this would affect
the proposed patch? Let us focus on getting the right blocks in core,
and worry about their visibility later.
Dries
Previous comments:
------------------------------------------------------------------------
December 9, 2004 - 08:12 : nysus
Attachment: http://drupal.org/files/issues/book_19.patch (4.85 KB)
Attached is a patch for the book module that does the following:
1) Allows book blocks to appear on any page at any time, not just when
a node from the book is being viewed.
2) Allows multiple book blocks to appear on the same page.
This functionality is achieved by the automatic creation of individual
blocks for each book when the book is created. Simply enable the
book's block to enjoy the benefits of 1 & 2 above. If the blocks are
not enabled, the blocks will appear only when a node from that block is
being viewed (the same way it works now).
------------------------------------------------------------------------
December 9, 2004 - 14:35 : andremolnar
Attachment: http://drupal.org/files/issues/book_19_1.patch (4.84 KB)
+1 This is great. A good many people have asked for something like
this, and I think its a nice solution. But in the end this isn't up to
me.
One minor error in the patch...
+ $result = db_query('SELECT n.title, b.block, b.nid FROM {book} b
INNER JOIN {node} n on n.nid = b.nid WHERE b.parent = 0');
b.block is not a valid field in this query. this updated patch removes
reference to it.
andre
------------------------------------------------------------------------
December 9, 2004 - 15:54 : nysus
Glad you like it and thanks for fixing that up. It was left over from
an older version of my patch.
------------------------------------------------------------------------
December 9, 2004 - 15:54 : andremolnar
Steve,
If I apply this patch, and I attempt to configure one of the newly
created blocks. I noticed that for some reason the block.module is
returning "true" at line 249 (of the block module)- and is creating a
form for module-specific configuration. But all that shows up on the
screen is the word "array".
Can you trace this back to see why - and maybe update the patch.
I can continue to test your changes - anything to help this patch make
it into core.
andre
------------------------------------------------------------------------
December 9, 2004 - 17:29 : nysus
Hmmm...probably because I tested my patch on my version of Drupal,
version 4.5.1. I'm having no problems. Are you using a cvs version of
Drupal to test. If so, I'll set up cvs on my site and track this down.
------------------------------------------------------------------------
December 9, 2004 - 20:53 : drumm
See http://drupal.org/node/12347 for information on how the block system
has been updated. When I saw that the elseif ($op == 'view') was taken
out I knew immedaitely that there was something weird about this patch.
------------------------------------------------------------------------
December 9, 2004 - 20:58 : nysus
OK, thanks for the tip. Sorry for the confusion. Still kind of new to
making open source contributions and it's easy for me to overlook some
obvious stuff like this. I'll fix this up when I get a chance.
------------------------------------------------------------------------
December 9, 2004 - 21:06 : Dries
Please do, because this being a new feature, it has no chance getting
committed to the DRUPAL-4-5 branch. The DRUPAL-4-5 branch is in
bugfix-mode. New features go into CVS HEAD.
------------------------------------------------------------------------
December 10, 2004 - 00:32 : Dries
There is quite a bit of duplicated code in the patch. Maybe it can be
simplified (using a function)? Either way, it is a little weird. I
haven't tried the path, and I'm not sure I understood the description.
It's somewhat vague. Is the book module exporting multiple blocks that
are nearly identical, yet have different display behavior? If so, why
not add a simple block configuration setting to the original book
block?
------------------------------------------------------------------------
December 10, 2004 - 04:51 : nysus
Dries,
Yes, there is some code that can be factored out and some general
cleaning up that can be done. It was a little tricky to write so I
left it kind of ragged around the edges until I'm sure there are no
bugs. It has worked very well on 4.5.1 but I obviously need to update
it to work with cvs. I'll be on that soon.
As far as what it does:
1) For every new book that a user creates, a new block is associated
with it. So if you create "Book A", "Book B", "Book C", you will get
three new blocks visible on the block administration page called "Book
A", "Book B", "Book C". The original "Book Navigation" block is still
there, too. The functionality of the "Book Navigation" block is not
affected at all by this patch.
2) If Book A's block is enabled, the block containing its menu will
appear not only when a node within Book A is viewed, but at all times
(unless the user suppresses it on certain pages with the "path"
feature). When any node is that is NOT in Book A is viewed, Book A's
menu still appears but it is fully collapsed. When a node that DOES
belong to Book A is viewed, Book A's Book menu expands accordingly.
3) The user can also enable Book B & C's block, and have their menus
appear in a block at all times as well.
4) If none of the book's blocks are enabled by the user, the module
will behave just as it did without the patch. That is, when the "Book
Navigation" block is enabled, the only time any book menu will appear
is when a book node is being viewed.
5) It's important to note (and this was the tricky part to write) that
if both the "Book Navigation" block is enabled and "Book A" is enabled,
they will play nice with each other and not do nasty things like create
the same book menu twice.
------------------------------------------------------------------------
December 10, 2004 - 07:51 : nysus
Attachment: http://drupal.org/files/issues/book_20.patch (5.37 KB)
Andre,
Attached is a new patch that will resolve the problem of the
block-specific stuff showing on the block configuration form.
Let me know if you spot any other bugs. If it looks good to you, I'll
go to work making the code look leaner and prettier.
------------------------------------------------------------------------
December 10, 2004 - 09:26 : nysus
Attachment: http://drupal.org/files/issues/book_21.patch (4 KB)
Dries, Andre:
Here is a new and improved streamlined version of the patch. Have a
look if you get a chance.
------------------------------------------------------------------------
December 10, 2004 - 10:11 : nysus
Attachment: http://drupal.org/files/issues/book_22.patch (4.15 KB)
Found a bug in the last version that would cause the block to jump to a
different location. I think this should do it. Everything appear to
work well (famous last words).
------------------------------------------------------------------------
December 10, 2004 - 16:49 : andremolnar
Steve: bugs appear to be gone, and I didn't run across any other
errors. This is functioning exactly as described.
everyone: I personally would encourage support for this functionality.
Book is a powerful navigation building tool in a site, not only with
its ability to move next and back through a hierarchy of pages - but
also its ability to build the appropriate navigation block without
further user intervention (unlike the admin features in the menu module
or taxonomy).
The most frequently cited complaint about the book module is its
inflexibility when it comes to when and where the block shows up. I
also frequently hear requests for the ability to show multiple book
blocks at the same time. Up until now the best alternatives suggested
required users to do a hack (e.g. build a custom block that calls such
and such a hook). Most abandon their request at that point because its
over their heads.
With this patch all those requests are covered and more. Now all books
can automatically have their own block and admins can easily decide when
and where each of those individual blocks show up (left right, up down)
and coupled with the new configuration features of the block module -
its very easy for admins to decide on which individual pages a block
will show up.
I would be interested what other have to say about this feature.
My only reservation (which is minor compared to the benefits of the
functionality offered) is that there is no way to turn this
functionality off. i.e. The default behaviour is to build individual
blocks for each book. If there could be a way to toggle this feature
on and off somewhere - it would be perfect. Still, AS IS - this is a
major improvement and offers great flexibility to admins and site
creators.
andre
------------------------------------------------------------------------
December 10, 2004 - 17:04 : Anonymous
Andre,
Thanks for the feedback on the usefulness of this module. Glad I could
pitch in and help.
I agree about the inability to turn the feature on/off and I was
thinking about that myself. I think it could easily be accomplished
by creating a checkbox in the "book navigation" block individual
configuration's settings. Call it "Enable individual book blocks."
When enabled, the individual book blocks will appear on the block
administration menu.
One question: Where would the state of this checkbox get saved? Has
Drupal moved away from serializing data in the data base?
------------------------------------------------------------------------
December 10, 2004 - 20:36 : Dries
I'm afraid that 'Enable individual book blocks.' is not
descriptive/clear at all. Are you suggesting a setting to toggle
between 'show block on all pages' and 'show block on book pages only'?
------------------------------------------------------------------------
December 10, 2004 - 21:45 : nysus
Dries,
No. Andre and I suggest a setting within the "Book Navigation"
cofiguration page, that would toggle whether or not individual books
appear on the list of all blocks on the block administration page.
Hence the name 'Enable individual book navigation blocks.' The help
text for this checkbox might read something like: "By default, a book's
navigation block is visible only when a page from that book is being
viewed. Check this box if you want more control over where and when an
book's navigation block is visible. You will then be able to control
the book's navigation block location and visibility settings on the
"admin/block" page."
Hope this makes it pretty clear.
------------------------------------------------------------------------
December 10, 2004 - 22:08 : Dries
I understand what you are trying to do, but not how you are trying to do
it, or how the setting is supposed to work. I guess I'll have to try it
when a new patch lands.
------------------------------------------------------------------------
December 11, 2004 - 11:52 : nysus
Attachment: http://drupal.org/files/issues/book_23.patch (5.02 KB)
Alright, fellas, I'm proud to unveil my crowning achievement in the open
source development world (no big deal to most of you guys but pretty
good for a hack like me).
Thanks for all the input so far. It's been helpful. I've streamlined
the heck out of it per Dries suggestion and I've created an option to
turn this functionality on an off per Andre's suggestion. Does this
look good to you guys? Anything else I have to fix or improve?
Thanks.
------------------------------------------------------------------------
December 11, 2004 - 15:02 : Dries
I tried the patch.
It works great but to me, the 'Give books their own block' settings
seems to be redundant. Why not export the current book navigation
block, along with an additional block for each book? Looks a lot
simpler to me.
I think I spotted a bug: orphaned book pages (or possibly book pages
that are unpublished) appear to be getting book navigation blocks.
------------------------------------------------------------------------
December 11, 2004 - 19:34 : nysus
I'll see if I can fix the bug. Might be tricky.
But I don't understand your recommendation to "export the current book
navigation block, along with an additional block for each book". Can
you expand on this thought?
------------------------------------------------------------------------
December 11, 2004 - 20:06 : nysus
Dries,
I am unable to duplicate the bug. I have three orphaned pages. I also
tried unpublishing some pages. But as far as I can tell, the patch
works as expected.
------------------------------------------------------------------------
December 11, 2004 - 21:00 : Dries
If you can't reproduce the problem, chances are my node/book table is
somewhat fubar.
As for the configuration option. I suggest removing it and to always
make these new blocks available on the /admin/block configuration
screen.
------------------------------------------------------------------------
December 11, 2004 - 21:52 : moshe weitzman
I am hoping that we maintain the option to keep the behavior where the
appropriate book block only shows up when its book page viewed. this is
a nice, tidy arrangement.
------------------------------------------------------------------------
December 11, 2004 - 22:03 : nysus
Yes, if you don't enable any of the individual book blocks, the a book's
block (i.e. navigation menu) will only appear when a page in a book is
viewed. In other words, you have the option to have the book block act
like this patch isn't even installed.
------------------------------------------------------------------------
December 11, 2004 - 22:54 : Dries
I guess I'll have to try the patch again, because I don't understand why
it works like this -- or at least, why it can't be made simpler.
------------------------------------------------------------------------
December 11, 2004 - 23:13 : nysus
Can you be more specific? Why it works like what?
------------------------------------------------------------------------
December 11, 2004 - 23:24 : nysus
Attachment: http://drupal.org/files/issues/book_24.patch (4.1 KB)
This patch reverses a change made in the last patch which required a
user to enable individual book block before they could enable any
individual book blocks.
------------------------------------------------------------------------
December 11, 2004 - 23:41 : andremolnar
As I mentioned earlier I am *fine* with either version of this patch as
long as it makes it to core.
But, as I said earlier, I clearly think the preference for admins would
be to have the option to enable or disable this functionality.
BTW - if this does make it in, I would be happy to create a Handbook
page that describes the new features - something like "how to build
robust site navigation using the book module". (on or after December
17th).
andre
------------------------------------------------------------------------
December 12, 2004 - 14:11 : Anonymous
I am not at all happy with these features. They are incosistant,
confusing and should use exising methods and UIs.
May main concern is the incosistancy: it is confusing, will require
extra attention with each core code change, adds extra logic to the
core, and is not re-useable.
so here are my questions:
1) why do we not use the menu system and apis to build and adminster
the trees? Saves code, does not add extra UIs, and gives users more
power.
2) why do we need that extra showing logic? a book block should not get
exeptional if clauses, it should use the existing path setting methods
on block admin. extra logic is confusing for administrators (hey, i set
the path so that the book-block should show up here, but it does not,
why?) we should really not provide extra logic in the block hook, but
should rather use default settings in block admin (the book could fill
the bookblock sql cells with custom paths, for example)
3) we should avoid extra UIs. We already have far too much, and far too
much different ones. Please rather improve the block admin, than add new
separate interfaces.
4) why do book blocks need al these expeptions in the first place? If
they are so exeptional, we could consider not using blocks, but
something else, like in-line book layout (pages with the index etc)
5) why did you not choose for a general, standard, block gerneation
API? that way modules, such as taxonomy, image gallery, weblinks,
article, etc can reuse it and introduce block gernation.
All that sayd, i like the idea of this functionality, but i fear for a
great useability downfall if we start introducing all sorts of
exeptions for all sorts of modules. Because now chances are very big
that taxonomy, image gallery etc will need to introduce other UIs,
other code, new methods and new documentation, if they too want some
sort of better block handling.
so a -1 from me.
------------------------------------------------------------------------
December 12, 2004 - 14:12 : Bèr Kessels
^^--- That was me (bèr kessels) forgot to log in.....
------------------------------------------------------------------------
December 12, 2004 - 14:34 : nysus
I'm not going to pretend I can argue if my patch does or does not fit
well into Drupal's larger architecture. My motivation for writing it
is that I had an immediate need to create an easy way to make it easy
for users to create menus.
I'll let others decide whether or not the patch has merit from the big
picture perspective. But if it doesn't, why not just use it for its
immediate benefits and then throw it away when something better comes
along?
------------------------------------------------------------------------
December 12, 2004 - 15:01 : Dries
My summary is this: +1 on the functionality, -1 on the implementation.
The code itself is good, but the usability/integration is not.
------------------------------------------------------------------------
December 12, 2004 - 15:13 : nysus
When you say "usability" is that from the user's perspective or the code
maintainers? I'm guessing it's the latter but I'm unsure.
What about the idea of using the patch until a more permanent solution
comes along? Yes, it's much better to live in a home with indoor
plumbing but why not use the outhouse while you wait for a toilet to
get installed? Or are there other considerations I'm overlooking that
would make this a bad idea?
------------------------------------------------------------------------
December 12, 2004 - 15:20 : Goba
nysus it is just generally against the Drupal philosophy to add
improperly implemented functionality until something better comes
along. There even used to be ocassions in Drupal releases, when some
functionality was removed (not fixed) for a release, because its
implementation was not adequate.
------------------------------------------------------------------------
December 12, 2004 - 15:55 : Dries
Usability for the user. The extra setting on the block configuration
page is both confusing and awkward. I don't understand why things must
be configured/enabled that way (see my and Ber's previous comments on
this issue).
------------------------------------------------------------------------
December 12, 2004 - 16:03 : nysus
Well, just for the record, I reversed that functionality per your
suggestion and uploaded the patch. The indiviudal book blocks now
appear automatically.
------------------------------------------------------------------------
December 13, 2004 - 15:56 : Bèr Kessels
Hi,
I am sure you can make not only simpler, but better usefull for admins
and users.
All you need to do is use the menus for this. i.e. make a menu entry
for each book page.
For each book make a menu on level 0, without a parent. that way they
become a seoprate menu, each with an own block.
it saves code, makes things more consistent, and most important, uses
drupal functionality where it should.
------------------------------------------------------------------------
December 13, 2004 - 16:50 : nysus
Ber,
Are you suggesting that for every single book page that a menu item be
created? That's really not practical. That was my main motivation
for writing this patch: to make it easy to put links, not necessarily
related to one another, into a block. Any more pages than 10 and the
sheer tedium of the job would prevent anyone from ever doing that. The
menu.module is great, but adding new menu items is far from quick and
painless. I just added about 10 to my menu for different taxonomies on
my site and it wasn't fun.
Plus, if you do as you suggest, there is also the problem of the book
showing up twice. It will be generated by the menu and then it will be
generated again by the book module which is programmed to design a
block. You'd have to put some logic in the book.module _block hook to
try to anticipate if a user has enabled a book in the menu. That
wouldn't be pretty code.
I'm all for putting automatic generation of book navigation blocks as
part of the menu module. It does make more sense to have it there.
But it forces me to ask the question: "Then why do we currently have
code in the book module that generates a menu? Shouldn't that belong
in the menu.module, too?"
------------------------------------------------------------------------
December 14, 2004 - 00:31 : killes(a)www.drop.org
I think what Ber is trying to say is that you can write a contrib module
that monitors the changes to the book table and creates menu items
automatically. nodeapi is your friend. I would also prefer this
solution.
------------------------------------------------------------------------
December 15, 2004 - 18:39 : Anonymous
""Then why do we currently have code in the book module that generates a
menu? Shouldn't that belong in the menu.module, too?"
"
Because it's old code. It would be nice if book.module generated this
block using its _menu hook, so that the admin would have a few options
in terms of configuration.
"Plus, if you do as you suggest, there is also the problem of the book
showing up twice. It will be generated by the menu and then it will be
generated again by the book module which is programmed to design a
block.
"
No. The old code which manually builds a block in book.module would be
removed. Book blocks would only be generated by the menu.
This would also have the added benefit of allow the administrator to
easily place a book in an appropriate spot in the menu tree, while
still allowing the possibility of displaying it in a separate block.
Because of menu caching, I don't expect a large performance hit for
creating the menu items.
"That was my main motivation for writing this patch: to make it easy to
put links, not necessarily related to one another, into a block.
"
It sounds like instead of (mis)using book.module, your time would be
better spent in a usability improvement to menu.module so it's easier
to do this.
------------------------------------------------------------------------
December 16, 2004 - 08:55 : nysus
Thanks for the feedback and input. I appreciate it. However, I would
also appreciate if you took more care to avoid the condescending tone
in your post:
"It sounds like instead of (mis)using book.module, your time would be
better spent in a usability improvement to menu.module so it's easier
to do this.
"
It's really quite unnecessary and off-putting. Though it won't stop me
from contributing to Drupal in the future, I'm sure others would be
really turned off by such a patronizing comment and it could dissuade
them. I'd like the Drupal community to be a welcoming and friendly
place that will inspire people to contribute, not discourage them.
Thanks.
------------------------------------------------------------------------
December 16, 2004 - 12:07 : Bèr Kessels
nyesus,
Please do not start /that/ discussion here. :) Drupal community is
known fo being direct, maybe because of the big number of
western-Europeans attending, maybe because of other reasons.
No-one commented that you are wasting your time. But the commentor was
telling you somthing likethis:
"If you would follow the previous sugeestions, your added feature would
be much better appreciated, and will probably work much better for you
too".
He/she was by no means telling you to stop your silly coding, or
anything in that line. He/She only wanted to show you the obvious and
better direction.
We often deal with issues that add some feature, and a complete new UI,
because the author does not like, or cannot use the existing UIs and
features. This is nogt good, because if that same author would have
spend his/her time on improving that existing functionality or UI
(improving is not neccesarily the same as extending!!) that code and
time would benefit all much better.
Thats what the commentor tried to say. And so is it here: If you
dislike the way the books handle the blocks, and if you do not want to
use the menu, because you do not like its UI, then do not add another
UI and more features, but rather merge these, and improve the parts (in
the menu) you dislike.
------------------------------------------------------------------------
December 16, 2004 - 12:35 : Dries
We can worry about the menu integration later. Let's focus on the new
option's usability/interaction design first.
------------------------------------------------------------------------
December 16, 2004 - 16:51 : nysus
I understand what the commenter was saying and like I said, I appreciate
and understand it. I'm not upset and I'm not looking for an argument, I
was just being direct as well. :) As part of the Drupal community
(albeit a minor player), all I'm saying is that I would like to see
folks not have a tin ear to the humanity in all of us. It will help
make Drupal an even stronger community and attract even more talented
developers.
Human interaction is part of the development process. Whether we like
it or not, we must cope and deal with it.
------------------------------------------------------------------------
December 18, 2004 - 11:54 : Dries
I thought some more about this and am starting to believe that
integration with the menu system should take priority. Here are common
cool scenario's:
I want to create a separate navigation block for the 'Drupal handbook'.
I want to add a menu item called 'handbook' to the user block. That
is, I want the book navigation to be part of the existing user
navigation block.
I want to add a menu item called 'handbook' to the top-level navigation
menu.
How would that work from a user's point of view? What do I have to
click and where to configure this?
------------------------------------------------------------------------
December 21, 2004 - 20:47 : andremolnar
I was actually thinking about the same thing last night (must have been
something in the arctic air).
/
2. I want to add a menu item called 'handbook' to the user block. That
is, I want the book navigation to be part of the existing user
navigation block.
3. I want to add a menu item called 'handbook' to the top-level
navigation menu.
/
This is already possible (to a certain extent) with the current
Book.module and Menu.module - A 'Books' menu item is created in the
user navigation by having the Book.module enabled. Menu.module allows
you to enable/disable this menu item. Menu.module also allows you to
re-name this menu item. But, this only helps if you only intend to
have 1 book (or else the name 'Handbook' is misleading if the user
finds more than 1 book listed). - so this isn't good enough (or is it).
/1. I want to create a separate navigation block for the 'Drupal
handbook'./
This is what I was trying to figure out. Not just this, but a
different way to do what Nysus was attempting. i.e. create a menu
block for each and every book that is created. There is a way to write
code that would (re)build a 'custom menu' on every add/edit/update to a
book page - or book outline update. Custom menu's automatically have a
block created for them. But, this I think would be a misleading use of
the 'custom' menu - as the menu would not be custom if they are a part
of core.
So, I would think that two new constants could be added to the menu.inc
file - MENU_BOOK_MENU - and MENU_BOOK_ITEM - each would behave as custom
menu's, but would be reserved for books. These menu types should NOT
show up in the Menu.module administration - because the administration
of the book_menu items would be done by administering the book itself
(adding an item, removing an item, moving an item up/down in the
hierarchy, assigning parents etc.).
However, the blocks that the book_menus would create would show up in
the block administration (so users could enable/disable each block -
and decide where on the site they show up). The existing book block
logic would be removed.
So the logic would be:
If a creates a book in the book administration page - the Book.module
automatically creates a new MENU_BOOK_MENU
Any time a user adds to or updates or delets a book page - the entire
book menu is deleted and recreated based on the hierarchy defined by
the book itself.
The blocks for each book would show up in block administration.
Any thoughts - I know that this doesn't exactly address points 1 and 2
- but it could act as a first step.
Is this approach a bad idea? It would add special conditions for the
proposed book menu's - but books would be a special case.
Even if people don't like this solution, maybe it will give someone a
better idea. I'd love to hear them.
andre
------------------------------------------------------------------------
December 22, 2004 - 00:55 : Boris Mann
+1 to this andre
I had promised to put down my thoughts on this matter, as it relates to
1) primary and secondary links and how they are managed and 2)
auto-generation of primary/secondary navigation based on book outlines
So, for 1), we currently have functional-but-not-very-usable plain
textfields to manage primary and secondary links. I would like to see
menu.module used to control all navigation links, whether it is the
navigation block or primary/secondary links. What is needed:
a) default system menus labelled "primary" and "secondary" which would
store; this is where modules could insert navigation
b) support for full URLs (e.g. http://myexternaldomain.com) instead of
just path
2) if we got 1 working, and andre does his book menus, this could get
taken care of automatically. Basically, for brochureware/business/etc.
sites that have static content, you could have a root book as one of
the primary navigation links, and then the secondary links are
generated automatically.
------------------------------------------------------------------------
December 22, 2004 - 23:02 : Dries
I agree with Andre that the book module's integration with the menu
system needs to be worked on. I support any effort that makes it
easier to structure pages and that makes it easy to link pages/nodes
from within a menu.
However, I'm opposed to putting book module specific names in the menu
system (eg. MENU_BOOK_MENU and MENU_BOOK_ITEM). I can imagine a
handful of modules that want to maintain a menu tree (or part thereof)
so I'm in favor of generic names.
I'd have to read up on the menu system code, but last time I checked
there was a MENU_MODIFIABLE_BY_ADMIN flag. You could choose not to set
this flag for the menu items generated by the book module. Maybe it's
already possible to implement to implement Andre's suggestion without
having to modify/extend the menu system.
Are you exploring this path?
------------------------------------------------------------------------
December 24, 2004 - 12:01 : andremolnar
I've had a few spare hours to work on this.
I've started to cobble together a solution - but in doing so I
discovered a bug in the menu module (for which I will submit a seperate
issue - if one doesn't already exist).
The principal I suggested works. I added some code to the book module
that creates custom menu's (MENU_CUSTOM_MENU with MENU_CUSTOM_ITEMs)
for each Book that exists in a site. This is just as a proof of
concept - I chose this menu type to start with because they
automatically have a block created for them in admin/block (which is
ultimatly the functionality we want).
I ran a test and the menus are created as expected - the blocks are
also created. But when I tested the menu blocks by enabling them I ran
into a problem.
It seems as though the menu system does not expand/explode sub menu
items if the node type is book. I'm not sure why this is, and I
haven't traced the source code yet - I thought I would ask if anyone
has intimate knowledge of the menu system if they can point me in the
right direction.
No patch attached because until that problem is fixed this proposed
change won't fly :(
andre
------------------------------------------------------------------------
December 24, 2004 - 13:11 : Dries
Just a wild guess: maybe it doesn't work because the book module's URI
scheme is not hierarchical?
------------------------------------------------------------------------
January 7, 2005 - 12:45 : andremolnar
Attachment: http://drupal.org/files/issues/book_module_1.patch (5.1 KB)
I finally took some time out to do some work on this. As mentioned in a
post to dev the problem I was having with menus not
expanding/contracting properly was some crud in my database. Once that
was cleared up my changes worked fine.
I am attaching a patch for comments and for the brave willing to give
it a test drive.
History: This thread and http://drupal.org/node/15198 and
http://drupal.org/node/15153
node/15198 has a patch that is required for this patch to work.
What this does:
Pretty straight forward.
Any time a user adds/updates/deletes book or book pages (including via
outline) - a function is called to create a new menu for each book.
Any existing book menus are wiped out and then the new book menus are
inserted - and the system menu is rebuilt to reflect the changes.
The menus created consist of type MENU_MODULE_MENU and
MENU_MODULE_ITEM. These menus show up in the menu/admin page so that
admins can be aware of them, but the menu types are not editable via
menu/admin (all changes are handled by the book module).
Since the menus are in the menu table, the menu_block() hook handles
the creation of the blocks for each of them. The blocks can then be
administered in the usual ways via the block/admin interface.
KNOWN ISSUE: (suggested solutions welcome)
Since the menu table is updated on every change to books - the blocks
associated with the menus are also recreated with default settings
(i.e. disabled, and with no path or throttle settings) requiring a user
to take an extra step and re-configure the book blocks for viewing on
their site. I think this is unacceptable. For the casual user of the
book module that only has a single book that doesn't change often, this
would not be a big deal. But, if anyone wanted to make use of book
module to handle dynamic site navigation this would create more work
than it saves.
Looking forward:
If the block generation issue can be solved in a tidy way, this patch
could allow users to use book to handle all their site navigation
generation needs.
Also, this patch could allow for the removal of a large chunk of code
in the book module dedicated to building its own block via the block
system. I left it there for now because I suppose there may be those
out there that want to have book.module work the way book.module always
worked (only show the book block when viewing a book page). Even so,
since each book would have its own block, an admin could specify when
and where the block shows via admin/block.
I would appreciate feedback. If nothing else I hope this gives someone
some new ideas.
I'll continue to work on the block regeneration issue.
andre
------------------------------------------------------------------------
January 7, 2005 - 12:47 : andremolnar
Attachment: http://drupal.org/files/issues/book_module_2.patch (4.98 KB)
sorry - here's a patch with prefered line breaks.
andre
------------------------------------------------------------------------
January 7, 2005 - 18:47 : nysus
Andre,
I've been meaning to give this a throrough look when I get a chance.
Hopefully this weekend.
---Steve
------------------------------------------------------------------------
January 20, 2005 - 04:02 : andremolnar
Attachment: http://drupal.org/files/issues/book_module_3.patch (6.66 KB)
Here is an updated patch.
Same comments as followup 51 - except that the known issue has been
resolved.
This changes book module so that any action taken on a book, including
adding new books or book pages will create a menu in the menu system
for that book - and thus create blocks for those menus that can be
administered in the usual ways.
This is my first attempt at a major patch to core - comments are
welcome
I will be happy to update documentation once revisions to the code have
been taken care of.
andre
------------------------------------------------------------------------
January 20, 2005 - 04:53 : andremolnar
Attachment: http://drupal.org/files/issues/book_module_4.patch (6.17 KB)
Sorry - would be nice if I removed some debugging code - ;-)
also previous patch also would have introduced a need for a change to
the menu table that isn't required yet.
This patch is a correct version
andre
------------------------------------------------------------------------
January 20, 2005 - 21:16 : Dries
The menu is recreated every time a book page is updated. I believe this
is unwanted behavior because it requires the book block to be
reconfigured upon every update.
------------------------------------------------------------------------
January 20, 2005 - 21:32 : andremolnar
The menu is indeed re-created with every book page edit - because if the
book page title changes the menu needs to reflect this change.
The book block re-configuration is not required by the user. The code
stores this information and re-sets the block settings.
I will see if I can test for 'title change' before forcing the re-build
of the menu - It would save a bit of processing power.
andre
------------------------------------------------------------------------
January 20, 2005 - 22:21 : andremolnar
Took another at this - and it turns out that I already have a check to
see if title or weight change (rather - they already existed in the
module and I used them).
andre
------------------------------------------------------------------------
January 24, 2005 - 04:22 : andremolnar
Attachment: http://drupal.org/files/issues/book_module_6.patch (7.42 KB)
Feedback received indicated that the original block generation code in
this module should be removed - since this patch hands block generation
off to the menu and block system.
This patch removes that code. BUT - It should be noted, that in order
to have book blocks only show up on pages of node type book - an
additional patch found at http://drupal.org/node/16074 is required.
andre
------------------------------------------------------------------------
January 24, 2005 - 20:36 : andremolnar
Attachment: http://drupal.org/files/issues/book_module_7.patch (7.43 KB)
Yet another patch:
Brings this patch in line with - http://drupal.org/node/16074 (i.e. the
column name change in {block} table)
andre
------------------------------------------------------------------------
January 25, 2005 - 20:56 : andremolnar
Attachment: http://drupal.org/files/issues/book_module_8.patch (7.53 KB)
And another minor patch to cover coding style and variable names.
Just to bring everyone up to speed:
1) Each book and its children pages have a menu created for them (on
any add, edit, delete to a book or book page).
2) Those book menus automatically have a block created for them via
menu_block hook.
3) Block settings are preserved any time there is a change to a book
that requires the menu and its associated block to be re-built.
4) book_block hook is removed because it is redundant.
5) With the addition of the configuration option in blocks to only show
blocks on certain node types (see: http://drupal.org/node/16074)
administrators have full control over when and where a book block shows
up (including maintaining the status quo of only having book blocks show
up on book pages).
6) This patch does require http://drupal.org/node/15198
andre
------------------------------------------------------------------------
January 25, 2005 - 23:49 : Boris Mann
+1!!
Book-based automatic navigation for corporate/brochureware sites!
Fantastic! Chris Messina, get in here and add a +1 to this.
------------------------------------------------------------------------
January 26, 2005 - 00:26 : Dries
Boris, have you tested this patch extensively, or are you just giving
this +1 based on what you've read? If you want to see this committed,
take the time to review/test it.
------------------------------------------------------------------------
January 26, 2005 - 07:45 : andremolnar
Yes - please - testers are welcome.
Nysus (aka Steve) has already agreed to give this a test drive when he
had a spare moment. I hope for some feedback in the near future. But
if you have a moment Borris I would appreciate it.
OR - If someone would just like to demo this:
http://s92258948.onlinehome.us/greenbeach/
UN: drupaldev PW: drupaldev
Don't mind the mess - it is my test environment and there is a lot of
junk data and settings floating around - but this patch is running
there along with the additional block configuration settings.
andre
------------------------------------------------------------------------
January 31, 2005 - 21:12 : Boris Mann
You got me, Dries...
I have since spent time testing the module on andre's site, and it does
everything I can foresee needing at this point, with no functional
errors.
This functionality alone will make creating standalone sites that are
composed of mainly static pages very, very simple.
------------------------------------------------------------------------
February 3, 2005 - 10:58 : Dries
The demo-site appears to be down. Bummer.
------------------------------------------------------------------------
February 4, 2005 - 00:59 : andremolnar
Attachment: http://drupal.org/files/issues/book_module_9.patch (7.58 KB)
Sorry - demo site was down while database was being cleaned and latest
CVS modules were being installed (general site maintenence for
testing).
The site is up now with a fresh database and install of Drupal CVS.
UN drupaldev PW drupaldev (has access to manage blocks, books, and
menus to see this patch in action)
Attached is an updated patch to make use of db_rewrite_sql (instead of
node_rewrite_sql). Also fixes a minor bug that appeared if you were
creating the first book in the site.
andre
------------------------------------------------------------------------
February 4, 2005 - 01:13 : Anonymous
This patch seems to modify the blocks and menu tables directly from
within book.module. (in the new book_build_menu_module_menu function)
This sets a terrible precedent for other modules. IMO, these tables
should be modified by functions defined within the block module or menu
module (or menu.inc). Modifying the tables randomly throughout many
other points in the code makes changes to these tables difficult and
makes troubleshooting problems much trickier. Let's not sacrafice code
organization for the sake of making the patch as small as possible.
------------------------------------------------------------------------
February 4, 2005 - 08:25 : andremolnar
With regard to the blocks - the block module is doing all the block
building (with the call to _block_rehash) - the only thing that this
code does is preserve block settings (the select sql) before the menu
is updated - and then restores those block settings (with the update
sql).
"Modifying the tables randomly throughout...
"
While I appreciate the point you are making, I would hesitate to call
this random. After all the menu items being added to the menu table
'belong' to book module.
There is a little bit of a discussion about the best way to go about
doing this sort of thing (i.e. allowing modules to build their own
menus) over at http://drupal.org/node/15198 - which includes the patch
required to make this patch work. Right now there is no existing hook
or method of doing this. hook_menu does not have anything in place as
of yet.
I have suggested the introduction of a new hook (name yet to be
determined) that would be in charge of these menu_module_menu type
inserts. But, I haven't heard any feedback on whether that is a good
idea - or if anyone has a better idea.
Anyone else have thoughts on the approach taken here or suggestions for
a different/better approach? I'm willing to write the code, I would
just like a little bit of input.
andre
------------------------------------------------------------------------
February 4, 2005 - 08:46 : Dries
I added a test page and all book blocks dissapeared? I also got an SQL
error (check your watchdog messages) upon pressing the 'Submit' button.
------------------------------------------------------------------------
February 5, 2005 - 09:31 : Steven
Doesn't this patch still suffer from the inability to show the menu
block only on the relevant book(s)? I know we can now restrict blocks
per node type, but that still doesn't help with multiple books.
By the way, big code style violation:
$menu_block_settings[$menu_block->path][weight]
"weight" should be surrounded by quotes. Right now it is being treated
as an undefined constant. This is not a good idea. Other than that
there's a bunch of missing spaces... if the code looks cramped, it
usually violates the coding style.
+ $menu_block_settings[$result->path] =
array('status'=>$result->status, 'weight'=>$result->weight,
'region'=>$result->region, 'throttle'=>$result->throttle,
'visibility'=>$result->visibility, 'pages'=>$result->pages,
'types'=>$result->types);
+ db_query('DELETE FROM {menu} WHERE type=%d OR type=%d',
MENU_MODULE_MENU | MENU_MODIFIED_BY_MODULE, MENU_MODULE_ITEM |
MENU_MODIFIED_BY_MODULE);
//restore menu_module_menu block settings
There are several more examples. Please pay attention to this. It's
incredibly annoying to have to fix it later, and it makes the code
harder to read as you cannot make certain assumptions about how it is
layed out.
------------------------------------------------------------------------
February 6, 2005 - 00:56 : andremolnar
I am fixing up the code as we speak.
I actually made a bigger faux pas by not escaping my INSERT sql string
properly - (There is a quick fix, but I understand 'addslashes' is not
good enough for drupal ;-). (Thank you to Dries for catching this right
away on his test of the demo site).
As for multiple books - you actually have more flexibility to enable as
many book menus as you like - not only based on type but also by path.
e.g. Could allow for:
drupal.org/handbook (with some general book menu blocks enabled)
drupal.org/handbook/developer (with developer book menu blocks enabled)
drupal.org/handbook/developer/theme (with theme developer book menu
blocks enabled)
etc. (the sky is the limit).
I will resubmit a patch with all of the coding style fixes requested -
and I will be more careful with the coding style in the future.
andre
------------------------------------------------------------------------
February 6, 2005 - 02:05 : andremolnar
Attachment: http://drupal.org/files/issues/book_module_10.patch (7.7 KB)
Updated Patch,
Implemented code style improvements as per Steven.
Fixed unescaped sql INSERT string (turns out that addslashes is more
than enough in this situation, because the 'title' string has already
been valided and properly inserted into the database before any of this
code ever runs - i.e. this isn't coming directly from user input, but
rather from the drupal database).
Demo site is once again live and welcomes all testers (Thank you again
to those that have already tested this out).
andre
------------------------------------------------------------------------
March 14, 2005 - 00:11 : killes(a)www.drop.org
Still applies.
Andre: Please create patches from the Drupal root directory.
------------------------------------------------------------------------
March 15, 2005 - 12:42 : Paul Iliano
This is exactly the functionality I have been waiting for. As I can see
many uses for showing a book menu on all pages, I never really
understood why book menus could only be shown on book pages.
I'm still at DP 4.5, but as soon as 4.6 is out I will install and try
it out. I'd love to see this kind of functionality
in core!
------------------------------------------------------------------------
March 18, 2005 - 09:43 : Steven
Your visibility 'fix' requires that the entire handbook has clean,
hierarchical URL aliasing. This is often not the case.
I understand that for sites where the book is used to organise the main
structure, you might want the block visible everywhere. But it is not
good for sites where this isn't the case, like Drupal.org itself.
------------------------------------------------------------------------
April 3, 2005 - 19:12 : Paul Iliano
As Steven says, for some type of sites (like perhaps drupal.org) it
could not be desirable to show book menus on all pages. But for other
sites it could be very much wanted.
It seems to me a good solution would be to let administrators simply
choose which node types they want book menus to appear on, like we
currently can opt to switch off author/date info per node type.
------------------------------------------------------------------------
April 4, 2005 - 00:49 : factoryjoe
In general, +1 on the auto-updating aspect of this. It does seem to be a
welcome improvement to the book system, though I question Boris’
assertion that this is really a good solution for “brochure-ware”
sites.
For one thing, when I design a site, no matter who I am, I highly doubt
that I would think “ah ha, make a book to contain the pages of my
site... and the navigation will be generated from it!” While the
functionality is pretty much spot-on, the name is not. So what we end
up with is an overloaded book module or worse, a misnamed module.
I also wonder about moving the book-generated menus around. For
example, the way I would like this patch to work is for there to be
top-level “chapters” (like sections of a website) and then book
pages within each section, representing the individual pages. I would
like to be able to put the “chapters” horizontally in the header of
my site (like on OurMedia.org [1]) and then have the pages show up in
the sidebar when you navigate to each section. I guess I just wonder
about the flexibility of this approach and whether or not it will serve
the functional needs that Boris thinks it might. What if I want the page
titles to be different from the menu link titles? It doesn't seem that
this is something I can control with the current partial menu
integration.
Anyway, with a specific eyes towards improving book navigation, I do
agree that it functionally looks pretty good.
I did find one serious bug and one major usability issue. It has to do
with integration with the menu system. Andre will hate me for this, but
I clicked through the “reset menus” command in the menu system.
Doing so wiped out all the book navigation menus. If these menu items
were really “locked”, then why were the book menu items wiped out?
How do I restore them?
As for the usability issue, it is unclear to me where I would go
reorganize my book-generated navigation menus. I looked in menus, where
it should logically exist, but everything was locked. Does this mean I
have to go to each individual node to sort my content?
[1] http://www.ourmedia.org/
------------------------------------------------------------------------
April 6, 2005 - 01:24 : menesis
>From what i see on demo site, there is no way to configure the site to
act as it does now in case of multiple books - show book navigation
block only for current book. I could only configure a block to show up
on any book page, but it is shown while viewing other books, too.
IMHO the preferred way to modify a module is to introduce new
functionality while keeping it's behavior without any extra
configuration. In this case, when creating a new book, a menu would be
created, the block enabled, and configured to show up only on pages of
that book.
Visibility by path will not work, because all book pages have url like
node/1653 by default, need a block setting for that (true="Show on any
book's pages", false="Show only on current book's pages", false by
default). The tricky part is that menu module, which provides the block
and it's configuration, does not know which book is being displayed.
------------------------------------------------------------------------
April 17, 2005 - 23:38 : bobo
For those of us without linux, can someone please add the latest patch
to the Drupal 4.5 book.module. I would like to use it asap for a
website I'm creating for an animal welfare group. This is really cool,
I hope you guys find a way to get it put into the latest version of
Drupal. It should be a core peice of code. It seems strange to me that
an option to click wether or not you want a book to display on the main
page doesn't exist...
Bobo
------------------------------------------------------------------------
April 18, 2005 - 19:23 : bomarmonk
This functionality seems crucial to completing the organizational
website that I have developed with Drupal. Yet, I've applied the
book.module and menu.inc patches to Drupal 4.6 and can't seem to get by
book blocks to display on the front page. Why not? Is it because I'm
using the front_page.module? I'm still using a themed drupal page for
the front page. I definitely need help with this.
------------------------------------------------------------------------
April 20, 2005 - 19:45 : andremolnar
It occurs to me, as I finally dive back into this, that the main issue
is that people want BOTH the new way of handling blocks (because of
greater flexibility of putting a book block anywhere they like) AND the
old way (because people have sites where they only want to book block to
show up in the current book's nodes - at the exclusion of all other
books).
At some point in the patch history, both options were available by
simply leaving book.module's own block generation code intact AND
implementing the new code that inserted items into the menu system and
let menu.module handle block creation.
This was deemed to be bad. (The argument being that there shouldn't be
two sections of code generating blocks for one module. Essentially
duplicating each other's efforts). The good news is that in an attempt
to address the issue I added a welcome configuration option to the
block.module - but it wasn't enough to REALLY solve the problem.
As menesis pointed out, the problem is that when the block generation
is handed off to the menu module, the menu module doesn't have any way
of knowing that the blocks it is generating based on the menu structure
are 'books' - and hence there is no way to know that a particular book's
navigation block should only show up in a particular book - it simply
becomes a block like any other block and must follow the same rules.
Quite frankly I think people should just suck up the fact that in order
to move forward and offer improved functionality some other
functionality is deprecated. But, I suspect that that argument won't
fly in this case - its just too big a paradigm shift *wink*.
All kidding aside, I am going to try and find a solution. In the mean
time a REAL issue was raised by factoryjoe when he noticed the blocks
could be wiped out simply by 'resetting' menu's in the menu admin.
This is NOT desired and should be reasonably easy to fix by telling
menu module not to dump menu's generated by modules during a 'reset'.
{And no joe I didn't mind you messing up the blocks on the demo site -
it raised a very valid problem}.
If anyone is interested I am going to be placing pre-patched files in
my sandbox - so if people like the new functionality despite losing
traditional book block functionality they can just download the
pre-patched files. And if people want to monkey with the code and
contribute solutions there is a place to do it.
http://cvs.drupal.org/viewcvs/drupal/contributions/sandbox/andremolnar/book…
More to come.
andre
------------------------------------------------------------------------
April 28, 2005 - 17:43 : bomarmonk
Andre,
I really appreciate the work you and others have put into making the
book module and menus/blocks better. I am fairly new to PHP, mySQL,
Drupal etc., but I would like to offer some suggestions anyway (even if
they are a bit naive). I have tested the patched versions of menu.inc
and book module and would like to offer my support for the following
added functionality:
1) Have support to allow or disallow the root page of the book in the
navigation block (this would definitely be desireable for many using
book blocks for hierarchal menus).
a) However-- another desired (more useful?) behavior might be to
not display the currently viewed
book page whether it is the root page, second page, third
page, etc. Food for thought.
2) Allow individual book blocks to behave in the traditional manner, or
to enable certain book blocks to show up on every page (as site
navigation).
3) This feature would also be complemented by the ability to show
navigation blocks for sections or chapters within a larger book. This
way, you could create a "master" book with a larger table of contents
(providing an index of book titles) and still allow concise, focused
navigation within the chapter nav blocks.
Again, I am sorry If I've repeated anything I've already said in
another post (a little double posting), but I felt this was the best
place to sum up some of the functionality I feel is currently missing
from the book module. Since I am a novice, feel free to correct me or
give me verbal lashings. I can take it.
------------------------------------------------------------------------
April 29, 2005 - 17:29 : mcduarte2000
I also have one desire...
I wanted an option to expand completly the menu block to the 2nd or 3rd
level. Not only the section the user is currently in, but all the
sections.
This would be great to be able to use it as well as site navigation.
And I believe is not difficult to implement.
Regards,
Miguel
1
0