Posted by googletorp on November 9, 2010 at 12:04pm
10 followers
| Project: | Drupal core |
| Version: | 8.x-dev |
| Component: | block.module |
| Category: | task |
| Priority: | normal |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | needs backport to D6, needs backport to D7, Novice |
Issue Summary
I've taken a look at how Drupal creates custom blocks, and something seems wrong.
The schema definition for the block_custom is as follows:
<?php
$schema['block_custom'] = array(
'description' => 'Stores contents of custom-made blocks.',
'fields' => array(
'bid' => array(
'type' => 'serial',
'unsigned' => TRUE,
'not null' => TRUE,
'description' => "The block's {block}.bid.",
),
// SNIP...
),
'unique keys' => array(
'info' => array('info'),
),
'primary key' => array('bid'),
);
?>It uses bid as primary key and according to the description, this should be a FK to {block}.bid. However the code for creating blocks is as follows:
<?php
function block_add_block_form_submit($form, &$form_state) {
$delta = db_insert('block_custom')
->fields(array(
'body' => $form_state['values']['body']['value'],
'info' => $form_state['values']['info'],
'format' => $form_state['values']['body']['format'],
))
->execute();
// Store block delta to allow other modules to work with new block.
$form_state['values']['delta'] = $delta;
// SNIP ..
}
?>So from what I see in the code (and the database), the PK for block_custom is the block delta for blocks created thourgh the block module.
Now I'm not sure if we are too late in the process to change column names in the database, it would require some changes to the block module, but we should at least change the description to something like:
<?php
'description' => "The block's {block}.delta.",
?>
Comments
#1
Seems fair. I think custom blocks will change a lot in 8.x, so it's better to leave as 7.x. Also affects 6.x probably.
#2
Merging #1126544: Custom blocks are looked up by delta, not bid as the description states.
The naive patch there actually didn't break much, except a few test cases relying on the column name. Should we go ahead and fix those tests accordingly?
@franz: I don't think we can change the column name in D7, can we?
#3
I meant just the last line as D7:
For D8, I'm unsure if it's worth to change it, as there is a number of issues that will deal with block names too, so I'd rather postpone this (and link other issues here too).
#4
Ok, I agree. The attached patch changes the description.
#5
I feel the description is somehow the wrong way round...
It's not the {block}.delta that we're using in this table; rather, the serial ID in {block_custom} is what the {block} table uses for its delta. Does that make sense?
#6
It does, and that's the point. However, description was totally wrong, and now it is right in the pure sense that the serial ID on block_custom correspond to {block}.delta (and never to {block}.bid). This could be a better explained as something as "The custom block ID, which is used as the {block}.delta value."
#7
This is really a task as nothing is broken and it would be a nice clean up. This needs to go into d8 first.
#8
@franz: I like your description, changed that.
@marcingy: Rerolled the patch.
Is block_update_8000 the correct update function?
#9
+1
#10
The delta should not be a serial id. There are to many places in the theme and block placement level where the delta needs to be hard coded.
With custom blocks becoming entities, this issue is also no longer valid #430886: Make blocks entities
#11
indytechcook, I don't think you payed attention to what this issue is about.
It's only changing the description. Alas, backporting this to D7 and D6 is good regardless of the changes on blocks in D8
#12
@franz, oh yes. Thus this issue is a good example of an issue that is only applicable to D7/D6 but must be in D8. yeah for process.
#13
Thanks for your work on this patch. Note that the Drupal 8.x patch will need to be rerolled, because the core directory structure for Drupal 8 has now changed. (For more information, see #22336: Move all core Drupal files under a /core folder to improve usability and upgrades). When the patch has been rerolled, please set the issue back to "Needs Review."
Tagging as novice for the task of rerolling the Drupal 8.x patch.
If you need help rerolling this patch, you can come to core office hours or ask in #drupal-gitsupport on IRC.
#14
Rerolled it with the new directory structure. Also a new block_update_N function got in, in the meantime, so I increased N.
#15
Thanks for the reroll.
I think since this is going into D7, there should be no update hook in the D8 patch, and instead one in the D7 version of the patch?
Once we have a D7 version of the patch with the D7 upate hook, we'll also want to manually test the upgrade path to make sure it works correctly:
Thanks!
#16
Update hook only for D7 now.
#17
Thanks @Niklas Fiekas, that looks good. Next let's get the manual upgrade testing described in #15.
#18
Does this tag makes sense for #15?
#19
Yep, thank you.
#20
Got trapped by this again, recently. So here is some testing. (Both patches still apply).
After applying the patch and executing update.php the table looks like this:
The description has been changed as expected and the data is still OK. After this the blocks were still displayed and I was able to create new ones and reposition blocks.
Note again that this patch is no API change. Were just changing a wrong description. I think this is good to go.
#21
Not sure why I removed this tag.
#22
The last submitted patch, 966556-custom-block-bid-delta-4-d7.patch, failed testing.
#23
Heh ... looks like 966556-custom-block-bid-delta-4-d7.patch has been ignored (since it was uploaded before the do-not-test thing). Now the testbot has reconsidered it ;)
#24
Yeah.
#25
'The custom block ID, which is used as the {block}.delta value.'is still confusing to me. What does it mean forblock.bidto be used as theblock.deltavalue?#26
That the {block} table is joined on {block_custom} via
{block}.delta = {block_custom}.bid, not{block}.bid = {block_custom}.bid.I agree that this is confusing and that the name is wrong in the first place, but this issue is about having something backportable (like a description change) before we really fix it in D8.
#28
I think Dries is saying the text needs further clarification.
#29
Mhh ... personally I think it's fine, but I am already too biased on this. If someone can come up with a better description I wouldn't reject that ;)
#30
Setting NR for some suggestions for a better text. :)
#31
Hmm, looks like I better follow this through.
I actually think that the description text is pretty good. The problem is that the column names are messed which in itself is confusing.
I don't know if some one can write up some brilliant and clear description, but I really think this is RTCB.
But that's just my 2 cents
#32
How about
"The custom block ID, referenced by {block.delta} and not related to {block.bid}."
It's hard to have a description that explains it without saying "I KNOW, this is a poor choice of a name for this column"
#33
#32 That's a good description, makes it clear that this is messy.
I added the patch for the new description, but we probably need some 3rd person to vouch for the new description text.
xjm - what do you think about the next description text?
#34
What's our character limit? We could add more detail to the description if possible, or to the
hook_schema()docblock otherwise.#35
#34
My finders are that for postgres it seems to be unlimited, while on MySQL (Maria DB variant) the max is 279 chars. But things get more complicated if we also want to make sure that we don't go over the limit for sqlite, MSSQL and others that could be added.
We could probably do a 127 char comment without getting into problems, maybe even 255.
#36
The MySQL column comment is a varchar(1024) (Crosspost ... mhh ... I looked this up in the information_schema table. Does this vary across versions and forks? Where did you find that?), PostgreSQL's limit seams to be much higher. SQLite doesn't support column descriptions. That should be enough for a thorough description, but the shorter it is, the more readable it is going to be in phpMyadmin and co.
#37
#36 I've tried testing on my local DB server, the exact version I run is
Ver 14.16 Distrib 5.2.8-MariaDB, for apple-darwin11.2.0 (i386) using readline 6.2
From using one GUI and setting a very long comment, I got the error that 279 was the max, using another GUI and the terminal I got the same error but with max comment size being 255.
Anyways since Drupal can be used for potentially many database types it's hard to know exactly when it can break something, but my hunch is that 255 should be safe.
Maybe we should start by working out a detailed description, and then figure out how much we can put into the DB and then put the rest into the PHP code, if we need a longer comment.
#38
"The custom block ID. Despite using the same identifier, this column is not related to {block}.bid and serves a different purpose. It is the {block}.delta column that references this field."
Does this explain a little better? (english can be improved).
#39
How about.
"The custom block ID. Despite using the same identifier, this column is not related to {block}.bid, instead the {block}.delta column references this field."