Follow up for #1871772-108: Convert custom blocks to content entities

Problem/Motivation

+++ b/core/modules/block/custom_block/custom_block.module
@@ -53,3 +157,161 @@ function theme_custom_block_block($variables) {
+function custom_block_add_body_field($block_type_id, $label = 'Block body') {
Minor: Should default to just "Body", I think.

+++ b/core/modules/block/custom_block/custom_block.module
@@ -53,3 +157,161 @@ function theme_custom_block_block($variables) {
+ $field = field_info_field('block_body');
+ $instance = field_info_instance('custom_block', 'block_body', $block_type_id);
...
+ 'field_name' => 'block_body',
I'm concerned about module namespace issues here; technically, this module-specific field should at least start with "custom_block_" as it is owned by custom_block.module.

Proposed resolution

Rename the field and the default title, refactor upgrade path

Remaining tasks

Rename the field and the default title, refactor upgrade path

CommentFileSizeAuthor
#14 custom-block-field-rename-1919916-14.patch2.4 KBoadaeh
#11 custom-block-field-name.11.patch9.31 KBoadaeh
#10 custom-block-field-name.10.patch11.66 KBlarowlan
#6 change-block-body-1919916-6.patch2.51 KBjessehs
#4 1919916-Refactored-label-of-custom-block-body-4.patch1.28 KBbabruix
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Issue tags: -Needs architectural review, -VDC

changing tags

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Status: Postponed » Active
Issue tags: -Entity system, -D8 upgrade path
babruix’s picture

babruix CreditAttribution: babruix commented
Issue tags: +SprintWeekend2013
babruix’s picture

babruix CreditAttribution: babruix commented
Status: Active » Needs review
FileSize
1919916-Refactored-label-of-custom-block-body-4.patch1.28 KB

Changed 'Block body' to just 'Body'.
About fixing title we need more clarification.

Also I`m concerned about this code: 

function custom_block_add_body_field($block_type_id, $label = 'Body') {
  // Add or remove the body field, as needed.
  $field = field_info_field('block_body');
  $instance = field_info_instance('custom_block', 'block_body', $block_type_id);
  if (empty($field)) {
    $field = array(
      'field_name' => 'block_body',
      'type' => 'text_with_summary',
      'entity_types' => array('custom_block'),
    );
    $field = field_create_field($field);
  }
....

Why we need $field variable is it never used after?

Status: Needs review » Needs work

The last submitted patch, 1919916-Refactored-label-of-custom-block-body-4.patch, failed testing.

jessehs’s picture

jessehs
he/him
Primary language English
Location Taos, New Mexico
CreditAttribution: jessehs commented
FileSize
change-block-body-1919916-6.patch2.51 KB

Re-roll of #4, but now also changing tests to look for the updated label.

jessehs’s picture

jessehs
he/him
Primary language English
Location Taos, New Mexico
CreditAttribution: jessehs commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, change-block-body-1919916-6.patch, failed testing.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Needs work » Postponed

This should be postponed until #1920862: Rename custom_block.module to block_content.module goes in, as the machine name of the module will be changed

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Postponed » Needs review
FileSize
custom-block-field-name.10.patch11.66 KB

Actually, not really - because the script used to do the rename in #1920862: Rename custom_block.module to block_content.module will catch the custom_block reference in the field machine name and rename that too.
This should fix the tests and update the machine name too.

oadaeh’s picture

oadaeh CreditAttribution: oadaeh commented
FileSize
custom-block-field-name.11.patch9.31 KB

The patch in #10 no longer applies.

I've manually recreated it.

It seems that testCustomBlock() no loner exists in core/modules/block/lib/Drupal/block/Tests/BlockTest.php, or in fact, anywhere.

The last submitted patch, custom-block-field-name.11.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Issue summary: View changes

I see no reason to make name prefixed with "custom_block_" please make it inline with node module - just name it "block", in d8 different entity types could have same-named fields

oadaeh’s picture

oadaeh CreditAttribution: oadaeh commented
Title: Refactor machine name and title of custom block default body field » Refactor title of custom block default body field
Status: Needs work » Needs review
FileSize
custom-block-field-rename-1919916-14.patch2.4 KB

Based on @andypost's comment and the fact that much of what the original patch modified is no longer available or already changed differently, I've attached a patch that basically does nothing more than change the default text from "Block body" to "Body".

sun’s picture

sun
Primary language German
Location Karlsruhe
CreditAttribution: sun commented
Title: Refactor title of custom block default body field » Change label of custom block default body field from "Block body" to just "Body"
Status: Needs review » Reviewed & tested by the community

I agree with the scope change. The machine name issue should have been a separate issue right from the start. :-)

RTBC, unless testbot disagrees ;)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 9d8aa13 and pushed to 8.x. Thanks!

  • Commit 9d8aa13 on 8.x by alexpott: 
    Issue #1919916 by oadaeh, larowlan, jessehs, babruix: Change label of...
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers commented

Yay! At last! :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.