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

Problem

  • custom* module names are the informal standard namespace for custom developed, site-specific modules.
  • I have at least a dozen of sites that have a custom_block.module already (among other custom* modules).

Proposed solution

  1. Rename custom_block.module to block_content.module.
  2. Move the module directly into /core/modules/
  3. Add a block_content.module component for issues to d.o.
Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Reroll the patch if it no longer applies. Instructions
Manually test the patch Novice Instructions Yes
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Instructions
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Title: Rename custom_block.module to block_clustom.module » Rename custom_block.module to block_custom.module
sun’s picture

The module should also be moved directly into /core/modules/. Updated the issue summary accordingly.

jibran’s picture

+1 for this change.

andypost’s picture

+1 here, this overlaps with a lot of custom modules done for specific sites.

larowlan’s picture

Status: Active » Postponed
Dave Reid’s picture

Oh god this just completely bit me for several hours. Adding a custom module completely hoses your Drupal 8 site the moment you enable it: #1929720: How the bleepity bleep do you debug missing use statements in classes? because custom_block_load() then becomes a hook_block_load() implementation with block entity objects being passed into it rather than entity IDs like it expects. And core *should* have failed gracefully if you pass in non-numeric entity IDs, except it doesn't. It explodes into a firey death from which you cannot recover due to #1003788: PostgreSQL: PDOException:Invalid text representation when attempting to load an entity with a string or non-scalar ID.

A death of a thousand suns should be administered to custom_block namespace.

EclipseGc’s picture

I'd like to suggest we rename this block_content instead of block_custom. Any issues with that?

Eclipse

sun’s picture

Status: Postponed » Active

Perhaps it's bikeshed time, even? :)

I actually wonder whether we couldn't find a name that doesn't have "block" in it… That is, because all of the function names and class names (à la CustomBlockBlock or custom_block_block_view_alter() :P) are a little bit confusing.

We apparently had a very nice and short name for certain blocks in D6 and below: Boxes.

box.module?

hass’s picture

I really had some hate with block_box_get()... Very inconsistent naming and once an object and once an array and sometimes a different array structure... :-(

tim.plunkett’s picture

tim.plunkett’s picture

Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, custom-block-block-custom.patch, failed testing.

larowlan’s picture

<paint target="bikeshed">how about content_block?</paint>

The last submitted patch, custom-block-block-custom.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
91.64 KB

Whoops, forgot the rename bit.

The good thing is, as long as the patch applies, picking a new name is easy, since you can just edit the patch file directly.

Status: Needs review » Needs work

The last submitted patch, custom-block-1920862-15.patch, failed testing.

larowlan’s picture

Assigned: Unassigned » larowlan

I'll kick this along later today, I was expecting the upgrade path to fail, thats why we were waiting on #1919910: Refactor custom_block upgrade path to create tables using hook_schema_0() instead of in block_update_N() - the rest looks straight forwardish

Dave Reid’s picture

Not content_block, because this would result in the same problem when a content.module exists and is enabled on the site.

EclipseGc’s picture

Right, I still really like block_content.module. I think it actually says what it is, and solves the other problems that lead to this issue.

Eclipse

larowlan’s picture

block_content works for me

jibran’s picture

+1 for block_content

larowlan’s picture

Status: Needs work » Needs review
FileSize
98.21 KB

So this uses block_content.
Upgrade path should pass this time because block_content != existing {block_custom} table in D7.
So change to any other name should be straight forward, using block_custom will require new upgrade path that adds fields to the existing table.

Note module is still called 'Custom Block' and so are entities, I don't think 'Block Content' fits in that regards. I still like 'Add custom block' better than 'Add block content'.

Status: Needs review » Needs work
Issue tags: -API clean-up

The last submitted patch, custom-block-rename.22.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: +API clean-up

#22: custom-block-rename.22.patch queued for re-testing.

larowlan’s picture

green :)
We just need to agree on whether we change the entity names, module name etc too

andypost’s picture

@larowlan please use -M25% to minimize a patch size
I think classes and other parts should be renamed too

andypost’s picture

