Update custom_block_schema() description for the bid field

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.

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?

I meant just the last line as D7:

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:

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).

Ok, I agree. The attached patch changes the description.

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?

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."

This is really a task as nothing is broken and it would be a nice clean up. This needs to go into d8 first.

@franz: I like your description, changed that.
@marcingy: Rerolled the patch.

Is block_update_8000 the correct update function?

+1

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 all blocks fieldable entities

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

@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.

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.

Rerolled it with the new directory structure. Also a new block_update_N function got in, in the meantime, so I increased N.

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:

1. Install Drupal and create some blocks.
2. Inspect the table in the database.
3. Apply the patch.
4. Run update.php.
5. Inspect the table again and ensure that it was updated properly (and that nothing weird happened).

Thanks!

Update hook only for D7 now.

Thanks @Niklas Fiekas, that looks good. Next let's get the manual upgrade testing described in #15.

Does this tag makes sense for #15?

Yep, thank you.

Got trapped by this again, recently. So here is some testing. (Both patches still apply).

1. Since there is no update function for D8 - the update will happen on D7 - I installed a clean Drupal 8 with the patch applied. I was still able to create and position custom blocks. The database update looks as expected:

   

2. On a Drupal 7 site with a few custom blocks the table looked like this:

   
   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.

Not sure why I removed this tag.

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 ;)

Yeah.

'The custom block ID, which is used as the {block}.delta value.' is still confusing to me. What does it mean for block.bid to be used as the block.delta value?

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.

I think Dries is saying the text needs further clarification.

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 ;)

Setting NR for some suggestions for a better text. :)

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"

What's our character limit? We could add more detail to the description if possible, or to the hook_schema() docblock otherwise.

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.

"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).

I still don't understand what "referenced by" means. If it's a foreign key let's just say so. I agree that shorter is better.

If this still makes sense for 8.x, this will need a reroll to move the change to custom_block.install.

There is no {block} table anymore, so instead we can just say:
The primary identifier for the custom block.

Re-rolled, greatly simplified.

Closed #1826298: block_custom.bid is not a bid; it's a delta. as a duplicate of this issue.

#44: 966556-custom-block-bid-delta-33.patch queued for re-testing.

#1871772: Convert custom blocks to content entities It looks like the description was also changed as part of this issue. The change there is less human-readable, but accomplishes the same basic goal.

+++ b/core/modules/block/custom_block/custom_block.installundefined
@@ -10,21 +10,20 @@
-        'description' => "The block's {block}.bid.",
+        'description' => "The block's {custom_block}.id.",

Appears this change is already in 8.x

Update custom block schema description in new (D8) location. As part of DrupalCon Portland core mentoring sprint.

#50: drupal-update-custom-block-schema-966556-7080710.patch queued for re-testing.

The patch from comment #50 has failed testing in Drupal 7 as it is written for Drupal 8.

If the more human readable description still is the direction we want to go then the Version should be changed back to 8.x-dev. and new issues should be started for any backports.

removing reroll tag

The file no longer exists in D8 code since #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities, so it seems that D7 is the only release concerned. Perhaps this should be closed?