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
June 2005
- 69 participants
- 404 discussions
Just how many people with this problem be affecting?
I am on Windows, so is my web host, and consider myself to be not-so-tech-savvy, and even I don't use FAT. It'd be interesting to see how many people still use FAT, as the only reasons I can see for doing that is:
1. Not knowing any better. Maybe they think FAT is better than NTFS as it is "PHAT"?
2. Ability to access the files through DOS. Even for this there is alternative solutions, such as NTFSPro.
K.
>>> karoly(a)negyesi.net 6/27/2005 3:04 PM >>>
On Monday 27 June 2005 15:00, David Norman wrote:
> ...and what about the people using FAT? Just tell them to use a real OS
> for load?
Is this a question for real?
1
0
Hi,
Since the bluemarine phptemplate theme is now in core, I would like to
know the best method of removing the cvs version from contributions. I
still want the older releases in cvs but the HEAD version I want to stop
from being packaged every night.
Can someone please enlighten me.
Thanks in advance.
Gordon.
2
5
[drupal-devel] [feature] Enable multiple block regions (not just "left" and "right" sidebars)
by deekayen 27 Jun '05
by deekayen 27 Jun '05
27 Jun '05
Issue status update for http://drupal.org/node/16216
Project: Drupal
Version: cvs
Component: block.module
Category: feature requests
Priority: normal
Assigned to: Anonymous
Reported by: paragkenia
Updated by: deekayen
Status: patch
Disclaimer: I offer no code for this, so this is just a bunch of
idealism I can't back up.
djnz's comment makes sense if you want quick and dirty, but it seems to
me the correct way to do things would be in terms of relatives. I would
think doing it right the first time would be easier than redoing block
location code twice. For example set a standard starting location;
perhaps the start is 100% height/width conceptually. If you want to put
something on the right, it would be the first block on the right of the
starting block - let the theme manage widths. Then if you want another
block on the right, or 10, just keep adding them relative to the others
kind of like the current ordering of book pages or like adding more TD
cells in a HTML table.
Using this example under the current standard layout, the main block
could be in the middle with with relative blocks on the left and right.
deekayen
Previous comments:
------------------------------------------------------------------------
January 26, 2005 - 04:27 : paragkenia
I read the comparision discussion between *Drupal* and *Mambo*. In
several messages it was outlined that Drupal can place blocks only in
right and left and not flexible to put them on anywhere where one want.
It will be great if this can be changed in upcoming versions.
I am no pro at PHP, so don't know how much time this task will take,
but I think it is very important.
parag
------------------------------------------------------------------------
April 14, 2005 - 19:44 : nedjo
This issue was apparently partially addressed in issue
http://drupal.org/node/19694 [1].
[1] http://drupal.org/node/19694
------------------------------------------------------------------------
April 16, 2005 - 18:24 : nedjo
Attachment: http://drupal.org/files/issues/block-dynamic-regions.patch (8.17 KB)
This much-requested functionality - to have the ability to place blocks
in more than the two predefined regions - was partially addressed in
issue http://drupal.org/node/19694. [2]. But "blocks" are still
limited to the "left" and "right" sidebars (hard-coded in
block.module).
This patch is a first step designed to enable multiple (eventually,
admin-definable) regions for blocks. I've moved the existing "left"
and "right" block regions to a 'region' table (with ids of 0 and 1, as
currently used in themes). Then all references to the regions are
drawn dynamically from the table. This way, if further records are
added, they will appear in the list of available regions for block
placement.
Doing this actually reduces some duplicated code, since it's no longer
necessary to repeat code blocks for each of "left" and "right".
As it stands, the patch doesn't add any new functionality--but I don't
think it breaks anything either. New functionality would need (a) new
regions defined, and (b) changes to themes. A simple first step might
be, e.g., to add a "footer" region and then add a call in the footer
generation to append any blocks assigned to the footer region there.
I'm setting this to patch, but I'm aware that it needs some discussion
and refining before it'll be ready to apply.
[2] http://drupal.org/node/19694.
------------------------------------------------------------------------
April 16, 2005 - 19:05 : nedjo
Attachment: http://drupal.org/files/issues/block_regions.png (5.65 KB)
Here's a screenshot showing the block admin page, with drop-downs for
region placement (the options are dynamically generated based on
defined regions).
------------------------------------------------------------------------
April 16, 2005 - 20:57 : adrian
The biggest problem with this is that you can have multiple themes, and
each of these themes can have different regions available.
Also, the method of defining which regions are available needs to be
standardised. Some of the work that me and Vlado are working on for the
install system would go towards solving that problem (ie: meta
information for modules, themes and styles).
This has been discussed to death, but the general consensus has been
that we _want_ to do this, but we need to solve a few other problems
properly first, the most pertinent being the interface issue. Chris
(factoryjoe) recently did a whole mess of workflows for something
related to this.
------------------------------------------------------------------------
April 16, 2005 - 22:09 : syscrusher
The first paragraph of adrian [3]'s post is a point that occurred to me
also, as I read this thread.
One suggestion would be to separate the definition and configuration of
blocks, on one hand, from the placement of those blocks, on the other
hand.
In other words, Drupal core provides a mechanism defining what blocks
exist, which of these are on by default or off by default and
user-selectable vs. which are forced on for all users, and the
configuration (if applicable) of specialized blocks defined by
particular modules.
Each theme provides a standard hook function that returns an array of
region names and help/description text, e.g., array('left'=>t('This
vertical region is left of the main content area'), 'right'=>t('This
vertical region is right of the main content area'), 'footer'=>t('The
footer is below the left, right, and main content areas of the page')).
The theme part of Drupal core (i.e., theme.module itself, not the
individual themes) provides a standard UI that is displayed within
config of *each theme* (but is one physical code base within
theme.module) that allows the administrator to map blocks defined by
Drupal core into regions defined by the theme, and storing that mapping
as an theme-to-block_ID-to-region_ID (with weight) table in the
database. From there, the actual page rendering is similar to what's
being done now, but there is more of it.
I guess what I'm trying to say is that the key to solving this problem
is breaking it along its degrees of orthogonality, and there are three
-- two in Drupal core and one in the individual theme.
Scott
[3] http://drupal.org//user/1517
------------------------------------------------------------------------
April 17, 2005 - 01:14 : Dries
Scott, is right. The theme should export its available regions. Then
the administrator's task is to assign blocks to regions (not to setup
regions). A theme could have configurable regions, but that internal
to the theme.
To me, the real challenge is the UI and the interaction design.
Configuring blocks could easily become a real pain (while most themes
continue to use 'left' and 'right'.)
Of course, we can implement all the functionality, default everything
to 'left' and 'right', and worry about the UI later. This should be
fairly straightforward to implement and nedjo's patch looks like a
first step in the right direction.
------------------------------------------------------------------------
April 17, 2005 - 15:09 : nedjo
I agree with the suggested approach of themes registering their regions.
I'd see this happening when a particular theme is activiated (or
upgraded). Regions would be an array of names (e.g., 'left', 'right',
'footer'). New records would be created in the region table only for
region names not previously registered.
Some areas I'm not clear on, or that need further work:
* Table naming
Should a new table be 'region' or 'regions' (I've used 'region')? I
don't see a convention among existing table names, some of which are
singular and others plural.
* Default values
I've kept the existing '0' and '1' ids for regions (left and right,
respectively), for backward compatibility. But this means we can't use
autoincrement for the region id (rid), since autoincrements seem to
begin with 1. Likely we should change to autogenerated ids.
* Initial records
Using the INSERT INTO statements as I've done for the initial region
entries is probably counterproductive, since sooner or later they'll
have to be dynamically generated. I was hoping this could be a
preliminary patch, with the main work coming later, but likely we need
to solve region registration by themes before this patch is applied. I
don't have a good idea of how region registration by themes would be
implemented (a hook on theme activation?), and invite suggestions or
implementations.
* Theme system changes
Besides region registration, the other change I'm seeing that would be
needed in the theme system is loading blocks by region name, rather
than region id. This is because the ids currently used ('0' and '1')
in theory might be diffferent on a particular install.
------------------------------------------------------------------------
April 17, 2005 - 15:18 : syscrusher
Instead of having themes "register" their regions, why not just add a
theme API hook called them_regions() that returns an associative array
of $region_name=>t($region_helptext)? This would be in keeping with
other similar API functions, such as those that return what permissions
apply to a module or what node types are defined by a module. Most
themes are going to define only a small number of regions (two being
the typical case now, but I could see five or six in a complex theme),
so returning a constant array will be faster than querying SQL to
obtain an array of rows.
There could be (optionally, at the discretion of whoever builds this
thing) another API hook like theme_region_properties($region_name) that
returns an associative array with detailed info for a given region, such
as extended help text with recommended usage instructions for the
region.
Scott
------------------------------------------------------------------------
April 17, 2005 - 15:19 : syscrusher
s/them_regions/theme_regions/g
------------------------------------------------------------------------
April 17, 2005 - 16:47 : adrian
Because. The most common 'theme' is a phptemplate theme.
And there needs to be a generic method for specifying the regions
available, in a non hook_function format..
Themes get their names from the directory in which they are contained..
when copying the theme to another directory, there must be _no_
modifications necessary to allow normal usage. This is one of the
tenets of the new template system I designed.
You would need a standard way to define meta-information for themes,
that does not need to be modified when copied to a new directory. We
are working on this in the install system work, as you need a way
external of Drupal to define the module dependencies and some other
things.
My approach would be to add a theme.dsc text file to the directory,
which would allow you to specify meta-information. For instance :
----
regions: left, right, footer, center, mountie
author: Johnny McAngstyPants
----
------------------------------------------------------------------------
April 17, 2005 - 20:00 : syscrusher
Adrian wrote:
"
Themes get their names from the directory in which they are contained..
when copying the theme to another directory, there must be _no_
modifications necessary to allow normal usage. This is one of the
tenets of the new template system I designed.
"
A very good point. But region names don't have to be unique across
different themes, so my hook function wouldn't have to be modified. The
suggestion I made for the mapping metadata was three factors: theme ID
or name, region ID or name, and block ID (plus one non-key weight value
to order the blocks within a region, but this is not relevant here).
I'm not familiar enough with PHP template to be able to respond to your
comments on that one, so if you say it's not feasible to work with my
schema, then I'll take your word for it. :-)
Scott
------------------------------------------------------------------------
April 17, 2005 - 20:03 : syscrusher
Oh, wait.....I see now what you mean! It's not the output of the
function that is the problem, but the *name* of the function.
Never mind. I concede your point.
Scott
------------------------------------------------------------------------
April 18, 2005 - 01:44 : Jaza
I'm probably jumping ahead a bit here... but something that this patch
will have to eventually account for, is how to allow regions to be
defined as inline. Allowing a theme to define a region's position as
being anywhere on the edge of the page (i.e. top, left, right, bottom,
corners, etc) is relatively easy. But what about having a region that's
in the middle of a node's body, or somewhere else that's not the edge of
the page?
What I'm thinking of, is eventually allowing the custom region system
to integrate with the CCK. Like I said, I admit that I'm jumping ahead
into the future - the CCK still has a long way to go before it's ready.
But ultimately, it would be cool if a theme could define a region as
being after, say, the "rating" field in nodes of type "movie review".
This would make the region (and the blocks in it) truly inline and in
context with the actual content of the node.
This would be a huge step forward compared to the current block system,
which doesn't allow you to mix the presentation of blocks and nodes at
all. In Drupal ATM, blocks and "the rest of the page" are totally
separate, and really you have no choice but to display them as such.
The result of this is that information in blocks that really should be
displayed as part of a node, gets literally "pushed to the side", and
always seems secondary to the actual content. Often the block has
content that is just as important as the content of the node itself.
Another option (of less merit, IMO - because of the extra maintenance
work, and lack of enforced consistency) would be to have a region
filter. You could then choose where to place an inline region on a
per-node basis, by entering text such as [region:2] (example based on
image_filter syntax) into the node body.
Anyway, just thought I'd throw that idea into the open. I don't expect
it will be implemented any time soon, but it's something to think
about.
Jeremy
------------------------------------------------------------------------
April 18, 2005 - 02:54 : stefan nagtegaal
Attachment: http://drupal.org/files/issues/regions---possibility-1.png (1.29 KB)
Imo the 'regions' are more than you guys name here.. FOr example I
attached 2 screens which only shows you the regions.. What I think is
that it should always be possible to have free names for the regions.
No matter if i call - the region where my content appears in -
'content' or 'site items'..
See attached screens..
------------------------------------------------------------------------
April 18, 2005 - 02:56 : stefan nagtegaal
Attachment: http://drupal.org/files/issues/regions---possibility-2.png (1.39 KB)
And the second...
See these links:
- http://drupal.org/files/issues/regions---possibility-1.png
- http://drupal.org/files/issues/regions---possibility-2.png
------------------------------------------------------------------------
April 18, 2005 - 09:22 : nedjo
Attachment: http://drupal.org/files/issues/block-dynamic-regions2.patch (13.39 KB)
Here's a revised patch implementing some of the ideas so far. I've used
the theme engine - rather than individual themes - to generate the list
of available regions. I know this isn't ideal, but e.g. for xtemplate
it's at present the only option--no logic is possible in the individual
theme.
region is changed to a varchar field from the current tinyint.
I've roughed in several regions--really just for demo purposes. More
thought and work would be needed to refine them (e.g., styling, etc.).
But this maybe moves the conversation forward.
------------------------------------------------------------------------
April 18, 2005 - 15:26 : Dries
This patch enables different regions to be used. That is a good thing.
The next question is: should we take into account that different themes
could assign blocks to different regions? Lots of problems would be
solved if (i) there could only be one active theme on a website or (ii)
if all themes would have a common set of regions.
(This "let users select their own theme"-thing is a left-over from the
early days, except for WAP stuff maybe.)
------------------------------------------------------------------------
April 18, 2005 - 15:37 : killes(a)www.drop.org
Giving up multiple theme support would be a great thing. People could
still select from multiple style sheets. WAP shoud not be a user
preference setting, but automatically detected and redirected to a
special site with a WAP theme.
------------------------------------------------------------------------
April 18, 2005 - 15:48 : adrian
You would then lose the ability to make a special front page theme for
use with the sections module, for instance.
Also, currently styles exist in the same namespace as themes, and we
would have to make more a distinction of styles vs templates (or
themes), but I do think this could help simplify things.
------------------------------------------------------------------------
April 21, 2005 - 10:54 : nedjo
Attachment: http://drupal.org/files/issues/block-dynamic-regions3.patch (15.69 KB)
The main issue raised, if I'm understanding well: if a theme or theme
engine is present that offers regions other than those available in the
default theme, any blocks assigned to those regions will be invisible to
most users.
Here's a revised patch addressing the concern. I've added a test for
each region to see whether it's also available in the current default
theme. (Actually, I'm realizing as I write this, I've used the theme
engine--so it would need to be updated to test first if an engine is
being used.)
In any case, any regions not available by default are starred, and a
message text appears (only if necessary).
If there's support for the approach, I'd be happy to finish the patch.
Remaining work:
* extend to work with non-engine themes
* finalize base set of regions to implement
* implement common region set in all core themes (for now I've only
done xtemplate)
* work on CSS display (how blocks in each region should look)
This last point I'm not too confident on and would look for
help/suggestions.
Screenshot to come.
------------------------------------------------------------------------
April 21, 2005 - 10:56 : nedjo
Attachment: http://drupal.org/files/issues/block_regions_message.png (12.34 KB)
Screenshot showing starring of non-default regions plus message.
------------------------------------------------------------------------
April 21, 2005 - 11:19 : killes(a)www.drop.org
I like that approach. +1
------------------------------------------------------------------------
April 21, 2005 - 11:23 : resmini
My two cents.
If this has to be done (and it should), better get rid of spatial
definitions for areas such as left, right, top, bottom, etc.
Use semantic names related to content and let the theme take care of
their positioning interely.
Also note that there in an ongoing debate in IA trying to build up a
standard naming convention for page regions in a way that is
semantically (and logically) correct. Just for the sake of it, you can
check
http://www.stuffandnonsense.co.uk/archives/whats_in_a_name.html
http://www.stuffandnonsense.co.uk/archives/whats_in_a_name_pt2.html
with follow-ups, including Eric Meyer's. There are more substantial
contributes in the AIfIA web site, but I can't seem to find them now.
I'll post a link or an abstract if I manage to.
--
vector at exea dot it
------------------------------------------------------------------------
April 21, 2005 - 12:15 : nedjo
Good points, useful references. The default regions I'm thinking of
are:
'banner' => t('banner'),
'left' => t('left sidebar'),
'right' => t('right sidebar'),
'body_top' => t('body top'),
'body_bottom' => t('body bottom'),
'footer' => t('footer')
Perhaps "body" would be better as "content"? It would be easy enough
to add in more regions, e.g., within the content (after title, after
help, etc.) One issue is that many of these regions will already have
rendered content. Should the blocks come before or after that other
content? I've assumed after for banner and footer, while giving both
options in body.
In any case, refinements or suggestions would be great.
------------------------------------------------------------------------
April 21, 2005 - 12:56 : resmini
Well,
'banner' => t('banner'),
'left' => t('left sidebar'),
'right' => t('right sidebar'),
'body_top' => t('body top'),
'body_bottom' => t('body bottom'),
'footer' => t('footer')
The classic (as far as classic goes for something Web related) scheme
comprises 5 spatial regions, much like stefan's
http://drupal.org/files/issues/regions---possibility-1.png with a left
area mirroring the one called 'blocks'. Call them top, left, center,
right, bottom.
Variants usually exclude one or more of these, layout-wise.
If we talk content (semantic) instead, things get a bit more
complicated, as these spatial regions normally include more than one
content-area. The top, for example, could include ad banners and the
site's header, or the main menu. But these merge seamlessly with CSS
definitions, which are definitely something that should get properly
standardized (most of the errors / inconsistencies I'm encountering
with Drupal today are related to this), but this is out of scope in
this thread.
What I somewhat object to the definitions above is that it mixes the
two approaches.
'banner' is not spatial, is content, as it is the distinction between
body top and bottom. If 'banner' goes there, it is 'top', whatever that
might be. I designed a couple of web sites which had
headers/logos/banners at the bottom of the page, and I'm not exactly
the wildest designer you'll probably meet.
And if we include body_top and _bottom, we should also add
'navigation', or 'menu' and some other content-related semantic region
(impressum, or company_data, or whatever).
I suggest that either we go along
'top' => t('banner'),
'left' => t('left sidebar'),
'center' => t('body'),
'right' => t('right sidebar'),
'bottom' => t('footer')
or we dedicate some time to detailing what could possibly fit there
semantically, which is perfectly possible without limiting design
freedom or whatever, since the vocabulary is not infinite, but not
something you do out of the blue.
Please note that I understand t('something') to be an example, since
such a layout only defines regions on the page, not semantic regions.
My main content could fit nicely in the bottom region.
As for the names for the regions, they are not important, as much as
they are coherent and enforced.
------------------------------------------------------------------------
April 21, 2005 - 15:16 : stefan nagtegaal
Attachment: http://drupal.org/files/issues/regions-final.png (2.39 KB)
Maybe something like this???
But, I have to say that I _really_ liked the solution to split the
'content'-region into the 'content' and 'beneath content'.. I love
that!
------------------------------------------------------------------------
April 21, 2005 - 16:20 : resmini
Stefan, yes.
That's basically what you can build if you stick with spatial, and this
layout can generate quite an amount of (sane) layout variations even if
you interpret it strictly (suppose a default) as
<div id="header">
</div>
<div id="left-bar">
</div>
<div id="content">
</div>
<div id="right-bar">
</div>
<div id="footer">
</div>
What you can do with this is left basically to the theme designers and
their use of CSS, CSS-P or tables (brrr), but this could be an easy and
proper way to identify macro-regions in which more accurate positioning
happens at a more specific level.
I'm all in favour of subsequent identification of sub-areas (top and
bottom in body), but I don't think it would be a good idea to mix the
different approaches I mentioned in my previous post.
A site with lots of things going on or which is not just blogging
surely has the need to use its content area in a wiser manner than just
putting there 'content', the number of modules doing precisely this is
more than proof, but that falls more in a 'Drupal definitive guide to
semantics', in which CSS selectors are used or suggested strategically
for such a scope.
>From this point of view and for the sake of this thread, any unique
area / div (#name) could (should?) be a target for blocks, and this in
my opinion (*) would be the perfect solution, but I do not know if and
how this is even remotely possible in the current Drupal.
(*) It's quite late, local time, and it was a looong working day.
------------------------------------------------------------------------
April 21, 2005 - 16:38 : resmini
Attachment: http://drupal.org/files/issues/regions-final-variation.png (1.9 KB)
Just to be clear, I add a variation of stefan's scheme that shows how
this is *not* by any means positional, but spatial, or better
relational. Use your CSS imagination to take out regions, reduce them
to empty containers, enlarge them, make them taller, shorter, whatever.
5 regions, do what you like.
------------------------------------------------------------------------
April 21, 2005 - 16:44 : resmini
Sorry, I forgot.
And this is also why I'm not too keen on calling anything 'content'. If
I enlarge my right region enough, why not put my content there?
Shouldn't this be a theme-level concern?
It's nothing serious, but if my center area is called 'center' (or
something similar and more sexy), I won't confuse anybody.
This is left, center, right: do with them what you prefer. Content in
the bottom? Show me.
If I call something 'content', I'm more than suggesting that content
goes there, and only there.
------------------------------------------------------------------------
April 22, 2005 - 02:17 : stefan nagtegaal
Resmini, maybe it is only me but I think that you are making a mistake
in your way of thinking..
I get the feeling that you're mixing the 'regions' and 'divs'.. the
'regioins' should have meaningfull sentences, but the divs
shouldn'thave..
As an example:
$region_name => $content_body
here we go:
'header' => $logo
'left sidebar' => theme('blocks', 'left')
'right sidebar' => theme('blocks', 'right')
'content' => $content
'footer' => $footer.
So, $region_name does not have to be equal to the id or class of the
divs..
------------------------------------------------------------------------
April 22, 2005 - 03:27 : resmini
Stefan,
>So, $region_name does not have to be equal to the id or class of the
divs..
Yes, absolutely. I probably didn't make myself very clear: what I'm
trying to point out is that in
'header' => $logo
'left sidebar' => theme('blocks', 'left')
'right sidebar' => theme('blocks', 'right')
'content' => $content
'footer' => $footer.
there is no reason at all to call 'content' the region that holds
$content.
nedjo asked in #24
'body_top' => t('body top'),
'body_bottom' => t('body bottom'),
if /Perhaps "body" would be better as "content"?/ and all my reasoning
is simply that no, that is not content, or it shouldn't be (or better:
it may happen that main content is elsewhere). As a matter of fact,
even the connotation 'sidebar' doesn't fit very well logically.
To make it short: Today's method works, but has limits. Mambo, which
paragkenia quotes at the beginning, handles more 'user regions' but in
pure Mambo style conventions are so loose that they are hardly useful.
If we define regions, as in nedjo's example, in my opinion these
regions shouldn't imply informational layouting, only relational
positioning (that is, where they stay in the browser viewport).
I know it may look like overdesigning, but Drupal constantly enforces
conventions and coding guidelines everywhere: this is one of the best
aspects of the project and I think it should carry to the layout.
------------------------------------------------------------------------
April 22, 2005 - 03:32 : stefan nagtegaal
While I think that 'body_*' is not as self explaining as 'content_*' I
vote for using 'content_*'..
------------------------------------------------------------------------
April 25, 2005 - 11:48 : chx
Let's get back to the code. If my understanding is correct the big
discussion is about how to name potiental rectangular areas _after_
this is commited. So, first get this committed. What's need to make
that happen?
------------------------------------------------------------------------
April 25, 2005 - 12:56 : chx
I spoke with Moshe, Steven, JonBob and the result is: keys shall be
numbers, and themes shall define the values. We will make region zero
and one required. There will be a define('REGION_CONTENT', 0) and a
define('REGION_SIDEBAR', 1); so you can have readable code still.
Either wait for me to code it or feel to do it yourself, just drop me a
mail via the contact form if you are doing it.
------------------------------------------------------------------------
April 27, 2005 - 10:14 : nedjo
Károly, it's great if you're willing to take on finishing this. I
tried to summarize remaining work in update 20, above [4].
The numerical keys look fine to me.
In terms of an initial default set of regions, one quirk is this: for
now, so far as I can see (and assuming the approach I sketched in),
available regions can be defined only in (a) theme engines and (b)
individual themes that don't use an engine. So, for instance, an
individual xtemplate based theme can't define its own custom regions;
they need to be defined in the theme engine. (Unless and until the
whole theme system should switch to the sort of configuration file
adrian suggested above [5].)
Which is to say that, under this approach, we'll be defining a set of
regions for each engine, and then all themes using that engine should
try to implement those regions. (Otherwise, if a site admin selects a
region defined in the engine but not implemented in the default theme,
the content won't appear.)
Please let me know if there's anything in what I've done that isn't
clear, or if you want to pass some or all of this back.
Thanks!
[4] http://www.drupal.org/node/16216#comment-27398
[5] http://www.drupal.org/node/16216#comment-27052
------------------------------------------------------------------------
May 9, 2005 - 14:36 : chx
I had a "short" discussion with the UI team leader ie. Ber Kessels. The
outcoming is that we will change hook_block so that it uses callbacks a
bit like menu system. Here's an example:
<?php
function module_block() {
$items = array();
$items['module_header'] = array('callback' => 'mymodule_page',
'weight' => -50, 'region' => 'content' , 'cache' => TRUE, 'pages' =>
"node/*\n<front>");
return $items;
}
?>
Please note that we are allowing caching the themed output of each
block independently.
The theme system will execute hook_block as it does now and it'll
execute the callbacks defined in module_block as well. Of course if the
callback is cacheable and in the cache, it'll print it from cache. If
it's themeable and not in the cache, it'll cache it before printing.
Pages contains a string (could be PHP code!) which can be overriden in
the block configuration screen.
Callbacks defined in hook_menu will have lot less functionality after
this, they will be used for non-themed pages mainly.
------------------------------------------------------------------------
May 9, 2005 - 14:45 : chx
More explanation is requested for the example.
module_header is the unqiue name of the block.
pages define on which pages the block is visible. ('pages' in the block
configuration screen.) Maybe I'll implement 'visibility' from the same
screen as well.
callback, weight are well known Drupal terms.
cache is a boolean which defines whether given output is cacheable or
not.
------------------------------------------------------------------------
May 9, 2005 - 14:47 : chx
Ber, Dries, do not kill me! Ber chaired the usability meeting in
Antwerp, that's all.
------------------------------------------------------------------------
May 9, 2005 - 14:52 : Bèr Kessels
Attachment: http://drupal.org/files/issues/more_regions_principle.png (123.29 KB)
attached is a schematic which might clarify this a little.
And I would also like to comment on me being the UI leader: I do not
consider myself any leader, no ofeence, though. Just to clarify that.
------------------------------------------------------------------------
May 9, 2005 - 16:42 : nedjo
I am not opposed to the idea of changing the _block() hook to use
callbacks (I have no opinion on it, not immediately grasping the need),
but this seems quite distinct from the aim of enabling multiple and
extensible regions. Why can't we just move ahead with the block
generalization?
I suggest:
* quickly implement the proposed block region generalization, using
only the two existing regions (i.e., implementing them in core themes)
* later worry about:
- adding further regions (the details of which are best done by those
primarily responsible for the particular themes)
- possible changes to the _block() hook.
Wouldn't this make sense? Or am I missing some pressing reason for
which the block generalization has to wait?
------------------------------------------------------------------------
May 9, 2005 - 19:28 : killes(a)www.drop.org
"Or am I missing some pressing reason for which the block generalization
has to wait?"
I might be missing something too, but I thought Chx was just talking
about the block generalization and proposing a way to code it.
------------------------------------------------------------------------
May 9, 2005 - 20:41 : factoryjoe
I wanted to chime in with this discussion because it's an important one
and has some really huge implications for Drupal moving forward. That
said, I also have a day job pulling all my attention which means that
the big ideas I have for this problem can't be put into a complete
visual form for at least a week or more. In the meantime, I'd like to
offer a few thoughts on the matter.
* *Regions do not belong in themes.* Blocks were originally separate
from themes for a reason. Themes should define the general look and
feel of a site -- and to some extent a site's behavior. Layouts which
define the spatial relationship between regions belong in some
intermediary between style and content, which does not currently exist
in Drupal.
* I would propose that *layouts become a new kind of CCK tool.* By that
I mean that they can be defined, shared and deployed very easily -- even
as simply as tossing a directory of some .layout textfiles into a
layouts/ directory. Note that this suggestion has nothing to do with
the way CCK /might actually/ work right now; instead I'm suggesting how
I would like it to work. I would like, for example, to create a CCK item
like a "person" and ship it with a .layout file that carries all the
semantic goodness needed to output that "person" to the screen in
juicy, semantic markup... which anyone can come along and theme to
their heart's content.
* *Creating shareable layouts should be relatively painless.* Dashboard
[6] sets the bar for easy hackability. It should be that easy to create
layouts for Drupal... I can imagine creating an "iTunes layout" or an
"Image Gallery" layout and so on... If we used something akin to PHPTal
to develop the layouts, we could keep development fairly standard while
making it possible for more folks to hack up cool and innovative
layouts.
* *Layouts should be extensible.. to a degree.* I see a huge problem
with how this discussion has floated back and forth between semantic
and spatial region naming. Because we talk about this problem in those
terms, we fail to identify the real problem and opportunities presented
to us. Here's the thing: when you predefine regions, you limit
creativity and the ability of people to really use the system. On the
other hand, when you make a system too flexibile, it quickly becomes
unwieldy. In all the mockups presented above, both classes of problems
would arise. Therefore we need to think about a way to stay flexibile
while "taming the beast."
* To that end, I suggest developing *self-contained layouts that are
contextually-complete.* So for the example of the iTunes search
layout... you would know before hand that you get a couple multiple
select boxes that you can hook up to whatever taxonomies you want to
progressively filter [nodes]. You could even change the .layout file,
allowing you to theme it to your liking -- even though that would
totally optional, given the layout's internal default styling. Once
you've got your layout, you would go into a layout module UI [7],
create a new layout [8], select the layout from a list [9] and then
give it a path so you can access the layout... You would then walk
through a wizard [10] that would help you populate that layout with all
kinds of nice Drupally goodies... feeds, node listings, PHP code, etc.
The power in this approach is multifold. For one, it snaps Drupal out
of its block mentality and allows designers to really innovate with
individual page designs. In fact, I've already thought out how I could
reimplement Flickr using this system, combined with Drupal's native
feeds, taxonomy and menu module, and I'd barely break a sweat... sort
of. ;)
But anyway, this is a very productive conversation to be having, but I
really think that we should consider whether the old paradigm, as such,
really pushes Drupal as far forward as we have the opportunity to go.
[6] http://drupal.org/apple.com/macosx/theater/dashboard.html
[7] http://photos4.flickr.com/6944826_90af237c99_o.png
[8] http://www.factorycity.net/demos/civicspace/layout/new.html
[9] http://www.factorycity.net/demos/civicspace/layout/new_layout.html
[10]
http://www.factorycity.net/demos/civicspace/layout/testing_add.html
------------------------------------------------------------------------
May 9, 2005 - 22:34 : chx
While factoryjoe may be right, I could be right as well.
Regions has definitely something to do with themes, after all it's the
theme that renders the page, and it can only render regions that it
knows of.
So, instead of postponing this into the far future when someone comes
out with a layout system, I code this now.
------------------------------------------------------------------------
May 9, 2005 - 23:22 : Dries
The proposed 'weight', 'region' and 'pages' would be configurable, not?
Are these just defaults? The 'callback' and 'cache' options are nice to
have.
------------------------------------------------------------------------
May 9, 2005 - 23:25 : chx
Yes, blocks can provide a default for everything on the block and and
configuration screen. Of course, you can override these in the UI.
------------------------------------------------------------------------
May 10, 2005 - 03:02 : Bèr Kessels
Dries: yes, the region, weight and pages will be configurable from an
admin interface. What we tried to address here, is a method for
developers to choose good defaults. Take for example "logo". that
should be enabled by default in a regions called header, rendered on
all pages, and have a very light weight. We should not force an admin
to visit the blocks admin before she can see or use the logo.
Chris: I do not know if you have ever tried typo3. Its concept is some
form of builder system. I do not like thar, because far too complex. If
we ever get sme kind of builder in core (if ever) it will require a lot
of work, and even more usability research. So for now such a builder is
far out of our reach. Lets aim at goals that are more realistic ;) So:
it does belong in themes, its the best place that is still realistic.
I do like your visions, Chris, but I hope you agree they are not
realistic, yet. So, maybe you can think of a path towards your visions,
ow we can reach such a builder-system, you describe?
------------------------------------------------------------------------
May 10, 2005 - 08:52 : factoryjoe
My browser crashed before I was able to submit my followup to chx's
comment... I have no problem with the current work going forward and am
happy to see it happening. I know that my ideas are typically
outlandlish and far off... That's the kind of design I enjoy doing!
In any case, I see chx's solution being a means to an end; once we have
abstracted the region code, it become much easier for me to get some
help creating my vision.
In the meantime, and as a demonstration of how I see this layout system
working (to answer, I hope, Ber's concerns about clunkiness), I've
created a workflow movie that shows off how fast you'd be able to add
content to the system.
Check it out:
http://factorycity.net/demos/civicspace/layout/media/layout_workflow_v1.mov
Feedback appreciated. And, if you want to try the demo out yourself
(it's all hardcoded, like a "movie set" for my UI), go here:
http://factorycity.net/demos/civicspace/layout/
------------------------------------------------------------------------
May 12, 2005 - 10:45 : nedjo
Attachment: http://drupal.org/files/issues/block-dynamic-regions4.patch (13.56 KB)
"I might be missing something too, but I thought Chx was just talking
about the block generalization and proposing a way to code it.
"
Well, if I can say so respectfully, I don't think so. Not so far as I
can see identifying problems with my original patch, this interesting
discussion is rather addressing the (useful, but logically subsequent)
question "once we have multiple regions, how should we work with
them?". It's perhaps demonstrating our propensity to try to do more
rather than less with a given patch--to talk for a long time when a
first step solution is already in place. So here is a (yes, interim!)
small, focused, and tested patch to remove the hard-coded regions.
I've stripped it down so it does nothing more than (a) make block
regions declared by themes rather than hard-coded and (b) implement the
existing left and right regions in existing core themes. Please, try it
out, I think you'll like it. I'm of the (I'm hoping not scandalous)
opinion that it can be applied right away, and so clear the way for
further progress. In particular, this patch leaves the adding and
configuring of additional regions to theme authors/maintainers, who are
best qualified for the work.
"I spoke with Moshe, Steven, JonBob and the result is: keys shall be
numbers, and themes shall define the values. We will make region zero
and one required.
"
The reason I moved from numbers to strings (e.g., "left") is that a
theme author defining regions has no way of knowing what regions might
have already been defined, and therefore what integers might have
aleady been used. It's like module and theme names--we'd have to use
random numbers to avoid conflicts, so we're better off with string
names.
------------------------------------------------------------------------
May 12, 2005 - 12:17 : Bèr Kessels
nedjo,
I am not a fan of your solution for blocks that dissapear. It breaks
the ability to allow more themes on a site. if important blocks dont
show up, your site will simply break. I would rather suggest that every
theme MUST have a region called 'default'. any blocks that want to go
into a non-existing region, will be placed there.
We must define a standard listy of suggested region names. to avoid a
wildgrowth of names.
I am not sure if i like the way you do function phptemplate_regions().
these regions should be defined in the templqtes, not the engine.
Otherwise all phptemplate themes will still all have two regions.
Note that i did not test your patch thouroughly, just applied and
looked at.
------------------------------------------------------------------------
May 12, 2005 - 13:38 : nedjo
Thanks for your comments. I think we're getting somewhere.
"I am not a fan of your solution for blocks that dissapear. It breaks
the ability to allow more themes on a site. if important blocks dont
show up, your site will simply break. I would rather suggest that every
theme MUST have a region called 'default'. any blocks that want to go
into a non-existing region, will be placed there.
"
Good suggestion. Having a default region for blocks to show up sounds
like a valuable backup. I'm thinking we might want to have a way to
designate a given region as the default, rather than having to have one
named 'default'. Thoughts?
"We must define a standard listy of suggested region names. to avoid a
wildgrowth of names.
"
Sure. But the patch doesn't prevent doing that--it enables it.
"I am not sure if i like the way you do function phptemplate_regions().
these regions should be defined in the templqtes, not the engine.
Otherwise all phptemplate themes will still all have two regions.
"
Oh, I wholeheartedly agree, and I've noted the issue above. But unless
we substantially rewrite the engines, some region definition is needed
in the engines, as they call blocks. Should we therefore define a base
set in the engine, then call a hook from the engine to add any other
regions from themes based on that engine? If so, where in the theme's
files should such a hook be located?
And a side note. Do let's make an effort to keep our comments
friendly. Taking the time to note one or two things that do work or
are done well as part of a review is I believe a valuable habit. It
helps to confirm what is indeed accomplished. Beyond this, it helps to
maintain the idea that we are collaborating on shared goals, rather than
pushing individual and competing visions, and that contributions and
work are appreciated, rather than being invitations to criticism.
Thanks, Nedjo
------------------------------------------------------------------------
May 12, 2005 - 13:42 : chx
I am implementing http://drupal.org/node/16216#comment-29282 but it
takes time. regions again have names. default and sidebar regions are
mandatory.
This has little to do with nedjo's code much more with his idea -- I
looked at the screenshot and said, OK this is great but... we'd like to
have a lot more 'blocks' if this comes through, henceforth the above
mentioned solution.
------------------------------------------------------------------------
May 12, 2005 - 14:39 : nedjo
Okay, thanks for the clarification, I hadn't understood that you were
taking a new approach rather than building on what I'd begun. Given
that that's the case, staging as I'd suggested obviously won't work so
I'm content to wait and see what you accomplish. I'd only suggest,
don't feel like you need necessarily to do everything all at once.
------------------------------------------------------------------------
May 13, 2005 - 02:34 : Bèr Kessels
Nedjo, sorry if the sound of my comment was hostile. it was never meant
to be that way. I am busy, and thus did not want to comment all the
things i liked about your patch, but chose to pick the much smaller
amount of things I thought could use some improvement. I will try to
keep in mind to be more friendly next times. Also, not all of us are as
fluent in English as our native English speakers. I think its good to
keep in mind that for a lot of us, communication in English is not as
easy as it may seem. For me, it often requires a lot of concentration
to get the subtleties right. I mean: I can type some English text, look
up words in the dictionary, but to get small yet important subtleties in
my texts is very hard, often.
But, that all said: I liked your patch.
------------------------------------------------------------------------
June 10, 2005 - 05:08 : jakeg
How far off is this from being officially released or at least usable?
If its usable now, what files will I need to implement it?
------------------------------------------------------------------------
June 10, 2005 - 06:19 : Kobus
Attachment: http://drupal.org/files/issues/regions---possibility-3.png (2.21 KB)
To add to Stefan's suggestions with the two screenshots, I modified one
of them a bit, and this is how I understand Jaza's post, and yes, I
agree, this would be a wonderful feature! I can't wait! Imagine how
good it would look if I had a theme that has a clear white background
(not visible blocks) and then these seemingly random boxes on the page,
as shown in the screenshot. Especially where I can place it anywhere
around the node like Jaza said with a [region:2] block etc.
I say again: I can't wait!
Kobus
------------------------------------------------------------------------
June 10, 2005 - 07:59 : chx
I need 3-4 hrs of peace to implement the callback based thing. That's a
lot these days. Let's hope I'll get on it in the following days.
------------------------------------------------------------------------
June 18, 2005 - 16:32 : chx
Here's an example again of how hook_block will look like:
<?php
function module_block() {
$items = array();
$items['module_header'] = array('callback' =>
'mymodule_function', 'callback arguments' => arg(1), 'weight' => -50,
'region' => 'content', 'cache' => TRUE, ''pages' => "node/*\n<front>");
return $items;
}
?>
Items are keyed by unique IDs. The template system will call
module_invoke_all('block'), put the rendered HTML of the blocks into
regions sorted by weight. The rendered HTML is cached if it is allowed.
More keys will be introduced later, but now basically we just throw
everything from block configure in here, and add callback, callback
arguments, region.
Node teasers or fullly rendered nodes are blocks, too. Almost
everything is. So, here what happens when we are on taxonomy/term/1:
taxonomy_render_nodes is called
it calls block_set('node_view', $weight, $node, $teaser, $page); a few
times, so a few node_view blocks will be on the page
later the theme system is invoked. It will ask the block system to
collect all blocks, dynamic and the static blocks as well. For all
dynamic blocks we have a block defined in hook_block somewhere, like
the definition above, with one exception: it will have a 'standalone'
=> FALSE pair, so it is not a standalone block which is automatically
rendered but something that needs to explicitely requested. Also, we
may define a 'suffix' => 'somefunction' pair which provides us with a
cache ID suffix. Look at the code below:
<?php
function block_collect_blocks() {
// get all block definitions
$static_blocks = module_invoke_all('block');
// iterate of the dynamic blocks set on the page
foreach (block_get() as $dynamic_block) {
// get the block definition for the given dynamic block
$static_block = $static_blocks[$dynamic_block['block']];
// merge the callback arguments from the dynamic and static.
dynamic_block['callback arguments'] is always set though
// may be empty
if (isset($static_block['callback arguments'])) {
$dynamic_block['callback arguments'] =
array_merge($static_block['arguments'], $dynamic_block['callback
arguments']);
}
// try to get a cache ID suffix
$suffix = function_exists($static_block['suffix']) &&
call_user_func_array($static_block['suffix'], $dynamic_block['callback
arguments']);
// if the block is cacheable or the suffix function says that
it is cacheable by setting a suffix
if ($static_block['cache'] || $suffix) {
// set cache ID
$cid = 'block:'. $dynamic_block['block'] .':'. $suffix;
// try to get the rendered HTML from cache
if ($block_rendered = cache_get($cid)) {
$return[$static_block['region'][$dynamic_block['weight']][]
= $static_block_rendered;
continue;
}
}
// render the block
$block_rendered =
call_user_func_array($static_block['callback'],
$dynamic_block['callback arguments']);
// store the block for return
$return[$static_block['region'][$dynamic_block['weight']][] =
$static_block_rendered;
if ($static_block['cache'] || $suffix) {
// if cacheable, do it
cache_set($cid, $static_block_rendered);
}
}
foreach ($static_blocks as $key => $static_block) {
// is this a stand alone block, like in Drupal 4.6?
if ($static_block['standalone']) {
// now, the above is repeated, save that we do not have a
dynamic_block here
$suffix = function_exists($static_block['suffix']) &&
call_user_func_array($static_block['suffix'], $static_block['callback
arguments']);
if ($static_block['cache'] || $suffix) {
$cid = "block:$key:$suffix";
if ($block_rendered = cache_get($cid)) {
$return[$static_block['region'][$static_block['weight']][]
= $static_block_rendered;
continue;
}
}
$block_rendered =
call_user_func_array($static_block['callback'], $static_block['callback
arguments']);
$return[$static_block['region']][$static_block['weight']][]
= $static_block_rendered;
if ($static_block['cache'] || $suffix) {
cache_set($cid, $static_block_rendered);
}
}
}
}
function block_set() {
static $blocks;
$args = func_get_args();
if (count($args)) {
$callback = array_shift($args);
$weight = array_shift($args);
$blocks[] = array('block' => $function, 'weight' => $weight,
'callback arguments' => $args);
}
return $blocks;
}
function block_get() {
return block_set();
}
?>
When to expire the block cache? Never. If functions can call
cache_clear all, they can be smart enough to cache_clear only those
blocks that are changed by them. Some blocks won't be cacheable
anyways, say there is little use in caching 'active forum topics'
------------------------------------------------------------------------
June 19, 2005 - 01:15 : Dries
Frankly, I don't understand what you are trying to say. What is a
'dynamic', a 'static' and a 'standalone block'? What is the suffix
for? What is the advantage of making node and taxonomy views blocks?
If there is no big advantage, this is just bloat. All in all, it looks
pretty complex to me.
If we make Drupal more flexible _and_ more complex, people will get
upset, and we end up being like Plone. Drupal's simplicity is
something to foster, as it allows people to go in, and change stuff.
The focus is to make Drupal simpler _and_ more flexible.
I merely spent 10 minutes looking at this (reading your explanation
twice, staring at the code), and the newly added complexity simply
turns me off. Maybe that is because I don't understand what we are
gaining.
------------------------------------------------------------------------
June 19, 2005 - 01:34 : Bèr Kessels
* The first thing we gain, is, very slmple: more regions.
But, how many? what regions? on what pages? in what location? bearing
which name? For all these issues we need some callback system. To allow
themers, administrators and module developers to give answers to all
these questions. what we have now: sidebars, with only admin settings,
is simply too limited: It takes serious hacking (~15 lines of code in
your theme) to get a login block in your header!
* The second thing we get for free: We are passing along chuncks of
HTML, so why not cache them in a central place.
Practice points out that our current cache is not used well. We leave
it to developers to cache. And developers are lazy by nature. Just
looks at all the places where caching should have been done, but is
not. 9/10contribs have at least one of these places.
You must actually see these issues separate. The new central blocks
pulling system, and the caching. the latter is something that we get
for free, a high level of advanced caching (how often has not been put
forward Drupal seiously lacks better/advanced caching?). But we get it
only for free if it is co-developed with the new regions blocks.
And Last but not least, everything is a block, from a themer point of
veiw. I know developers tend to think: I do the content, and do not
care about where and how it turns up. That results in a content-centric
system, what we have now. Everythng is built around aa single chunk of
content. while, in fact content, is yet another block. Look at
http://www.terminus1525.ca/index.php?l=en or
http://www.franklinpennsylvania.us/ as it stands now, its up to the
developer to concenate all the HTML in the center area, into a huge
chunk of HTML. While in fact everyone can see that these are a lot of
separate blocks.
------------------------------------------------------------------------
June 19, 2005 - 01:39 : Dries
Ber: I was talking about chx's latest changes, not the block system in
general.
------------------------------------------------------------------------
June 20, 2005 - 12:06 : nedjo
I appreciate the update on directions being contemplated, although I
find it confusing.
Again, I'd caution, let's keep focused on the aim at hand, and on a
staged solution. A bigger patch means greater complexity, more
difficulty getting meaningful review, and greater delays in getting a
priority change. We're currently in a position where we have two
hard-coded regions for blocks, a major issue if nothing else than
because it reflects poorly on Drupal's usability and versatility.
Moving from here directly to something like "everything you see
anywhere is a (nested) block in its own (nested) region" strikes me as
a bigger step than we need to take.
And in belated reply to Ber's note (#53 above [11]), thanks for the
thoughtful comments, I didn't mean to single you out and acknowledge
that language & cultural differences play a role, I feel we can all
benefit from attention to communication.
[11] http://www.drupal.org/node/16216#comment-29633
------------------------------------------------------------------------
June 21, 2005 - 13:39 : Dries
Even with the explanation in chx's sandbox, this is too complex for my
likings. Here is a suggestion: let's drop the caching and the
'suffix'-stuff. Let's also drop the 'complete' and 'incomplete' stuff.
Keep it _simple_, chx. IMO, you're creating a monster.
------------------------------------------------------------------------
June 22, 2005 - 14:47 : chx
nedjo, take over. I am out.
------------------------------------------------------------------------
June 22, 2005 - 16:51 : djnz
Quick and dirty solution? Let blocks be assignable to 9 locations - Top
left, middle, right, middle left, middle right, bottom left, middle,
right.
The actual location of these areas within a theme is up to a theme
designer, but the designer should include each of them - how about
$sidebar['tl'], $sidebar['tm'], $sidebar['tr'] ...
Backwards theme compatibility could easily be maintained by only
creating the $sidebar array if the theme has set (eg in its
template.php file for a PHPtemplate theme) a flag, otherwise generate
$sidebar_left from the blocks set (in sequence) to top left, middle
left, bottom left, top middle and middle middle (arbitrary break) with
$sidebar_right the remainder.
I appreciate I have no karma here and only superficial knowledge of the
Drupal code, but sometimes the clarity of a view unclouded by too much
information is useful ;)
------------------------------------------------------------------------
June 27, 2005 - 00:26 : nedjo
Attachment: http://drupal.org/files/issues/regioning.patch (24.74 KB)
"
nedjo, take over. I am out.
"
Um, okay. I've given the question some more thought and come to the
following conclusions:
1. If we take regions to be areas of the page that specific types of
content are rendered into, and a regioning system to be a system to
accomplish this, we already have a number of regions and different
types of regioning systems. They include:
(a) our current blocks system, and
(b) the various drupal_set_[region]() functions, like
drupal_set_comment().
(c) specialized hooks like our help system, which assembles content to
be rendered into a region.
2. LIkely we should tease out regions from blocks.
So attached is a next rough cut at regioning. Features:
* Default region is defined in general admin settings page
* Regions are defined in both engines and themes. A particular
engine-based theme has the regions it defines plus the engine's default
ones.
* Content can be written to regions in two ways: through blocks or
through drupal_set_content($region, $content) calls.
Initial comments are welcome. I'll work on getting a more polished
patch and also more explanation of the approach.
1
0
[drupal-devel] [feature] Enable multiple block regions (not just "left" and "right" sidebars)
by nedjo 27 Jun '05
by nedjo 27 Jun '05
27 Jun '05
Issue status update for http://drupal.org/node/16216
Project: Drupal
Version: cvs
Component: block.module
Category: feature requests
Priority: normal
Assigned to: Anonymous
Reported by: paragkenia
Updated by: nedjo
Status: patch
Attachment: http://drupal.org/files/issues/regioning.patch (24.74 KB)
"
nedjo, take over. I am out.
"
Um, okay. I've given the question some more thought and come to the
following conclusions:
1. If we take regions to be areas of the page that specific types of
content are rendered into, and a regioning system to be a system to
accomplish this, we already have a number of regions and different
types of regioning systems. They include:
(a) our current blocks system, and
(b) the various drupal_set_[region]() functions, like
drupal_set_comment().
(c) specialized hooks like our help system, which assembles content to
be rendered into a region.
2. LIkely we should tease out regions from blocks.
So attached is a next rough cut at regioning. Features:
* Default region is defined in general admin settings page
* Regions are defined in both engines and themes. A particular
engine-based theme has the regions it defines plus the engine's default
ones.
* Content can be written to regions in two ways: through blocks or
through drupal_set_content($region, $content) calls.
Initial comments are welcome. I'll work on getting a more polished
patch and also more explanation of the approach.
nedjo
Previous comments:
------------------------------------------------------------------------
January 26, 2005 - 06:27 : paragkenia
I read the comparision discussion between *Drupal* and *Mambo*. In
several messages it was outlined that Drupal can place blocks only in
right and left and not flexible to put them on anywhere where one want.
It will be great if this can be changed in upcoming versions.
I am no pro at PHP, so don't know how much time this task will take,
but I think it is very important.
parag
------------------------------------------------------------------------
April 14, 2005 - 21:44 : nedjo
This issue was apparently partially addressed in issue
http://drupal.org/node/19694 [1].
[1] http://drupal.org/node/19694
------------------------------------------------------------------------
April 16, 2005 - 20:24 : nedjo
Attachment: http://drupal.org/files/issues/block-dynamic-regions.patch (8.17 KB)
This much-requested functionality - to have the ability to place blocks
in more than the two predefined regions - was partially addressed in
issue http://drupal.org/node/19694. [2]. But "blocks" are still
limited to the "left" and "right" sidebars (hard-coded in
block.module).
This patch is a first step designed to enable multiple (eventually,
admin-definable) regions for blocks. I've moved the existing "left"
and "right" block regions to a 'region' table (with ids of 0 and 1, as
currently used in themes). Then all references to the regions are
drawn dynamically from the table. This way, if further records are
added, they will appear in the list of available regions for block
placement.
Doing this actually reduces some duplicated code, since it's no longer
necessary to repeat code blocks for each of "left" and "right".
As it stands, the patch doesn't add any new functionality--but I don't
think it breaks anything either. New functionality would need (a) new
regions defined, and (b) changes to themes. A simple first step might
be, e.g., to add a "footer" region and then add a call in the footer
generation to append any blocks assigned to the footer region there.
I'm setting this to patch, but I'm aware that it needs some discussion
and refining before it'll be ready to apply.
[2] http://drupal.org/node/19694.
------------------------------------------------------------------------
April 16, 2005 - 21:05 : nedjo
Attachment: http://drupal.org/files/issues/block_regions.png (5.65 KB)
Here's a screenshot showing the block admin page, with drop-downs for
region placement (the options are dynamically generated based on
defined regions).
------------------------------------------------------------------------
April 16, 2005 - 22:57 : adrian
The biggest problem with this is that you can have multiple themes, and
each of these themes can have different regions available.
Also, the method of defining which regions are available needs to be
standardised. Some of the work that me and Vlado are working on for the
install system would go towards solving that problem (ie: meta
information for modules, themes and styles).
This has been discussed to death, but the general consensus has been
that we _want_ to do this, but we need to solve a few other problems
properly first, the most pertinent being the interface issue. Chris
(factoryjoe) recently did a whole mess of workflows for something
related to this.
------------------------------------------------------------------------
April 17, 2005 - 00:09 : syscrusher
The first paragraph of adrian [3]'s post is a point that occurred to me
also, as I read this thread.
One suggestion would be to separate the definition and configuration of
blocks, on one hand, from the placement of those blocks, on the other
hand.
In other words, Drupal core provides a mechanism defining what blocks
exist, which of these are on by default or off by default and
user-selectable vs. which are forced on for all users, and the
configuration (if applicable) of specialized blocks defined by
particular modules.
Each theme provides a standard hook function that returns an array of
region names and help/description text, e.g., array('left'=>t('This
vertical region is left of the main content area'), 'right'=>t('This
vertical region is right of the main content area'), 'footer'=>t('The
footer is below the left, right, and main content areas of the page')).
The theme part of Drupal core (i.e., theme.module itself, not the
individual themes) provides a standard UI that is displayed within
config of *each theme* (but is one physical code base within
theme.module) that allows the administrator to map blocks defined by
Drupal core into regions defined by the theme, and storing that mapping
as an theme-to-block_ID-to-region_ID (with weight) table in the
database. From there, the actual page rendering is similar to what's
being done now, but there is more of it.
I guess what I'm trying to say is that the key to solving this problem
is breaking it along its degrees of orthogonality, and there are three
-- two in Drupal core and one in the individual theme.
Scott
[3] http://drupal.org//user/1517
------------------------------------------------------------------------
April 17, 2005 - 03:14 : Dries
Scott, is right. The theme should export its available regions. Then
the administrator's task is to assign blocks to regions (not to setup
regions). A theme could have configurable regions, but that internal
to the theme.
To me, the real challenge is the UI and the interaction design.
Configuring blocks could easily become a real pain (while most themes
continue to use 'left' and 'right'.)
Of course, we can implement all the functionality, default everything
to 'left' and 'right', and worry about the UI later. This should be
fairly straightforward to implement and nedjo's patch looks like a
first step in the right direction.
------------------------------------------------------------------------
April 17, 2005 - 17:09 : nedjo
I agree with the suggested approach of themes registering their regions.
I'd see this happening when a particular theme is activiated (or
upgraded). Regions would be an array of names (e.g., 'left', 'right',
'footer'). New records would be created in the region table only for
region names not previously registered.
Some areas I'm not clear on, or that need further work:
* Table naming
Should a new table be 'region' or 'regions' (I've used 'region')? I
don't see a convention among existing table names, some of which are
singular and others plural.
* Default values
I've kept the existing '0' and '1' ids for regions (left and right,
respectively), for backward compatibility. But this means we can't use
autoincrement for the region id (rid), since autoincrements seem to
begin with 1. Likely we should change to autogenerated ids.
* Initial records
Using the INSERT INTO statements as I've done for the initial region
entries is probably counterproductive, since sooner or later they'll
have to be dynamically generated. I was hoping this could be a
preliminary patch, with the main work coming later, but likely we need
to solve region registration by themes before this patch is applied. I
don't have a good idea of how region registration by themes would be
implemented (a hook on theme activation?), and invite suggestions or
implementations.
* Theme system changes
Besides region registration, the other change I'm seeing that would be
needed in the theme system is loading blocks by region name, rather
than region id. This is because the ids currently used ('0' and '1')
in theory might be diffferent on a particular install.
------------------------------------------------------------------------
April 17, 2005 - 17:18 : syscrusher
Instead of having themes "register" their regions, why not just add a
theme API hook called them_regions() that returns an associative array
of $region_name=>t($region_helptext)? This would be in keeping with
other similar API functions, such as those that return what permissions
apply to a module or what node types are defined by a module. Most
themes are going to define only a small number of regions (two being
the typical case now, but I could see five or six in a complex theme),
so returning a constant array will be faster than querying SQL to
obtain an array of rows.
There could be (optionally, at the discretion of whoever builds this
thing) another API hook like theme_region_properties($region_name) that
returns an associative array with detailed info for a given region, such
as extended help text with recommended usage instructions for the
region.
Scott
------------------------------------------------------------------------
April 17, 2005 - 17:19 : syscrusher
s/them_regions/theme_regions/g
------------------------------------------------------------------------
April 17, 2005 - 18:47 : adrian
Because. The most common 'theme' is a phptemplate theme.
And there needs to be a generic method for specifying the regions
available, in a non hook_function format..
Themes get their names from the directory in which they are contained..
when copying the theme to another directory, there must be _no_
modifications necessary to allow normal usage. This is one of the
tenets of the new template system I designed.
You would need a standard way to define meta-information for themes,
that does not need to be modified when copied to a new directory. We
are working on this in the install system work, as you need a way
external of Drupal to define the module dependencies and some other
things.
My approach would be to add a theme.dsc text file to the directory,
which would allow you to specify meta-information. For instance :
----
regions: left, right, footer, center, mountie
author: Johnny McAngstyPants
----
------------------------------------------------------------------------
April 17, 2005 - 22:00 : syscrusher
Adrian wrote:
"
Themes get their names from the directory in which they are contained..
when copying the theme to another directory, there must be _no_
modifications necessary to allow normal usage. This is one of the
tenets of the new template system I designed.
"
A very good point. But region names don't have to be unique across
different themes, so my hook function wouldn't have to be modified. The
suggestion I made for the mapping metadata was three factors: theme ID
or name, region ID or name, and block ID (plus one non-key weight value
to order the blocks within a region, but this is not relevant here).
I'm not familiar enough with PHP template to be able to respond to your
comments on that one, so if you say it's not feasible to work with my
schema, then I'll take your word for it. :-)
Scott
------------------------------------------------------------------------
April 17, 2005 - 22:03 : syscrusher
Oh, wait.....I see now what you mean! It's not the output of the
function that is the problem, but the *name* of the function.
Never mind. I concede your point.
Scott
------------------------------------------------------------------------
April 18, 2005 - 03:44 : Jaza
I'm probably jumping ahead a bit here... but something that this patch
will have to eventually account for, is how to allow regions to be
defined as inline. Allowing a theme to define a region's position as
being anywhere on the edge of the page (i.e. top, left, right, bottom,
corners, etc) is relatively easy. But what about having a region that's
in the middle of a node's body, or somewhere else that's not the edge of
the page?
What I'm thinking of, is eventually allowing the custom region system
to integrate with the CCK. Like I said, I admit that I'm jumping ahead
into the future - the CCK still has a long way to go before it's ready.
But ultimately, it would be cool if a theme could define a region as
being after, say, the "rating" field in nodes of type "movie review".
This would make the region (and the blocks in it) truly inline and in
context with the actual content of the node.
This would be a huge step forward compared to the current block system,
which doesn't allow you to mix the presentation of blocks and nodes at
all. In Drupal ATM, blocks and "the rest of the page" are totally
separate, and really you have no choice but to display them as such.
The result of this is that information in blocks that really should be
displayed as part of a node, gets literally "pushed to the side", and
always seems secondary to the actual content. Often the block has
content that is just as important as the content of the node itself.
Another option (of less merit, IMO - because of the extra maintenance
work, and lack of enforced consistency) would be to have a region
filter. You could then choose where to place an inline region on a
per-node basis, by entering text such as [region:2] (example based on
image_filter syntax) into the node body.
Anyway, just thought I'd throw that idea into the open. I don't expect
it will be implemented any time soon, but it's something to think
about.
Jeremy
------------------------------------------------------------------------
April 18, 2005 - 04:54 : stefan nagtegaal
Attachment: http://drupal.org/files/issues/regions---possibility-1.png (1.29 KB)
Imo the 'regions' are more than you guys name here.. FOr example I
attached 2 screens which only shows you the regions.. What I think is
that it should always be possible to have free names for the regions.
No matter if i call - the region where my content appears in -
'content' or 'site items'..
See attached screens..
------------------------------------------------------------------------
April 18, 2005 - 04:56 : stefan nagtegaal
Attachment: http://drupal.org/files/issues/regions---possibility-2.png (1.39 KB)
And the second...
See these links:
- http://drupal.org/files/issues/regions---possibility-1.png
- http://drupal.org/files/issues/regions---possibility-2.png
------------------------------------------------------------------------
April 18, 2005 - 11:22 : nedjo
Attachment: http://drupal.org/files/issues/block-dynamic-regions2.patch (13.39 KB)
Here's a revised patch implementing some of the ideas so far. I've used
the theme engine - rather than individual themes - to generate the list
of available regions. I know this isn't ideal, but e.g. for xtemplate
it's at present the only option--no logic is possible in the individual
theme.
region is changed to a varchar field from the current tinyint.
I've roughed in several regions--really just for demo purposes. More
thought and work would be needed to refine them (e.g., styling, etc.).
But this maybe moves the conversation forward.
------------------------------------------------------------------------
April 18, 2005 - 17:26 : Dries
This patch enables different regions to be used. That is a good thing.
The next question is: should we take into account that different themes
could assign blocks to different regions? Lots of problems would be
solved if (i) there could only be one active theme on a website or (ii)
if all themes would have a common set of regions.
(This "let users select their own theme"-thing is a left-over from the
early days, except for WAP stuff maybe.)
------------------------------------------------------------------------
April 18, 2005 - 17:37 : killes(a)www.drop.org
Giving up multiple theme support would be a great thing. People could
still select from multiple style sheets. WAP shoud not be a user
preference setting, but automatically detected and redirected to a
special site with a WAP theme.
------------------------------------------------------------------------
April 18, 2005 - 17:48 : adrian
You would then lose the ability to make a special front page theme for
use with the sections module, for instance.
Also, currently styles exist in the same namespace as themes, and we
would have to make more a distinction of styles vs templates (or
themes), but I do think this could help simplify things.
------------------------------------------------------------------------
April 21, 2005 - 12:54 : nedjo
Attachment: http://drupal.org/files/issues/block-dynamic-regions3.patch (15.69 KB)
The main issue raised, if I'm understanding well: if a theme or theme
engine is present that offers regions other than those available in the
default theme, any blocks assigned to those regions will be invisible to
most users.
Here's a revised patch addressing the concern. I've added a test for
each region to see whether it's also available in the current default
theme. (Actually, I'm realizing as I write this, I've used the theme
engine--so it would need to be updated to test first if an engine is
being used.)
In any case, any regions not available by default are starred, and a
message text appears (only if necessary).
If there's support for the approach, I'd be happy to finish the patch.
Remaining work:
* extend to work with non-engine themes
* finalize base set of regions to implement
* implement common region set in all core themes (for now I've only
done xtemplate)
* work on CSS display (how blocks in each region should look)
This last point I'm not too confident on and would look for
help/suggestions.
Screenshot to come.
------------------------------------------------------------------------
April 21, 2005 - 12:56 : nedjo
Attachment: http://drupal.org/files/issues/block_regions_message.png (12.34 KB)
Screenshot showing starring of non-default regions plus message.
------------------------------------------------------------------------
April 21, 2005 - 13:19 : killes(a)www.drop.org
I like that approach. +1
------------------------------------------------------------------------
April 21, 2005 - 13:23 : resmini
My two cents.
If this has to be done (and it should), better get rid of spatial
definitions for areas such as left, right, top, bottom, etc.
Use semantic names related to content and let the theme take care of
their positioning interely.
Also note that there in an ongoing debate in IA trying to build up a
standard naming convention for page regions in a way that is
semantically (and logically) correct. Just for the sake of it, you can
check
http://www.stuffandnonsense.co.uk/archives/whats_in_a_name.html
http://www.stuffandnonsense.co.uk/archives/whats_in_a_name_pt2.html
with follow-ups, including Eric Meyer's. There are more substantial
contributes in the AIfIA web site, but I can't seem to find them now.
I'll post a link or an abstract if I manage to.
--
vector at exea dot it
------------------------------------------------------------------------
April 21, 2005 - 14:15 : nedjo
Good points, useful references. The default regions I'm thinking of
are:
'banner' => t('banner'),
'left' => t('left sidebar'),
'right' => t('right sidebar'),
'body_top' => t('body top'),
'body_bottom' => t('body bottom'),
'footer' => t('footer')
Perhaps "body" would be better as "content"? It would be easy enough
to add in more regions, e.g., within the content (after title, after
help, etc.) One issue is that many of these regions will already have
rendered content. Should the blocks come before or after that other
content? I've assumed after for banner and footer, while giving both
options in body.
In any case, refinements or suggestions would be great.
------------------------------------------------------------------------
April 21, 2005 - 14:56 : resmini
Well,
'banner' => t('banner'),
'left' => t('left sidebar'),
'right' => t('right sidebar'),
'body_top' => t('body top'),
'body_bottom' => t('body bottom'),
'footer' => t('footer')
The classic (as far as classic goes for something Web related) scheme
comprises 5 spatial regions, much like stefan's
http://drupal.org/files/issues/regions---possibility-1.png with a left
area mirroring the one called 'blocks'. Call them top, left, center,
right, bottom.
Variants usually exclude one or more of these, layout-wise.
If we talk content (semantic) instead, things get a bit more
complicated, as these spatial regions normally include more than one
content-area. The top, for example, could include ad banners and the
site's header, or the main menu. But these merge seamlessly with CSS
definitions, which are definitely something that should get properly
standardized (most of the errors / inconsistencies I'm encountering
with Drupal today are related to this), but this is out of scope in
this thread.
What I somewhat object to the definitions above is that it mixes the
two approaches.
'banner' is not spatial, is content, as it is the distinction between
body top and bottom. If 'banner' goes there, it is 'top', whatever that
might be. I designed a couple of web sites which had
headers/logos/banners at the bottom of the page, and I'm not exactly
the wildest designer you'll probably meet.
And if we include body_top and _bottom, we should also add
'navigation', or 'menu' and some other content-related semantic region
(impressum, or company_data, or whatever).
I suggest that either we go along
'top' => t('banner'),
'left' => t('left sidebar'),
'center' => t('body'),
'right' => t('right sidebar'),
'bottom' => t('footer')
or we dedicate some time to detailing what could possibly fit there
semantically, which is perfectly possible without limiting design
freedom or whatever, since the vocabulary is not infinite, but not
something you do out of the blue.
Please note that I understand t('something') to be an example, since
such a layout only defines regions on the page, not semantic regions.
My main content could fit nicely in the bottom region.
As for the names for the regions, they are not important, as much as
they are coherent and enforced.
------------------------------------------------------------------------
April 21, 2005 - 17:16 : stefan nagtegaal
Attachment: http://drupal.org/files/issues/regions-final.png (2.39 KB)
Maybe something like this???
But, I have to say that I _really_ liked the solution to split the
'content'-region into the 'content' and 'beneath content'.. I love
that!
------------------------------------------------------------------------
April 21, 2005 - 18:20 : resmini
Stefan, yes.
That's basically what you can build if you stick with spatial, and this
layout can generate quite an amount of (sane) layout variations even if
you interpret it strictly (suppose a default) as
<div id="header">
</div>
<div id="left-bar">
</div>
<div id="content">
</div>
<div id="right-bar">
</div>
<div id="footer">
</div>
What you can do with this is left basically to the theme designers and
their use of CSS, CSS-P or tables (brrr), but this could be an easy and
proper way to identify macro-regions in which more accurate positioning
happens at a more specific level.
I'm all in favour of subsequent identification of sub-areas (top and
bottom in body), but I don't think it would be a good idea to mix the
different approaches I mentioned in my previous post.
A site with lots of things going on or which is not just blogging
surely has the need to use its content area in a wiser manner than just
putting there 'content', the number of modules doing precisely this is
more than proof, but that falls more in a 'Drupal definitive guide to
semantics', in which CSS selectors are used or suggested strategically
for such a scope.
>From this point of view and for the sake of this thread, any unique
area / div (#name) could (should?) be a target for blocks, and this in
my opinion (*) would be the perfect solution, but I do not know if and
how this is even remotely possible in the current Drupal.
(*) It's quite late, local time, and it was a looong working day.
------------------------------------------------------------------------
April 21, 2005 - 18:38 : resmini
Attachment: http://drupal.org/files/issues/regions-final-variation.png (1.9 KB)
Just to be clear, I add a variation of stefan's scheme that shows how
this is *not* by any means positional, but spatial, or better
relational. Use your CSS imagination to take out regions, reduce them
to empty containers, enlarge them, make them taller, shorter, whatever.
5 regions, do what you like.
------------------------------------------------------------------------
April 21, 2005 - 18:44 : resmini
Sorry, I forgot.
And this is also why I'm not too keen on calling anything 'content'. If
I enlarge my right region enough, why not put my content there?
Shouldn't this be a theme-level concern?
It's nothing serious, but if my center area is called 'center' (or
something similar and more sexy), I won't confuse anybody.
This is left, center, right: do with them what you prefer. Content in
the bottom? Show me.
If I call something 'content', I'm more than suggesting that content
goes there, and only there.
------------------------------------------------------------------------
April 22, 2005 - 04:17 : stefan nagtegaal
Resmini, maybe it is only me but I think that you are making a mistake
in your way of thinking..
I get the feeling that you're mixing the 'regions' and 'divs'.. the
'regioins' should have meaningfull sentences, but the divs
shouldn'thave..
As an example:
$region_name => $content_body
here we go:
'header' => $logo
'left sidebar' => theme('blocks', 'left')
'right sidebar' => theme('blocks', 'right')
'content' => $content
'footer' => $footer.
So, $region_name does not have to be equal to the id or class of the
divs..
------------------------------------------------------------------------
April 22, 2005 - 05:27 : resmini
Stefan,
>So, $region_name does not have to be equal to the id or class of the
divs..
Yes, absolutely. I probably didn't make myself very clear: what I'm
trying to point out is that in
'header' => $logo
'left sidebar' => theme('blocks', 'left')
'right sidebar' => theme('blocks', 'right')
'content' => $content
'footer' => $footer.
there is no reason at all to call 'content' the region that holds
$content.
nedjo asked in #24
'body_top' => t('body top'),
'body_bottom' => t('body bottom'),
if /Perhaps "body" would be better as "content"?/ and all my reasoning
is simply that no, that is not content, or it shouldn't be (or better:
it may happen that main content is elsewhere). As a matter of fact,
even the connotation 'sidebar' doesn't fit very well logically.
To make it short: Today's method works, but has limits. Mambo, which
paragkenia quotes at the beginning, handles more 'user regions' but in
pure Mambo style conventions are so loose that they are hardly useful.
If we define regions, as in nedjo's example, in my opinion these
regions shouldn't imply informational layouting, only relational
positioning (that is, where they stay in the browser viewport).
I know it may look like overdesigning, but Drupal constantly enforces
conventions and coding guidelines everywhere: this is one of the best
aspects of the project and I think it should carry to the layout.
------------------------------------------------------------------------
April 22, 2005 - 05:32 : stefan nagtegaal
While I think that 'body_*' is not as self explaining as 'content_*' I
vote for using 'content_*'..
------------------------------------------------------------------------
April 25, 2005 - 13:48 : chx
Let's get back to the code. If my understanding is correct the big
discussion is about how to name potiental rectangular areas _after_
this is commited. So, first get this committed. What's need to make
that happen?
------------------------------------------------------------------------
April 25, 2005 - 14:56 : chx
I spoke with Moshe, Steven, JonBob and the result is: keys shall be
numbers, and themes shall define the values. We will make region zero
and one required. There will be a define('REGION_CONTENT', 0) and a
define('REGION_SIDEBAR', 1); so you can have readable code still.
Either wait for me to code it or feel to do it yourself, just drop me a
mail via the contact form if you are doing it.
------------------------------------------------------------------------
April 27, 2005 - 12:14 : nedjo
Károly, it's great if you're willing to take on finishing this. I
tried to summarize remaining work in update 20, above [4].
The numerical keys look fine to me.
In terms of an initial default set of regions, one quirk is this: for
now, so far as I can see (and assuming the approach I sketched in),
available regions can be defined only in (a) theme engines and (b)
individual themes that don't use an engine. So, for instance, an
individual xtemplate based theme can't define its own custom regions;
they need to be defined in the theme engine. (Unless and until the
whole theme system should switch to the sort of configuration file
adrian suggested above [5].)
Which is to say that, under this approach, we'll be defining a set of
regions for each engine, and then all themes using that engine should
try to implement those regions. (Otherwise, if a site admin selects a
region defined in the engine but not implemented in the default theme,
the content won't appear.)
Please let me know if there's anything in what I've done that isn't
clear, or if you want to pass some or all of this back.
Thanks!
[4] http://www.drupal.org/node/16216#comment-27398
[5] http://www.drupal.org/node/16216#comment-27052
------------------------------------------------------------------------
May 9, 2005 - 16:36 : chx
I had a "short" discussion with the UI team leader ie. Ber Kessels. The
outcoming is that we will change hook_block so that it uses callbacks a
bit like menu system. Here's an example:
<?php
function module_block() {
$items = array();
$items['module_header'] = array('callback' => 'mymodule_page',
'weight' => -50, 'region' => 'content' , 'cache' => TRUE, 'pages' =>
"node/*\n<front>");
return $items;
}
?>
Please note that we are allowing caching the themed output of each
block independently.
The theme system will execute hook_block as it does now and it'll
execute the callbacks defined in module_block as well. Of course if the
callback is cacheable and in the cache, it'll print it from cache. If
it's themeable and not in the cache, it'll cache it before printing.
Pages contains a string (could be PHP code!) which can be overriden in
the block configuration screen.
Callbacks defined in hook_menu will have lot less functionality after
this, they will be used for non-themed pages mainly.
------------------------------------------------------------------------
May 9, 2005 - 16:45 : chx
More explanation is requested for the example.
module_header is the unqiue name of the block.
pages define on which pages the block is visible. ('pages' in the block
configuration screen.) Maybe I'll implement 'visibility' from the same
screen as well.
callback, weight are well known Drupal terms.
cache is a boolean which defines whether given output is cacheable or
not.
------------------------------------------------------------------------
May 9, 2005 - 16:47 : chx
Ber, Dries, do not kill me! Ber chaired the usability meeting in
Antwerp, that's all.
------------------------------------------------------------------------
May 9, 2005 - 16:52 : Bèr Kessels
Attachment: http://drupal.org/files/issues/more_regions_principle.png (123.29 KB)
attached is a schematic which might clarify this a little.
And I would also like to comment on me being the UI leader: I do not
consider myself any leader, no ofeence, though. Just to clarify that.
------------------------------------------------------------------------
May 9, 2005 - 18:42 : nedjo
I am not opposed to the idea of changing the _block() hook to use
callbacks (I have no opinion on it, not immediately grasping the need),
but this seems quite distinct from the aim of enabling multiple and
extensible regions. Why can't we just move ahead with the block
generalization?
I suggest:
* quickly implement the proposed block region generalization, using
only the two existing regions (i.e., implementing them in core themes)
* later worry about:
- adding further regions (the details of which are best done by those
primarily responsible for the particular themes)
- possible changes to the _block() hook.
Wouldn't this make sense? Or am I missing some pressing reason for
which the block generalization has to wait?
------------------------------------------------------------------------
May 9, 2005 - 21:28 : killes(a)www.drop.org
"Or am I missing some pressing reason for which the block generalization
has to wait?"
I might be missing something too, but I thought Chx was just talking
about the block generalization and proposing a way to code it.
------------------------------------------------------------------------
May 9, 2005 - 22:41 : factoryjoe
I wanted to chime in with this discussion because it's an important one
and has some really huge implications for Drupal moving forward. That
said, I also have a day job pulling all my attention which means that
the big ideas I have for this problem can't be put into a complete
visual form for at least a week or more. In the meantime, I'd like to
offer a few thoughts on the matter.
* *Regions do not belong in themes.* Blocks were originally separate
from themes for a reason. Themes should define the general look and
feel of a site -- and to some extent a site's behavior. Layouts which
define the spatial relationship between regions belong in some
intermediary between style and content, which does not currently exist
in Drupal.
* I would propose that *layouts become a new kind of CCK tool.* By that
I mean that they can be defined, shared and deployed very easily -- even
as simply as tossing a directory of some .layout textfiles into a
layouts/ directory. Note that this suggestion has nothing to do with
the way CCK /might actually/ work right now; instead I'm suggesting how
I would like it to work. I would like, for example, to create a CCK item
like a "person" and ship it with a .layout file that carries all the
semantic goodness needed to output that "person" to the screen in
juicy, semantic markup... which anyone can come along and theme to
their heart's content.
* *Creating shareable layouts should be relatively painless.* Dashboard
[6] sets the bar for easy hackability. It should be that easy to create
layouts for Drupal... I can imagine creating an "iTunes layout" or an
"Image Gallery" layout and so on... If we used something akin to PHPTal
to develop the layouts, we could keep development fairly standard while
making it possible for more folks to hack up cool and innovative
layouts.
* *Layouts should be extensible.. to a degree.* I see a huge problem
with how this discussion has floated back and forth between semantic
and spatial region naming. Because we talk about this problem in those
terms, we fail to identify the real problem and opportunities presented
to us. Here's the thing: when you predefine regions, you limit
creativity and the ability of people to really use the system. On the
other hand, when you make a system too flexibile, it quickly becomes
unwieldy. In all the mockups presented above, both classes of problems
would arise. Therefore we need to think about a way to stay flexibile
while "taming the beast."
* To that end, I suggest developing *self-contained layouts that are
contextually-complete.* So for the example of the iTunes search
layout... you would know before hand that you get a couple multiple
select boxes that you can hook up to whatever taxonomies you want to
progressively filter [nodes]. You could even change the .layout file,
allowing you to theme it to your liking -- even though that would
totally optional, given the layout's internal default styling. Once
you've got your layout, you would go into a layout module UI [7],
create a new layout [8], select the layout from a list [9] and then
give it a path so you can access the layout... You would then walk
through a wizard [10] that would help you populate that layout with all
kinds of nice Drupally goodies... feeds, node listings, PHP code, etc.
The power in this approach is multifold. For one, it snaps Drupal out
of its block mentality and allows designers to really innovate with
individual page designs. In fact, I've already thought out how I could
reimplement Flickr using this system, combined with Drupal's native
feeds, taxonomy and menu module, and I'd barely break a sweat... sort
of. ;)
But anyway, this is a very productive conversation to be having, but I
really think that we should consider whether the old paradigm, as such,
really pushes Drupal as far forward as we have the opportunity to go.
[6] http://drupal.org/apple.com/macosx/theater/dashboard.html
[7] http://photos4.flickr.com/6944826_90af237c99_o.png
[8] http://www.factorycity.net/demos/civicspace/layout/new.html
[9] http://www.factorycity.net/demos/civicspace/layout/new_layout.html
[10]
http://www.factorycity.net/demos/civicspace/layout/testing_add.html
------------------------------------------------------------------------
May 10, 2005 - 00:34 : chx
While factoryjoe may be right, I could be right as well.
Regions has definitely something to do with themes, after all it's the
theme that renders the page, and it can only render regions that it
knows of.
So, instead of postponing this into the far future when someone comes
out with a layout system, I code this now.
------------------------------------------------------------------------
May 10, 2005 - 01:22 : Dries
The proposed 'weight', 'region' and 'pages' would be configurable, not?
Are these just defaults? The 'callback' and 'cache' options are nice to
have.
------------------------------------------------------------------------
May 10, 2005 - 01:25 : chx
Yes, blocks can provide a default for everything on the block and and
configuration screen. Of course, you can override these in the UI.
------------------------------------------------------------------------
May 10, 2005 - 05:02 : Bèr Kessels
Dries: yes, the region, weight and pages will be configurable from an
admin interface. What we tried to address here, is a method for
developers to choose good defaults. Take for example "logo". that
should be enabled by default in a regions called header, rendered on
all pages, and have a very light weight. We should not force an admin
to visit the blocks admin before she can see or use the logo.
Chris: I do not know if you have ever tried typo3. Its concept is some
form of builder system. I do not like thar, because far too complex. If
we ever get sme kind of builder in core (if ever) it will require a lot
of work, and even more usability research. So for now such a builder is
far out of our reach. Lets aim at goals that are more realistic ;) So:
it does belong in themes, its the best place that is still realistic.
I do like your visions, Chris, but I hope you agree they are not
realistic, yet. So, maybe you can think of a path towards your visions,
ow we can reach such a builder-system, you describe?
------------------------------------------------------------------------
May 10, 2005 - 10:52 : factoryjoe
My browser crashed before I was able to submit my followup to chx's
comment... I have no problem with the current work going forward and am
happy to see it happening. I know that my ideas are typically
outlandlish and far off... That's the kind of design I enjoy doing!
In any case, I see chx's solution being a means to an end; once we have
abstracted the region code, it become much easier for me to get some
help creating my vision.
In the meantime, and as a demonstration of how I see this layout system
working (to answer, I hope, Ber's concerns about clunkiness), I've
created a workflow movie that shows off how fast you'd be able to add
content to the system.
Check it out:
http://factorycity.net/demos/civicspace/layout/media/layout_workflow_v1.mov
Feedback appreciated. And, if you want to try the demo out yourself
(it's all hardcoded, like a "movie set" for my UI), go here:
http://factorycity.net/demos/civicspace/layout/
------------------------------------------------------------------------
May 12, 2005 - 12:45 : nedjo
Attachment: http://drupal.org/files/issues/block-dynamic-regions4.patch (13.56 KB)
"I might be missing something too, but I thought Chx was just talking
about the block generalization and proposing a way to code it.
"
Well, if I can say so respectfully, I don't think so. Not so far as I
can see identifying problems with my original patch, this interesting
discussion is rather addressing the (useful, but logically subsequent)
question "once we have multiple regions, how should we work with
them?". It's perhaps demonstrating our propensity to try to do more
rather than less with a given patch--to talk for a long time when a
first step solution is already in place. So here is a (yes, interim!)
small, focused, and tested patch to remove the hard-coded regions.
I've stripped it down so it does nothing more than (a) make block
regions declared by themes rather than hard-coded and (b) implement the
existing left and right regions in existing core themes. Please, try it
out, I think you'll like it. I'm of the (I'm hoping not scandalous)
opinion that it can be applied right away, and so clear the way for
further progress. In particular, this patch leaves the adding and
configuring of additional regions to theme authors/maintainers, who are
best qualified for the work.
"I spoke with Moshe, Steven, JonBob and the result is: keys shall be
numbers, and themes shall define the values. We will make region zero
and one required.
"
The reason I moved from numbers to strings (e.g., "left") is that a
theme author defining regions has no way of knowing what regions might
have already been defined, and therefore what integers might have
aleady been used. It's like module and theme names--we'd have to use
random numbers to avoid conflicts, so we're better off with string
names.
------------------------------------------------------------------------
May 12, 2005 - 14:17 : Bèr Kessels
nedjo,
I am not a fan of your solution for blocks that dissapear. It breaks
the ability to allow more themes on a site. if important blocks dont
show up, your site will simply break. I would rather suggest that every
theme MUST have a region called 'default'. any blocks that want to go
into a non-existing region, will be placed there.
We must define a standard listy of suggested region names. to avoid a
wildgrowth of names.
I am not sure if i like the way you do function phptemplate_regions().
these regions should be defined in the templqtes, not the engine.
Otherwise all phptemplate themes will still all have two regions.
Note that i did not test your patch thouroughly, just applied and
looked at.
------------------------------------------------------------------------
May 12, 2005 - 15:38 : nedjo
Thanks for your comments. I think we're getting somewhere.
"I am not a fan of your solution for blocks that dissapear. It breaks
the ability to allow more themes on a site. if important blocks dont
show up, your site will simply break. I would rather suggest that every
theme MUST have a region called 'default'. any blocks that want to go
into a non-existing region, will be placed there.
"
Good suggestion. Having a default region for blocks to show up sounds
like a valuable backup. I'm thinking we might want to have a way to
designate a given region as the default, rather than having to have one
named 'default'. Thoughts?
"We must define a standard listy of suggested region names. to avoid a
wildgrowth of names.
"
Sure. But the patch doesn't prevent doing that--it enables it.
"I am not sure if i like the way you do function phptemplate_regions().
these regions should be defined in the templqtes, not the engine.
Otherwise all phptemplate themes will still all have two regions.
"
Oh, I wholeheartedly agree, and I've noted the issue above. But unless
we substantially rewrite the engines, some region definition is needed
in the engines, as they call blocks. Should we therefore define a base
set in the engine, then call a hook from the engine to add any other
regions from themes based on that engine? If so, where in the theme's
files should such a hook be located?
And a side note. Do let's make an effort to keep our comments
friendly. Taking the time to note one or two things that do work or
are done well as part of a review is I believe a valuable habit. It
helps to confirm what is indeed accomplished. Beyond this, it helps to
maintain the idea that we are collaborating on shared goals, rather than
pushing individual and competing visions, and that contributions and
work are appreciated, rather than being invitations to criticism.
Thanks, Nedjo
------------------------------------------------------------------------
May 12, 2005 - 15:42 : chx
I am implementing http://drupal.org/node/16216#comment-29282 but it
takes time. regions again have names. default and sidebar regions are
mandatory.
This has little to do with nedjo's code much more with his idea -- I
looked at the screenshot and said, OK this is great but... we'd like to
have a lot more 'blocks' if this comes through, henceforth the above
mentioned solution.
------------------------------------------------------------------------
May 12, 2005 - 16:39 : nedjo
Okay, thanks for the clarification, I hadn't understood that you were
taking a new approach rather than building on what I'd begun. Given
that that's the case, staging as I'd suggested obviously won't work so
I'm content to wait and see what you accomplish. I'd only suggest,
don't feel like you need necessarily to do everything all at once.
------------------------------------------------------------------------
May 13, 2005 - 04:34 : Bèr Kessels
Nedjo, sorry if the sound of my comment was hostile. it was never meant
to be that way. I am busy, and thus did not want to comment all the
things i liked about your patch, but chose to pick the much smaller
amount of things I thought could use some improvement. I will try to
keep in mind to be more friendly next times. Also, not all of us are as
fluent in English as our native English speakers. I think its good to
keep in mind that for a lot of us, communication in English is not as
easy as it may seem. For me, it often requires a lot of concentration
to get the subtleties right. I mean: I can type some English text, look
up words in the dictionary, but to get small yet important subtleties in
my texts is very hard, often.
But, that all said: I liked your patch.
------------------------------------------------------------------------
June 10, 2005 - 07:08 : jakeg
How far off is this from being officially released or at least usable?
If its usable now, what files will I need to implement it?
------------------------------------------------------------------------
June 10, 2005 - 08:19 : Kobus
Attachment: http://drupal.org/files/issues/regions---possibility-3.png (2.21 KB)
To add to Stefan's suggestions with the two screenshots, I modified one
of them a bit, and this is how I understand Jaza's post, and yes, I
agree, this would be a wonderful feature! I can't wait! Imagine how
good it would look if I had a theme that has a clear white background
(not visible blocks) and then these seemingly random boxes on the page,
as shown in the screenshot. Especially where I can place it anywhere
around the node like Jaza said with a [region:2] block etc.
I say again: I can't wait!
Kobus
------------------------------------------------------------------------
June 10, 2005 - 09:59 : chx
I need 3-4 hrs of peace to implement the callback based thing. That's a
lot these days. Let's hope I'll get on it in the following days.
------------------------------------------------------------------------
June 18, 2005 - 18:32 : chx
Here's an example again of how hook_block will look like:
<?php
function module_block() {
$items = array();
$items['module_header'] = array('callback' =>
'mymodule_function', 'callback arguments' => arg(1), 'weight' => -50,
'region' => 'content', 'cache' => TRUE, ''pages' => "node/*\n<front>");
return $items;
}
?>
Items are keyed by unique IDs. The template system will call
module_invoke_all('block'), put the rendered HTML of the blocks into
regions sorted by weight. The rendered HTML is cached if it is allowed.
More keys will be introduced later, but now basically we just throw
everything from block configure in here, and add callback, callback
arguments, region.
Node teasers or fullly rendered nodes are blocks, too. Almost
everything is. So, here what happens when we are on taxonomy/term/1:
taxonomy_render_nodes is called
it calls block_set('node_view', $weight, $node, $teaser, $page); a few
times, so a few node_view blocks will be on the page
later the theme system is invoked. It will ask the block system to
collect all blocks, dynamic and the static blocks as well. For all
dynamic blocks we have a block defined in hook_block somewhere, like
the definition above, with one exception: it will have a 'standalone'
=> FALSE pair, so it is not a standalone block which is automatically
rendered but something that needs to explicitely requested. Also, we
may define a 'suffix' => 'somefunction' pair which provides us with a
cache ID suffix. Look at the code below:
<?php
function block_collect_blocks() {
// get all block definitions
$static_blocks = module_invoke_all('block');
// iterate of the dynamic blocks set on the page
foreach (block_get() as $dynamic_block) {
// get the block definition for the given dynamic block
$static_block = $static_blocks[$dynamic_block['block']];
// merge the callback arguments from the dynamic and static.
dynamic_block['callback arguments'] is always set though
// may be empty
if (isset($static_block['callback arguments'])) {
$dynamic_block['callback arguments'] =
array_merge($static_block['arguments'], $dynamic_block['callback
arguments']);
}
// try to get a cache ID suffix
$suffix = function_exists($static_block['suffix']) &&
call_user_func_array($static_block['suffix'], $dynamic_block['callback
arguments']);
// if the block is cacheable or the suffix function says that
it is cacheable by setting a suffix
if ($static_block['cache'] || $suffix) {
// set cache ID
$cid = 'block:'. $dynamic_block['block'] .':'. $suffix;
// try to get the rendered HTML from cache
if ($block_rendered = cache_get($cid)) {
$return[$static_block['region'][$dynamic_block['weight']][]
= $static_block_rendered;
continue;
}
}
// render the block
$block_rendered =
call_user_func_array($static_block['callback'],
$dynamic_block['callback arguments']);
// store the block for return
$return[$static_block['region'][$dynamic_block['weight']][] =
$static_block_rendered;
if ($static_block['cache'] || $suffix) {
// if cacheable, do it
cache_set($cid, $static_block_rendered);
}
}
foreach ($static_blocks as $key => $static_block) {
// is this a stand alone block, like in Drupal 4.6?
if ($static_block['standalone']) {
// now, the above is repeated, save that we do not have a
dynamic_block here
$suffix = function_exists($static_block['suffix']) &&
call_user_func_array($static_block['suffix'], $static_block['callback
arguments']);
if ($static_block['cache'] || $suffix) {
$cid = "block:$key:$suffix";
if ($block_rendered = cache_get($cid)) {
$return[$static_block['region'][$static_block['weight']][]
= $static_block_rendered;
continue;
}
}
$block_rendered =
call_user_func_array($static_block['callback'], $static_block['callback
arguments']);
$return[$static_block['region']][$static_block['weight']][]
= $static_block_rendered;
if ($static_block['cache'] || $suffix) {
cache_set($cid, $static_block_rendered);
}
}
}
}
function block_set() {
static $blocks;
$args = func_get_args();
if (count($args)) {
$callback = array_shift($args);
$weight = array_shift($args);
$blocks[] = array('block' => $function, 'weight' => $weight,
'callback arguments' => $args);
}
return $blocks;
}
function block_get() {
return block_set();
}
?>
When to expire the block cache? Never. If functions can call
cache_clear all, they can be smart enough to cache_clear only those
blocks that are changed by them. Some blocks won't be cacheable
anyways, say there is little use in caching 'active forum topics'
------------------------------------------------------------------------
June 19, 2005 - 03:15 : Dries
Frankly, I don't understand what you are trying to say. What is a
'dynamic', a 'static' and a 'standalone block'? What is the suffix
for? What is the advantage of making node and taxonomy views blocks?
If there is no big advantage, this is just bloat. All in all, it looks
pretty complex to me.
If we make Drupal more flexible _and_ more complex, people will get
upset, and we end up being like Plone. Drupal's simplicity is
something to foster, as it allows people to go in, and change stuff.
The focus is to make Drupal simpler _and_ more flexible.
I merely spent 10 minutes looking at this (reading your explanation
twice, staring at the code), and the newly added complexity simply
turns me off. Maybe that is because I don't understand what we are
gaining.
------------------------------------------------------------------------
June 19, 2005 - 03:34 : Bèr Kessels
* The first thing we gain, is, very slmple: more regions.
But, how many? what regions? on what pages? in what location? bearing
which name? For all these issues we need some callback system. To allow
themers, administrators and module developers to give answers to all
these questions. what we have now: sidebars, with only admin settings,
is simply too limited: It takes serious hacking (~15 lines of code in
your theme) to get a login block in your header!
* The second thing we get for free: We are passing along chuncks of
HTML, so why not cache them in a central place.
Practice points out that our current cache is not used well. We leave
it to developers to cache. And developers are lazy by nature. Just
looks at all the places where caching should have been done, but is
not. 9/10contribs have at least one of these places.
You must actually see these issues separate. The new central blocks
pulling system, and the caching. the latter is something that we get
for free, a high level of advanced caching (how often has not been put
forward Drupal seiously lacks better/advanced caching?). But we get it
only for free if it is co-developed with the new regions blocks.
And Last but not least, everything is a block, from a themer point of
veiw. I know developers tend to think: I do the content, and do not
care about where and how it turns up. That results in a content-centric
system, what we have now. Everythng is built around aa single chunk of
content. while, in fact content, is yet another block. Look at
http://www.terminus1525.ca/index.php?l=en or
http://www.franklinpennsylvania.us/ as it stands now, its up to the
developer to concenate all the HTML in the center area, into a huge
chunk of HTML. While in fact everyone can see that these are a lot of
separate blocks.
------------------------------------------------------------------------
June 19, 2005 - 03:39 : Dries
Ber: I was talking about chx's latest changes, not the block system in
general.
------------------------------------------------------------------------
June 20, 2005 - 14:06 : nedjo
I appreciate the update on directions being contemplated, although I
find it confusing.
Again, I'd caution, let's keep focused on the aim at hand, and on a
staged solution. A bigger patch means greater complexity, more
difficulty getting meaningful review, and greater delays in getting a
priority change. We're currently in a position where we have two
hard-coded regions for blocks, a major issue if nothing else than
because it reflects poorly on Drupal's usability and versatility.
Moving from here directly to something like "everything you see
anywhere is a (nested) block in its own (nested) region" strikes me as
a bigger step than we need to take.
And in belated reply to Ber's note (#53 above [11]), thanks for the
thoughtful comments, I didn't mean to single you out and acknowledge
that language & cultural differences play a role, I feel we can all
benefit from attention to communication.
[11] http://www.drupal.org/node/16216#comment-29633
------------------------------------------------------------------------
June 21, 2005 - 15:39 : Dries
Even with the explanation in chx's sandbox, this is too complex for my
likings. Here is a suggestion: let's drop the caching and the
'suffix'-stuff. Let's also drop the 'complete' and 'incomplete' stuff.
Keep it _simple_, chx. IMO, you're creating a monster.
------------------------------------------------------------------------
June 22, 2005 - 16:47 : chx
nedjo, take over. I am out.
------------------------------------------------------------------------
June 22, 2005 - 18:51 : djnz
Quick and dirty solution? Let blocks be assignable to 9 locations - Top
left, middle, right, middle left, middle right, bottom left, middle,
right.
The actual location of these areas within a theme is up to a theme
designer, but the designer should include each of them - how about
$sidebar['tl'], $sidebar['tm'], $sidebar['tr'] ...
Backwards theme compatibility could easily be maintained by only
creating the $sidebar array if the theme has set (eg in its
template.php file for a PHPtemplate theme) a flag, otherwise generate
$sidebar_left from the blocks set (in sequence) to top left, middle
left, bottom left, top middle and middle middle (arbitrary break) with
$sidebar_right the remainder.
I appreciate I have no karma here and only superficial knowledge of the
Drupal code, but sometimes the clarity of a view unclouded by too much
information is useful ;)
1
0
Issue status update for http://drupal.org/node/25816
Project: Drupal
Version: cvs
Component: base system
Category: bug reports
Priority: normal
Assigned to: Anonymous
Reported by: robertDouglass
Updated by: robertDouglass
Status: patch
Attachment: http://drupal.org/files/issues/bootstrap_patch.txt (540 bytes)
case 'page cache':
require_once './includes/bootstrap.inc';
robertDouglass
1
0
[drupal-devel] [bug] file_copy returns original (not final) filename on FILE_EXISTS_RENAME
by ejort 27 Jun '05
by ejort 27 Jun '05
27 Jun '05
Issue status update for http://drupal.org/node/24923
Project: Drupal
Version: cvs
Component: file system
-Category: feature requests
+Category: bug reports
Priority: normal
Assigned to: Anonymous
Reported by: cidenton
Updated by: ejort
-Status: active
+Status: patch
Attachment: http://drupal.org/files/issues/file.inc.update_filename.patch (623 bytes)
Is there anybody who has an opinion on this? Personally I find it
counter-intuitive for the filepath to be updated but not the filename,
and I can't think of any uses where the original filename is
preferable to the actual filename. In any case the caller still has
access to the original filename if they care about it, what's returned
should reflect reality IMO.
I've been using this patch (against 4.6) so that the flexinode file
field is usable, as cidenton said it's a trivial fix, but without it
any file that is uploaded through the flexinode file field that must be
renamed cannot be downloaded (making this unusable in production).
Because of this I'm changing this from a feature request to a bug
report.
Cheers,
Eric
ejort
Previous comments:
------------------------------------------------------------------------
June 14, 2005 - 02:22 : cidenton
The file_copy function in file.inc provides automatic file renaming in
case of name conflict, but doesn't put the final name in
$file->filename. This makes it harder for modules to determine whether
they own a file presented to hook_file_download(); in particular,
flexinode modules will fail to determine that they own a file that got
renamed.
If it is agreed that the final filename should be returned in
$file->filename, that behavior is easily imlemented by updating
$basename as well as $dest around line 261. I will be happy to build
and test a patch for this. If the current behavior is by design, I
will hack on my flexinode module fields instead. What is the call?
Regards,
Claude
1
0
Issue status update for http://drupal.org/node/7582
Project: Drupal
Version: cvs
Component: node system
Category: bug reports
Priority: normal
Assigned to: killes(a)www.drop.org
Reported by: killes(a)www.drop.org
Updated by: killes(a)www.drop.org
Status: patch
hehe, one only has to whine and bug enough and one gets some feedback.
;)
@junyor: legacy.module would be a good place. my current idea is to
auto-enable it in update.php and then disable it again in legacy_cron
after all nodes are updated.. ;)
@chx: When somebody wants to look at revisions of a node that node
could be auto-updated.
The only problem are contrib modules: they's need to have some hook in
order to update their own data. When somebody looks at the revisions of
a node than cannot be updated because the contrib module in question has
no such hook, we can optionally let the user discard old revisions I
guess.
Dries, what do you think?
killes(a)www.drop.org
Previous comments:
------------------------------------------------------------------------
May 5, 2004 - 18:25 : killes(a)www.drop.org
Currently all node revisions are stored in a serialized field in
node.table and retrieved for _each_ page view although they are rarely
needed. However, we have agreed that serializing data is bad and that
we should try to keep the memory foot print pf Drupal small.
Therefore I propose to create a separate revisions table which would be
in principle identical to the node table, only that it could have
several old copies of the same node. Extra data added by other modules
could be added in a serialized field unless we find a better solution.
------------------------------------------------------------------------
May 5, 2004 - 19:06 : jhriggs
I too think the serialized approach is less than desirable, but here's
an alternative. This would likely take some considerable rework in
core and contrib, but the following is how we handle similar types of
situations in our databases at work. It is more elegant that a
separate table, and avoids the (almost exact) duplication of a table.
Instead of separate tables, keep all revisions of nodes in the node
table as follows:
* add field: active (0/1 or Y/N)
* add field: revision
* every revision of a node is stored in the node table; however, only
one revision can be active at any given time
* nid can no longer be unique -- primary/unique key becomes (nid,
active)
* any time a node is loaded, updated (without revision), etc., the
active version is used.
Thoughts?
------------------------------------------------------------------------
May 5, 2004 - 19:57 : killes(a)www.drop.org
I am not opposed to your scheme, but I want to stress the following:
* Duplicating a table's structure is not bad (IMHO) as long as the
content is different.
* having two tables will allow us to have a rather small node table.
This is (maybe) a performance gain.
------------------------------------------------------------------------
May 5, 2004 - 20:37 : jhriggs
I don't necessarily think that duplicating a table's structure is _bad_.
It just seems to be wasteful and a pain to maintain. (Every change to
the node table is made twice...easy to do, but also easy to miss
perhaps.)
As for performance, as long as nid and the active indicator are
indexed, there shouldn't be any performance loss. Also, archiving an
old version when making a new revision will be much simpler: just
change the active indicator rather than copying an entire node to
another table (and ensuring everything gets copied...again a potential
maintainance issue).
To be honest, I would just like to see the serialized data go away,
regarless of what approach is taken.
------------------------------------------------------------------------
July 30, 2004 - 21:49 : Nick Nassar
Attachment: http://drupal.org/files/issues/Drupal-Improved_Revision_Schema_07-30-2004.p… (10.47 KB)
I'm interested in using Drupal for a large scale wiki-type project. In
order to do this, I need revisions to be in their own table.
Attached is a patch to do just that. Most of the changes are pretty
self explanatory. Spreading out node data across two tables meant that
I had to add database functions to do locking/transactions. Without
this, race conditions in which the database becomes corrupted are
possible.
------------------------------------------------------------------------
July 30, 2004 - 21:54 : Nick Nassar
Oh yeah... The patch is a diff against Drupal CVS
------------------------------------------------------------------------
July 31, 2004 - 02:00 : Anonymous
Gerhard speaking.
Nick, thanks a lot for your nice patch! It saves me a great deal of
labour. I looked through it and immediately liked it. You not only put
the old revisions into a new table but also the current one. Do you
have an estimate how much more expensive the additional join is?
Besides a few minor coding style issues I found a major one: Just a few
hours before you uploaded your patch JonBob's node access patch hit
core. That means your patch won't apply anymore as all the queries you
change have been changed. Can I bug you to update your patch?
------------------------------------------------------------------------
July 31, 2004 - 03:11 : Anonymous
Also I think that your upgrade path loses existing revisions.
------------------------------------------------------------------------
July 31, 2004 - 04:39 : drumm
I think this is the proper way to do things. No columns are duplicated,
there is no serialized data, and only the fields that are logically
revised are stored. Nothing jumped out at me as a way to have my node
modules be able to keep a table of revisions of additional fields. I'm
guessing this could be done within the confines of _insert and _update.
Assuming the upgrade path works and modules can extend it I give it a
+1.
------------------------------------------------------------------------
July 31, 2004 - 16:40 : Nick Nassar
It figures that just as I finish a big patch, another patch comes along
and breaks it. Oh well, it should be a pretty easy to fix. I'll work on
it.
Fixing the upgrade path to keep revisions should be fairly painless.
I found another issue that needs to be fixed before this patch gets
merged. There format of a node needs to be stored for each revision.
Otherwise, for modules that store a format for the nodes, such as page
and book, if you write one revision in PHP and the next in HTML, the
PHP revision will be displayed as HTML. This is part of a larger issue
of how node modules should store revisions of additional fields. I
think each module that wants to do this should create another table
with (nid, revid) as the primary key. Just as when they want to add
fields to a node they create another table with nid as the primary key.
As far as performance goes, for sites that make heavy use of revisions,
an extra join on primary keys is going to be a lot faster than grabbing
all of the revisions from that database everytime. We would need to run
benchmarks to determine is the overall difference in speed is for an
average site is a gain or a loss. I'm guessing it's very minor either
way.
------------------------------------------------------------------------
August 23, 2004 - 15:55 : Nick Nassar
Attachment: http://drupal.org/files/issues/Drupal-Improved_Revision_Schema_08-23-2004.p… (10.92 KB)
Here's an updated patch against CVS that puts revisions in their own
table, provides an upgrade path, and fixes the format related bugs in
the last patch.
Hopefully, this can make it into CVS as soon as the freeze is over.
------------------------------------------------------------------------
August 23, 2004 - 16:10 : moshe weitzman
Interesting patch ... drumm's question is still outstanding. how do
modules store revisions of their fields? Are they expected to manage
this on their own? Thats not how it works today.
As an aside, i am seeing profile_ fields in my node.revisions column.
One could argue that those need not be saved. They pertain to the node
author, not to the node itself.
------------------------------------------------------------------------
August 23, 2004 - 18:14 : Nick Nassar
Having modules be responsible for storing revisions of their own fields
is a side-effect of storing revision data in tables. There's really no
way around it. However, revisions generally don't make sense for node
types that don't have PHP/HTML content, such as polls. I think it's
going to be a pretty rare scenario for a new node type to want another
field to change per-revision, so it's a pretty good trade-off.
Storing fields that shouldn't be part of revisions, such as the
profile_ fields, is a side-effect of storing revisions as serialized
objects. Applying this patch will free up that wasted space. :)
------------------------------------------------------------------------
August 23, 2004 - 19:20 : Anonymous
There should be a hook that let's the module choose whether it supports
history. This way a module author can prevent the user from doing
something that may break his module or just cause undefined behavior.
If the module doesn't support history then don't let the user/admin
choose to add history to nodes of that type.
Craig
------------------------------------------------------------------------
August 23, 2004 - 21:23 : Nick Nassar
I agree, there should be an API change to make specifying support for
revisions easier. In the interests of keeping patches small and keeping
to one change per patch, I think the API change should be a separate
issue.
A sort of ad-hoc API to decide whether or not a module supports
revisions by default already exists. Instead of having a hook, modules
set the default value of the "Create new revision" field in the edit
form. The admin can change this option in
admin/node/configure/defaults. This patch doesn't change that.
Revisions are broken for node types that have their own database
structure, like polls, even when storing them as serialized objects.
This patch doesn't change that, either.
------------------------------------------------------------------------
October 26, 2004 - 04:35 : moshe weitzman
I'm guessing that someone is going to have to demonstrate that this
patch performs as well as current drupal before it gets comitted. i
think this patch is a few benchmarks from being comitted.
------------------------------------------------------------------------
October 27, 2004 - 03:04 : Nick Nassar
Attachment: http://drupal.org/files/issues/Drupal-Improved_Revision_Schema_10-26-2004.p… (11 KB)
I ran some really unscientific benchmarks, and it looks like this patch
has a negligible affect on performance.
I used apache bench and the database from theregular.org, which doesn't
contain any revisions (worst case scenario for this patch) and contains
several hundred nodes. Both the patched and unpatched versions hovered
between 2.36 and 2.38 requests per second.
The command I used was:
ab -n50 -C 'PHPSESSID=b01a9f92880ef215b0ed6f1314a5eba2'
http://192.168.0.100/
An updated patch that should apply to CVS is attached.
------------------------------------------------------------------------
October 27, 2004 - 03:05 : Nick Nassar
I ran some really unscientific benchmarks, and it looks like this patch
has a negligible affect on performance.
I used apache bench and the database from theregular.org, which doesn't
contain any revisions (worst case scenario for this patch) and contains
several hundred nodes. Both the patched and unpatched versions hovered
between 2.36 and 2.38 requests per second.
The command I used was:
ab -n50 -C 'PHPSESSID=b01a9f92880ef215b0ed6f1314a5eba2'
http://192.168.0.100/
An updated patch that should apply to CVS is attached.
------------------------------------------------------------------------
October 27, 2004 - 03:05 : Nick Nassar
Attachment: http://drupal.org/files/issues/Drupal-Improved_Revision_Schema_10-26-2004.p… (11 KB)
I ran some really unscientific benchmarks, and it looks like this patch
has a negligible affect on performance.
I used apache bench and the database from theregular.org, which doesn't
contain any revisions (worst case scenario for this patch) and contains
several hundred nodes. Both the patched and unpatched versions hovered
between 2.36 and 2.38 requests per second.
The command I used was:
ab -n50 -C 'PHPSESSID=b01a9f92880ef215b0ed6f1314a5eba2'
http://192.168.0.100/
An updated patch that should apply to CVS is attached.
------------------------------------------------------------------------
November 15, 2004 - 07:05 : elias1884
please overthink the revision system default workflow as well. don't
look at the revision system as an isolated system but as a part of the
whole workflow system!
if you combine revisions with the moderation queue the most logic
default workflow would be like that:
auth user creates node (revision #0)
admin approves the node (status = 1, moderation = 0)
=> node publicly available
auth user finds typo and changes node (revision #1, status = 0,
moderation = 1)
-------
what happens at that point at the moment is, that the node is not
accessible anymore at all until the new revision is approved by admin.
of course the new revision should not go online until reviewed and
approved, this is absolutely correct, but there is no reason to not
take the old revision offline, since it was already approved and should
therefore be online until the new revision is approved. it is not
practical if a node disappears only because the author corrected a
typo.
-------
admin approves the node (status = 1, moderation = 0)
eventhough I first thought a plain boolean active field would not be
capable of providing that functionality if finally came to the
conclusion, that it can. The only thing to do is to not set that bit,
when a new revision is created, but when it is approved (in case
moderation is activated under default workflow). Every revision should
have its own moderation, status and active field and on approval they
are set like this (status=1, moderation=0, active=Y).
When you wanna rollback to an old revision, you can chose between all
revisions that already have the moderation bit set back to 0 again and
the published to 1. There should be an extra permission for rollback!
another concern that I have about the default workflow is, that users
can't see the content, they have just created, when moderation is
enabled. Eventhough, there is a big fat "submission accepted" presented
after submissions, unexperienced users tend to question the information
those stupid tincans give them, if they can't find their content
afterwards. Many users are really lazy bastards and they don't even
read the status messages. The best feedback about whether his story was
submitted successfully or not of course is, if he can find the story
somewhere on the site, maybe with a status message on top of it,
mentioning, that the content is currently not publicly available since
it has not been approved yet. there should be a my content section
under my account, like somebody is trying to do with the workspace
module I guess.
so my suggestion is to make (status=0, moderation=1) still available
for the creator under a my content section somewhere!
------------------------------------------------------------------------
November 24, 2004 - 06:21 : Nick Nassar
I agree. The current workflow for moderation queues and revisions needs
to change, but this patch isn't the place for it. The patch is already
too big, and it only does the backend stuff.
Instead of adding more to this patch and making it take even longer to
get into core, would you mind creating a new issue for your UI
suggestions, so the those changes can be added as a separate patch?
Thanks,
Nick
------------------------------------------------------------------------
December 11, 2004 - 14:26 : Dries
This patch is _much_ needed so I'd love to see someone revive it. In
order for this patch to be accepted, the following needs to be done:
* Update this patch to CVS HEAD.
* Rename revid to vid.
* Rename node_rev to node_revisions.
* Rename node_rev.changed to node_revisions.timestamp.
* Rename $rnode to $revision.
* Fix the coding style to match Drupal's: proper spacing, single quotes
where possible, proper variable names.
* Benchmark this patch with a large database with enough revisions.
I'd be happy to benchmark this on my local copy of the drupal.org
database.
* The book.log field should probably move to the node_revisions table.
This can be done in a separate patch.
* Investigate whether transactions are well-supported.
------------------------------------------------------------------------
December 13, 2004 - 02:25 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/Drupal-Improved_Revision_Schema_10-26-2004-r… (11.02 KB)
I've worked a bit on the patch (coding style issues as mentioned by
Dries). One thing I noticed is that the patch uses REPLACE. IIRC this
needs to be chagned to "UPDATE, if fail INSERT" for pgsql
compatibility.
Nick, are you still interested in working on that patch? I'd like to
know how it works on your site and work on getting it into core.
------------------------------------------------------------------------
December 13, 2004 - 14:33 : Dries
Gerhard: your patch does not apply.
------------------------------------------------------------------------
December 13, 2004 - 19:10 : killes(a)www.drop.org
Yes, I know, that was the same version as I mailed to you earlier.
------------------------------------------------------------------------
December 13, 2004 - 23:02 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions.patch (52.96 KB)
Ok, upüdated the patch to cvs.
------------------------------------------------------------------------
December 14, 2004 - 10:58 : Dries
Some more comments:
* db_begin_transaction() and db_end_transaction() do not belong in
database.inc, but in database.mysql.inc and database.pgsql.inc
respectively.
* The node module calls node_revisionsision_list() which is not
defined. (Fxed that on my local copy.)
* Do db_begin_transaction() and db_end_transaction() deprecate Jeremy's
table locking patch?
* The upgrade path assigns the wrong user ID to each revision.
* The upgrade path assigns the wrong date to each revision (that or a
node's revision page shows the wrong usernames/dates).
* The coding style needs a bit of work, but we can worry about that
later.
------------------------------------------------------------------------
December 14, 2004 - 19:34 : Nick Nassar
If you need any help getting those things fixed, just let me know.
------------------------------------------------------------------------
December 14, 2004 - 19:50 : Nick Nassar
How this relates to Jeremy's node locking patch:
There was lots of discussion, and node locking was decided against
because from an end user point of view you never want a node to be
locked. He's now advocating for a much simpler patch that warns users
if their changes will overwrite someone elses. That patch still has a
race condition, which might be fixed using db_begin_transaction().
http://drupal.org/node/6025
------------------------------------------------------------------------
December 15, 2004 - 00:26 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_0.patch (55.96 KB)
Here is an updated patch that tries to address Dries concerns.
------------------------------------------------------------------------
December 15, 2004 - 10:32 : Dries
Attachment: http://drupal.org/files/issues/revisions-bug.png (76.06 KB)
It didn't fix the aforementioned bugs. See attached screenshot.
------------------------------------------------------------------------
January 6, 2005 - 22:15 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_1.patch (51.77 KB)
Ok, here is a new version. Dries and myself worked hart at it, so please
have a look.
what is still missing
- database upgrades for the core modules with an own table
- contrib modules need an upgrade too.
- do we need nid and vid in both the node and the node_revisions table?
- the amount of sql queries means a good stress testing for large
databases.
------------------------------------------------------------------------
January 19, 2005 - 23:43 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_2.patch (49.49 KB)
Here is an updated patch. We discussed to keep the current title in node
module and also in the revisiosn table. This is content duplication but
will save many joins as many queries only need the title of a node.
Discussion is welcome.
------------------------------------------------------------------------
January 20, 2005 - 01:33 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_3.patch (29.93 KB)
I've implemented the aforementioned solution. This makes the patch much
smaller. The patch now also removes taxonomy_node_has_term() which
wasn't used anywhere. I'd really apprciate if some people could test
drive the patch. It will be another huge improvement for 4.6.
------------------------------------------------------------------------
January 20, 2005 - 02:05 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_4.patch (30 KB)
Another revision. Steven didn't like my literal $node->vid in queries.
------------------------------------------------------------------------
January 20, 2005 - 03:10 : killes(a)www.drop.org
- database upgrades for the core modules with an own table
- contrib modules need an upgrade too.
- do we need nid and vid in both the node and the node_revisions table?
- the amount of sql queries means a good stress testing for large
databases.
These issues are still open, btw. Especially the first one needs to be
tackled.
------------------------------------------------------------------------
January 25, 2005 - 22:11 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_5.patch (51.13 KB)
Here is a patch that has the database tables updated for forum, book,
and page module.
------------------------------------------------------------------------
January 30, 2005 - 00:55 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_6.patch (49.18 KB)
Yet another update to keep it working with head. The patch now also
removes the table definitons for the page table.
------------------------------------------------------------------------
January 30, 2005 - 00:57 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_7.patch (55.69 KB)
Sorry, that was the old version, this is the right one.
------------------------------------------------------------------------
January 31, 2005 - 21:55 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_8.patch (55.71 KB)
Updated once more.
------------------------------------------------------------------------
January 31, 2005 - 22:52 : Dries
Anyone to help review/test this?
------------------------------------------------------------------------
January 31, 2005 - 23:22 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_9.patch (49.29 KB)
Updated again, the update functions occurred twice. Thanks Bart.
------------------------------------------------------------------------
February 2, 2005 - 02:27 : killes(a)www.drop.org
Don't know if the db I am using is corrupted or what. I still do have
some didficulties.
The latest patch is attached.
------------------------------------------------------------------------
February 2, 2005 - 02:27 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_10.patch (49.67 KB)
I am probably slowly going mad ...
------------------------------------------------------------------------
February 2, 2005 - 03:54 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_11.patch (48.95 KB)
The update issue still needs investigating. This patch is updated for
cvs.
------------------------------------------------------------------------
February 2, 2005 - 22:20 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_12.patch (49.83 KB)
Ok, here is a new version. I've solved my troubles with book.module.
There are still some issues with forum module. Possibly due to
inconsistent database.
------------------------------------------------------------------------
February 2, 2005 - 23:31 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_13.patch (49.83 KB)
Turns out the drupal.org database had indeed some quirks. Please run
this query in your oldest db and tell me the result:
select nid,type from node where type like '%/%';
If you get a non-zero result we might need to add another security
update.
The patch could use still more testing, though.
------------------------------------------------------------------------
February 3, 2005 - 03:16 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_14.patch (49 KB)
Ok, we are getting somewhere. At a first glance the update is working.
There is a problem remaining: the revisions tab will be shown whether
the node has revisions or not. Not sure we can/need to fix this.
People with a drupal.org account can log in at
http://killes.drupaldevs.org/revision/ and poke around. Your
permissions will be the same as on drupal.org. Feel free to vreak
everything but don't forget to file complaints here. (Note: this is
only a pruned version of the drupal.org database with all project nodes
and nodes with nids > 7000 dropped).
------------------------------------------------------------------------
February 3, 2005 - 06:19 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_15.patch (52.39 KB)
There was some error in node_save and also the patches to the
database.inc files got lost...
------------------------------------------------------------------------
February 3, 2005 - 09:07 : robertDouglass
Submitting book pages doesn't work on your test site. It puts the entire
content of the preview inside the body textarea. I wrote a sentence in
the body and the log, and pressing preview put several lines of HTML
containing both sentences in the body textarea on the preview page,
plus the book page wouldn't submit.
-R
------------------------------------------------------------------------
February 3, 2005 - 09:50 : Junyor
0 results here. I started using Drupal with version 4.4, though.
------------------------------------------------------------------------
February 4, 2005 - 01:56 : killes(a)www.drop.org
@Junyor: Thanks, that's a good sign. Maybe somebody else has an older db
to try.
@robertDouglass: The first effect you describe is due to drupaldevs
running on PHP 5. I am unsure why the second thing does not work. In
node_save() the node object has a nid although there is none in the
form. Very strange.
I've enabled display of db queries on the testsite.
------------------------------------------------------------------------
February 4, 2005 - 21:17 : dmjossel
No results here on the query:
select nid,type from node where type like '%/%';
On a database that was put in place prior to Drupal 4 and is now
running on 4.5.2.
------------------------------------------------------------------------
February 4, 2005 - 22:44 : killes(a)www.drop.org
@dmjossel: thanks.
@all. The strange problem I reported was apparently php 5 related.
After applying Steven's php 5 patch it went away. One error is
remaining: If I create a new forum topic it will be shown as part of
the book on preview. Hmm, that was due to a db that got corrupted
during testing so that is fixed too.
Please poke around at the test site and look if you find more errors.
------------------------------------------------------------------------
February 5, 2005 - 09:16 : Steven
By the way, I just remembered that Drupal.org has some blogs lingering
on in the database even though blog.module is not enabled. Perhaps this
is causing troubles?
------------------------------------------------------------------------
February 5, 2005 - 13:22 : Anonymous
I can't see why it would. Drupal.org will need extra updates for images
and project nodes because those have their own tables. GK.
------------------------------------------------------------------------
February 6, 2005 - 14:49 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_16.patch (52.49 KB)
Updated to apply to cvs again.
------------------------------------------------------------------------
February 22, 2005 - 22:15 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_17.patch (49.64 KB)
Updated again.
All we need is a patch to upload module and an upgrade path for it.
------------------------------------------------------------------------
March 4, 2005 - 06:22 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_18.patch (52.31 KB)
Updated once more. Moved log field from book to node_revisions table as
discussed in Antwerp. upload module still missing.
We need to decide under which circumstances the log field should be
displayed. Should that be added to the workflow? Should it depend on
the revisions setting?
------------------------------------------------------------------------
March 5, 2005 - 21:27 : Anonymous
Attachment: http://drupal.org/files/issues/revisions_20.patch (75.52 KB)
Ok, here it is: Yet another revision of this grrrrreat patch.
Changes from previous versions:
- supports versioning for uploaded files. A problem is that if you
delete a file, it will be gone for all revisions.
- the log field is now in the node_revsions table, but each module has
to decide whether to show it or not.
I've implemented it for the page and the book type odes. Also, the
field can be edited when adding non-book nodes to the book. The log is
displayed on the revisions page and if a node is moderated.
- the revisions are moved to an old_revisions table to a) get the node
table smaller and b) still leave the mavailable for contrib modules
that want to retreive old version data.
The patch has been applied to killes.drupaldevs.org/revision where it
can be tested by anybody (especially people who have "site admin"
rights on drupal.org)
The database is from drupal.org and you should b able to log in with
your pass or simply mail yourself a new one.
Gerhard
------------------------------------------------------------------------
March 5, 2005 - 21:51 : Anonymous
Attachment: http://drupal.org/files/issues/revisions_21.patch (59.42 KB)
BTW, I marked this a bug because atm the revisions field can grow quite
big. Neil has reported problems from some users who were not able to
load some nodes due to to many large revisions.
Also, som unrelated stuff crept into the patch. New version attached.
------------------------------------------------------------------------
March 8, 2005 - 07:56 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_22.patch (60.29 KB)
Ok, I think I got it.
Changes to last version:
- uploads are no properly versioned.
Missing are still pgsql checks and updates.
------------------------------------------------------------------------
March 10, 2005 - 18:58 : Anonymous
Was able to get http://drupal.org/files/issues/revisions_21.patch to
work with drupal-cvs.tar.gz (10 March 2005) by:
- includes/database.mysql.inc: Commenting out duplicates for functions
function db_begin_transaction and function db_commit_transaction
- modules: node.module: Removing "'title' => $node->title," from
$node_table_values variable declaration and removing "'title' =>
"'%s'"," from "$node_table_types" variable declaration.
Happy to submit a patch if requested. I'll watch this thread.
------------------------------------------------------------------------
March 12, 2005 - 01:59 : killes(a)www.drop.org
The duplicate function has been removed in rev 22 of this patch.
Why do you think the changes in node_save are needed? Titles are saved
in both tables for performance reasons.
------------------------------------------------------------------------
March 13, 2005 - 18:12 : jlerner
Hi - I posted comment #62. The changes to node_save appear to be needed
because recent patches (both 21 and 22) remove the field 'title' from
table 'node'. So without the changes to node_save, node.module is
broken and generates errors.
Joshua
------------------------------------------------------------------------
March 13, 2005 - 18:29 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_23.patch (61.17 KB)
Thanks, Joshua, for catching this. node:title is there to stay.
------------------------------------------------------------------------
April 13, 2005 - 18:29 : moshe weitzman
since HEAD is open again, perhaps it is a good time to revisit this
patch.
once this is committed, lets address - http://drupal.org/node/11071
"node_validate does not respect group editing"
------------------------------------------------------------------------
April 18, 2005 - 17:43 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_24.patch (60.39 KB)
Updated.
------------------------------------------------------------------------
April 18, 2005 - 18:16 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_24_0.patch (60.39 KB)
Updated.
------------------------------------------------------------------------
April 19, 2005 - 07:19 : Dries
I'll commit this patch later this week! If you haven't checked this
patch already, I urge you to test/check it out because it will have
significant impact on existing code and modules!
------------------------------------------------------------------------
April 19, 2005 - 07:21 : Dries
Also, what do people think about the n.title being duplicated?
------------------------------------------------------------------------
April 19, 2005 - 07:26 : chx
I won't lose any sleep because of duplicated titles...
------------------------------------------------------------------------
April 19, 2005 - 20:35 : killes(a)www.drop.org
Let me explain why I have chosen to duplicate the title (and also the
uid): If you look at all the queries in Drupal, you will find that most
of them only need the title and th uid of a node. That is, by
duplicating it, we save expensive joins on the node_revisions table.
Due to this fact, this patch is actually a performance improvement.
A note about updating contrib module:
Strictly speaking they wouldn't need to be updated. They only need to
if their authors decide that their info should be available for
revisioning. The upgrade path for forum.module in my update.inc patch
(+ the forum patch)
should show you what needs to be done.
I will write a note for the update page once the patch hits core.
------------------------------------------------------------------------
April 24, 2005 - 23:21 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_25.patch (60.38 KB)
Updated to cvs.
Dries: Based on some remarks in #drupal this is the last update I am
going to do. Apply it or won't fix it.
------------------------------------------------------------------------
April 30, 2005 - 05:42 : Jeremy(a)kerneltrap.org
Attachment: http://drupal.org/files/issues/revisions_25.patch.patch (528 bytes)
That's a big patch. I've only started looking through it. I noticed
one little typo, affecting updates. A patch to your last patch is
attached.
I'm running with the revision patch on my dev server now happily. I
like the concept.
What happens if you click 'stop' on your browser in the middle of a
MySQL "transaction"? I assume that kills the connection to MySQL, and
the lock is freed? But this then leaves changes only partially
applied?
What exactly does locking/unlocking the tables buy us in MySQL? I
don't see anywhere that we detect if an apply fails part way through,
and thus roll back...? What am I missing?
------------------------------------------------------------------------
April 30, 2005 - 09:11 : Dries
Jeremy: many of us are worried about the performance ramifications this
patch introduces. Early experiments showed a small performance
improvement (while a performance regression might be expected). More
performance reports from large sites like kerneltrap.org will certainly
help this patch. Mind to do a quick performance comparision and to
report back with some numbers? Thanks.
------------------------------------------------------------------------
April 30, 2005 - 14:38 : Jeremy(a)kerneltrap.org
Dries: I'm not running HEAD on kerneltrap, so this really isn't a
possibility. Furthermore, until I understand why we're locking tables,
I don't like it. The idea of revisions in their own tables is great.
The idea of locking tables to get (without any obvious benefit) there
really worries me.
------------------------------------------------------------------------
April 30, 2005 - 16:16 : killes(a)www.drop.org
@Jeremy: Thanks for looking at the patch! Also for catching the typo. :)
Did you try to upgrade your database? If yes, how did it go? One of
Dries' concerns is the complexity of the upgrade. How many nodes and
revisions did the db have?
About database locking: This part of the patch was created by Nick and
I simply continued to use it.
Maybe the code should rather be:
if(db_begin_transaction(array('{node}', '{node_revisions}',
'{watchdog}', '{sessions}', '{files}'))) {
db_query($node_query, $node_table_values);
db_query($revisions_query, $revisions_table_values);
db_commit_transaction();
...
}
The idea is probably to avoid two updates at the same time. I don't
think the locking helps if you abort the script at an inconvenient
time. Rollbacks aren't implemented in all mysql versions.
We could omit the db locking if deemed inappropriate. Maybe Nick can
explain his ideas behind this.
@Dries: I wonder who the "many of us" are. They certainly haven't
spoken to me. Moshe had some reservations about the upgrade path and
project module, but the time that project module abused revisions to
store issue updates was long ago and his reservations were resolved.
Nobody else (besides you of course and now Jeremy) has voiced
reservations in a way that was audible to me.
If you grep through the patch you will notice that there are only four
queries which have a join on the node_revisions table. Two of them are
in node_load and in the other cases the join replaced a join on the
node table. The two queries in node_load are the only ones that have
both a join on the node and the revisions query. Thus, loading of
individual nodes might become somewhat slower. All other queries will
be faster since the node table is now much smaller. Also, node loading
does not have to be slower, it depends on your node table. If you had
a lot of revisions and thus a large table, then the new scheme will
make your queries actually faster since we do not load the revisions
on each and every node load anymore. If you didn't have many revisions
your node_load migth be somewhat slower.
WRT to the update script Karoly pointed out that we could use multiple
insert queries instead one query per revision. This would probably
make the update somewhat faster. I am willing to work on this iff you
declare that you will commit the patch afterwards. I'd need to know if
this will work on pgsql and on all supported mysql versions before I
work on it.
About locking: Database locking is dog slow, at least on mysql. I was
using locks in an earlier version of the upgrade script but had to
remove it for (serious!) performance reasons.
------------------------------------------------------------------------
May 9, 2005 - 17:07 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_26.patch (46.45 KB)
Ok, another update, cause I need it myself.
I've left out the transaction stuff for now. It is in principle
unrelated to this patch and should be discussed elsewhere.
This also makes the patch smaller and easier to review (hint, hint).
------------------------------------------------------------------------
May 9, 2005 - 22:32 : killes(a)www.drop.org
The patch contained the update functions twice.
------------------------------------------------------------------------
May 9, 2005 - 22:32 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_27.patch (39.05 KB)
The patch contained the update functions twice.
------------------------------------------------------------------------
May 9, 2005 - 23:23 : Dries
Got one error during the upgrade path:
ALTER TABLE {book} ADD PRIMARY KEY vid (vid)
FAILED
------------------------------------------------------------------------
May 9, 2005 - 23:26 : killes(a)www.drop.org
This had happend to me as well, when I tested this patch. The reason is
that for some reason the vid is not unique. Most likely there are some
entries with vid = 0 in there. Can you check which node types those
have? it always was an error in the test database. See:
http://drupal.org/node/7582#comment-20678
------------------------------------------------------------------------
May 9, 2005 - 23:27 : Dries
Actually, I got 2850 errors during the upgrade.
Some of these:
sprintf() [function.sprintf]: Too few arguments in
drupal-cvs/includes/database.inc on line 154.
Some of these:
Query was empty query: in drupal-cvs/includes/database.mysql.inc on
line 66.
And this:
Unknown table 'n' in field list query: SELECT n.nid, n.vid FROM node
INNER JOIN files f ON n.nid = f.nid in
drupal-cvs/includes/database.mysql.inc on line 66.
:-)
------------------------------------------------------------------------
May 9, 2005 - 23:29 : Dries
Or this:
user error: Unknown column 'log' in 'field list'
query: SELECT parent, weight, log FROM book WHERE nid = 1 in
drupal-cvs/includes/database.mysql.inc on line 66
------------------------------------------------------------------------
May 9, 2005 - 23:52 : Dries
The time required to generate my main page went from 902 ms (before
upgrade) to 2139 ms (after upgrade).
The time required to generate a forum listing (?q=forum/x) went from
1872 ms (before upgrade) to 2874 ms (after upgrade).
Maybe this is because my database is not consistent as the result of
the upgrade errors (yet I don't see any errors on the pages I
benchmarked).
------------------------------------------------------------------------
May 10, 2005 - 02:24 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_28.patch (53.47 KB)
Ok, let me get to this from the bottom to the top:
- my test runs indicated a different development wrt timing. If I had
gotten your results, I had stopped working on this long ago. So your
results are wrong for some reason.
- user error: Unknown column 'log' in 'field list'
Wasn't my day, the book patch got lost. It is contained now. First -R
the old patch, then apply this one.
- Unknown table 'n' in field list query:
Walkah found this, but I forgot to fix it. Fixed now.
- I've no idea where the other queries come from. I am hoping that
either your test db is borken or they are follow ups from the other
ones.
If you let me have your test db, I'll try some debugging.
Thanks for wasting your time, too.
------------------------------------------------------------------------
May 10, 2005 - 07:07 : Dries
I double-checked and the numbers don't seem to lie. I'll test some more
after work on another machine to make sure it is not platform-specific.
------------------------------------------------------------------------
May 11, 2005 - 05:32 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_29.patch (54.83 KB)
Ok, here I am again.
What I did:
1) Ask Dries to let me have drupal.org database
2) get 400MB of SQL inserts...
3) take 23 minutes to import said data
4) Remove all image and project nodes (don't want to install their
modules), 11765 nodes left
5) back up data
6) take tests on non-cached /node page (as anonymous user).
ab results:
-c 1 -n 25:
Requests per second: 1.29 [#/sec] (mean)
Connection Times (ms)
min mean[+/-sd] median max
Total: 663 775 179.7 689 1264
7) Do the same for the tracker page:
Requests per second: 0.83 [#/sec] (mean)
Total: 1182 1199 7.4 1199 1217
8) Apply my patch (rev. 28).
9) run db update and hold breath
10) update times out...
11) play back backup from 5)
12) wait
13) getting annoyed and removing cache, watchdog, and accesslog before
playing in backup.
14) wait again. Understand why Dries doesn't try this patch often.
Maybe a smaller DB would do for testing?
15) wait more. get really annoyed.
16) Set time limit to 18000 in update.php
17) try again
18) fails again before the second update is completed.
19) curse.
20) delete search stuff from db. Ooops, sooo much smaller...
21) import again, below 2 minutes...
22) rewrite to use extended insert. Found a bug.
23) still does not complete. Mysql logging to the rescue!
24) tid = 0? Not good.
25) Well, the update works fine till node 10834. 5595 nodes done, 6136
to go.
26) Writing shell based update script. Discovery: 24MB aren't enough.
Hopefully 64 are. Nope.
extended inserts for revisions are apparently not the brightest idea:
Huge memory consumption.
Hmm, no, all updates got through. Selecting the revisions to put them
into old_revisions table screwed it. Learned about CREATE TABLE
old_revisions SELECT syntax.
Yay! finally. 24 MB are just not enough the update.php script seems to
still break.
27) Benchmarks!
/node
Requests per second: 1.54 [#/sec] (mean)
Connection Times (ms)
min mean[+/-sd] median max
Total: 632 649 40.5 636 791
/tracker
Requests per second: 0.86 [#/sec] (mean)
Total: 1119 1165 65.8 1160 1461
Ok: So we get an improvement for many node_loads, but none for simple
selects from node.
More tests can be done.
28) roll new patch
Ain't Drupal fun?
------------------------------------------------------------------------
May 18, 2005 - 15:38 : Dries
I did another round of tests on _another_ machine and it is not looking
good:
Before upgrade After upgrade
?q= (main page) 218 ms/request 340 ms/request
?q=forum (forum overview) 754 ms/request 1520 ms/request
?q=about (book page) 375 ms/request 5400 ms/request
The upgrade process itself gave me a number of 'query was empty' and
'sprintf(): too few arguments' reports. Everything seems to work fine
though.
Looking at the ?q=about page, I see that the following query is
executed twice _and_ that each time, it take more than 2 seconds to
complete:
SELECT n.nid, n.title, b.parent, b.weight FROM node n INNER JOIN book b
ON n.vid = b.vid WHERE n.status = 1 AND n.moderate = 0 ORDER BY
b.weight, n.title;
--8 SHOW INDEX FROM book;
+-------+------------+-------------+--------------+-------------+-----------+-------------+----------+--------+------+------------+---------+
| Table | Non_unique | Key_name | Seq_in_index | Column_name |
Collation | Cardinality | Sub_part | Packed | Null | Index_type |
Comment |
+-------+------------+-------------+--------------+-------------+-----------+-------------+----------+--------+------+------------+---------+
| book | 1 | book_parent | 1 | parent | A
| 92 | NULL | NULL | | BTREE | |
| book | 1 | nid | 1 | nid | A
| 369 | NULL | NULL | | BTREE | |
+-------+------------+-------------+--------------+-------------+-----------+-------------+----------+--------+------+------------+---------+
2 rows in set (0.00 sec)
The book module does not appear to have a primary key? Sounds like a
bad idea so I added one:
mysql> ALTER TABLE book ADD PRIMARY KEY nid (nid);
Query OK, 369 rows affected (0.02 sec)
Records: 369 Duplicates: 0 Warnings: 0
Next, I wanted to make the vid column a unique key in all node tables:
mysql> ALTER TABLE node ADD UNIQUE vid (vid);
Query OK, 20392 rows affected (0.81 sec)
Records: 20392 Duplicates: 0 Warnings: 0
mysql> ALTER TABLE book ADD UNIQUE vid (vid);
ERROR 1062: Duplicate entry '0' for key 2
mysql> ALTER TABLE forum ADD UNIQUE vid (vid);
Query OK, 10806 rows affected (0.10 sec)
Records: 10806 Duplicates: 0 Warnings: 0
As you can see, it fails for the book table which makes me believe
there is some inconsistent data ... I set out to fix that:
mysql> SELECT nid, COUNT(nid) AS vids FROM book GROUP BY vid HAVING
vids > 1;
+-----+------+
| nid | vids |
+-----+------+
| 871 | 2 |
+-----+------+
1 row in set (0.00 sec)
mysql> SELECT title FROM node WHERE nid = 871;
Empty set (0.00 sec)
mysql> DELETE FROM book WHERE nid = 871;
Query OK, 1 row affected (0.00 sec)
mysql> ALTER TABLE book ADD UNIQUE vid (vid);
Query OK, 368 rows affected (0.01 sec)
Records: 368 Duplicates: 0 Warnings: 0
Looks like everything is well now. Ran some new benchmarks:
Before upgrade After upgrade With
indices
?q= (main page) 218 ms/request 340 ms/request 336
ms/request
?q=forum (forum overview) 754 ms/request 1520 ms/request 1531
ms/request
?q=about (book page) 375 ms/request 5400 ms/request 475
ms/request
Unfortunately, we're still slower than the original code.
------------------------------------------------------------------------
May 18, 2005 - 23:53 : killes(a)www.drop.org
Dries, thanks for testing it again.
I do think the broken queries you observer have something to do with
the bad performance after the update. Please log the queries any I will
have a look at them. I've never seen any such queries.
My update script also tries to create the appropriate indices, but it
will of course fail if the database contains cruft. the indices for the
forum are probably missing, too.
I am still convinced that the patch is core worthy.
------------------------------------------------------------------------
May 19, 2005 - 06:36 : Dries
It wouldn't hurt if more people would benchmark this patch. The patch's
current performance worries me.
Did you check your watchdog messages after upgrading the drupal.org
database? Depending on your settings, errors might only be shown in
the watchdog. I'll look into the remaining glitches as time permits.
Thanks for your persistence in keeping this patch up-to-date. :)
------------------------------------------------------------------------
May 19, 2005 - 13:59 : killes(a)www.drop.org
Dries: Can you please let me have your updated database? I want to have
a look at it and try my own benchmarks with it.
And yes, if I did learn something on this project is how to be
persistant. ;)
------------------------------------------------------------------------
June 24, 2005 - 18:25 : killes(a)www.drop.org
Here is an idea that occurred to me:
The problem with the upgrade process is that keeping the existing
revisions requires a lot of work to do. This generates a huge amount of
sql queries for a large database and also requires a huge amount of
memory.
My suggestions is to let update.php only handle the basic upgrade, ie
without old revisions. An additional module could be created that would
implement a cron based approach to upgrading old revisions one node at a
time. it could expose a hook to let contrib modules do their own
upgrades.
Dries, what do you think? (I am writing "Dries" because he seems to be
the only one who is interested in getting this into core...)
------------------------------------------------------------------------
June 25, 2005 - 00:25 : Junyor
Killes:
I'm also interested in seeing this hit core. What about adding
something to legacy.module to do it?
------------------------------------------------------------------------
June 26, 2005 - 23:14 : chx
This is a sensible approach. Maybe this is the _only_ sensible approach.
I have a little problem though: while the conversion is running somehow
both revision handlers should be available.
1
0
Issue status update for http://drupal.org/node/7582
Project: Drupal
Version: cvs
Component: node system
Category: bug reports
Priority: normal
Assigned to: killes(a)www.drop.org
Reported by: killes(a)www.drop.org
Updated by: chx
Status: patch
This is a sensible approach. Maybe this is the _only_ sensible approach.
I have a little problem though: while the conversion is running somehow
both revision handlers should be available.
chx
Previous comments:
------------------------------------------------------------------------
May 5, 2004 - 18:25 : killes(a)www.drop.org
Currently all node revisions are stored in a serialized field in
node.table and retrieved for _each_ page view although they are rarely
needed. However, we have agreed that serializing data is bad and that
we should try to keep the memory foot print pf Drupal small.
Therefore I propose to create a separate revisions table which would be
in principle identical to the node table, only that it could have
several old copies of the same node. Extra data added by other modules
could be added in a serialized field unless we find a better solution.
------------------------------------------------------------------------
May 5, 2004 - 19:06 : jhriggs
I too think the serialized approach is less than desirable, but here's
an alternative. This would likely take some considerable rework in
core and contrib, but the following is how we handle similar types of
situations in our databases at work. It is more elegant that a
separate table, and avoids the (almost exact) duplication of a table.
Instead of separate tables, keep all revisions of nodes in the node
table as follows:
* add field: active (0/1 or Y/N)
* add field: revision
* every revision of a node is stored in the node table; however, only
one revision can be active at any given time
* nid can no longer be unique -- primary/unique key becomes (nid,
active)
* any time a node is loaded, updated (without revision), etc., the
active version is used.
Thoughts?
------------------------------------------------------------------------
May 5, 2004 - 19:57 : killes(a)www.drop.org
I am not opposed to your scheme, but I want to stress the following:
* Duplicating a table's structure is not bad (IMHO) as long as the
content is different.
* having two tables will allow us to have a rather small node table.
This is (maybe) a performance gain.
------------------------------------------------------------------------
May 5, 2004 - 20:37 : jhriggs
I don't necessarily think that duplicating a table's structure is _bad_.
It just seems to be wasteful and a pain to maintain. (Every change to
the node table is made twice...easy to do, but also easy to miss
perhaps.)
As for performance, as long as nid and the active indicator are
indexed, there shouldn't be any performance loss. Also, archiving an
old version when making a new revision will be much simpler: just
change the active indicator rather than copying an entire node to
another table (and ensuring everything gets copied...again a potential
maintainance issue).
To be honest, I would just like to see the serialized data go away,
regarless of what approach is taken.
------------------------------------------------------------------------
July 30, 2004 - 21:49 : Nick Nassar
Attachment: http://drupal.org/files/issues/Drupal-Improved_Revision_Schema_07-30-2004.p… (10.47 KB)
I'm interested in using Drupal for a large scale wiki-type project. In
order to do this, I need revisions to be in their own table.
Attached is a patch to do just that. Most of the changes are pretty
self explanatory. Spreading out node data across two tables meant that
I had to add database functions to do locking/transactions. Without
this, race conditions in which the database becomes corrupted are
possible.
------------------------------------------------------------------------
July 30, 2004 - 21:54 : Nick Nassar
Oh yeah... The patch is a diff against Drupal CVS
------------------------------------------------------------------------
July 31, 2004 - 02:00 : Anonymous
Gerhard speaking.
Nick, thanks a lot for your nice patch! It saves me a great deal of
labour. I looked through it and immediately liked it. You not only put
the old revisions into a new table but also the current one. Do you
have an estimate how much more expensive the additional join is?
Besides a few minor coding style issues I found a major one: Just a few
hours before you uploaded your patch JonBob's node access patch hit
core. That means your patch won't apply anymore as all the queries you
change have been changed. Can I bug you to update your patch?
------------------------------------------------------------------------
July 31, 2004 - 03:11 : Anonymous
Also I think that your upgrade path loses existing revisions.
------------------------------------------------------------------------
July 31, 2004 - 04:39 : drumm
I think this is the proper way to do things. No columns are duplicated,
there is no serialized data, and only the fields that are logically
revised are stored. Nothing jumped out at me as a way to have my node
modules be able to keep a table of revisions of additional fields. I'm
guessing this could be done within the confines of _insert and _update.
Assuming the upgrade path works and modules can extend it I give it a
+1.
------------------------------------------------------------------------
July 31, 2004 - 16:40 : Nick Nassar
It figures that just as I finish a big patch, another patch comes along
and breaks it. Oh well, it should be a pretty easy to fix. I'll work on
it.
Fixing the upgrade path to keep revisions should be fairly painless.
I found another issue that needs to be fixed before this patch gets
merged. There format of a node needs to be stored for each revision.
Otherwise, for modules that store a format for the nodes, such as page
and book, if you write one revision in PHP and the next in HTML, the
PHP revision will be displayed as HTML. This is part of a larger issue
of how node modules should store revisions of additional fields. I
think each module that wants to do this should create another table
with (nid, revid) as the primary key. Just as when they want to add
fields to a node they create another table with nid as the primary key.
As far as performance goes, for sites that make heavy use of revisions,
an extra join on primary keys is going to be a lot faster than grabbing
all of the revisions from that database everytime. We would need to run
benchmarks to determine is the overall difference in speed is for an
average site is a gain or a loss. I'm guessing it's very minor either
way.
------------------------------------------------------------------------
August 23, 2004 - 15:55 : Nick Nassar
Attachment: http://drupal.org/files/issues/Drupal-Improved_Revision_Schema_08-23-2004.p… (10.92 KB)
Here's an updated patch against CVS that puts revisions in their own
table, provides an upgrade path, and fixes the format related bugs in
the last patch.
Hopefully, this can make it into CVS as soon as the freeze is over.
------------------------------------------------------------------------
August 23, 2004 - 16:10 : moshe weitzman
Interesting patch ... drumm's question is still outstanding. how do
modules store revisions of their fields? Are they expected to manage
this on their own? Thats not how it works today.
As an aside, i am seeing profile_ fields in my node.revisions column.
One could argue that those need not be saved. They pertain to the node
author, not to the node itself.
------------------------------------------------------------------------
August 23, 2004 - 18:14 : Nick Nassar
Having modules be responsible for storing revisions of their own fields
is a side-effect of storing revision data in tables. There's really no
way around it. However, revisions generally don't make sense for node
types that don't have PHP/HTML content, such as polls. I think it's
going to be a pretty rare scenario for a new node type to want another
field to change per-revision, so it's a pretty good trade-off.
Storing fields that shouldn't be part of revisions, such as the
profile_ fields, is a side-effect of storing revisions as serialized
objects. Applying this patch will free up that wasted space. :)
------------------------------------------------------------------------
August 23, 2004 - 19:20 : Anonymous
There should be a hook that let's the module choose whether it supports
history. This way a module author can prevent the user from doing
something that may break his module or just cause undefined behavior.
If the module doesn't support history then don't let the user/admin
choose to add history to nodes of that type.
Craig
------------------------------------------------------------------------
August 23, 2004 - 21:23 : Nick Nassar
I agree, there should be an API change to make specifying support for
revisions easier. In the interests of keeping patches small and keeping
to one change per patch, I think the API change should be a separate
issue.
A sort of ad-hoc API to decide whether or not a module supports
revisions by default already exists. Instead of having a hook, modules
set the default value of the "Create new revision" field in the edit
form. The admin can change this option in
admin/node/configure/defaults. This patch doesn't change that.
Revisions are broken for node types that have their own database
structure, like polls, even when storing them as serialized objects.
This patch doesn't change that, either.
------------------------------------------------------------------------
October 26, 2004 - 04:35 : moshe weitzman
I'm guessing that someone is going to have to demonstrate that this
patch performs as well as current drupal before it gets comitted. i
think this patch is a few benchmarks from being comitted.
------------------------------------------------------------------------
October 27, 2004 - 03:04 : Nick Nassar
Attachment: http://drupal.org/files/issues/Drupal-Improved_Revision_Schema_10-26-2004.p… (11 KB)
I ran some really unscientific benchmarks, and it looks like this patch
has a negligible affect on performance.
I used apache bench and the database from theregular.org, which doesn't
contain any revisions (worst case scenario for this patch) and contains
several hundred nodes. Both the patched and unpatched versions hovered
between 2.36 and 2.38 requests per second.
The command I used was:
ab -n50 -C 'PHPSESSID=b01a9f92880ef215b0ed6f1314a5eba2'
http://192.168.0.100/
An updated patch that should apply to CVS is attached.
------------------------------------------------------------------------
October 27, 2004 - 03:05 : Nick Nassar
I ran some really unscientific benchmarks, and it looks like this patch
has a negligible affect on performance.
I used apache bench and the database from theregular.org, which doesn't
contain any revisions (worst case scenario for this patch) and contains
several hundred nodes. Both the patched and unpatched versions hovered
between 2.36 and 2.38 requests per second.
The command I used was:
ab -n50 -C 'PHPSESSID=b01a9f92880ef215b0ed6f1314a5eba2'
http://192.168.0.100/
An updated patch that should apply to CVS is attached.
------------------------------------------------------------------------
October 27, 2004 - 03:05 : Nick Nassar
Attachment: http://drupal.org/files/issues/Drupal-Improved_Revision_Schema_10-26-2004.p… (11 KB)
I ran some really unscientific benchmarks, and it looks like this patch
has a negligible affect on performance.
I used apache bench and the database from theregular.org, which doesn't
contain any revisions (worst case scenario for this patch) and contains
several hundred nodes. Both the patched and unpatched versions hovered
between 2.36 and 2.38 requests per second.
The command I used was:
ab -n50 -C 'PHPSESSID=b01a9f92880ef215b0ed6f1314a5eba2'
http://192.168.0.100/
An updated patch that should apply to CVS is attached.
------------------------------------------------------------------------
November 15, 2004 - 07:05 : elias1884
please overthink the revision system default workflow as well. don't
look at the revision system as an isolated system but as a part of the
whole workflow system!
if you combine revisions with the moderation queue the most logic
default workflow would be like that:
auth user creates node (revision #0)
admin approves the node (status = 1, moderation = 0)
=> node publicly available
auth user finds typo and changes node (revision #1, status = 0,
moderation = 1)
-------
what happens at that point at the moment is, that the node is not
accessible anymore at all until the new revision is approved by admin.
of course the new revision should not go online until reviewed and
approved, this is absolutely correct, but there is no reason to not
take the old revision offline, since it was already approved and should
therefore be online until the new revision is approved. it is not
practical if a node disappears only because the author corrected a
typo.
-------
admin approves the node (status = 1, moderation = 0)
eventhough I first thought a plain boolean active field would not be
capable of providing that functionality if finally came to the
conclusion, that it can. The only thing to do is to not set that bit,
when a new revision is created, but when it is approved (in case
moderation is activated under default workflow). Every revision should
have its own moderation, status and active field and on approval they
are set like this (status=1, moderation=0, active=Y).
When you wanna rollback to an old revision, you can chose between all
revisions that already have the moderation bit set back to 0 again and
the published to 1. There should be an extra permission for rollback!
another concern that I have about the default workflow is, that users
can't see the content, they have just created, when moderation is
enabled. Eventhough, there is a big fat "submission accepted" presented
after submissions, unexperienced users tend to question the information
those stupid tincans give them, if they can't find their content
afterwards. Many users are really lazy bastards and they don't even
read the status messages. The best feedback about whether his story was
submitted successfully or not of course is, if he can find the story
somewhere on the site, maybe with a status message on top of it,
mentioning, that the content is currently not publicly available since
it has not been approved yet. there should be a my content section
under my account, like somebody is trying to do with the workspace
module I guess.
so my suggestion is to make (status=0, moderation=1) still available
for the creator under a my content section somewhere!
------------------------------------------------------------------------
November 24, 2004 - 06:21 : Nick Nassar
I agree. The current workflow for moderation queues and revisions needs
to change, but this patch isn't the place for it. The patch is already
too big, and it only does the backend stuff.
Instead of adding more to this patch and making it take even longer to
get into core, would you mind creating a new issue for your UI
suggestions, so the those changes can be added as a separate patch?
Thanks,
Nick
------------------------------------------------------------------------
December 11, 2004 - 14:26 : Dries
This patch is _much_ needed so I'd love to see someone revive it. In
order for this patch to be accepted, the following needs to be done:
* Update this patch to CVS HEAD.
* Rename revid to vid.
* Rename node_rev to node_revisions.
* Rename node_rev.changed to node_revisions.timestamp.
* Rename $rnode to $revision.
* Fix the coding style to match Drupal's: proper spacing, single quotes
where possible, proper variable names.
* Benchmark this patch with a large database with enough revisions.
I'd be happy to benchmark this on my local copy of the drupal.org
database.
* The book.log field should probably move to the node_revisions table.
This can be done in a separate patch.
* Investigate whether transactions are well-supported.
------------------------------------------------------------------------
December 13, 2004 - 02:25 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/Drupal-Improved_Revision_Schema_10-26-2004-r… (11.02 KB)
I've worked a bit on the patch (coding style issues as mentioned by
Dries). One thing I noticed is that the patch uses REPLACE. IIRC this
needs to be chagned to "UPDATE, if fail INSERT" for pgsql
compatibility.
Nick, are you still interested in working on that patch? I'd like to
know how it works on your site and work on getting it into core.
------------------------------------------------------------------------
December 13, 2004 - 14:33 : Dries
Gerhard: your patch does not apply.
------------------------------------------------------------------------
December 13, 2004 - 19:10 : killes(a)www.drop.org
Yes, I know, that was the same version as I mailed to you earlier.
------------------------------------------------------------------------
December 13, 2004 - 23:02 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions.patch (52.96 KB)
Ok, upüdated the patch to cvs.
------------------------------------------------------------------------
December 14, 2004 - 10:58 : Dries
Some more comments:
* db_begin_transaction() and db_end_transaction() do not belong in
database.inc, but in database.mysql.inc and database.pgsql.inc
respectively.
* The node module calls node_revisionsision_list() which is not
defined. (Fxed that on my local copy.)
* Do db_begin_transaction() and db_end_transaction() deprecate Jeremy's
table locking patch?
* The upgrade path assigns the wrong user ID to each revision.
* The upgrade path assigns the wrong date to each revision (that or a
node's revision page shows the wrong usernames/dates).
* The coding style needs a bit of work, but we can worry about that
later.
------------------------------------------------------------------------
December 14, 2004 - 19:34 : Nick Nassar
If you need any help getting those things fixed, just let me know.
------------------------------------------------------------------------
December 14, 2004 - 19:50 : Nick Nassar
How this relates to Jeremy's node locking patch:
There was lots of discussion, and node locking was decided against
because from an end user point of view you never want a node to be
locked. He's now advocating for a much simpler patch that warns users
if their changes will overwrite someone elses. That patch still has a
race condition, which might be fixed using db_begin_transaction().
http://drupal.org/node/6025
------------------------------------------------------------------------
December 15, 2004 - 00:26 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_0.patch (55.96 KB)
Here is an updated patch that tries to address Dries concerns.
------------------------------------------------------------------------
December 15, 2004 - 10:32 : Dries
Attachment: http://drupal.org/files/issues/revisions-bug.png (76.06 KB)
It didn't fix the aforementioned bugs. See attached screenshot.
------------------------------------------------------------------------
January 6, 2005 - 22:15 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_1.patch (51.77 KB)
Ok, here is a new version. Dries and myself worked hart at it, so please
have a look.
what is still missing
- database upgrades for the core modules with an own table
- contrib modules need an upgrade too.
- do we need nid and vid in both the node and the node_revisions table?
- the amount of sql queries means a good stress testing for large
databases.
------------------------------------------------------------------------
January 19, 2005 - 23:43 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_2.patch (49.49 KB)
Here is an updated patch. We discussed to keep the current title in node
module and also in the revisiosn table. This is content duplication but
will save many joins as many queries only need the title of a node.
Discussion is welcome.
------------------------------------------------------------------------
January 20, 2005 - 01:33 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_3.patch (29.93 KB)
I've implemented the aforementioned solution. This makes the patch much
smaller. The patch now also removes taxonomy_node_has_term() which
wasn't used anywhere. I'd really apprciate if some people could test
drive the patch. It will be another huge improvement for 4.6.
------------------------------------------------------------------------
January 20, 2005 - 02:05 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_4.patch (30 KB)
Another revision. Steven didn't like my literal $node->vid in queries.
------------------------------------------------------------------------
January 20, 2005 - 03:10 : killes(a)www.drop.org
- database upgrades for the core modules with an own table
- contrib modules need an upgrade too.
- do we need nid and vid in both the node and the node_revisions table?
- the amount of sql queries means a good stress testing for large
databases.
These issues are still open, btw. Especially the first one needs to be
tackled.
------------------------------------------------------------------------
January 25, 2005 - 22:11 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_5.patch (51.13 KB)
Here is a patch that has the database tables updated for forum, book,
and page module.
------------------------------------------------------------------------
January 30, 2005 - 00:55 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_6.patch (49.18 KB)
Yet another update to keep it working with head. The patch now also
removes the table definitons for the page table.
------------------------------------------------------------------------
January 30, 2005 - 00:57 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_7.patch (55.69 KB)
Sorry, that was the old version, this is the right one.
------------------------------------------------------------------------
January 31, 2005 - 21:55 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_8.patch (55.71 KB)
Updated once more.
------------------------------------------------------------------------
January 31, 2005 - 22:52 : Dries
Anyone to help review/test this?
------------------------------------------------------------------------
January 31, 2005 - 23:22 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_9.patch (49.29 KB)
Updated again, the update functions occurred twice. Thanks Bart.
------------------------------------------------------------------------
February 2, 2005 - 02:27 : killes(a)www.drop.org
Don't know if the db I am using is corrupted or what. I still do have
some didficulties.
The latest patch is attached.
------------------------------------------------------------------------
February 2, 2005 - 02:27 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_10.patch (49.67 KB)
I am probably slowly going mad ...
------------------------------------------------------------------------
February 2, 2005 - 03:54 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_11.patch (48.95 KB)
The update issue still needs investigating. This patch is updated for
cvs.
------------------------------------------------------------------------
February 2, 2005 - 22:20 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_12.patch (49.83 KB)
Ok, here is a new version. I've solved my troubles with book.module.
There are still some issues with forum module. Possibly due to
inconsistent database.
------------------------------------------------------------------------
February 2, 2005 - 23:31 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_13.patch (49.83 KB)
Turns out the drupal.org database had indeed some quirks. Please run
this query in your oldest db and tell me the result:
select nid,type from node where type like '%/%';
If you get a non-zero result we might need to add another security
update.
The patch could use still more testing, though.
------------------------------------------------------------------------
February 3, 2005 - 03:16 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_14.patch (49 KB)
Ok, we are getting somewhere. At a first glance the update is working.
There is a problem remaining: the revisions tab will be shown whether
the node has revisions or not. Not sure we can/need to fix this.
People with a drupal.org account can log in at
http://killes.drupaldevs.org/revision/ and poke around. Your
permissions will be the same as on drupal.org. Feel free to vreak
everything but don't forget to file complaints here. (Note: this is
only a pruned version of the drupal.org database with all project nodes
and nodes with nids > 7000 dropped).
------------------------------------------------------------------------
February 3, 2005 - 06:19 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_15.patch (52.39 KB)
There was some error in node_save and also the patches to the
database.inc files got lost...
------------------------------------------------------------------------
February 3, 2005 - 09:07 : robertDouglass
Submitting book pages doesn't work on your test site. It puts the entire
content of the preview inside the body textarea. I wrote a sentence in
the body and the log, and pressing preview put several lines of HTML
containing both sentences in the body textarea on the preview page,
plus the book page wouldn't submit.
-R
------------------------------------------------------------------------
February 3, 2005 - 09:50 : Junyor
0 results here. I started using Drupal with version 4.4, though.
------------------------------------------------------------------------
February 4, 2005 - 01:56 : killes(a)www.drop.org
@Junyor: Thanks, that's a good sign. Maybe somebody else has an older db
to try.
@robertDouglass: The first effect you describe is due to drupaldevs
running on PHP 5. I am unsure why the second thing does not work. In
node_save() the node object has a nid although there is none in the
form. Very strange.
I've enabled display of db queries on the testsite.
------------------------------------------------------------------------
February 4, 2005 - 21:17 : dmjossel
No results here on the query:
select nid,type from node where type like '%/%';
On a database that was put in place prior to Drupal 4 and is now
running on 4.5.2.
------------------------------------------------------------------------
February 4, 2005 - 22:44 : killes(a)www.drop.org
@dmjossel: thanks.
@all. The strange problem I reported was apparently php 5 related.
After applying Steven's php 5 patch it went away. One error is
remaining: If I create a new forum topic it will be shown as part of
the book on preview. Hmm, that was due to a db that got corrupted
during testing so that is fixed too.
Please poke around at the test site and look if you find more errors.
------------------------------------------------------------------------
February 5, 2005 - 09:16 : Steven
By the way, I just remembered that Drupal.org has some blogs lingering
on in the database even though blog.module is not enabled. Perhaps this
is causing troubles?
------------------------------------------------------------------------
February 5, 2005 - 13:22 : Anonymous
I can't see why it would. Drupal.org will need extra updates for images
and project nodes because those have their own tables. GK.
------------------------------------------------------------------------
February 6, 2005 - 14:49 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_16.patch (52.49 KB)
Updated to apply to cvs again.
------------------------------------------------------------------------
February 22, 2005 - 22:15 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_17.patch (49.64 KB)
Updated again.
All we need is a patch to upload module and an upgrade path for it.
------------------------------------------------------------------------
March 4, 2005 - 06:22 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_18.patch (52.31 KB)
Updated once more. Moved log field from book to node_revisions table as
discussed in Antwerp. upload module still missing.
We need to decide under which circumstances the log field should be
displayed. Should that be added to the workflow? Should it depend on
the revisions setting?
------------------------------------------------------------------------
March 5, 2005 - 21:27 : Anonymous
Attachment: http://drupal.org/files/issues/revisions_20.patch (75.52 KB)
Ok, here it is: Yet another revision of this grrrrreat patch.
Changes from previous versions:
- supports versioning for uploaded files. A problem is that if you
delete a file, it will be gone for all revisions.
- the log field is now in the node_revsions table, but each module has
to decide whether to show it or not.
I've implemented it for the page and the book type odes. Also, the
field can be edited when adding non-book nodes to the book. The log is
displayed on the revisions page and if a node is moderated.
- the revisions are moved to an old_revisions table to a) get the node
table smaller and b) still leave the mavailable for contrib modules
that want to retreive old version data.
The patch has been applied to killes.drupaldevs.org/revision where it
can be tested by anybody (especially people who have "site admin"
rights on drupal.org)
The database is from drupal.org and you should b able to log in with
your pass or simply mail yourself a new one.
Gerhard
------------------------------------------------------------------------
March 5, 2005 - 21:51 : Anonymous
Attachment: http://drupal.org/files/issues/revisions_21.patch (59.42 KB)
BTW, I marked this a bug because atm the revisions field can grow quite
big. Neil has reported problems from some users who were not able to
load some nodes due to to many large revisions.
Also, som unrelated stuff crept into the patch. New version attached.
------------------------------------------------------------------------
March 8, 2005 - 07:56 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_22.patch (60.29 KB)
Ok, I think I got it.
Changes to last version:
- uploads are no properly versioned.
Missing are still pgsql checks and updates.
------------------------------------------------------------------------
March 10, 2005 - 18:58 : Anonymous
Was able to get http://drupal.org/files/issues/revisions_21.patch to
work with drupal-cvs.tar.gz (10 March 2005) by:
- includes/database.mysql.inc: Commenting out duplicates for functions
function db_begin_transaction and function db_commit_transaction
- modules: node.module: Removing "'title' => $node->title," from
$node_table_values variable declaration and removing "'title' =>
"'%s'"," from "$node_table_types" variable declaration.
Happy to submit a patch if requested. I'll watch this thread.
------------------------------------------------------------------------
March 12, 2005 - 01:59 : killes(a)www.drop.org
The duplicate function has been removed in rev 22 of this patch.
Why do you think the changes in node_save are needed? Titles are saved
in both tables for performance reasons.
------------------------------------------------------------------------
March 13, 2005 - 18:12 : jlerner
Hi - I posted comment #62. The changes to node_save appear to be needed
because recent patches (both 21 and 22) remove the field 'title' from
table 'node'. So without the changes to node_save, node.module is
broken and generates errors.
Joshua
------------------------------------------------------------------------
March 13, 2005 - 18:29 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_23.patch (61.17 KB)
Thanks, Joshua, for catching this. node:title is there to stay.
------------------------------------------------------------------------
April 13, 2005 - 18:29 : moshe weitzman
since HEAD is open again, perhaps it is a good time to revisit this
patch.
once this is committed, lets address - http://drupal.org/node/11071
"node_validate does not respect group editing"
------------------------------------------------------------------------
April 18, 2005 - 17:43 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_24.patch (60.39 KB)
Updated.
------------------------------------------------------------------------
April 18, 2005 - 18:16 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_24_0.patch (60.39 KB)
Updated.
------------------------------------------------------------------------
April 19, 2005 - 07:19 : Dries
I'll commit this patch later this week! If you haven't checked this
patch already, I urge you to test/check it out because it will have
significant impact on existing code and modules!
------------------------------------------------------------------------
April 19, 2005 - 07:21 : Dries
Also, what do people think about the n.title being duplicated?
------------------------------------------------------------------------
April 19, 2005 - 07:26 : chx
I won't lose any sleep because of duplicated titles...
------------------------------------------------------------------------
April 19, 2005 - 20:35 : killes(a)www.drop.org
Let me explain why I have chosen to duplicate the title (and also the
uid): If you look at all the queries in Drupal, you will find that most
of them only need the title and th uid of a node. That is, by
duplicating it, we save expensive joins on the node_revisions table.
Due to this fact, this patch is actually a performance improvement.
A note about updating contrib module:
Strictly speaking they wouldn't need to be updated. They only need to
if their authors decide that their info should be available for
revisioning. The upgrade path for forum.module in my update.inc patch
(+ the forum patch)
should show you what needs to be done.
I will write a note for the update page once the patch hits core.
------------------------------------------------------------------------
April 24, 2005 - 23:21 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_25.patch (60.38 KB)
Updated to cvs.
Dries: Based on some remarks in #drupal this is the last update I am
going to do. Apply it or won't fix it.
------------------------------------------------------------------------
April 30, 2005 - 05:42 : Jeremy(a)kerneltrap.org
Attachment: http://drupal.org/files/issues/revisions_25.patch.patch (528 bytes)
That's a big patch. I've only started looking through it. I noticed
one little typo, affecting updates. A patch to your last patch is
attached.
I'm running with the revision patch on my dev server now happily. I
like the concept.
What happens if you click 'stop' on your browser in the middle of a
MySQL "transaction"? I assume that kills the connection to MySQL, and
the lock is freed? But this then leaves changes only partially
applied?
What exactly does locking/unlocking the tables buy us in MySQL? I
don't see anywhere that we detect if an apply fails part way through,
and thus roll back...? What am I missing?
------------------------------------------------------------------------
April 30, 2005 - 09:11 : Dries
Jeremy: many of us are worried about the performance ramifications this
patch introduces. Early experiments showed a small performance
improvement (while a performance regression might be expected). More
performance reports from large sites like kerneltrap.org will certainly
help this patch. Mind to do a quick performance comparision and to
report back with some numbers? Thanks.
------------------------------------------------------------------------
April 30, 2005 - 14:38 : Jeremy(a)kerneltrap.org
Dries: I'm not running HEAD on kerneltrap, so this really isn't a
possibility. Furthermore, until I understand why we're locking tables,
I don't like it. The idea of revisions in their own tables is great.
The idea of locking tables to get (without any obvious benefit) there
really worries me.
------------------------------------------------------------------------
April 30, 2005 - 16:16 : killes(a)www.drop.org
@Jeremy: Thanks for looking at the patch! Also for catching the typo. :)
Did you try to upgrade your database? If yes, how did it go? One of
Dries' concerns is the complexity of the upgrade. How many nodes and
revisions did the db have?
About database locking: This part of the patch was created by Nick and
I simply continued to use it.
Maybe the code should rather be:
if(db_begin_transaction(array('{node}', '{node_revisions}',
'{watchdog}', '{sessions}', '{files}'))) {
db_query($node_query, $node_table_values);
db_query($revisions_query, $revisions_table_values);
db_commit_transaction();
...
}
The idea is probably to avoid two updates at the same time. I don't
think the locking helps if you abort the script at an inconvenient
time. Rollbacks aren't implemented in all mysql versions.
We could omit the db locking if deemed inappropriate. Maybe Nick can
explain his ideas behind this.
@Dries: I wonder who the "many of us" are. They certainly haven't
spoken to me. Moshe had some reservations about the upgrade path and
project module, but the time that project module abused revisions to
store issue updates was long ago and his reservations were resolved.
Nobody else (besides you of course and now Jeremy) has voiced
reservations in a way that was audible to me.
If you grep through the patch you will notice that there are only four
queries which have a join on the node_revisions table. Two of them are
in node_load and in the other cases the join replaced a join on the
node table. The two queries in node_load are the only ones that have
both a join on the node and the revisions query. Thus, loading of
individual nodes might become somewhat slower. All other queries will
be faster since the node table is now much smaller. Also, node loading
does not have to be slower, it depends on your node table. If you had
a lot of revisions and thus a large table, then the new scheme will
make your queries actually faster since we do not load the revisions
on each and every node load anymore. If you didn't have many revisions
your node_load migth be somewhat slower.
WRT to the update script Karoly pointed out that we could use multiple
insert queries instead one query per revision. This would probably
make the update somewhat faster. I am willing to work on this iff you
declare that you will commit the patch afterwards. I'd need to know if
this will work on pgsql and on all supported mysql versions before I
work on it.
About locking: Database locking is dog slow, at least on mysql. I was
using locks in an earlier version of the upgrade script but had to
remove it for (serious!) performance reasons.
------------------------------------------------------------------------
May 9, 2005 - 17:07 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_26.patch (46.45 KB)
Ok, another update, cause I need it myself.
I've left out the transaction stuff for now. It is in principle
unrelated to this patch and should be discussed elsewhere.
This also makes the patch smaller and easier to review (hint, hint).
------------------------------------------------------------------------
May 9, 2005 - 22:32 : killes(a)www.drop.org
The patch contained the update functions twice.
------------------------------------------------------------------------
May 9, 2005 - 22:32 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_27.patch (39.05 KB)
The patch contained the update functions twice.
------------------------------------------------------------------------
May 9, 2005 - 23:23 : Dries
Got one error during the upgrade path:
ALTER TABLE {book} ADD PRIMARY KEY vid (vid)
FAILED
------------------------------------------------------------------------
May 9, 2005 - 23:26 : killes(a)www.drop.org
This had happend to me as well, when I tested this patch. The reason is
that for some reason the vid is not unique. Most likely there are some
entries with vid = 0 in there. Can you check which node types those
have? it always was an error in the test database. See:
http://drupal.org/node/7582#comment-20678
------------------------------------------------------------------------
May 9, 2005 - 23:27 : Dries
Actually, I got 2850 errors during the upgrade.
Some of these:
sprintf() [function.sprintf]: Too few arguments in
drupal-cvs/includes/database.inc on line 154.
Some of these:
Query was empty query: in drupal-cvs/includes/database.mysql.inc on
line 66.
And this:
Unknown table 'n' in field list query: SELECT n.nid, n.vid FROM node
INNER JOIN files f ON n.nid = f.nid in
drupal-cvs/includes/database.mysql.inc on line 66.
:-)
------------------------------------------------------------------------
May 9, 2005 - 23:29 : Dries
Or this:
user error: Unknown column 'log' in 'field list'
query: SELECT parent, weight, log FROM book WHERE nid = 1 in
drupal-cvs/includes/database.mysql.inc on line 66
------------------------------------------------------------------------
May 9, 2005 - 23:52 : Dries
The time required to generate my main page went from 902 ms (before
upgrade) to 2139 ms (after upgrade).
The time required to generate a forum listing (?q=forum/x) went from
1872 ms (before upgrade) to 2874 ms (after upgrade).
Maybe this is because my database is not consistent as the result of
the upgrade errors (yet I don't see any errors on the pages I
benchmarked).
------------------------------------------------------------------------
May 10, 2005 - 02:24 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_28.patch (53.47 KB)
Ok, let me get to this from the bottom to the top:
- my test runs indicated a different development wrt timing. If I had
gotten your results, I had stopped working on this long ago. So your
results are wrong for some reason.
- user error: Unknown column 'log' in 'field list'
Wasn't my day, the book patch got lost. It is contained now. First -R
the old patch, then apply this one.
- Unknown table 'n' in field list query:
Walkah found this, but I forgot to fix it. Fixed now.
- I've no idea where the other queries come from. I am hoping that
either your test db is borken or they are follow ups from the other
ones.
If you let me have your test db, I'll try some debugging.
Thanks for wasting your time, too.
------------------------------------------------------------------------
May 10, 2005 - 07:07 : Dries
I double-checked and the numbers don't seem to lie. I'll test some more
after work on another machine to make sure it is not platform-specific.
------------------------------------------------------------------------
May 11, 2005 - 05:32 : killes(a)www.drop.org
Attachment: http://drupal.org/files/issues/revisions_29.patch (54.83 KB)
Ok, here I am again.
What I did:
1) Ask Dries to let me have drupal.org database
2) get 400MB of SQL inserts...
3) take 23 minutes to import said data
4) Remove all image and project nodes (don't want to install their
modules), 11765 nodes left
5) back up data
6) take tests on non-cached /node page (as anonymous user).
ab results:
-c 1 -n 25:
Requests per second: 1.29 [#/sec] (mean)
Connection Times (ms)
min mean[+/-sd] median max
Total: 663 775 179.7 689 1264
7) Do the same for the tracker page:
Requests per second: 0.83 [#/sec] (mean)
Total: 1182 1199 7.4 1199 1217
8) Apply my patch (rev. 28).
9) run db update and hold breath
10) update times out...
11) play back backup from 5)
12) wait
13) getting annoyed and removing cache, watchdog, and accesslog before
playing in backup.
14) wait again. Understand why Dries doesn't try this patch often.
Maybe a smaller DB would do for testing?
15) wait more. get really annoyed.
16) Set time limit to 18000 in update.php
17) try again
18) fails again before the second update is completed.
19) curse.
20) delete search stuff from db. Ooops, sooo much smaller...
21) import again, below 2 minutes...
22) rewrite to use extended insert. Found a bug.
23) still does not complete. Mysql logging to the rescue!
24) tid = 0? Not good.
25) Well, the update works fine till node 10834. 5595 nodes done, 6136
to go.
26) Writing shell based update script. Discovery: 24MB aren't enough.
Hopefully 64 are. Nope.
extended inserts for revisions are apparently not the brightest idea:
Huge memory consumption.
Hmm, no, all updates got through. Selecting the revisions to put them
into old_revisions table screwed it. Learned about CREATE TABLE
old_revisions SELECT syntax.
Yay! finally. 24 MB are just not enough the update.php script seems to
still break.
27) Benchmarks!
/node
Requests per second: 1.54 [#/sec] (mean)
Connection Times (ms)
min mean[+/-sd] median max
Total: 632 649 40.5 636 791
/tracker
Requests per second: 0.86 [#/sec] (mean)
Total: 1119 1165 65.8 1160 1461
Ok: So we get an improvement for many node_loads, but none for simple
selects from node.
More tests can be done.
28) roll new patch
Ain't Drupal fun?
------------------------------------------------------------------------
May 18, 2005 - 15:38 : Dries
I did another round of tests on _another_ machine and it is not looking
good:
Before upgrade After upgrade
?q= (main page) 218 ms/request 340 ms/request
?q=forum (forum overview) 754 ms/request 1520 ms/request
?q=about (book page) 375 ms/request 5400 ms/request
The upgrade process itself gave me a number of 'query was empty' and
'sprintf(): too few arguments' reports. Everything seems to work fine
though.
Looking at the ?q=about page, I see that the following query is
executed twice _and_ that each time, it take more than 2 seconds to
complete:
SELECT n.nid, n.title, b.parent, b.weight FROM node n INNER JOIN book b
ON n.vid = b.vid WHERE n.status = 1 AND n.moderate = 0 ORDER BY
b.weight, n.title;
--8 SHOW INDEX FROM book;
+-------+------------+-------------+--------------+-------------+-----------+-------------+----------+--------+------+------------+---------+
| Table | Non_unique | Key_name | Seq_in_index | Column_name |
Collation | Cardinality | Sub_part | Packed | Null | Index_type |
Comment |
+-------+------------+-------------+--------------+-------------+-----------+-------------+----------+--------+------+------------+---------+
| book | 1 | book_parent | 1 | parent | A
| 92 | NULL | NULL | | BTREE | |
| book | 1 | nid | 1 | nid | A
| 369 | NULL | NULL | | BTREE | |
+-------+------------+-------------+--------------+-------------+-----------+-------------+----------+--------+------+------------+---------+
2 rows in set (0.00 sec)
The book module does not appear to have a primary key? Sounds like a
bad idea so I added one:
mysql> ALTER TABLE book ADD PRIMARY KEY nid (nid);
Query OK, 369 rows affected (0.02 sec)
Records: 369 Duplicates: 0 Warnings: 0
Next, I wanted to make the vid column a unique key in all node tables:
mysql> ALTER TABLE node ADD UNIQUE vid (vid);
Query OK, 20392 rows affected (0.81 sec)
Records: 20392 Duplicates: 0 Warnings: 0
mysql> ALTER TABLE book ADD UNIQUE vid (vid);
ERROR 1062: Duplicate entry '0' for key 2
mysql> ALTER TABLE forum ADD UNIQUE vid (vid);
Query OK, 10806 rows affected (0.10 sec)
Records: 10806 Duplicates: 0 Warnings: 0
As you can see, it fails for the book table which makes me believe
there is some inconsistent data ... I set out to fix that:
mysql> SELECT nid, COUNT(nid) AS vids FROM book GROUP BY vid HAVING
vids > 1;
+-----+------+
| nid | vids |
+-----+------+
| 871 | 2 |
+-----+------+
1 row in set (0.00 sec)
mysql> SELECT title FROM node WHERE nid = 871;
Empty set (0.00 sec)
mysql> DELETE FROM book WHERE nid = 871;
Query OK, 1 row affected (0.00 sec)
mysql> ALTER TABLE book ADD UNIQUE vid (vid);
Query OK, 368 rows affected (0.01 sec)
Records: 368 Duplicates: 0 Warnings: 0
Looks like everything is well now. Ran some new benchmarks:
Before upgrade After upgrade With
indices
?q= (main page) 218 ms/request 340 ms/request 336
ms/request
?q=forum (forum overview) 754 ms/request 1520 ms/request 1531
ms/request
?q=about (book page) 375 ms/request 5400 ms/request 475
ms/request
Unfortunately, we're still slower than the original code.
------------------------------------------------------------------------
May 18, 2005 - 23:53 : killes(a)www.drop.org
Dries, thanks for testing it again.
I do think the broken queries you observer have something to do with
the bad performance after the update. Please log the queries any I will
have a look at them. I've never seen any such queries.
My update script also tries to create the appropriate indices, but it
will of course fail if the database contains cruft. the indices for the
forum are probably missing, too.
I am still convinced that the patch is core worthy.
------------------------------------------------------------------------
May 19, 2005 - 06:36 : Dries
It wouldn't hurt if more people would benchmark this patch. The patch's
current performance worries me.
Did you check your watchdog messages after upgrading the drupal.org
database? Depending on your settings, errors might only be shown in
the watchdog. I'll look into the remaining glitches as time permits.
Thanks for your persistence in keeping this patch up-to-date. :)
------------------------------------------------------------------------
May 19, 2005 - 13:59 : killes(a)www.drop.org
Dries: Can you please let me have your updated database? I want to have
a look at it and try my own benchmarks with it.
And yes, if I did learn something on this project is how to be
persistant. ;)
------------------------------------------------------------------------
June 24, 2005 - 18:25 : killes(a)www.drop.org
Here is an idea that occurred to me:
The problem with the upgrade process is that keeping the existing
revisions requires a lot of work to do. This generates a huge amount of
sql queries for a large database and also requires a huge amount of
memory.
My suggestions is to let update.php only handle the basic upgrade, ie
without old revisions. An additional module could be created that would
implement a cron based approach to upgrading old revisions one node at a
time. it could expose a hook to let contrib modules do their own
upgrades.
Dries, what do you think? (I am writing "Dries" because he seems to be
the only one who is interested in getting this into core...)
------------------------------------------------------------------------
June 25, 2005 - 00:25 : Junyor
Killes:
I'm also interested in seeing this hit core. What about adding
something to legacy.module to do it?
1
0
[drupal-devel] [feature] Custom registration fields should appear on admin/user/create
by killes 26 Jun '05
by killes 26 Jun '05
26 Jun '05
Issue status update for http://drupal.org/node/23536
Project: Drupal
Version: cvs
Component: user.module
Category: feature requests
Priority: normal
Assigned to: chx
Reported by: moshe weitzman
Updated by: killes(a)www.drop.org
Status: patch
I like this patch.
killes(a)www.drop.org
Previous comments:
------------------------------------------------------------------------
May 24, 2005 - 21:02 : moshe weitzman
When creating a new user as an admin, it is useful and even expected to
be able to complete all the profile fields which are available on the
usual registration form ... the approach might be to consolidate
registration so that both users and admins use a single form.
------------------------------------------------------------------------
June 26, 2005 - 00:27 : chx
Attachment: http://drupal.org/files/issues/user_reg.patch (8.69 KB)
At first I thought it'd be better if I'd keep the separate form, but
this version results in shorter code and it's not so ugly. However, if
you do not like it, I have the version which only adds the $extra to
user_admin_create.
1
0
Issue status update for http://drupal.org/node/25138
Project: Drupal
Version: cvs
Component: base system
Category: feature requests
Priority: normal
Assigned to: Anonymous
Reported by: budda
Updated by: Thox
Status: patch
-1
That's a lot of extra markup for every <select> control. Since it has
very limited use, I'd suggest it shouldn't be part of core.
I do like the idea of using it for your specific case.
Thox
Previous comments:
------------------------------------------------------------------------
June 16, 2005 - 14:54 : budda
i'd like to CSS my select menu list. However the drupal common.inc
form_select() doesn't assign each with a unique id/class.
the option list id's could be generated in the form:
<select-name>-<option value>
Which could produce something like:
<select class="card" name="edit[card]" id="edit-card">
<option value="" selected="selected">Please Choose</option>
<option value="VISA" id="card-visa">Visa</option>
<option value="MC" id="card-mc">Mastercard</option>
<option value="SWITCH" id="card-switch">Switch</option>
<option value="SOLO" id="card-solo">Solo</option>
</select>
The above would then allow individual styling to each - in my case
displaying an image next to each option.
------------------------------------------------------------------------
June 23, 2005 - 12:08 : budda
Attachment: http://drupal.org/files/issues/formselectcss.patch (1.82 KB)
Patch for above CSS modification request.
Most of what can currently be done with CSS on elements is only useful
on Mozilla browsers. IE can't do anything exciting, it just ignores the
content.
3
2