Issue tags: -API clean-up

#22: custom-block-rename.22.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +API clean-up

The last submitted patch, custom-block-rename.22.patch, failed testing.

effulgentsia’s picture

Title: Rename custom_block.module to block_custom.module » Rename custom_block.module to block_content.module
jessebeach’s picture

I had entered a duplicate here #2027101: Remove the word "custom" from block module UI elements. There are nice images and descriptions in the dupe.

tim.plunkett’s picture

Component: block.module » custom_block.module

Ironic, but yeah.

tim.plunkett’s picture

Issue summary: View changes

Updated issue summary.

larowlan’s picture

Issue summary: View changes
Issue tags: +beta target, +rename module
martin107’s picture

Issue tags: +Needs reroll
sun’s picture

In particular this rename/move still is highly important due to the inofficial but common "custom" module name clashes. I'm tempted to bump this to critical.

Do we have an idea of when a good time-frame for this rename (and others) might be?

Perhaps the same time-frame when we're going to switch to PSR-4?

effulgentsia’s picture

I don't see any reason why this couldn't land whenever it's RTBC. There are no other beta blocker or beta target issues for custom_block.module, so I doubt committing this will create much reroll pain for other issues.

xan.ps’s picture

xan.ps’s picture

Apologies. By mistake there was some issue while uploading the patch.
The new patch file has been attached.

JayeshSolanki’s picture

Status: Needs work » Needs review

The last submitted patch, 37: Renamed-custom_block-to-block_content-1920862-37.patch, failed testing.

larowlan’s picture

Issue tags: +Novice
pwolanin’s picture

The issue summary does not match the recent patches

Berdir’s picture

Status: Needs review » Needs work

Last patch didn't pass.

cilefen’s picture

FileSize
254.82 KB

Just want to see how the tests turn out. It still needs some UI renames.

cilefen’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 43: 1920862-43.patch, failed testing.

cilefen’s picture

Do we want to rename this module in the GUI or leave it "Custom Block"?

cilefen’s picture

Issue summary: View changes
cilefen’s picture

Oh no, are we renaming this to block_content or block_custom? The title of this issue didn't match the summary then I just updated the summary to make matters more confusing.

Oh, forget it - I just read the thread.

cilefen’s picture

Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 46: 1920862-46.patch, failed testing.

cilefen’s picture

Status: Needs work » Needs review
FileSize
4 KB
256.5 KB
Mixologic’s picture

FileSize
257.09 KB

Here's a reroll of the current. Found some new introductions of CustomBlock in HEAD, as well as fixed about nine merge conflicts.

6eec619 is the branch point from head for this patch, if future rerollers want to use that.

This would be nice to have land either pre or post psr-4 disruption.

Status: Needs review » Needs work

The last submitted patch, 52: 1920862-52-6eec619.patch, failed testing.

martin107’s picture

FileSize
257.1 KB
818 bytes

Nibbling at the edges

Drupal\config_translation\Tests\ConfigTranslationListUiTest now passes.

martin107’s picture

Status: Needs work » Needs review

Triggering testbot

Status: Needs review » Needs work

The last submitted patch, 54: 1920862-54.patch, failed testing.

cilefen’s picture

This needs the PSR-4 reroll.

cilefen’s picture

Status: Needs work » Needs review
FileSize
253.15 KB
378.63 KB

Trying the PSR-4 reroll.

Status: Needs review » Needs work

The last submitted patch, 58: 1920862-58.patch, failed testing.

cilefen’s picture

58: 1920862-58.patch queued for re-testing.

The last submitted patch, 58: 1920862-58.patch, failed testing.

cilefen’s picture

Status: Needs work » Needs review
FileSize
256.44 KB
5.13 KB
cilefen’s picture

cilefen’s picture

FileSize
256.54 KB
9.6 KB
cilefen’s picture

FileSize
271.12 KB
9.6 KB

Status: Needs review » Needs work

The last submitted patch, 65: 1920862-65.patch, failed testing.

cilefen’s picture

FileSize
255.97 KB
1.8 KB

Oops, reroll.

