Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
- Rename custom_block.module to block_content.module.
- Move the module directly into
/core/modules/
- Add a block_content.module component for issues to d.o.
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 |
Comment | File | Size | Author |
---|---|---|---|
#92 | interdiff-90-92.txt | 501 bytes | cilefen |
#92 | 1920862-92.patch | 136.34 KB | cilefen |
#90 | 1920862-90.patch | 136.34 KB | cilefen |
Comments
Comment #1
sunComment #2
sunThe module should also be moved directly into /core/modules/. Updated the issue summary accordingly.
Comment #3
jibran+1 for this change.
Comment #4
andypost+1 here, this overlaps with a lot of custom modules done for specific sites.
Comment #5
larowlanThis will clash with #1919910: Refactor custom_block upgrade path to create tables using hook_schema_0() instead of in block_update_N() - can we get that in first?
Needs review.
Comment #6
Dave ReidOh 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.
Comment #7
EclipseGc CreditAttribution: EclipseGc commentedI'd like to suggest we rename this block_content instead of block_custom. Any issues with that?
Eclipse
Comment #8
sunPerhaps 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
orcustom_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?
Comment #9
hass CreditAttribution: hass commentedI 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... :-(
Comment #10
tim.plunkett#1919910: Refactor custom_block upgrade path to create tables using hook_schema_0() instead of in block_update_N() is in, I'm just curious to see what breaks with a relatively dumb script.
Comment #11
tim.plunkettComment #13
larowlan<paint target="bikeshed">how about content_block?</paint>
Comment #15
tim.plunkettWhoops, 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.
Comment #17
larowlanI'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
Comment #18
Dave ReidNot content_block, because this would result in the same problem when a content.module exists and is enabled on the site.
Comment #19
EclipseGc CreditAttribution: EclipseGc commentedRight, 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
Comment #20
larowlanblock_content works for me
Comment #21
jibran+1 for block_content
Comment #22
larowlanSo 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'.
Comment #24
tim.plunkett#22: custom-block-rename.22.patch queued for re-testing.
Comment #25
larowlangreen :)
We just need to agree on whether we change the entity names, module name etc too
Comment #26
andypost@larowlan please use
-M25%
to minimize a patch sizeI think classes and other parts should be renamed too
Comment #27
andypost#22: custom-block-rename.22.patch queued for re-testing.
Comment #29
effulgentsia CreditAttribution: effulgentsia commentedComment #30
jessebeach CreditAttribution: jessebeach commentedI had entered a duplicate here #2027101: Remove the word "custom" from block module UI elements. There are nice images and descriptions in the dupe.
Comment #31
tim.plunkettIronic, but yeah.
Comment #31.0
tim.plunkettUpdated issue summary.
Comment #32
larowlanComment #33
martin107 CreditAttribution: martin107 commentedComment #34
sunIn 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?
Comment #35
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #36
xan.ps CreditAttribution: xan.ps commentedComment #37
xan.ps CreditAttribution: xan.ps commentedApologies. By mistake there was some issue while uploading the patch.
The new patch file has been attached.
Comment #38
JayeshSolanki CreditAttribution: JayeshSolanki commentedComment #40
larowlanComment #41
pwolanin CreditAttribution: pwolanin commentedThe issue summary does not match the recent patches
Comment #42
BerdirLast patch didn't pass.
Comment #43
cilefen CreditAttribution: cilefen commentedJust want to see how the tests turn out. It still needs some UI renames.
Comment #44
cilefen CreditAttribution: cilefen commentedComment #46
cilefen CreditAttribution: cilefen commentedDo we want to rename this module in the GUI or leave it "Custom Block"?
Comment #47
cilefen CreditAttribution: cilefen commentedComment #48
cilefen CreditAttribution: cilefen commentedOh 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.
Comment #49
cilefen CreditAttribution: cilefen commentedComment #51
cilefen CreditAttribution: cilefen commentedComment #52
MixologicHere'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.
Comment #54
martin107 CreditAttribution: martin107 commentedNibbling at the edges
Drupal\config_translation\Tests\ConfigTranslationListUiTest now passes.
Comment #55
martin107 CreditAttribution: martin107 commentedTriggering testbot
Comment #57
cilefen CreditAttribution: cilefen commentedThis needs the PSR-4 reroll.
Comment #58
cilefen CreditAttribution: cilefen commentedTrying the PSR-4 reroll.
Comment #60
cilefen CreditAttribution: cilefen commented58: 1920862-58.patch queued for re-testing.
Comment #62
cilefen CreditAttribution: cilefen commentedComment #63
cilefen CreditAttribution: cilefen commentedComment #64
cilefen CreditAttribution: cilefen commentedRerolled for #2253735: WSOD on custom block creation / listing if config translation enabled.
Comment #65
cilefen CreditAttribution: cilefen commentedComment #67
cilefen CreditAttribution: cilefen commentedOops, reroll.
Comment #68
cilefen CreditAttribution: cilefen commentedComment #69
larowlanhi @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
If it is already, could you use git diff -M25% to set a lower rename/move limit?
Thanks @larowlan
Comment #70
cilefen CreditAttribution: cilefen commentedRerolled for #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities and #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities and smaller patch.
Comment #71
cilefen CreditAttribution: cilefen commentedComment #72
cilefen CreditAttribution: cilefen commentedGetting ready for the Austin DrupalCon sprint, following http://www.hook42.com/blog/prepping-drupalcon-austin-sprints-sprint-lead...
Comment #73
cilefen CreditAttribution: cilefen commented70: 1920862-70.patch queued for re-testing.
Comment #74
cilefen CreditAttribution: cilefen commentedReroll for #2278051: Remove uses of non-existing 'max_length' setting on EntityRef fields.
Comment #76
cilefen CreditAttribution: cilefen commentedComment #77
aaronschachter CreditAttribution: aaronschachter commentedTested on simplytest.me, everything is working as expected.
Comment #78
aaronschachter CreditAttribution: aaronschachter commentedReviewed the code, and also pulled down locally to test. All looks good. Marking as RTBTC.
Comment #79
aaronschachter CreditAttribution: aaronschachter commented@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.
Comment #80
MixologicNot to mention that changing UI text has an impact on translations.
Comment #81
webchickSomehow this managed to miraculously still apply, so committed and pushed to 8.x. Thanks!
Comment #82
MixologicDid this get pushed? I don't see it in my HEAD when I pull.
Comment #83
effulgentsia CreditAttribution: effulgentsia commented76: 1920862-76.patch queued for re-testing.
Comment #85
effulgentsia CreditAttribution: effulgentsia commentedPer #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?
Comment #86
webchickOh 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.
Comment #87
webchickOops. Cross-post.
Comment #88
cilefen CreditAttribution: cilefen commentedRerolled because of #2277941: Allow injecting an arbitrary FieldConfig when building a FieldInstanceConfig
Comment #90
cilefen CreditAttribution: cilefen commentedComment #92
cilefen CreditAttribution: cilefen commentedSorry, testbots! revison_log should be nullable as per #2248991: Rename the 'log' field to 'revision_log' in Node and CustomBlock.
Comment #93
cilefen CreditAttribution: cilefen commentedComment #94
MixologicLGTM - Thanks @cilefen for all the diligence in keeping up with the head chasing re-rolls.
Comment #95
webchickOk, got it this time. :) Thanks SO much!