cilefen’s picture

Status: Needs work » Needs review
larowlan’s picture

hi @cilifen, do you know if your git client is using renames for moves, if not, can you set it to do so - it might make this patch smaller.
Please see http://drupal.org/node/1542048 - specifically

[diff]
  renames = copies

If it is already, could you use git diff -M25% to set a lower rename/move limit?

Thanks @larowlan

cilefen’s picture

cilefen’s picture

cilefen’s picture

Issue summary: View changes

Getting ready for the Austin DrupalCon sprint, following http://www.hook42.com/blog/prepping-drupalcon-austin-sprints-sprint-lead...

cilefen’s picture

70: 1920862-70.patch queued for re-testing.

cilefen’s picture

FileSize
136.78 KB

Status: Needs review » Needs work

The last submitted patch, 74: 1920862-74.patch, failed testing.

cilefen’s picture

Status: Needs work » Needs review
FileSize
137.41 KB
512 bytes
aaronschachter’s picture

Issue summary: View changes

Tested on simplytest.me, everything is working as expected.

aaronschachter’s picture

Status: Needs review » Reviewed & tested by the community

Reviewed the code, and also pulled down locally to test. All looks good. Marking as RTBTC.

aaronschachter’s picture

@cilefen and I were discussing whether or not the labels in the UI should read as "Add block content" instead of "Add custom block", but the original scope is this issue to move it out of the block module. If we want to change the button labels and page headlines, a separate issue should be opened.

Mixologic’s picture

Not to mention that changing UI text has an impact on translations.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Somehow this managed to miraculously still apply, so committed and pushed to 8.x. Thanks!

Mixologic’s picture

Did this get pushed? I don't see it in my HEAD when I pull.

effulgentsia’s picture

76: 1920862-76.patch queued for re-testing.

Status: Fixed » Needs work

The last submitted patch, 76: 1920862-76.patch, failed testing.

effulgentsia’s picture

Issue tags: +Needs reroll

Per #82, this hasn't been pushed. #2277941: Allow injecting an arbitrary FieldConfig when building a FieldInstanceConfig landed in the meantime, so #76 no longer applies. Anyone up for another reroll?

webchick’s picture

Issue tags: -Needs reroll

Oh dang it. :P This is what happens when I push and then shut the laptop thinking everything was going to go fine. :P

Push didn't go through, and now the patch no longer applies. :( Sorry about that. I'd mark it "needs work" but it already is.

webchick’s picture

Issue tags: +Needs reroll

Oops. Cross-post.

cilefen’s picture

Status: Needs work » Needs review
FileSize
136.75 KB

Status: Needs review » Needs work

The last submitted patch, 88: 1920862-88.patch, failed testing.

cilefen’s picture

Status: Needs work » Needs review
FileSize
136.34 KB

Status: Needs review » Needs work

The last submitted patch, 90: 1920862-90.patch, failed testing.

cilefen’s picture

Status: Needs work » Needs review
FileSize
136.34 KB
501 bytes

Sorry, testbots! revison_log should be nullable as per #2248991: Rename the 'log' field to 'revision_log' in Node and CustomBlock.

cilefen’s picture

Issue tags: -Needs reroll
Mixologic’s picture

Status: Needs review » Reviewed & tested by the community

LGTM - Thanks @cilefen for all the diligence in keeping up with the head chasing re-rolls.

webchick’s picture

Status: Reviewed & tested by the community » Fixed
$ git push
Counting objects: 147, done.
Delta compression using up to 4 threads.
Compressing objects: 100% (95/95), done.
Writing objects: 100% (103/103), 40.30 KiB | 0 bytes/s, done.
Total 103 (delta 30), reused 0 (delta 0)
To webchick@git.drupal.org:project/drupal.git
   c4728bc..95725e2  8.x -> 8.x

Ok, got it this time. :) Thanks SO much!

  • Commit 95725e2 on 8.x by webchick:
    Issue #1920862 by martin107, Mixologic, cilefen, larowlan, xan.ps, tim....

Status: Fixed » Closed (fixed)

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